Bug 182749 - Web Automation: combine session commands to resize and move top-level browsing contexts
Summary: Web Automation: combine session commands to resize and move top-level browsin...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebDriver (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-13 15:06 PST by BJ Burg
Modified: 2018-02-27 12:23 PST (History)
12 users (show)

See Also:


Attachments
Proposed Fix (16.24 KB, patch)
2018-02-13 17:24 PST, BJ Burg
no flags Details | Formatted Diff | Diff
Updated patch (19.76 KB, patch)
2018-02-13 23:52 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch v2 (20.37 KB, patch)
2018-02-14 11:30 PST, BJ Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2018-02-13 15:06:49 PST
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.
Comment 1 Radar WebKit Bug Importer 2018-02-13 15:07:07 PST
<rdar://problem/37515170>
Comment 2 BJ Burg 2018-02-13 17:24:34 PST
Created attachment 333752 [details]
Proposed Fix

Self-reminder to land this manually with Internal changes.
Comment 3 Carlos Garcia Campos 2018-02-13 23:49:10 PST
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.
Comment 4 Carlos Garcia Campos 2018-02-13 23:52:45 PST
Created attachment 333772 [details]
Updated patch

I've only changed the WebDriver implementation to send a single command from setWindowRect().
Comment 5 BJ Burg 2018-02-14 09:02:08 PST
(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.
Comment 6 BJ Burg 2018-02-14 10:12:58 PST
Comment on attachment 333772 [details]
Updated patch

Marking this obsolete, adding a revised patch with Carlos' changes and suggestions.
Comment 7 BJ Burg 2018-02-14 11:30:53 PST
Created attachment 333824 [details]
Patch v2
Comment 8 Andy Estes 2018-02-14 14:06:22 PST
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.
Comment 9 Carlos Garcia Campos 2018-02-14 22:29:59 PST
(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.
Comment 10 Carlos Garcia Campos 2018-02-14 22:33:10 PST
Comment on attachment 333824 [details]
Patch v2

LGTM, thanks!
Comment 11 BJ Burg 2018-02-15 13:01:20 PST
(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.
Comment 12 BJ Burg 2018-02-20 16:43:56 PST
(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.
Comment 13 BJ Burg 2018-02-20 20:13:49 PST
Committed r228856: <https://trac.webkit.org/changeset/228856>