Bug 178136 – Web Inspector: Make 3D objects selectable in Layers visualization

• Component: Web Inspector
• Status: RESOLVED FIXED

[Ross Kirsling]

Implement selection behavior for the 3D objects in the Layers tab visualization.
This selection state should be properly synced with the data grid in the sidebar.

[Ross Kirsling]

Created attachment 323322 [details]
Patch

[Devin Rousso]

Comment on attachment 323322 [details]
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.

[Ross Kirsling]

(In reply to Devin Rousso from comment #2)
> > 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?

Map#get returns undefined for *anything* not found so layerGroup is always either a THREE.Group or undefined.

I originally had
`let layerGroup = layerId ? this._layerGroupsById.get(layerId) : null;`
here but removed it because it adds nothing functionality-wise. I can put that back if you think it's more readable.

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

This approach was based on existing code in the sidebar. I can update both.

> > 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:219
> > +        if (!layerGroup)
> > +            return;
> 
> Ditto (189).

Good call. This is defensive coding based on the type of Map#get and comes from the analogous code in the sidebar, but I suppose it would be good to call an error an error here. I can update both.

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

I had it that way first but couldn't decide which was more idiomatic for WI. As for the Symbol, I saw it both ways and thought the Symbol approach might be the newer--doesn't matter to me otherwise.

[Ross Kirsling]

Created attachment 323355 [details]
Patch

[Devin Rousso]

Comment on attachment 323355 [details]
Patch

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

r=me, with a few remaining NITs.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:117
> +        this._renderer.domElement.addEventListener("mousedown", this._canvasMouseDown.bind(this), false);

NIT: the `false` is unnecessary.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:281
> +WI.Layers3DContentView._unselectedColor = {

The word "unselect" is very weird.  Maybe just use "_color" and "_selectedColor"?

[Ross Kirsling]

Created attachment 323450 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 323450 [details]
Patch

Clearing flags on attachment: 323450

Committed r223209: <https://trac.webkit.org/changeset/223209>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34941009>