Bug 182837 – Web Automation: support enter/exit fullscreen and hide/restore window operations

• Status: RESOLVED
• Resolution: FIXED
• Component: WebDriver

[Blaze Burg]

WebAutomationSession needs the following pieces for use in Set Window Rect and Minimize Window commands.

- unfullscreen the window

    This should be doable by using WebFullScreenManagerProxy directly and waiting for a notification or callback into the session.

- miniaturize/minimize/hide the window

   At a minimum this needs a platform method. We might need to call out to _WKAutomationSessionDelegate.

- deminiaturize/restore the window

   At a minimum this needs a platform method. We might need to call out to _WKAutomationSessionDelegate.


I think it's best to implement the Fullscreen Window command using Execute Async Script: `document.documentElement.webkitRequestFullscreen()` and use an event listener to determine when the operation succeeds or fails. We don't need a separate command for that.

[Radar WebKit Bug Importer]

<rdar://problem/37580732>

[Blaze Burg]

Created attachment 336553 [details]
Proposed Fix

[Blaze Burg]

Created attachment 336558 [details]
Proposed Fix

[EWS Watchlist]

Attachment 336558 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/Cocoa/AutomationSessionClient.mm:108:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/UIProcess/Cocoa/AutomationSessionClient.mm:122:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/UIProcess/Cocoa/AutomationSessionClient.mm:136:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 336558 [details]
Proposed Fix

Attachment 336558 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7110750

New failing tests:
http/tests/preload/download_resources.html

[EWS Watchlist]

Created attachment 336571 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[WebKit Commit Bot]

Comment on attachment 336558 [details]
Proposed Fix

Clearing flags on attachment: 336558

Committed r229998: <https://trac.webkit.org/changeset/229998>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

Comment on attachment 336558 [details]
Proposed Fix

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

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:361
> +        this->restoreWindowForPage(*page, [callback = WTFMove(callback), page = WTFMove(page), width, height, x, y]() mutable {

This is not correct. page is used and moved. This is handled by some compilers, that's why you didn't notice it, it works for me locally, but fails in the bots, so it's not even consistent among GCC versions. I've seen crashes due to this before. You need to save the reference in a local variable and pass that to restoreWindowForPage().

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:362
> +            page->getWindowFrameWithCallback([callback = WTFMove(callback), page = WTFMove(page), width, height, x, y](WebCore::FloatRect originalFrame) mutable {

And here again, in this case you need to save the ref and do pageRef.getWindowFrameWithCallback()

[Michael Catanzaro]

It probably works in some compilers because order of evaluation of the arguments is undefined.

[Blaze Burg]

Comment on attachment 336558 [details]
Proposed Fix

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

>> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:361
>> +        this->restoreWindowForPage(*page, [callback = WTFMove(callback), page = WTFMove(page), width, height, x, y]() mutable {
> 
> This is not correct. page is used and moved. This is handled by some compilers, that's why you didn't notice it, it works for me locally, but fails in the bots, so it's not even consistent among GCC versions. I've seen crashes due to this before. You need to save the reference in a local variable and pass that to restoreWindowForPage().

D'oh! nice catch. I guess I had assumed the order was well-defined, since it behaves that way with Clang.

I still have a lot to learn about async C++!