Bug 174861 - WebDriver: fix return value of close window command
Summary: WebDriver: fix return value of close window command
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-07-26 05:09 PDT by Carlos Garcia Campos
Modified: 2017-08-16 04:39 PDT (History)
4 users (show)

See Also:


Attachments
Patch (18.30 KB, patch)
2017-07-26 05:20 PDT, Carlos Garcia Campos
bburg: review-
Details | Formatted Diff | Diff
Patch (11.90 KB, patch)
2017-07-28 02:46 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.
Description Carlos Garcia Campos 2017-07-26 05:09:02 PDT
10.2 Close Window
https://w3c.github.io/webdriver/webdriver-spec.html#close-window

Return the result of running the remote end steps for the Get Window Handles command.

We are currently returning null. We should also try to close the session if there aren't more browsing contexts.
Comment 1 Carlos Garcia Campos 2017-07-26 05:20:03 PDT
Created attachment 316445 [details]
Patch
Comment 2 Build Bot 2017-07-26 05:21:25 PDT
Attachment 316445 [details] did not pass style-queue:


ERROR: Source/WebDriver/Session.h:110:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.h:111:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.h:112:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.cpp:60:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.cpp:308:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.cpp:336:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 6 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Brian Burg 2017-07-26 15:19:57 PDT
Comment on attachment 316445 [details]
Patch

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

Absent a good justification, I don't want to add a redundant command. The web service parts look good.

> Source/WebDriver/Session.cpp:308
> +void Session::continueGetWindowHandles(const InspectorObject& result, Function<void (CommandResult&&)>&& completionHandler)

Nit: I prefer "fetchWindowHandles" or just "getWindowHandles".

> Source/WebKit/UIProcess/Automation/Automation.json:250
> +            "returns": [

I don't like this. It's redundant with another command. Can you just compose the two Automation commands in the service implementation? That's what safaridriver does.
Comment 4 Carlos Garcia Campos 2017-07-26 22:51:05 PDT
(In reply to Brian Burg from comment #3)
> Comment on attachment 316445 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316445&action=review
> 
> Absent a good justification, I don't want to add a redundant command. The
> web service parts look good.
> 
> > Source/WebDriver/Session.cpp:308
> > +void Session::continueGetWindowHandles(const InspectorObject& result, Function<void (CommandResult&&)>&& completionHandler)
> 
> Nit: I prefer "fetchWindowHandles" or just "getWindowHandles".

There's already getWindowHandles, this is what we call when we get the response of getWindowHandles.

> > Source/WebKit/UIProcess/Automation/Automation.json:250
> > +            "returns": [
> 
> I don't like this. It's redundant with another command. Can you just compose
> the two Automation commands in the service implementation? That's what
> safaridriver does.

Yes, I just thought it was more convenient, and it simplifies the implementation in the driver.
Comment 5 Carlos Garcia Campos 2017-07-28 02:46:50 PDT
Created attachment 316630 [details]
Patch
Comment 6 Build Bot 2017-07-28 02:48:47 PDT
Attachment 316630 [details] did not pass style-queue:


ERROR: Source/WebDriver/Session.h:110:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.h:111:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.cpp:60:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/Session.cpp:308:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Brian Burg 2017-08-03 17:06:51 PDT
Comment on attachment 316630 [details]
Patch

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

r=me with naming change

> Source/WebDriver/ChangeLog:24
> +        (WebDriver::Session::closeWindow): Call closeTopLevelBrowsingContext() passing the current toplvel browsing context.

Nit: toplvel

> Source/WebDriver/Session.cpp:60
> +void Session::closeNextToplevelBrowsingContext(const String& toplevelBrowsingContext, Function<void (CommandResult&&)>&& completionHandler)

Shouldn't this entry point be closeAllToplevelBrowsingContexts? It closes more than just the next one.
Comment 8 Carlos Garcia Campos 2017-08-16 04:38:33 PDT
Committed r220794: <http://trac.webkit.org/changeset/220794>
Comment 9 Radar WebKit Bug Importer 2017-08-16 04:39:45 PDT
<rdar://problem/33916648>