Bug 182837 - Web Automation: support enter/exit fullscreen and hide/restore window operations
Summary: Web Automation: support enter/exit fullscreen and hide/restore window operations
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: 180398
  Show dependency treegraph
 
Reported: 2018-02-15 13:15 PST by BJ Burg
Modified: 2018-03-28 09:29 PDT (History)
12 users (show)

See Also:


Attachments
Proposed Fix (20.41 KB, patch)
2018-03-26 16:45 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Proposed Fix (40.41 KB, patch)
2018-03-26 17:25 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.15 MB, application/zip)
2018-03-27 01:41 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2018-02-15 13:15:10 PST
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.
Comment 1 Radar WebKit Bug Importer 2018-02-15 13:15:41 PST
<rdar://problem/37580732>
Comment 2 BJ Burg 2018-03-26 16:45:17 PDT
Created attachment 336553 [details]
Proposed Fix
Comment 3 BJ Burg 2018-03-26 17:25:09 PDT
Created attachment 336558 [details]
Proposed Fix
Comment 4 EWS Watchlist 2018-03-26 17:26:12 PDT
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.
Comment 5 EWS Watchlist 2018-03-27 01:41:29 PDT
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
Comment 6 EWS Watchlist 2018-03-27 01:41:40 PDT
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
Comment 7 WebKit Commit Bot 2018-03-27 09:37:49 PDT
Comment on attachment 336558 [details]
Proposed Fix

Clearing flags on attachment: 336558

Committed r229998: <https://trac.webkit.org/changeset/229998>
Comment 8 WebKit Commit Bot 2018-03-27 09:37:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Carlos Garcia Campos 2018-03-28 02:53:59 PDT
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()
Comment 10 Michael Catanzaro 2018-03-28 08:02:37 PDT
It probably works in some compilers because order of evaluation of the arguments is undefined.
Comment 11 BJ Burg 2018-03-28 09:29:46 PDT
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++!