Bug 180398 – WebDriver: implement maximize, minimize and fullscreen window commands

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

[Carlos Garcia Campos]

https://w3c.github.io/webdriver/webdriver-spec.html#maximize-window
https://w3c.github.io/webdriver/webdriver-spec.html#minimize-window
https://w3c.github.io/webdriver/webdriver-spec.html#fullscreen-window

[Carlos Garcia Campos]

Created attachment 340572 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebDriver/Session.h:93:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.h:94:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.h:95:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/WebDriverService.cpp:974:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/WebDriverService.cpp:982:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/WebDriverService.cpp:990:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/WebDriverService.h:82:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/WebDriverService.h:83:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/WebDriverService.h:84:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.cpp:777:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.cpp:802:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.cpp:827:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 12 in 19 files


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

[Carlos Garcia Campos]

EWS failure seems to be an icecc issue.

ICECC[11888] 04:48:37: flush_writebuf() failed Broken pipe
ICECC[11888] 04:48:37: no server found behind given hostname 127.0.0.1:0
ICECC[11888] 04:48:37: got exception Error 2 - no server found at 127.0.0.1 (127.0.0.1) 
[4727/5056] Building CXX object Source/WebKit/CMakeFiles/WebKit.dir/UIProcess/API/glib/WebKitWebContext.cpp.o
ICECC[11820] 04:48:36: got exception Error 14 - error reading message from remote (192.168.10.107) 
ninja: build stopped: subcommand failed.

[Carlos Garcia Campos]

Created attachment 340956 [details]
Try to fix iOS builds

[EWS Watchlist]

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


ERROR: Source/WebDriver/Session.h:93:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.h:94:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.h:95:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/WebDriverService.cpp:974:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/WebDriverService.cpp:982:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/WebDriverService.cpp:990:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/WebDriverService.h:82:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/WebDriverService.h:83:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/WebDriverService.h:84:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.cpp:777:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.cpp:802:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.cpp:827:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 12 in 19 files


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

[Blaze Burg]

Comment on attachment 340956 [details]
Try to fix iOS builds

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

r=me except GObject parts in WebKitWebViewGTK.cpp, which I do not understand.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp:118
> +        completeTimer.startOneShot(1_s);

Seems like a good idea, though I've never had it flake out on macOS yet.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp:190
> +#if ENABLE(DEVELOPER_MODE)

Why is this part guarded?

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp:196
> +            gtk_window_move(window, screenRect.x(), screenRect.y());

Should this be (0,0), or are the coordinates flipped somehow?

[Carlos Garcia Campos]

Comment on attachment 340956 [details]
Try to fix iOS builds

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

>> Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp:118
>> +        completeTimer.startOneShot(1_s);
> 
> Seems like a good idea, though I've never had it flake out on macOS yet.

I think it's in the spec, but it doesn't say how long the timer should be, IIRC

>> Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp:190
>> +#if ENABLE(DEVELOPER_MODE)
> 
> Why is this part guarded?

Because this code is only for the tests, it's a workaround for Xvfb, and tests are only built when developer mode is enabled.

>> Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp:196
>> +            gtk_window_move(window, screenRect.x(), screenRect.y());
> 
> Should this be (0,0), or are the coordinates flipped somehow?

screenAvailableRect takes into account if there's a top panel at 0,0 for example.

[Michael Catanzaro]

Comment on attachment 340956 [details]
Try to fix iOS builds

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

The bottom portions of webkitWebViewMaximizeWindow and webkitWebViewRestoreWindow feel pretty crowded, I would add some more blank lines in there.

> Source/WebKit/UIProcess/API/glib/WebKitAutomationSession.cpp:117
> +    void requestMaximizeWindowOfPage(WebAutomationSession&, WebPageProxy& page, CompletionHandler<void()>&& completionHandler) override
> +    {
> +        auto* webView = webkitWebContextGetWebViewForPage(m_session->priv->webContext, &page);
> +        if (!webView)
> +            completionHandler();
> +        webkitWebViewMaximizeWindow(webView, WTFMove(completionHandler));
> +    }
> +
> +    void requestHideWindowOfPage(WebAutomationSession&, WebPageProxy& page, CompletionHandler<void()>&& completionHandler) override
> +    {
> +        auto* webView = webkitWebContextGetWebViewForPage(m_session->priv->webContext, &page);
> +        if (!webView)
> +            completionHandler();
> +        webkitWebViewMinimizeWindow(webView, WTFMove(completionHandler));
> +    }
> +
> +    void requestRestoreWindowOfPage(WebAutomationSession&, WebPageProxy& page, CompletionHandler<void()>&& completionHandler) override
> +    {
> +        auto* webView = webkitWebContextGetWebViewForPage(m_session->priv->webContext, &page);
> +        if (!webView)
> +            completionHandler();
> +        webkitWebViewRestoreWindow(webView, WTFMove(completionHandler));
> +    }

These all look wrong to me, shouldn't it be something like:

if (auto* webView = webkitWebContextGetWebViewForPage(m_session->priv->webContext, &page))
    webkitWebViewMaximizeWindow(webView, WTFMove(completionHandler));
else
    completionHandler();

Otherwise you're calling WebKitWebView methods with a NULL webview, and the completion handler could be called twice.

r=me on the rest of the GTK bits.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp:117
> +        // Complete the event if not done after 1 second.

Nit: "one second."

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp:134
> +    Type type;
> +    CompletionHandler<void()> completionHandler;
> +    RunLoop::Timer<WindowStateEvent> completeTimer;

How about some m_ for these?

[Carlos Garcia Campos]

Comment on attachment 340956 [details]
Try to fix iOS builds

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

>> Source/WebKit/UIProcess/API/glib/WebKitAutomationSession.cpp:117
>> +    }
> 
> These all look wrong to me, shouldn't it be something like:
> 
> if (auto* webView = webkitWebContextGetWebViewForPage(m_session->priv->webContext, &page))
>     webkitWebViewMaximizeWindow(webView, WTFMove(completionHandler));
> else
>     completionHandler();
> 
> Otherwise you're calling WebKitWebView methods with a NULL webview, and the completion handler could be called twice.
> 
> r=me on the rest of the GTK bits.

Oops, you are right, there's a missing return in all those cases after completionHandler(); I'll better use your proposal

>> Source/WebKit/UIProcess/API/gtk/WebKitWebViewGtk.cpp:134
>> +    RunLoop::Timer<WindowStateEvent> completeTimer;
> 
> How about some m_ for these?

This is a struct.

[Carlos Garcia Campos]

Committed r232150: <https://trac.webkit.org/changeset/232150>

[Radar WebKit Bug Importer]

<rdar://problem/40514627>