Bug 193813 – Web Inspector: provide a way to edit page settings on a remote target

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

This includes the following items from the "Develop" menu on macOS:
 - Disable Images
 - Disable Styles
 - Disable JavaScript
 - Disable Site-specific Hacks
 - Disable Cross-Origin Restrictions

Toggling these settings from within WebInspector will only affect that page (tab), meaning that pages opened in different tabs/windows will not also have these settings applied.

Any overrides set by the frontend will be reverted when WebInspector closes/disconnects.

[Devin Rousso]

<rdar://problem/47359510>

[Devin Rousso]

Created attachment 360080 [details]
Patch

[Devin Rousso]

Created attachment 360081 [details]
[Image] After Patch is applied

[Devin Rousso]

(In reply to Devin Rousso from comment #2)
> Created attachment 360080 [details]
> Patch
One thing I'm not sure about is how to test this.  Disabling JavaScript seems like a bad idea, so maybe just toggle "Disable Styles" and check that no CSS affects any element?

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Devin Rousso]

Created attachment 360189 [details]
Patch

Had to fix some `WI.Popover` logic so that it can be disiplayed directly underneath the toolbar button

[Devin Rousso]

Created attachment 360190 [details]
[Image] After Patch is applied

[Joseph Pecoraro]

Comment on attachment 360189 [details]
Patch

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

r=me, but if you want I can review an iteration

> Source/WebInspectorUI/ChangeLog:31
> +        * UserInterface/Images/Device.svg: Added.

Nit: Missing the SVG.

> Source/JavaScriptCore/inspector/protocol/Page.json:137
> +                { "name": "value", "type": "boolean", "optional": true, "description": "Value to override the setting with. If this value is not provided, the override is removed. Overrides are removed when WebInspector closes/disconnects." }

Typo: "Web Inspector"

> Source/WebInspectorUI/UserInterface/Base/Main.js:450
> +    // if (InspectorFrontendHost.isRemote && WI.sharedApp.debuggableType === WI.DebuggableType.Web && InspectorBackend.domains.Page && InspectorBackend.domains.Page.overrideSetting) {
> +    if (WI.sharedApp.debuggableType === WI.DebuggableType.Web && InspectorBackend.domains.Page && InspectorBackend.domains.Page.overrideSetting) {

Nit: Sounds like you wanted the commented line so this only happens in remote.

> Source/WebInspectorUI/UserInterface/Base/Main.js:596
> +    if (!WI.__didPerformPageInitialization && target.PageAgent) {
> +        WI.__didPerformPageInitialization = true;
> +        for (let setting of this._overridenDeviceSettings)
> +            target.PageAgent.overrideSetting(setting, true);
> +    }

This is not the right place to initialize PageAgent state per-target. This will happen once. That means when we do a PSON navigation the next page will not have the expected setting overrides.

Scenario:
1. Go to webkit.org
2. Override a page settings
3. Navigate to apple.com
  => Expect overrides to happen

You want to put this somewhere in an `initializeTarget` like place. Currently PageAgent state happens in `Target.initialize` (for something similar) and `NetworkManager.prototype.initializeTarget`. If you want you can move that out (I think you're considering a global WI.initializeTarget, if so you'd want it in Test.js as well).

> Source/WebInspectorUI/UserInterface/Base/Main.js:1977
> +                // We're about to override to `true`, so clear the override instead of applying it.

I'd rather this say something like: "we are about to set the default value so clear the override on the backend to get default behavior"

> Source/WebInspectorUI/UserInterface/Base/Main.js:1988
> +                

Style: Remove blank line

> Source/WebInspectorUI/UserInterface/Base/Main.js:2021
> +    let targetFrame = WI.Rect.rectFromClientRect(this._deviceSettingsToolbarButton.element.getBoundingClientRect());
> +    popover.presentNewContentWithFrame(contentElement, targetFrame, [WI.RectEdge.MAX_Y, WI.RectEdge.MIN_X, WI.RectEdge.MAX_X]);

This popover needs a `windowResizeHandler` to reposition itself when the undocked window resizes.

> Source/WebInspectorUI/UserInterface/Views/Main.css:405
> +.device-settings-content .columns > .column + .column {
> +    -webkit-margin-start: 20px;
> +}

In the image, the text looks a little close to the checkboxes, but I suspect this UI will go through some iterations.

[Devin Rousso]

Created attachment 360208 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 360208 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Test/Test.js:156
> +WI.initializeTarget = function(target)
> +{
> +};

Typically we just (target) => {};
I think we did that for localizedStrings.

> LayoutTests/inspector/page/overrideSetting-expected.txt:4
> +== Running test suite: Page.overrideSetting

Nice test!

[WebKit Commit Bot]

Comment on attachment 360208 [details]
Patch

Clearing flags on attachment: 360208

Committed r240540: <https://trac.webkit.org/changeset/240540>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.