[webkit-reviews] review denied: [Bug 213242] Web Inspector: Add setScreenSizeOverride API to the Page agent : [Attachment 404702] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 213242: Web Inspector: Add setScreenSizeOverride API to the Page agent
https://bugs.webkit.org/show_bug.cgi?id=213242

Attachment 404702: Patch

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




--- Comment #23 from Brian Burg <bburg at apple.com> ---
Comment on attachment 404702
  --> https://bugs.webkit.org/attachment.cgi?id=404702
Patch

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

The code changes seem straightforward.

I don't think it's a good solution for automation. It will not actually
simulate the screen being of a different dimension on any Cocoa platform. For
example, if the size is overridden to 720p and a webview is on a left-of-center
display, screen-relative coordinates for input events will be incorrect.
Changing screen size on a mobile device seems nonsensical as well–if a device
has a different screen size this would materially affect layout, but this
approach does not affect layout or viewport sizing at all.

Perhaps none of these criticisms apply to the GTK implementation. If you wish
to proceed down this route, the patch needs to:

1) be present in WebInspectorUI
2) conditionally not shown when inspecting Cocoa targets (mac, iOS, tv, watch,
etc.)
3) conditionally not shown for older targets

Thanks for doing (1), I think it's a reasonable place to add the UI. For (2)
and (3), there needs to be a runtime check to see if the protocol method
exists.

(2) can be handled at the protocol level by adding `condition:
!(defined(WTF_PLATFORM_COCOA) && WTF_PLATFORM_COCOA)`

(3) should be a runtime check, such as `if
(InspectorBackend.hasCommand("Page.setScreenSizeOverride"))`

As a result of (2) you'll need to similarly conditionalize the backend agent
implementation and add platform-specific test expectations. It would be
clearest to add a separate ENABLE flag rather than baking WTF_PLATFORM_COCOA
into everything.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:93
> +localizedStrings["1080P"] = "1080P";

Nit: 1080p and 720p are more common.