Bug 174861 – WebDriver: fix return value of close window command

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 316445 [details]
Patch

[Build Bot]

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.

[Blaze Burg]

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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Created attachment 316630 [details]
Patch

[Build Bot]

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.

[Blaze Burg]

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.

[Carlos Garcia Campos]

Committed r220794: <http://trac.webkit.org/changeset/220794>

[Radar WebKit Bug Importer]

<rdar://problem/33916648>