Bug 174618 - WebDriver: properly handle capabilities and process firstMatch too
Summary: WebDriver: properly handle capabilities and process firstMatch too
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: 166679 175130 175183 175184
  Show dependency treegraph
 
Reported: 2017-07-18 00:47 PDT by Carlos Garcia Campos
Modified: 2017-08-05 02:28 PDT (History)
5 users (show)

See Also:


Attachments
Patch (33.41 KB, patch)
2017-08-02 10:07 PDT, Carlos Garcia Campos
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (1.17 MB, application/zip)
2017-08-02 11:13 PDT, Build Bot
no flags Details
Patch (35.80 KB, patch)
2017-08-03 03:16 PDT, Carlos Garcia Campos
bburg: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (871.37 KB, application/zip)
2017-08-03 04:45 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2017-07-18 00:47:16 PDT
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
Comment 1 Carlos Garcia Campos 2017-08-02 10:07:53 PDT
Created attachment 316969 [details]
Patch
Comment 2 Build Bot 2017-08-02 10:10:50 PDT
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.
Comment 3 Build Bot 2017-08-02 11:13:00 PDT
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
Comment 4 Build Bot 2017-08-02 11:13:01 PDT
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
Comment 5 Carlos Garcia Campos 2017-08-03 03:16:58 PDT
Created attachment 317108 [details]
Patch
Comment 6 Build Bot 2017-08-03 03:19:02 PDT
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.
Comment 7 Build Bot 2017-08-03 04:45:07 PDT
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.
Comment 8 Build Bot 2017-08-03 04:45:08 PDT
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
Comment 9 Brian Burg 2017-08-04 10:04:44 PDT
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.
Comment 10 Brian Burg 2017-08-04 10:50:57 PDT
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.
Comment 11 Carlos Garcia Campos 2017-08-05 02:20:30 PDT
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.
Comment 12 Carlos Garcia Campos 2017-08-05 02:22:49 PDT
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 :-)
Comment 13 Carlos Garcia Campos 2017-08-05 02:24:28 PDT
(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.
Comment 14 Carlos Garcia Campos 2017-08-05 02:27:25 PDT
Committed r220315: <http://trac.webkit.org/changeset/220315>
Comment 15 Radar WebKit Bug Importer 2017-08-05 02:28:44 PDT
<rdar://problem/33739607>