[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

--- Comment #3 from Ross Kirsling <ross.kirsling at sony.com> ---
(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.

-- 
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/9e9ee164/attachment.html>