Bug 182749 – Web Automation: combine session commands to resize and move top-level browsing contexts

• Component: WebDriver
• Status: RESOLVED FIXED

[Blaze Burg]

In the W3C dialect, this is the same endpoint (Set Window Rect). There's no point having two separate commands as they both set the window frame under the hood.

[Radar WebKit Bug Importer]

<rdar://problem/37515170>

[Blaze Burg]

Created attachment 333752 [details]
Proposed Fix

Self-reminder to land this manually with Internal changes.

[Carlos Garcia Campos]

Comment on attachment 333752 [details]
Proposed Fix

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

> Source/WebDriver/Session.cpp:726
> -    m_host->sendCommandToBackend(ASCIILiteral("moveWindowOfBrowsingContext"), WTFMove(parameters), [this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)](SessionHost::CommandResponse&& response) {
> +    m_host->sendCommandToBackend(ASCIILiteral("setWindowFrameOfBrowsingContext"), WTFMove(parameters), [this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)](SessionHost::CommandResponse&& response) {

We no longer need these method, we can simplify the whole thing by removing moveToplevelBrowsingContextWindow and resizeToplevelBrowsingContextWindow. then we can simply send a single message from setWindowRect directly.

> Source/WebKit/UIProcess/Automation/Automation.json:293
> +            "description": "Moves and/or resizes the window of the specified top-level browsing context to the specified size and origin. Either the size or origin may be omitted, but not both.",

Both can be omitted according to the spec, there are more actions to be performed by setWindowRect like exiting fullscreen and restoring the window, those should happen even if position and size doesn't don't change.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:324
> +    if (!optionalOriginObject && !optionalSizeObject)
> +        FAIL_WITH_PREDEFINED_ERROR_AND_DETAILS(MissingParameter, "Either size or origin must be specified.");

This breaks all imported/w3c/webdriver/tests/set_window_rect.py::test_no_change tests. We could add FIXMEs below for the additional actions, exiting fullscreen and restoring the window, covered by imported/w3c/webdriver/tests/set_window_rect.py::test_fully_exit_fullscreen, imported/w3c/webdriver/tests/set_window_rect.py::test_restore_from_minimized and imported/w3c/webdriver/tests/set_window_rect.py::test_restore_from_maximized currently marked as expected failures, because we don't implement fullscreen/maximize/minimize yet.

[Carlos Garcia Campos]

Created attachment 333772 [details]
Updated patch

I've only changed the WebDriver implementation to send a single command from setWindowRect().

[Blaze Burg]

(In reply to Carlos Garcia Campos from comment #3)
> Comment on attachment 333752 [details]
> Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333752&action=review
> 
> > Source/WebDriver/Session.cpp:726
> > -    m_host->sendCommandToBackend(ASCIILiteral("moveWindowOfBrowsingContext"), WTFMove(parameters), [this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)](SessionHost::CommandResponse&& response) {
> > +    m_host->sendCommandToBackend(ASCIILiteral("setWindowFrameOfBrowsingContext"), WTFMove(parameters), [this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)](SessionHost::CommandResponse&& response) {
> 
> We no longer need these method, we can simplify the whole thing by removing
> moveToplevelBrowsingContextWindow and resizeToplevelBrowsingContextWindow.
> then we can simply send a single message from setWindowRect directly.

Good point. I was trying to minimize changes since I can't easily build GTK right now.

> 
> > Source/WebKit/UIProcess/Automation/Automation.json:293
> > +            "description": "Moves and/or resizes the window of the specified top-level browsing context to the specified size and origin. Either the size or origin may be omitted, but not both.",
> 
> Both can be omitted according to the spec, there are more actions to be
> performed by setWindowRect like exiting fullscreen and restoring the window,
> those should happen even if position and size doesn't don't change.
> 
> > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:324
> > +    if (!optionalOriginObject && !optionalSizeObject)
> > +        FAIL_WITH_PREDEFINED_ERROR_AND_DETAILS(MissingParameter, "Either size or origin must be specified.");
> 
> This breaks all
> imported/w3c/webdriver/tests/set_window_rect.py::test_no_change tests. We
> could add FIXMEs below for the additional actions, exiting fullscreen and
> restoring the window, covered by
> imported/w3c/webdriver/tests/set_window_rect.py::test_fully_exit_fullscreen,
> imported/w3c/webdriver/tests/set_window_rect.py::test_restore_from_minimized
> and
> imported/w3c/webdriver/tests/set_window_rect.py::test_restore_from_maximized
> currently marked as expected failures, because we don't implement
> fullscreen/maximize/minimize yet.


That's a good point, and I think we should support that use case (running Set Window Rect just for side effects). I'll remove that branch and update the protocol definition accordingly.

I'll also add FIXMEs. I'm in the process of figuring out how to implement minimize/maximize/fullscreen in Cocoa ports, but it's likely that we'll want the WebKit-side session to take care of un-fullscreening and de-miniaturizing where appropriate.

[Blaze Burg]

Comment on attachment 333772 [details]
Updated patch

Marking this obsolete, adding a revised patch with Carlos' changes and suggestions.

[Blaze Burg]

Created attachment 333824 [details]
Patch v2

[Andy Estes]

Comment on attachment 333824 [details]
Patch v2

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

> Source/WebDriver/Session.cpp:747
> +        RefPtr<JSON::Object> parameters = JSON::Object::create();

I'd use auto here, unless you really need this to be a RefPtr (maybe sendCommandToBackend() needs a copyable lambda?).

> Source/WebDriver/Session.cpp:750
> +            RefPtr<JSON::Object> windowOrigin = JSON::Object::create();

Ditto.

[Carlos Garcia Campos]

(In reply to Brian Burg from comment #5)
[...] 
> I'll also add FIXMEs. I'm in the process of figuring out how to implement
> minimize/maximize/fullscreen in Cocoa ports, but it's likely that we'll want
> the WebKit-side session to take care of un-fullscreening and
> de-miniaturizing where appropriate.

Maybe we can simply add a new message setWindowStateOfBrowsingContext that receives either fullscreen, maximize or minimize. In the WebKit side we just need platform hooks.

[Carlos Garcia Campos]

Comment on attachment 333824 [details]
Patch v2

LGTM, thanks!

[Blaze Burg]

(In reply to Carlos Garcia Campos from comment #9)
> (In reply to Brian Burg from comment #5)
> [...] 
> > I'll also add FIXMEs. I'm in the process of figuring out how to implement
> > minimize/maximize/fullscreen in Cocoa ports, but it's likely that we'll want
> > the WebKit-side session to take care of un-fullscreening and
> > de-miniaturizing where appropriate.
> 
> Maybe we can simply add a new message setWindowStateOfBrowsingContext that
> receives either fullscreen, maximize or minimize. In the WebKit side we just
> need platform hooks.

I'm going to hook this up to safaridriver and experiment to see what's natural. Let's use a different bug.

[Blaze Burg]

(In reply to Carlos Garcia Campos from comment #9)
> (In reply to Brian Burg from comment #5)
> [...] 
> > I'll also add FIXMEs. I'm in the process of figuring out how to implement
> > minimize/maximize/fullscreen in Cocoa ports, but it's likely that we'll want
> > the WebKit-side session to take care of un-fullscreening and
> > de-miniaturizing where appropriate.
> 
> Maybe we can simply add a new message setWindowStateOfBrowsingContext that
> receives either fullscreen, maximize or minimize. In the WebKit side we just
> need platform hooks.

The current state of things is that we can fullscreen/unfullscreen purely through WebFullScreenManagerProxy and a JS atom. Minimize/maximize will need an AutomationClient delegate method for both operations. And, the automation command to set the window rect is sufficient for internal purposes when we are asked to unfullscreen + restore the window.

[Blaze Burg]

Committed r228856: <https://trac.webkit.org/changeset/228856>