Bug 213242 – Web Inspector: Add setScreenSizeOverride API to the Page agent

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Philippe Normand]

For automation purposes it would be great to allow the page agent to override the screen size.

[Philippe Normand]

Created attachment 401993 [details]
Patch

[EWS Watchlist]

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

[Philippe Normand]

Created attachment 401994 [details]
Patch

[Philippe Normand]

Created attachment 401996 [details]
Patch

[Philippe Normand]

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?

[Philippe Normand]

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.

[Philippe Normand]

I've prototyped a web-inspector UI for this, https://cloud.igalia.com/s/HPN3etE9mMALodT

[Blaze Burg]

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?

[Blaze Burg]

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

[Philippe Normand]

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

[Philippe Normand]

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

[Philippe Normand]

Created attachment 402213 [details]
Patch

[Philippe Normand]

Created attachment 402214 [details]
Additional patch for the WebInspector UI

[Philippe Normand]

Created attachment 402454 [details]
Patch

[Philippe Normand]

Created attachment 402455 [details]
WebInspector UI addition

[Darin Adler]

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?

[Philippe Normand]

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.

[Philippe Normand]

Created attachment 402480 [details]
Patch

[Philippe Normand]

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

[Philippe Normand]

(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 :)

[Philippe Normand]

Created attachment 404702 [details]
Patch

[Philippe Normand]

Ping?

[Blaze Burg]

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.

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Created attachment 411308 [details]
Patch

Rebased and updated following Brian's suggestions.

[Darin Adler]

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.

[Devin Rousso]

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)
```

[Carlos Garcia Campos]

Created attachment 411419 [details]
Patch

[Devin Rousso]

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

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Created attachment 411741 [details]
Patch for landing

[EWS]

Committed r268716: <https://trac.webkit.org/changeset/268716>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 411741 [details].

[Radar WebKit Bug Importer]

<rdar://problem/70474647>

[Darin Adler]

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.

[Philippe Normand]

Following up with Darin's feedback in https://bugs.webkit.org/show_bug.cgi?id=218013