[webkit-reviews] review granted: [Bug 178966] Web Inspector: Improve UX of Layers tab visualization : [Attachment 325372] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <webkit at devinrousso.com> has granted Ross Kirsling
<ross.kirsling at sony.com>'s request for review:
Bug 178966: Web Inspector: Improve UX of Layers tab visualization
https://bugs.webkit.org/show_bug.cgi?id=178966

Attachment 325372: Patch

https://bugs.webkit.org/attachment.cgi?id=325372&action=review




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

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

r=me.  This feels a LOT better =D

My only negative is that when you select a layer, in addition to moving the
camera to center on the layer, it also resets the angle.  I would personally
not expect that to happen, and found it annoying, especially on layer-heavy
pages where it isn't always obvious how the layers interact.

> Source/WebInspectorUI/UserInterface/Views/LayerDetailsSidebarPanel.js:70
> +	   const suppressEvent = true;

NIT: use the actual name `suppressSelectEvent`

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:113
> +	   this._controls.minPolarAngle = Math.PI / 2;
> +	   this._controls.maxPolarAngle = Math.PI / 2;

I think the user should also be able to pan the Y axis in addition to the X. 
If my page is very long (lots of scrolling), I might want to see what the
layers look like for items down the page while still scrolled to the top.  It
seems oddly restrictive to allow horizontal view changes, but not vertical
ones, especially since we often move vertically when changing layer selection.

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:116
> +	   this._renderer.domElement.addEventListener("contextmenu", (event) =>
{ event.stopPropagation(); });

Is there a specific reason we want to prevent contextmenu?

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:313
> +	   this._controls.target.set(x + width / 2, -y - height / 2, 0);
> +	   this._camera.position.set(x + width / 2, -y - height / 2,
this._selectedLayerGroup.position.z + WI.Layers3DContentView._zPadding / 2);

Style: we typically wrap different arithmetic operations in parenthesis for
clarity.

this._controls.target.set(x + (width / 2), -y - (height / 2), 0);
this._camera.position.set(x + (width / 2), -y - (height / 2),
this._selectedLayerGroup.position.z + (WI.Layers3DContentView._zPadding / 2));

> Source/WebInspectorUI/UserInterface/Views/Layers3DContentView.js:320
> +	   this._controls.target.set(x + width / 2, -y - height / 2, 0);
> +	   this._camera.position.set(x + width / 2, -y - height / 2,
this._controls.maxDistance - WI.Layers3DContentView._zPadding / 2);

Ditto (312).