Bug 193813 - Web Inspector: provide a way to edit page settings on a remote target
Summary: Web Inspector: provide a way to edit page settings on a remote target
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
Keywords: InRadar
Depends on:
Blocks: 193862 193863
  Show dependency treegraph
Reported: 2019-01-24 22:03 PST by Devin Rousso
Modified: 2019-07-19 15:44 PDT (History)
10 users (show)

See Also:

Patch (29.00 KB, patch)
2019-01-24 22:15 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (498.91 KB, image/png)
2019-01-24 22:15 PST, Devin Rousso
no flags Details
Patch (43.43 KB, patch)
2019-01-25 16:41 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (53.86 KB, image/png)
2019-01-25 16:41 PST, Devin Rousso
no flags Details
Patch (50.37 KB, patch)
2019-01-25 18:43 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-01-24 22:03:43 PST
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.
Comment 1 Devin Rousso 2019-01-24 22:04:24 PST
Comment 2 Devin Rousso 2019-01-24 22:15:14 PST
Created attachment 360080 [details]
Comment 3 Devin Rousso 2019-01-24 22:15:29 PST
Created attachment 360081 [details]
[Image] After Patch is applied
Comment 4 Devin Rousso 2019-01-24 22:16:24 PST
(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?
Comment 5 EWS Watchlist 2019-01-24 22:18:18 PST
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment 6 Devin Rousso 2019-01-25 16:41:10 PST
Created attachment 360189 [details]

Had to fix some `WI.Popover` logic so that it can be disiplayed directly underneath the toolbar button
Comment 7 Devin Rousso 2019-01-25 16:41:34 PST
Created attachment 360190 [details]
[Image] After Patch is applied
Comment 8 Joseph Pecoraro 2019-01-25 17:38:03 PST
Comment on attachment 360189 [details]

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.

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.
Comment 9 Devin Rousso 2019-01-25 18:43:37 PST
Created attachment 360208 [details]
Comment 10 Joseph Pecoraro 2019-01-25 19:08:04 PST
Comment on attachment 360208 [details]

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!
Comment 11 WebKit Commit Bot 2019-01-25 19:57:23 PST
Comment on attachment 360208 [details]

Clearing flags on attachment: 360208

Committed r240540: <https://trac.webkit.org/changeset/240540>
Comment 12 WebKit Commit Bot 2019-01-25 19:57:25 PST
All reviewed patches have been landed.  Closing bug.