Bug 180398 - WebDriver: implement maximize, minimize and fullscreen window commands
Summary: WebDriver: implement maximize, minimize and fullscreen window commands
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebDriver (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 182837
Blocks:
  Show dependency treegraph
 
Reported: 2017-12-05 01:53 PST by Carlos Garcia Campos
Modified: 2018-05-24 01:55 PDT (History)
6 users (show)

See Also:


Attachments
Patch (35.37 KB, patch)
2018-05-17 04:44 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix iOS builds (35.36 KB, patch)
2018-05-21 22:49 PDT, Carlos Garcia Campos
bburg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Carlos Garcia Campos 2018-05-17 04:44:20 PDT
Created attachment 340572 [details]
Patch
Comment 2 Build Bot 2018-05-17 04:46:39 PDT
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.
Comment 3 Carlos Garcia Campos 2018-05-17 05:30:45 PDT
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.
Comment 4 Carlos Garcia Campos 2018-05-21 22:49:59 PDT
Created attachment 340956 [details]
Try to fix iOS builds
Comment 5 Build Bot 2018-05-21 22:51:56 PDT
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.
Comment 6 Brian Burg 2018-05-22 13:18:52 PDT
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?
Comment 7 Carlos Garcia Campos 2018-05-23 06:31:51 PDT
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.
Comment 8 Michael Catanzaro 2018-05-23 08:10:28 PDT
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?
Comment 9 Carlos Garcia Campos 2018-05-23 08:21:57 PDT
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.
Comment 10 Carlos Garcia Campos 2018-05-24 01:54:46 PDT
Committed r232150: <https://trac.webkit.org/changeset/232150>
Comment 11 Radar WebKit Bug Importer 2018-05-24 01:55:22 PDT
<rdar://problem/40514627>