Bug 148067 – Web Inspector: TreeOutline should just dispatch events via WebInspector.Object

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

* SUMMARY
TreeOutline should just dispatch events via WebInspector.Object.

[Radar WebKit Bug Importer]

<rdar://problem/22301243>

[Matt Baker]

Created attachment 266222 [details]
[Patch] Proposed Fix

[Blaze Burg]

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.

[Matt Baker]

(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.

[Timothy Hatcher]

I suggested some better event names on IRC.

[Matt Baker]

(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

[Matt Baker]

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);
}

[Matt Baker]

Created attachment 266406 [details]
[Patch] Proposed Fix

Left r? since a bunch of names have changed

[Timothy Hatcher]

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.

[WebKit Commit Bot]

Comment on attachment 266406 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 266406

Committed r192936: <http://trac.webkit.org/changeset/192936>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.