Bug 175728 – Web Inspector: Create experimental Layers tab

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

[Ross Kirsling]

Introduce stage one of the Layers tab, guarded by an experimental feature flag.

[Ross Kirsling]

Created attachment 318531 [details]
Screenshot

[Matt Baker]

CCing Simon and Zalan, who work on rendering.

[Ross Kirsling]

Created attachment 318552 [details]
Patch

[Ross Kirsling]

As of this patch, the Layers tab houses a single content view, which is a 3D visualization of the current document's layer tree.

ContentBrowserTabContentView may be overkill at this point given that the nav bar is empty, but if we plan to subsequently bring the layer info data grid from the Elements tab sidebar into a second subview here, I think it may be justified in the end.

Please also note that three.js currently generates a lot of warning noise in WebKit, so we may wish to prioritize fixing https://bugs.webkit.org/show_bug.cgi?id=171054.

[Simon Fraser (smfr)]

That's pretty cool. Whether we want this is up to the inspector folks and their thoughts on the HI.

[Devin Rousso]

Comment on attachment 318552 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318552&action=review

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:54
> +            this._layersChangedWhileHidden = false;
> +            this.updateLayout();

Style: add newline.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:57
> +        if (this._didInitialLayout)

There is a protected getter for this.  Use that instead.

    if (this.didInitialLayout)

> Source/WebInspectorUI/UserInterface/Views/LayersTabContentView.js:33
> +        super("layers", "layers", tabBarItem, null, null, true);

For the `null` and `true` values, we like to use const variables to help explain what those values mean.

    const navigationSidebarPanelConstructor = null;
    const detailsSidebarPanelConstructors = null;
    const disableBackForward = true;
    super("layers", "layers", tabBarItem, navigationSidebarPanelConstructor, detailsSidebarPanelConstructors, disableBackForward);

[Ross Kirsling]

Created attachment 318641 [details]
Patch

[Devin Rousso]

Comment on attachment 318641 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318641&action=review

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:34
> +        WI.layerTreeManager.addEventListener(WI.LayerTreeManager.Event.LayerTreeDidChange, this._layerTreeDidChange, this);

This listener needs to be removed when the ContentView is closed().

Additionally, this event is going to be firing whenever the view is not visible.  It might be better to move the add/remove of this event listener to shown()/hidden().  As far as I understand, layout() should be called when shown() gets called, so you're already going to redraw anyways.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:36
> +        this._layersChangedWhileHidden = false;

Along the lines of the previous comment, moving the event listener to shown()/hidden() will remove the need for this variable.

[Timothy Hatcher]

Neat!

[Ross Kirsling]

(In reply to Devin Rousso from comment #8)
> Comment on attachment 318641 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318641&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:34
> > +        WI.layerTreeManager.addEventListener(WI.LayerTreeManager.Event.LayerTreeDidChange, this._layerTreeDidChange, this);
> 
> This listener needs to be removed when the ContentView is closed().
> 
> Additionally, this event is going to be firing whenever the view is not
> visible.  It might be better to move the add/remove of this event listener
> to shown()/hidden().  As far as I understand, layout() should be called when
> shown() gets called, so you're already going to redraw anyways.
> 
> > Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:36
> > +        this._layersChangedWhileHidden = false;
> 
> Along the lines of the previous comment, moving the event listener to
> shown()/hidden() will remove the need for this variable.

These are actually brand-new changes from after WIP Patch #4, and serve to address two issues that I was facing with the shown()/hidden() approach (which you reviewed at that time).

Issue #1 is that a Dirty layout() doesn't occur when a tab is restored from a previous inspector session. Issue #2 is that LayerTreeDidChange would be ignored if we get a new document while a different inspector tab is being viewed and then return to Layers. _layersChangedWhileHidden fixes both of these issues.

I can certainly add a removeEventListener in closed() if you'd like. The reason I didn't is because it seems like most View classes that call addEventListener from the constructor do not bother to call removeEventListener (presumably due to implicit destruction in JS).

[Ross Kirsling]

Created attachment 318907 [details]
Patch

[Matt Baker]

Comment on attachment 318907 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318907&action=review

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:110
> +    layout() {

Style: open brace should be on it's own line.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:118
> +                    this._addLayer(childLayers[i], i);

Since this is the only place Layers3DContentView.prototype._addLayer is called, and layer meshes are always added in order, the index can be omitted and the loop rewritten as a for...of.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:128
> +        this._camera.aspect = this.element.offsetWidth / this.element.offsetHeight;

Check for DBZ.

> Source/WebInspectorUI/UserInterface/Views/LayersTabContentView.js:58
> +    get type()

Style: simple getters that return a constant of member variable can be a single line.

> Source/WebInspectorUI/UserInterface/Views/LayersTabContentView.js:63
> +    get supportsSplitContentBrowser()

Ditto.

[Matt Baker]

Comment on attachment 318907 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318907&action=review

>> Source/WebInspectorUI/UserInterface/Views/LayersTabContentView.js:58
>> +    get type()
> 
> Style: simple getters that return a constant of member variable can be a single line.

OR member variable

[Ross Kirsling]

(In reply to Matt Baker from comment #12)
> > Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:118
> > +                    this._addLayer(childLayers[i], i);
> 
> Since this is the only place Layers3DContentView.prototype._addLayer is
> called, and layer meshes are always added in order, the index can be omitted
> and the loop rewritten as a for...of.

I'd love to, but I need the index for the Z coordinate. :P

> > Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:128
> > +        this._camera.aspect = this.element.offsetWidth / this.element.offsetHeight;
> 
> Check for DBZ.

(Just noting that we discussed this on IRC.)

[Ross Kirsling]

Created attachment 318915 [details]
Patch

[Devin Rousso]

Comment on attachment 318641 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=318641&action=review

>>> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:34
>>> +        WI.layerTreeManager.addEventListener(WI.LayerTreeManager.Event.LayerTreeDidChange, this._layerTreeDidChange, this);
>> 
>> This listener needs to be removed when the ContentView is closed().
>> 
>> Additionally, this event is going to be firing whenever the view is not visible.  It might be better to move the add/remove of this event listener to shown()/hidden().  As far as I understand, layout() should be called when shown() gets called, so you're already going to redraw anyways.
> 
> These are actually brand-new changes from after WIP Patch #4, and serve to address two issues that I was facing with the shown()/hidden() approach (which you reviewed at that time).
> 
> Issue #1 is that a Dirty layout() doesn't occur when a tab is restored from a previous inspector session. Issue #2 is that LayerTreeDidChange would be ignored if we get a new document while a different inspector tab is being viewed and then return to Layers. _layersChangedWhileHidden fixes both of these issues.
> 
> I can certainly add a removeEventListener in closed() if you'd like. The reason I didn't is because it seems like most View classes that call addEventListener from the constructor do not bother to call removeEventListener (presumably due to implicit destruction in JS).

The problem with _layersChangedWhileHidden is that it does unnecessary work while the tab isn't visible.  What I'm proposing is that you always call needsLayout() in shown(), which fixes issues #1 and #2, and move the addEventListener to shown() so that work is done while the tab is not visible.

    shown()
    {
        super.shown();

        this.needsLayout();

        WI.layerTreeManager.addEventListener(WI.LayerTreeManager.Event.LayerTreeDidChange, this._layerTreeDidChange, this);

        this._animate();
    }

    hidden()
    {
        WI.layerTreeManager.removeEventListener(WI.LayerTreeManager.Event.LayerTreeDidChange, this._layerTreeDidChange, this);

        this._stopAnimation();

        super.hidden();
    }

I am still not very convinced that having layersForNode() in layout() is the right way to go here.  I understand that it lets us avoid duplicate logic, but I also think that it's a bit of a hack and isn't being used properly.

[Ross Kirsling]

(In reply to Devin Rousso from comment #16)
> The problem with _layersChangedWhileHidden is that it does unnecessary work
> while the tab isn't visible.  What I'm proposing is that you always call
> needsLayout() in shown(), which fixes issues #1 and #2, and move the
> addEventListener to shown() so that work is done while the tab is not
> visible.
> 
>     shown()
>     {
>         super.shown();
> 
>         this.needsLayout();
> 
>        
> WI.layerTreeManager.addEventListener(WI.LayerTreeManager.Event.
> LayerTreeDidChange, this._layerTreeDidChange, this);
> 
>         this._animate();
>     }
> 
>     hidden()
>     {
>        
> WI.layerTreeManager.removeEventListener(WI.LayerTreeManager.Event.
> LayerTreeDidChange, this._layerTreeDidChange, this);
> 
>         this._stopAnimation();
> 
>         super.hidden();
>     }
> 
> I am still not very convinced that having layersForNode() in layout() is the
> right way to go here.  I understand that it lets us avoid duplicate logic,
> but I also think that it's a bit of a hack and isn't being used properly.

I certainly did try that before reluctantly adding the flag. :)

I believe the issue was that needsLayout() gets drowned out by this early out:
https://trac.webkit.org/browser/webkit/trunk/Source/WebInspectorUI/UserInterface/Views/View.js#L263

So I had to switch it to updateLayout(), at which point I figured the _layersChangedWhileHidden approach would be wise to avoid repulling layer data when switching tabs on a static page. The event listener may apply when the tab is hidden, but the only work it does is manipulating a flag.

I forgot to mention here Issue #3, which is that hidden() gets called twice for some reason when a tab is closed. This means that removing a specific event listener from the ContentView's hidden() hook results in an assert being hit. (One workaround is to do removeEventListener(null, null, true) but this seems like quite a cop-out.)

If you have a fundamentally different approach, I'm all ears; I've certainly spent quite a few hours beating my head against this one. :P But I don't really know that it's a "hack" per se? We're still laying out objects on screen, even though they're not DOM elements, and it's very convenient to be able to use the scheduler... I am the newbie here though, so my judgments are only based on the code that I've attempted to mimic. :D

[WebKit Commit Bot]

Comment on attachment 318915 [details]
Patch

Clearing flags on attachment: 318915

Committed r221166: <http://trac.webkit.org/changeset/221166>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34068970>