Bug 174618

Created attachment 316969 [details] Patch

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

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

Created attachment 317108 [details] Patch

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

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 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 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 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 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 :-)

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

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

<rdar://problem/33739607>