[webkit-reviews] review granted: [Bug 193813] Web Inspector: provide a way to edit page settings on a remote target : [Attachment 360189] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 193813: Web Inspector: provide a way to edit page settings on a remote
target
https://bugs.webkit.org/show_bug.cgi?id=193813

Attachment 360189: Patch

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




--- Comment #8 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 360189
  --> https://bugs.webkit.org/attachment.cgi?id=360189
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.getBoundin
gClientRect());
> +    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.