Bug 179211 – Web Inspector: Move Show Compositing Borders/Paint Flashing buttons from Elements tab to Layers tab

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Ross Kirsling]

As the new home for compositing layer-related information, the nav bar toggle buttons for the Compositing Borders and Paint Flashing page overlays should be moved to the Layers tab.

[Ross Kirsling]

Created attachment 325930 [details]
Patch

[Joseph Pecoraro]

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

[Ross Kirsling]

Created attachment 325941 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 325941 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 325941 [details]
Patch

Clearing flags on attachment: 325941

Committed r224434: <https://trac.webkit.org/changeset/224434>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35567364>