Bug 178136

Summary: Web Inspector: Make 3D objects selectable in Layers visualization
Product: WebKit Reporter: Ross Kirsling <ross.kirsling>
Component: Web InspectorAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, don.olmstead, drousso, inspector-bugzilla-changes, philliphuncho, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 174176    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Ross Kirsling 2017-10-10 10:46:56 PDT
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.
Comment 1 Ross Kirsling 2017-10-10 11:03:00 PDT
Created attachment 323322 [details]
Patch
Comment 2 Devin Rousso 2017-10-10 13:00:06 PDT
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.
Comment 3 Ross Kirsling 2017-10-10 13:49:00 PDT
(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.
Comment 4 Ross Kirsling 2017-10-10 15:51:47 PDT
Created attachment 323355 [details]
Patch
Comment 5 Devin Rousso 2017-10-11 11:16:25 PDT
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"?
Comment 6 Ross Kirsling 2017-10-11 12:58:14 PDT
Created attachment 323450 [details]
Patch
Comment 7 WebKit Commit Bot 2017-10-11 13:54:59 PDT
Comment on attachment 323450 [details]
Patch

Clearing flags on attachment: 323450

Committed r223209: <https://trac.webkit.org/changeset/223209>
Comment 8 WebKit Commit Bot 2017-10-11 13:55:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2017-10-11 13:55:53 PDT
<rdar://problem/34941009>