Bug 213242 - Web Inspector: Add setScreenSizeOverride API to the Page agent
Summary: Web Inspector: Add setScreenSizeOverride API to the Page agent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-16 03:55 PDT by Philippe Normand
Modified: 2020-10-29 19:11 PDT (History)
23 users (show)

See Also:


Attachments
Patch (13.37 KB, patch)
2020-06-16 04:21 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (13.38 KB, patch)
2020-06-16 04:25 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (14.51 KB, patch)
2020-06-16 05:10 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (14.90 KB, patch)
2020-06-18 09:51 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Additional patch for the WebInspector UI (10.89 KB, patch)
2020-06-18 09:52 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (16.23 KB, patch)
2020-06-22 02:16 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
WebInspector UI addition (5.60 KB, patch)
2020-06-22 02:18 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (15.96 KB, patch)
2020-06-22 09:02 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (22.32 KB, patch)
2020-07-20 01:46 PDT, Philippe Normand
bburg: review-
Details | Formatted Diff | Diff
Patch (22.25 KB, patch)
2020-10-14 02:59 PDT, Carlos Garcia Campos
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (21.83 KB, patch)
2020-10-15 02:23 PDT, Carlos Garcia Campos
drousso: review+
drousso: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (21.69 KB, patch)
2020-10-19 05:56 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2020-06-16 03:55:56 PDT
For automation purposes it would be great to allow the page agent to override the screen size.
Comment 1 Philippe Normand 2020-06-16 04:21:38 PDT
Created attachment 401993 [details]
Patch
Comment 2 EWS Watchlist 2020-06-16 04:22:18 PDT
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment 3 Philippe Normand 2020-06-16 04:25:52 PDT
Created attachment 401994 [details]
Patch
Comment 4 Philippe Normand 2020-06-16 05:10:48 PDT
Created attachment 401996 [details]
Patch
Comment 5 Philippe Normand 2020-06-16 05:23:49 PDT
I wonder if it would make sense to expose a WebInspectorUI remote device setting about this. Something similar to the user-agent override for instance. Any thoughts?
Comment 6 Philippe Normand 2020-06-16 07:24:07 PDT
Comment on attachment 401996 [details]
Patch

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

> Source/JavaScriptCore/inspector/protocol/Page.json:314
> +                { "name": "width", "type": "integer", "description": "Screen width" },
> +                { "name": "height", "type": "integer", "description": "Screen height" }

Might be good to make those optional, so that we can reset to the non-overridden screen size.
Comment 7 Philippe Normand 2020-06-16 08:17:02 PDT
I've prototyped a web-inspector UI for this, https://cloud.igalia.com/s/HPN3etE9mMALodT
Comment 8 BJ Burg 2020-06-16 14:14:13 PDT
Hi Philippe, I don't quite understand the use case. If this is for automation, why is this needed? WKTR can resize the browser window. WebDriver allows to set window size via its API, and this affects the physical window. Neither injects overridden values for window.screen, because it doesn't materially affect layout.

Why is your approach needed?
Comment 9 BJ Burg 2020-06-16 14:14:50 PDT
(In reply to Philippe Normand from comment #7)
> I've prototyped a web-inspector UI for this,
> https://cloud.igalia.com/s/HPN3etE9mMALodT

Additionally, I don't know how to view this file. Can you please encode it using H.264 or something widely available?
Comment 10 Philippe Normand 2020-06-17 07:33:17 PDT
I've prototyped a web-inspector UI for this, https://cloud.igalia.com/s/HPN3etE9mMALodT(In reply to Brian Burg from comment #9)
> (In reply to Philippe Normand from comment #7)
> > I've prototyped a web-inspector UI for this,
> > https://cloud.igalia.com/s/HPN3etE9mMALodT
> 
> Additionally, I don't know how to view this file. Can you please encode it
> using H.264 or something widely available?

WebM is quite widely available and supported in browser nowadays. Safari is the only browser not supporting it. Anyway, here's the same video encoded in H.264... https://cloud.igalia.com/s/Rq55z855xD2ejMp
Comment 11 Philippe Normand 2020-06-18 08:48:21 PDT
(In reply to Brian Burg from comment #8)
> Hi Philippe, I don't quite understand the use case. If this is for
> automation, why is this needed? WKTR can resize the browser window.
> WebDriver allows to set window size via its API, and this affects the
> physical window. Neither injects overridden values for window.screen,
> because it doesn't materially affect layout.
> 
> Why is your approach needed?

Having a way to override the screen size would be useful for CSS media queries such as max-device-{width,height}.
Comment 12 Philippe Normand 2020-06-18 09:51:28 PDT
Created attachment 402213 [details]
Patch
Comment 13 Philippe Normand 2020-06-18 09:52:18 PDT
Created attachment 402214 [details]
Additional patch for the WebInspector UI
Comment 14 Philippe Normand 2020-06-22 02:16:42 PDT
Created attachment 402454 [details]
Patch
Comment 15 Philippe Normand 2020-06-22 02:18:07 PDT
Created attachment 402455 [details]
WebInspector UI addition
Comment 16 Darin Adler 2020-06-22 08:05:06 PDT
Comment on attachment 402454 [details]
Patch

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

> Source/WebCore/css/MediaQueryEvaluator.cpp:391
> +    auto size = frame.page()->screenSize();

What guarantees page is not null here?
Comment 17 Philippe Normand 2020-06-22 08:20:32 PDT
Comment on attachment 402454 [details]
Patch

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

>> Source/WebCore/css/MediaQueryEvaluator.cpp:391
>> +    auto size = frame.page()->screenSize();
> 
> What guarantees page is not null here?

Good point. This needs to be revisited indeed.
Comment 18 Philippe Normand 2020-06-22 09:02:52 PDT
Created attachment 402480 [details]
Patch
Comment 19 Philippe Normand 2020-07-02 09:04:04 PDT
About the win EWS failure, I suspect the issue is that there's dependency between InspectorPageAgent.cpp (with its header) and the generated DerivedSources/ForwardingHeaders/WebCore/InspectorPageAgent.h
Comment 20 Philippe Normand 2020-07-02 09:04:32 PDT
(In reply to Philippe Normand from comment #19)
> About the win EWS failure, I suspect the issue is that there's dependency
> between InspectorPageAgent.cpp (with its header) and the generated
> DerivedSources/ForwardingHeaders/WebCore/InspectorPageAgent.h

NO dependency, I mean :)
Comment 21 Philippe Normand 2020-07-20 01:46:53 PDT
Created attachment 404702 [details]
Patch
Comment 22 Philippe Normand 2020-07-28 06:21:45 PDT
Ping?
Comment 23 BJ Burg 2020-07-28 11:07:52 PDT
Comment on attachment 404702 [details]
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.
Comment 24 Carlos Garcia Campos 2020-08-26 02:15:15 PDT
(In reply to Brian Burg from comment #23)
> Comment on attachment 404702 [details]
> 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.

The same would happen in GTK indeed, the events coming from gtk will use the actual screen coordinates. Conversion to/from screen coords happens in the UI process, which is unaware of the override, again using the actual screen coordinates.

> 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.

I think this would be useful to be able to test media queries on different screen sizes, to check the layout is done correctly for every case. Maybe we can limit the screen size override to only the media queries, if that's the goal of this patch. So, something like emulateScreenSizeForMediaQueries() that doesn't change the value returned by PlatformScreen, and it's only used on the media queries impl.
Comment 25 Carlos Garcia Campos 2020-10-13 03:20:58 PDT
(In reply to Carlos Garcia Campos from comment #24)
> (In reply to Brian Burg from comment #23)
> > Comment on attachment 404702 [details]
> > 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.
> 
> The same would happen in GTK indeed, the events coming from gtk will use the
> actual screen coordinates. Conversion to/from screen coords happens in the
> UI process, which is unaware of the override, again using the actual screen
> coordinates.

After checking this again, I've realized that the patch affects Screen.cpp but not PlatformScreen.cpp, so it's only the size exposed to the DOM, not the internal screen size used for coordinate conversions. So, this shouldn't affect the events at all.

> > 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.
> 
> I think this would be useful to be able to test media queries on different
> screen sizes, to check the layout is done correctly for every case. Maybe we
> can limit the screen size override to only the media queries, if that's the
> goal of this patch. So, something like emulateScreenSizeForMediaQueries()
> that doesn't change the value returned by PlatformScreen, and it's only used
> on the media queries impl.
Comment 26 Carlos Garcia Campos 2020-10-14 02:59:58 PDT
Created attachment 411308 [details]
Patch

Rebased and updated following Brian's suggestions.
Comment 27 Darin Adler 2020-10-14 13:01:00 PDT
Comment on attachment 411308 [details]
Patch

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

> Source/WebCore/inspector/agents/InspectorPageAgent.h:124
> +#if !PLATFORM(COCOA)
> +    Inspector::Protocol::ErrorStringOr<void> setScreenSizeOverride(Optional<int>&& width, Optional<int>&& height) override;
> +#endif

This is not compiling on Windows, so it seems the conditional here is not correct.
Comment 28 Devin Rousso 2020-10-14 13:39:39 PDT
Comment on attachment 411308 [details]
Patch

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

> LayoutTests/inspector/page/setScreenSizeOverride.html:3
> +    <head>

NIT: we normally only indent the descendants of the immediate children of `<head>`/`<body>` so that there's less whitespace :)
```
    <html>
    <head>
    <script src="../../http/tests/inspector/resources/inspector-test.js"></script>
    <style>
    @media only screen and (max-device-width: 1000px) {
        body {
    ... 
```

> LayoutTests/inspector/page/setScreenSizeOverride.html:6
> +         @media only screen and (max-device-width: 1000px) {

extra leading space

> LayoutTests/inspector/page/setScreenSizeOverride.html:8
> +                 color: lightblue;

perhaps use a simpler color (e.g. `red`) so that it's easier to compare below?

> LayoutTests/inspector/page/setScreenSizeOverride.html:13
> +         function test()

ditto (:6)

> LayoutTests/inspector/page/setScreenSizeOverride.html:23
> +                     InspectorTest.log("Current screen size: " + originalScreenWidth + "x" + originalScreenHeight);

Rather than log the original screen size (which could change depending on the test machine), perhaps just save those values and compare them with the new ones to make sure they've changed?
```
    InspectorTest.expectNotEqual(newScreenWidth, originalScreenWidth, "Should change screen width.");
```

> LayoutTests/inspector/page/setScreenSizeOverride.html:26
> +                     InspectorTest.expectEqual(currentBackgroundColor, "rgb(0, 0, 0)", "Body should be white");

Style: expectation comments should be sentences, so please include an ending "." :)

> LayoutTests/inspector/page/setScreenSizeOverride.html:38
> +

Style: unnecessary newline

> LayoutTests/inspector/page/setScreenSizeOverride.html:52
> +             WI.domManager.requestDocument((node) => {

Why is this needed?

> Source/JavaScriptCore/inspector/protocol/Page.json:310
> +            "description": "Overrides screen size with provided values.",

Please be more specific about what you mean by "screen" here (this appears to only affect `window.screen` and device media queries, not anything in the UIProcess).  Maybe it should have a more specific name if it really only does override things exposed to JavaScript/CSS 🤔

> Source/WebCore/css/MediaQueryEvaluator.cpp:393
> -    auto size = screenRect(frame.mainFrame().view()).size();
> +    auto size = frame.screenSize();

Why was the `.mainFrame()` removed?

> Source/WebCore/inspector/agents/InspectorPageAgent.h:123
> +    Inspector::Protocol::ErrorStringOr<void> setScreenSizeOverride(Optional<int>&& width, Optional<int>&& height) override;

NIT: the `override` isn't necessary since this class is `final`

>> Source/WebCore/inspector/agents/InspectorPageAgent.h:124
>> +#endif
> 
> This is not compiling on Windows, so it seems the conditional here is not correct.

The windows bot sometimes has issues regenerating the backend dispatcher classes (in this case `PageBackendDispatcherHandler`) from the protocol JSON.  I think this is probably fine.

> Source/WebCore/page/Frame.h:379
> +    Optional<int> m_overrideScreenWidth;
> +    Optional<int> m_overrideScreenHeight;

Rather than use two `Optional<int>`, why not use an `IntSize`/`FloatSize`?  You could use `isEmpty` as the "not set" value.

> Source/WebInspectorUI/UserInterface/Base/Main.js:2217
> +                        console.error(error);

please use `WI.reportInternalError` so that this is surfaced in engineering builds, thereby making it more likely that issues are reported

> Source/WebInspectorUI/UserInterface/Base/Main.js:2231
> +                        console.error(error);

ditto (:2217)

> Source/WebInspectorUI/UserInterface/Base/Main.js:2257
> +                { name: WI.UIString("Default"), value: "default" },

Style: we normally don't add leading/trailing spaces for inline objects/arrays

> Source/WebInspectorUI/UserInterface/Base/Main.js:2290
> +            let defaultSize = window.screen.width + "x" + window.screen.height;

I'm not sure if an equivalent concept exists outside of iOS devices (<https://webkit.org/web-inspector/enabling-web-inspector/#ios-device>), but this won't work for remote device inspection :(

NIT: i'd just inline this
```
    WI._overridenDeviceScreenSize || (window.screen.width + "x" + window.screen.height)
```
Comment 29 Carlos Garcia Campos 2020-10-15 02:23:50 PDT
Created attachment 411419 [details]
Patch
Comment 30 Devin Rousso 2020-10-16 16:04:36 PDT
Comment on attachment 411419 [details]
Patch

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

r=me with a few fixes

> Source/WebCore/page/Frame.cpp:1069
> +    if (!m_overrideScreenSize.isZero())

I think `isEmpty` is probably enough (and simpler), but I don't have a strong preference.

> Source/WebCore/page/Frame.h:380
> +    Optional<int> m_overrideScreenWidth;
> +    Optional<int> m_overrideScreenHeight;

these don't appear to be necessary anymore

> Source/WebInspectorUI/UserInterface/Base/Main.js:2290
> +            screenSizeValueInput.value = screenSizeValueInput.placeholder = WI._overridenDeviceScreenSize || window.screen.width + "x" + window.screen.height;

NIT: I'd add `(` `)` around the stuff after the `||` so it's explicitly clear
Comment 31 Carlos Garcia Campos 2020-10-19 05:54:15 PDT
(In reply to Devin Rousso from comment #30)
> Comment on attachment 411419 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411419&action=review
> 
> r=me with a few fixes
> 
> > Source/WebCore/page/Frame.cpp:1069
> > +    if (!m_overrideScreenSize.isZero())
> 
> I think `isEmpty` is probably enough (and simpler), but I don't have a
> strong preference.

Ok, me neither.

> > Source/WebCore/page/Frame.h:380
> > +    Optional<int> m_overrideScreenWidth;
> > +    Optional<int> m_overrideScreenHeight;
> 
> these don't appear to be necessary anymore

Right, I forgot to remove them.

> > Source/WebInspectorUI/UserInterface/Base/Main.js:2290
> > +            screenSizeValueInput.value = screenSizeValueInput.placeholder = WI._overridenDeviceScreenSize || window.screen.width + "x" + window.screen.height;
> 
> NIT: I'd add `(` `)` around the stuff after the `||` so it's explicitly clear

Ok.
Comment 32 Carlos Garcia Campos 2020-10-19 05:56:09 PDT
Created attachment 411741 [details]
Patch for landing
Comment 33 EWS 2020-10-20 01:00:59 PDT
Committed r268716: <https://trac.webkit.org/changeset/268716>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411741 [details].
Comment 34 Radar WebKit Bug Importer 2020-10-20 01:01:23 PDT
<rdar://problem/70474647>
Comment 35 Darin Adler 2020-10-20 11:39:37 PDT
Comment on attachment 411741 [details]
Patch for landing

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

> Source/WebCore/page/Screen.cpp:61
> -    long height = static_cast<long>(screenRect(frame->view()).height());
> +    long height = static_cast<long>(frame->screenSize().height());
>      return static_cast<unsigned>(height);

I don’t understand the value of converting float to *long* and then to unsigned. Why is long involved?

> Source/WebCore/page/Screen.cpp:72
> -    long width = static_cast<long>(screenRect(frame->view()).width());
> +    long width = static_cast<long>(frame->screenSize().width());
>      return static_cast<unsigned>(width);

Ditto.
Comment 36 Philippe Normand 2020-10-21 02:41:20 PDT
Following up with Darin's feedback in https://bugs.webkit.org/show_bug.cgi?id=218013