* SUMMARY TreeOutline should just dispatch events via WebInspector.Object.
<rdar://problem/22301243>
Created attachment 266222 [details] [Patch] Proposed Fix
Comment on attachment 266222 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=266222&action=review r=me, would like Tim to comment on event names though. Please rename these events and related code: s/SelectedElementChanged/SelectionChanged/ s/ElementHiddenChanged/ElementVisibilityChanged/ > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:142 > + if (this.treeOutline) Do we ever need this check? We assume it exists in some calls to dispatchEventToListeners(), but not others. This one seems to be a more paranoid check than was there before, while others are neutral. > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:1018 > + if (this.onselect) What is this for? I thought we removed onselect > Source/WebInspectorUI/UserInterface/Views/TreeOutlineDataGridSynchronizer.js:-52 > - // FIXME: This is a hack. TreeOutline should just dispatch events via WebInspector.Object. Good riddance! > Source/WebInspectorUI/UserInterface/Views/TreeOutlineDataGridSynchronizer.js:208 > + _treeElementSelected(event) This should match the new event name.
(In reply to comment #3) > Comment on attachment 266222 [details] > [Patch] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=266222&action=review > > r=me, would like Tim to comment on event names though. > > Please rename these events and related code: > > s/SelectedElementChanged/SelectionChanged/ > s/ElementHiddenChanged/ElementVisibilityChanged/ > > > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:142 > > + if (this.treeOutline) > > Do we ever need this check? We assume it exists in some calls to > dispatchEventToListeners(), but not others. This one seems to be a more > paranoid check than was there before, while others are neutral. I'll take a look and see if it is ever null. > > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:1018 > > + if (this.onselect) > > What is this for? I thought we removed onselect This is calling TreeElement.onselect. DOMTreeElement still overrides this, oncollapse, and other methods. > > Source/WebInspectorUI/UserInterface/Views/TreeOutlineDataGridSynchronizer.js:-52 > > - // FIXME: This is a hack. TreeOutline should just dispatch events via WebInspector.Object. > > Good riddance! > > > Source/WebInspectorUI/UserInterface/Views/TreeOutlineDataGridSynchronizer.js:208 > > + _treeElementSelected(event) > > This should match the new event name.
I suggested some better event names on IRC.
(In reply to comment #5) > I suggested some better event names on IRC. Are we happy with the following? ElementAdded ElementDidChange ElementRemoved ElementDisclosureDidChange ElementVisibilityDidChange SelectionDidChange
Comment on attachment 266222 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=266222&action=review >>> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:142 >>> + if (this.treeOutline) >> >> Do we ever need this check? We assume it exists in some calls to dispatchEventToListeners(), but not others. This one seems to be a more paranoid check than was there before, while others are neutral. > > I'll take a look and see if it is ever null. These checks are needed. Many operations in TreeOutline that dispatch events are also bound to TreeElement, which may be in a detached state. I missed a few checks in the TreeOutline.prototype.remove* family of methods which I'll add. I think we should reconsider TreeOutline's design at some point. BindingTreeOutline methods to another class to achieve reuse makes the code harder to follow. Instead, TreeOutline should have a public getter for an invisible root that always exists: let tree = new WebInspector.TreeOutline; tree.root.appendChild(childElement); If having to get the root is too verbose, TreeOutline could encapsulate the invisible root and forward operations to it: TreeOutline.prototype.appendChild = function(child) { this.root.appendChild(child); }
Created attachment 266406 [details] [Patch] Proposed Fix Left r? since a bunch of names have changed
Comment on attachment 266406 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=266406&action=review > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:864 > + // Set this before onpopulate. Since onpopulate can add elements and dispatch an ElementAdded event, We should rename onpopulate, to sound more like a subclass able method than an event.
Comment on attachment 266406 [details] [Patch] Proposed Fix Clearing flags on attachment: 266406 Committed r192936: <http://trac.webkit.org/changeset/192936>
All reviewed patches have been landed. Closing bug.