<rdar://problem/47359510>

Created attachment 360080 [details]
Patch

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

(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?

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

Created attachment 360189 [details]
Patch

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

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

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.

Created attachment 360208 [details]
Patch

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!

Comment on attachment 360208 [details]
Patch

Clearing flags on attachment: 360208

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

All reviewed patches have been landed.  Closing bug.