[webkit-reviews] review denied: [Bug 179211] Web Inspector: Move Show Compositing Borders/Paint Flashing buttons from Elements tab to Layers tab : [Attachment 325930] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Ross Kirsling
<ross.kirsling at sony.com>'s request for review:
Bug 179211: Web Inspector: Move Show Compositing Borders/Paint Flashing buttons
from Elements tab to Layers tab
https://bugs.webkit.org/show_bug.cgi?id=179211

Attachment 325930: Patch

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




--- Comment #2 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 325930
  --> https://bugs.webkit.org/attachment.cgi?id=325930
Patch

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

r- just because I want to see an updated patch. The Layers ContentView changes
look good to me.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:34
> +	   if (!WI.settings.experimentalEnableLayersTab.value) {

Sometimes, to make things less error prone, we just always create the
properties and only make them show up in the UI based on settings.

This makes the code more resilient (someone might try to use
this._compositingBordersButtonNavigationItem and forget that its only sometimes
available).

For example here you probably want to listen for a change in the
WI.settings.experimentalEnableLayersTab.value setting value and dispatch an
update to our navigation items. (up to you really, but that would be the most
appropriate thing to do).

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:136
> +	   if (!WI.settings.experimentalEnableLayersTab.value)
> +	       this._updateCompositingBordersButtonToMatchPageSettings();

Regardless of what you decide to above, you should put the bail inside of
_updateCompositingBordersButtonToMatchPageSettings, not at the callsite,
because if someone adds a new callsite they might forget to add the check.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeContentView.js:157
> +	   if (!WI.settings.experimentalEnableLayersTab.value)
> +	       WI.showPaintRectsSetting.removeEventListener(null, null, this);

Here is a case where unambiguously adding an event listener and removing an
event listener would be a win. If the setting's value changes throughout the
lifetime of this view being opened / closed, then we can have an add without a
remove, or a remove without an add.