Bug 191659 – Web Inspector: Audit: automatically add to tab bar when the experimental setting is enabled

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

Having to enable the experimental setting, reload WebInspector, and then right-click the tab bar to access the new tab is unnecessary.  It should automatically be added when enabling the setting.

[Devin Rousso]

Created attachment 354856 [details]
Patch

[Matt Baker]

Comment on attachment 354856 [details]
Patch

Does this need to be rebased? it doesn't apply locally for me, and doesn't match trunk: https://trac.webkit.org/browser/webkit/trunk/Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js

That aside, I like the idea of automatically adding tabs when enabling the corresponding experimental setting. i wonder about the following case though:

- User enables the Layers Tab (for example)
- Layers tab is automatically added
- User hides Layers tab, leaving the setting enabled
- Reload Web Inspector

=> Layers tab will be added again

Ideally we would want to honor the user's decision to hide the tab. Maybe this isn't an issue, since these are experimental features.

[Devin Rousso]

(In reply to Matt Baker from comment #2)
> That aside, I like the idea of automatically adding tabs when enabling the corresponding experimental setting. i wonder about the following case though:
> 
> - User enables the Layers Tab (for example)
> - Layers tab is automatically added
> - User hides Layers tab, leaving the setting enabled
> - Reload Web Inspector
> 
> => Layers tab will be added again
> 
> Ideally we would want to honor the user's decision to hide the tab. Maybe
> this isn't an issue, since these are experimental features.
The tab would be hidden, as `WI.AuditTabContentView.isTabAllowed` only returns true if the experimental setting is enabled (the same is true for the layers tab as well).

[Devin Rousso]

Created attachment 355606 [details]
Patch

Rebase

[Joseph Pecoraro]

Comment on attachment 355606 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:267
> +            let newTabs = WI._openTabsSetting.value.slice();

Maybe move this to WI.settings, or public? It’s default value though uses a bunch of other classes.

[Devin Rousso]

Comment on attachment 355606 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:267
>> +            let newTabs = WI._openTabsSetting.value.slice();
> 
> Maybe move this to WI.settings, or public? It’s default value though uses a bunch of other classes.

Frankly, this is a bit of a "hack", so I'm not as concerned with the idea of public/private, not to mention the fact that if/when these are made non-experimental, the code here will disappear.

[WebKit Commit Bot]

Comment on attachment 355606 [details]
Patch

Clearing flags on attachment: 355606

Committed r238500: <https://trac.webkit.org/changeset/238500>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46248193>