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-
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
Patch (35.80 KB, patch)
2017-08-03 03:16 PDT, Carlos Garcia Campos
bburg: review+
buildbot: commit-queue-
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
Carlos Garcia Campos
Comment 1 2017-08-02 10:07:53 PDT
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
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
Radar WebKit Bug Importer
Comment 15 2017-08-05 02:28:44 PDT
Note You need to log in before you can comment on or make changes to this bug.