[Webkit-unassigned] [Bug 174618] WebDriver: properly handle capabilities and process firstMatch too

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=174618

--- Comment #9 from Brian Burg <bburg at apple.com> ---
Comment on attachment 317108
  --> https://bugs.webkit.org/attachment.cgi?id=317108
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170804/6077b497/attachment.html>