Bug 174618 – WebDriver: properly handle capabilities and process firstMatch too

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Carlos Garcia Campos]

For now, we are just getting the capabilities from alwaysMatch and using them to spawn the browser. But we should actually validate the capabilities, process the firstMatch and return the resolved capabilities.

https://www.w3.org/TR/webdriver/#capabilities

[Carlos Garcia Campos]

Created attachment 316969 [details]
Patch

[Build Bot]

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


ERROR: Source/WebDriver/WebDriverService.cpp:424:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/WebDriverService.cpp:491:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/WebDriverService.h:104:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 7 files


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

[Build Bot]

Comment on attachment 316969 [details]
Patch

Attachment 316969 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4240726

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-no-preflight.any.worker.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-basic.any.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-basic.any.worker.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.any.worker.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-origin.any.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-no-preflight.any.html

[Build Bot]

Created attachment 316976 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Carlos Garcia Campos]

Created attachment 317108 [details]
Patch

[Build Bot]

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


ERROR: Source/WebDriver/WebDriverService.cpp:424:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/WebDriverService.cpp:493:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebDriver/WebDriverService.h:106:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 7 files


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

[Build Bot]

Comment on attachment 317108 [details]
Patch

Attachment 317108 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4245802

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 317112 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Blaze Burg]

Comment on attachment 317108 [details]
Patch

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

> Source/WebDriver/WebDriverService.cpp:265
> +        if (!it->value->asInteger(timeoutMS) || timeoutMS < 0 || timeoutMS > INT_MAX)

According to the spec, this check is performed after the key check. I don't think it matters as they return the same error code.

> Source/WebDriver/WebDriverService.cpp:322
> +    auto result = InspectorObject::create();

I find the double-parsing to be kind of weird, but it's actually in the spec 🙃

> Source/WebDriver/WebDriverService.cpp:325
> +        if (it->value->isNull())

This section may be easier to read if you add comments to mark off cases by the type of the value (bool, string, enum). Alternatively, you could parse the capability key in a separate function and turn this into a switch to check the types of already parsed keys. If key parsing fails (unknown capability), then the entire thing fails. That will also centralize where we need to maintain the list of "known" capabilities.

> Source/WebDriver/WebDriverService.cpp:341
> +            // FIXME: implement pageLoadStrategy.

According to the spec [editors], we should be validating these capabilities that we know about but don't support. Later when we do capability matching is when it will error out.

> Source/WebDriver/WebDriverService.cpp:344
> +            // FIXME: implement proxy support.

Ditto.

> Source/WebDriver/WebDriverService.cpp:354
> +            // FIXME: implement prompts support.

Ditto.

[Blaze Burg]

Comment on attachment 317108 [details]
Patch

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

r=me

Please fix the capability parsing vs matching so that unimplemented ones are recognized but not matchable.

> Source/WebDriver/WebDriverService.cpp:451
> +    if (!capabilitiesObject->getValue(ASCIILiteral("firstMatch"), firstMatchCapabilitiesValue)) {

Referencing steps in the spec would help here, as the names here are quite verbose yet not clear what the logic is.

> Source/WebDriver/WebDriverService.cpp:481
> +            completionHandler(CommandResult::fail(CommandResult::ErrorCode::SessionNotCreated, String("Invalid capabilities")));

The error message could be better. This should only fail if a capability is in both firstMatch and alwaysMatch.

[Carlos Garcia Campos]

Comment on attachment 317108 [details]
Patch

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

>> Source/WebDriver/WebDriverService.cpp:325
>> +        if (it->value->isNull())
> 
> This section may be easier to read if you add comments to mark off cases by the type of the value (bool, string, enum). Alternatively, you could parse the capability key in a separate function and turn this into a switch to check the types of already parsed keys. If key parsing fails (unknown capability), then the entire thing fails. That will also centralize where we need to maintain the list of "known" capabilities.

I'm not sure I follow what you mean here. What would the function parsing the key return?

>> Source/WebDriver/WebDriverService.cpp:341
>> +            // FIXME: implement pageLoadStrategy.
> 
> According to the spec [editors], we should be validating these capabilities that we know about but don't support. Later when we do capability matching is when it will error out.

It's a bit weird to deserialize things here that we don't support yet. We are at least checking the type is correct here. This is done in patch attached to bug #175183, though.

>> Source/WebDriver/WebDriverService.cpp:344
>> +            // FIXME: implement proxy support.
> 
> Ditto.

I don't even know how I'm going to handle proxies, I prefer to keep this as a FIXME and add the deserialize method when we add proxy support.

>> Source/WebDriver/WebDriverService.cpp:354
>> +            // FIXME: implement prompts support.
> 
> Ditto.

And I plan to implement prompts next week, so this FIXME will be gone too.

[Carlos Garcia Campos]

Comment on attachment 317108 [details]
Patch

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

>>> Source/WebDriver/WebDriverService.cpp:325
>>> +        if (it->value->isNull())
>> 
>> This section may be easier to read if you add comments to mark off cases by the type of the value (bool, string, enum). Alternatively, you could parse the capability key in a separate function and turn this into a switch to check the types of already parsed keys. If key parsing fails (unknown capability), then the entire thing fails. That will also centralize where we need to maintain the list of "known" capabilities.
> 
> I'm not sure I follow what you mean here. What would the function parsing the key return?

I want to land this, because two other patches depend on this one. I'll make any refactoring to this method in a follow up, once I understand what you propose here :-)

[Carlos Garcia Campos]

(In reply to Brian Burg from comment #10)
> Comment on attachment 317108 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317108&action=review
> 
> r=me
> 
> Please fix the capability parsing vs matching so that unimplemented ones are
> recognized but not matchable.

Ah, I'll do this in a follow up too, soon we will only need to this for proxy.

[Carlos Garcia Campos]

Committed r220315: <http://trac.webkit.org/changeset/220315>

[Radar WebKit Bug Importer]

<rdar://problem/33739607>