WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
174618
WebDriver: properly handle capabilities and process firstMatch too
https://bugs.webkit.org/show_bug.cgi?id=174618
Summary
WebDriver: properly handle capabilities and process firstMatch too
Carlos Garcia Campos
Reported
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
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Carlos Garcia Campos
Comment 1
2017-08-02 10:07:53 PDT
Created
attachment 316969
[details]
Patch
Build Bot
Comment 2
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.
Build Bot
Comment 3
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
Build Bot
Comment 4
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
Carlos Garcia Campos
Comment 5
2017-08-03 03:16:58 PDT
Created
attachment 317108
[details]
Patch
Build Bot
Comment 6
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.
Build Bot
Comment 7
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.
Build Bot
Comment 8
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
Blaze Burg
Comment 9
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.
Blaze Burg
Comment 10
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.
Carlos Garcia Campos
Comment 11
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.
Carlos Garcia Campos
Comment 12
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 :-)
Carlos Garcia Campos
Comment 13
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.
Carlos Garcia Campos
Comment 14
2017-08-05 02:27:25 PDT
Committed
r220315
: <
http://trac.webkit.org/changeset/220315
>
Radar WebKit Bug Importer
Comment 15
2017-08-05 02:28:44 PDT
<
rdar://problem/33739607
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug