Bug 224082 – Web Inspector: add setting to allow inspecting Web Inspector

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

[Blaze Burg]

This broke in Catalina releases due to changes in Safari's storage model. Let's put it in the settings UI in case it's needed.

We could additionally restrict this to STP, but what's the point?

[Blaze Burg]

<rdar://75695002>

[Blaze Burg]

Created attachment 424957 [details]
Patch v1.0

[Devin Rousso]

Comment on attachment 424957 [details]
Patch v1.0

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

> Source/WebCore/en.lproj/Localizable.strings:152
> +/* Message for geolocation prompt */
> +"Allow â%@â to use your current location?" = "Allow â%@â to use your current location?";

While I do appreciate this, is it strictly speaking necessary in this patch?  Could you just do an unreviewed cleanup fix followup instead?

> Source/WebCore/inspector/InspectorFrontendHost.cpp:583
> +    return m_frontendPage ? m_frontendPage->settings().developerExtrasEnabled() : false;

```
    return m_frontendPage && m_frontendPage->settings().developerExtrasEnabled();
```

> Source/WebCore/inspector/InspectorFrontendHost.idl:96
> +    boolean allowsInspectingInspector();
> +    undefined setAllowsInspectingInspector(boolean allow);

Why not just have `attribute boolean allowsInspectingInspector`?

Also, is it ever actually read anywhere in the frontend?

> Source/WebInspectorUI/UserInterface/Base/Main.js:166
> +//    InspectorFrontendHost.setAllowsInspectingInspector(WI.settings.experimentalAllowInspectingInspector.value);

oops

> Source/WebInspectorUI/UserInterface/Base/Setting.js:232
> +    experimentalAllowInspectingInspector: new WI.Setting("experimental-allow-inspecting-inspector", false),

Is it really necessary to have a `WI.ExperimentalSetting` for this when `DeveloperExtrasEnabled` is already a setting saved on disk?

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:411
> +        let diagnosticsGroup = experimentalSettingsView.addGroup(WI.UIString("Diagnostics:", "Diagnostics: @ Experimental Settings", "Category label for experimental settings related to Web Inspector diagnostics."), WI.settings.experimentalCollapseBlackboxedCallFrames, WI.UIString("Collapse blackboxed call frames", "Collapse blackboxed call frames @ Experimental Settings", "Setting to collapse blackboxed call frames in the debugger."));

Why is `WI.settings.experimentalCollapseBlackboxedCallFrames` related to this?  Or was this a copypasta mistake?  :P

[Patrick Angle]

Comment on attachment 424957 [details]
Patch v1.0

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

> Source/WebCore/ChangeLog:12
> +        * en.lproj/Localizable.strings:

Should probably add a comment here that this is a drive-by change and not related to this patch. I was confused how location permission was related  to Inspector^2 at first.

> Source/WebInspectorUI/UserInterface/Base/Main.js:166
> +//    InspectorFrontendHost.setAllowsInspectingInspector(WI.settings.experimentalAllowInspectingInspector.value);

Oops?

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:412
> +        diagnosticsGroup.addSetting(WI.settings.experimentalAllowInspectingInspector, WI.UIString("Allow Inspecting Web Inspector", "Label for setting that allows the user to inspect the Web Inspector user interface."));

I think the use of the two-argument version of this has fallen out of favor (bug 215400) and that we prefer to explicitly provide a key each string.

[Blaze Burg]

Comment on attachment 424957 [details]
Patch v1.0

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

Apologies for the sloppiness in this patch. New one coming up.

>> Source/WebCore/ChangeLog:12
>> +        * en.lproj/Localizable.strings:
> 
> Should probably add a comment here that this is a drive-by change and not related to this patch. I was confused how location permission was related  to Inspector^2 at first.

This was included by accident, I ran the localized strings extractor over the whole codebase apparently.

>> Source/WebInspectorUI/UserInterface/Base/Setting.js:232
>> +    experimentalAllowInspectingInspector: new WI.Setting("experimental-allow-inspecting-inspector", false),
> 
> Is it really necessary to have a `WI.ExperimentalSetting` for this when `DeveloperExtrasEnabled` is already a setting saved on disk?

DeveloperExtrasEnabled can't be set for containerized apps, and having it set the global domain does not necessarily mean WebCore will be able to read it.

[Blaze Burg]

Created attachment 425945 [details]
Patch v1.1

[Devin Rousso]

Comment on attachment 425945 [details]
Patch v1.1

r=me

[EWS]

commit-queue failed to commit attachment 425945 [details] to WebKit repository. To retry, please set cq+ flag again.

[Blaze Burg]

Committed r275982 (236534@main): <https://commits.webkit.org/236534@main>