[Webkit-unassigned] [Bug 178136] Web Inspector: Make 3D objects selectable in Layers visualization

[bugzilla-daemon at webkit.org]

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

Devin Rousso <webkit at devinrousso.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |webkit at devinrousso.com

--- Comment #2 from Devin Rousso <webkit at devinrousso.com> ---
Comment on attachment 323322
  --> https://bugs.webkit.org/attachment.cgi?id=323322
Patch

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

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:44
> +        this._fillColor = "hsl(76, 49%, 75%)";
> +        this._strokeColor = "hsl(79, 45%, 50%)";
> +        this._selectedFillColor = "hsl(208, 66%, 79%)";
> +        this._selectedStrokeColor = "hsl(202, 57%, 68%)";

Since these are shared by all Layers3DContentView, you could make them static (put them at the end of the file).

    WI.Layers3DContentView._fillColor = "hsl(76, 49%, 75%)";
    WI.Layers3DContentView._strokeColor = "hsl(79, 45%, 50%)";
    WI.Layers3DContentView._selectedFillColor = "hsl(208, 66%, 79%)";
    WI.Layers3DContentView._selectedStrokeColor = "hsl(202, 57%, 68%)";

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:92
> +        let layerGroup = this._layerGroupsById.get(layerId);

Is it ever possible for a layerId to be given that isn't valid?  Maybe add a console.assert?

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:181
> +    _updateLayers(previousLayers)

This is very weird.  I would expect an "update" function to pass in the new value, and internally do it's own updating.

    _updateLayers(newLayers)
    {
        let {removals, additions, preserved} = WI.layerTreeManager.layerTreeMutations(this._layers, newLayers);

        ...

        this._layers = newLayers;
    }

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:190
> +            if (!layerGroup)
> +                return;

Will this ever get hit?  I think it should be considered an "error" if we try to remove something that we didn't previously have, as we shouldn't have missed any layers.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:200
> +        additions.forEach(this._addLayerGroup, this);
> +        preserved.forEach(this._updateLayerGroupPosition, this);

Nice!

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:219
> +        if (!layerGroup)
> +            return;

Ditto (189).

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:257
> +        let recursive = true;

Style: should be const or inlined.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:274
> +            let [plane, outline] = this._selectedLayerGroup.children;
> +            plane.material.color.set(this._fillColor);
> +            outline.material.color.set(this._strokeColor);

NIT: you could make this into a local function if you want.

    let changeLayerColors = (fillColor, strokeColor) => {
        let [plane, outline] = this._selectedLayerGroup.children;
        plane.material.color.set(fillColor);
        outline.material.color.set(strokeColor);
    };

    changeLayerColors(WI.Layers3DContentView._fillColor, WI.Layers3DContentView._strokeColor);

> Source/WebInspectorUI/UserInterface/Views/LayersTabContentView.js:88
> +    SelectedLayerChanged: Symbol("selected-layer-changed")

This event is dispatched by LayersDetailsSidebarPanel and Layers3DContentView.  It shouldn't be an event on LayersTabContentView, and each of the two dispatchers should instead dispatch their own event.  Also, we typically don't use Symbol for event listeners.

-- 
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/20171010/49c7d123/attachment-0001.html>