[Webkit-unassigned] [Bug 174176] Web Inspector: [META] Add 3D layer visualization

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=174176

Devin Rousso <drousso at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |drousso at apple.com

--- Comment #10 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 316485
  --> https://bugs.webkit.org/attachment.cgi?id=316485
WIP Patch #2

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

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:37
> +    static _createLayerMesh(rect, index, isOutline = false)

This function doesn't look like it needs to be static.  You can just make it a regular private function.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:39
> +        const Z_INTERVAL = 25;

Style: lowercase

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:45
> +        const geometry = new THREE.Geometry();
> +        geometry.vertices.push(new THREE.Vector3(rect.x,              -rect.y,               index * Z_INTERVAL));
> +        geometry.vertices.push(new THREE.Vector3(rect.x + rect.width, -rect.y,               index * Z_INTERVAL));
> +        geometry.vertices.push(new THREE.Vector3(rect.x + rect.width, -rect.y - rect.height, index * Z_INTERVAL));
> +        geometry.vertices.push(new THREE.Vector3(rect.x,              -rect.y - rect.height, index * Z_INTERVAL));

We typically only use `const` for variables that won't be different for any given invocation of a function (such as zInterval).  Also, we don't usually add spacing like this, although I'm not against this personally :P

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:54
> +        const material = new THREE.MeshBasicMaterial({color: "hsl(76, 49%, 75%)", transparent: true, opacity: 0.4, side: THREE.DoubleSide});

Style: this object has enough properties that it might be worth separating on new lines.  Up to you though.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:60
> +    shown()

This should be under Protected.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:67
> +    hidden()

Ditto.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:69
> +        super.hidden();

NIT: I tend to put `super.hidden()` last just in case a parent class does something weird to the element.  It looks like it's fine here.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:81
> +        this._setup();
> +        this._animate();

NIT: I think you can just move the contents of both of these functions to initialLayout, but I don't feel too strongly so it's up to you.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:93
> +            WebInspector.layerTreeManager.layersForNode(node, (layerForNode, childLayers) => {
> +                this._clearLayers();
> +                for (let i = 0; i < childLayers.length; i++)
> +                    this._addLayer(childLayers[i], i);
> +            });

Adding this to layout() means that anytime the view changes (such as a resize) you are fetching the layer information from the backend.  I'm not sure we want to do that.  It might be better to put this in its own function, which is called in shown() and _layerTreeDidChange.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:116
> +        this._renderer = new THREE.WebGLRenderer({antialias: true});

Please add all of these member variables as null in the constructor.  It helps with readability.

    this._renderer = null;
    this._camera = null;
    this._controls = null;
    this._boundsGroup = null;
    this._compositedBoundsGroup = null;
    this._scene = null;

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:129
> +        this._controls.maxAzimuthAngle =  Math.PI / 2;

Style: extra space.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:145
> +        requestAnimationFrame(() => { this._animate(); });

We should cancelAnimationFrame whenever hidden() is called, as we don't need to be rendering when the view is not visible.  Consequently, this should first start animating in shown(), not initialLayout().

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:162
> +        const compositedBounds = {x: layer.bounds.x, y: layer.bounds.y, width: layer.compositedBounds.width, height: layer.compositedBounds.height};

Style: put each of the keys on a new line:

    let compositedBounds = {
        x: layer.bounds.x,
        y: layer.bounds.y,
        width: layer.compositedBounds.width, 
        height: layer.compositedBounds.height,
    };

> Source/WebInspectorUI/UserInterface/Views/LayersTabContentView.js:26
> +WebInspector.LayersTabContentView = class LayersTabContentView extends WebInspector.ContentBrowserTabContentView

Extending ContentBrowserTabContentView means that you get a bunch of extra UI (sidebars, back/forward, navigation bar, etc.) that it doesn't look like you need here.  I think you should just extend from TabContentView (see NewTabContentView for an example).

> Source/WebInspectorUI/UserInterface/Views/LayersTabContentView.js:28
> +    constructor(identifier)

This is an old style of constructing.  You can drop the `identifier`.

> Source/WebInspectorUI/UserInterface/Views/LayersTabContentView.js:35
> +

// Static.

> Source/WebInspectorUI/UserInterface/Views/LayersTabContentView.js:52
> +    static shouldSaveTab()
> +    {
> +        return false;
> +    }

Is there a reason that we don't want to save this tab?  In that case, do we also want to hide it in the New Tab view (isEphemeral)?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170726/946f942b/attachment-0001.html>