Bug 173986

Summary: Web Inspector: Remove unnecessary hasChildren from TreeOutline
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED WONTFIX    
Severity: Normal CC: buildbot, commit-queue, ggaren, inspector-bugzilla-changes, joepeck
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=173807
Bug Depends on: 174042    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
joepeck: review+, joepeck: commit-queue-
Patch none

Devin Rousso
Reported 2017-06-29 12:54:22 PDT
.
Attachments
Patch (2.16 KB, patch)
2017-06-29 14:37 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Patch (2.23 KB, patch)
2017-06-29 14:39 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Patch (2.23 KB, patch)
2017-06-29 14:39 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Patch (5.81 KB, patch)
2017-06-29 16:02 PDT, Devin Rousso 		joepeck: review+
joepeck: commit-queue-
DetailsFormatted DiffDiff
Patch (5.34 KB, patch)
2017-06-29 18:48 PDT, Devin Rousso 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Devin Rousso
Comment 1 2017-06-29 14:37:09 PDT
Created attachment 314170 [details] Patch
Build Bot
Comment 2 2017-06-29 14:38:16 PDT
Attachment 314170 [details] did not pass style-queue: ERROR: Source/WebInspectorUI/ChangeLog:3: ChangeLog entry has no bug number [changelog/bugnumber] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Devin Rousso
Comment 3 2017-06-29 14:39:10 PDT
Created attachment 314172 [details] Patch Oops. Forgot bug :P
Devin Rousso
Comment 4 2017-06-29 14:39:35 PDT
Created attachment 314173 [details] Patch Oops. Forgot bug :P
Devin Rousso
Comment 5 2017-06-29 16:02:53 PDT
Created attachment 314180 [details] Patch
Joseph Pecoraro
Comment 6 2017-06-29 17:21:44 PDT
Comment on attachment 314180 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314180&action=review > Source/WebInspectorUI/ChangeLog:24 > +2017-06-29 Devin Rousso <drousso@apple.com> > + > + Web Inspector: TreeOutlines should mark hasChildren as false when calling removeChildren > + https://bugs.webkit.org/show_bug.cgi?id=173986 Double changelog. Seems fine. r=me
Devin Rousso
Comment 7 2017-06-29 18:48:50 PDT
Created attachment 314205 [details] Patch
WebKit Commit Bot
Comment 8 2017-06-29 19:16:09 PDT
Comment on attachment 314205 [details] Patch Clearing flags on attachment: 314205 Committed r218983: <http://trac.webkit.org/changeset/218983>
WebKit Commit Bot
Comment 9 2017-06-29 19:16:10 PDT
All reviewed patches have been landed. Closing bug.
WebKit Commit Bot
Comment 10 2017-06-30 14:44:52 PDT
Re-opened since this is blocked by bug 174042
Joseph Pecoraro
Comment 11 2017-06-30 14:51:04 PDT
This transitively affected TreeElement, because some TreeElement operations were TreeOutline operations: appendChild() { return WebInspector.TreeOutline.prototype.appendChild.apply(this, arguments); } insertChild() { return WebInspector.TreeOutline.prototype.insertChild.apply(this, arguments); } removeChild() { return WebInspector.TreeOutline.prototype.removeChild.apply(this, arguments); } removeChildAtIndex() { return WebInspector.TreeOutline.prototype.removeChildAtIndex.apply(this, arguments); } removeChildren() { return WebInspector.TreeOutline.prototype.removeChildren.apply(this, arguments); } removeChildrenRecursive() { return WebInspector.TreeOutline.prototype.removeChildrenRecursive.apply(this, arguments); } selfOrDescendant() { return WebInspector.TreeOutline.prototype.selfOrDescendant.apply(this, arguments); } So this was not the right change. It is being rolled out. Ultimately the underlying issue "TreeOutline's hasChildren doesn't match children.length" still exists. The "hasChildren" property is misleading. It may mean "Doesn't have children yet but needs to populate and will have children", so it is unclear. Lets leave this the way it is and correct any individual issues as needed.
Geoffrey Garen
Comment 12 2017-07-06 17:00:19 PDT
Seems like a good idea to add a test case for the backtraces we temporarily broke. That's a pretty significant regression not to catch by automated testing.
Note You need to log in before you can comment on or make changes to this bug.