[Webkit-unassigned] [Bug 166682] Add initial implementation of WebDriver process to run the HTTP server

[bugzilla-daemon at webkit.org]

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

--- Comment #6 from Brian Burg <bburg at apple.com> ---
Comment on attachment 315232
  --> https://bugs.webkit.org/attachment.cgi?id=315232
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315232&action=review

General comments:

It would be useful for you to state up front that namespace WebDriver is for HTTP service code only.

You should also say what it depends on (it seems to link against JavaScriptCore and WTF and depend on files from WebKit2).

> Source/WebDriver/CommandResult.cpp:36
> +    : m_status(status)

I think "status" is the wrong name here for the type and the member. How about std::optional<ErrorCode>? This matches what the spec calls it, in section 6.6.

> Source/WebDriver/CommandResult.cpp:58
> +    case -32700: // ParseError

Please use an enum rather than obscure numbers, and add a note that these are defined in JSON-RPC 2.0, Section 5.1.

safaridriver calls this the ProtocolErrorCode enum to differentiate from command-specific error codes (those defined in §6.6).

> Source/WebDriver/CommandResult.cpp:63
> +        m_errorMessage = errorMessage;

Since these JSON-RPC calls to the backend are entirely generated by the web server, I don't think it makes sense to report this to the outside world as an invalid argument. It is always a programming error internal to the web service and/or the UIProcess backend. If the REST API call provided a bogus argument, then that should be checked before sending out the command to the backend.

For every error in this switch except ServerError (which here is correctly interpreted as a failed command), safaridriver lumps these together as an HTTP 500 / "unknown error" and propagates the underlying error in non-production builds. This is the only error code from §6.6 that makes sense for errors we can't attribute to the user.

> Source/WebDriver/CommandResult.cpp:68
> +        if (errorMessage == "WindowNotFound")

If you have the right generators turned on, you can instead make this automated by splitting at the first ';' if found, then calling Inspector::fromProtocolString<WDProtocolAutomationErrorMessage>(firstPart) to get an enum that matches those defined in Automation.json's "ErrorMessage" type.

EDIT: oops, these helpers are only generated for the ObjC "frontend" used by safaridriver. You could easily extend this to generate enum helpers for the C++ "frontend".

> Source/WebDriver/CommandResult.cpp:74
> +        else if (errorMessage.startsWith("JavaScriptError;")) {

I see you figured out my hacky encoding. ;-)

> Source/WebDriver/CommandResult.cpp:77
> +            m_errorMessage = errorMessage.substring(position + 1);

You should extract the string manipulation code out to the top and use std::optional for m_errorMessage rather than an empty string.

> Source/WebDriver/CommandResult.cpp:104
> +    switch (m_status) {

Please reference the table in §6.6.

> Source/WebDriver/CommandResult.cpp:128
> +String CommandResult::error() const

NIt: errorString() or errorMessage()

> Source/WebDriver/CommandResult.h:40
> +        Ok,

Please add cross reference to the spec section that defines the status codes.

> Source/WebDriver/HTTPServer.h:46
> +    virtual void handleRequest(const String& method, const String& path, const char* data, size_t length, Function<void (unsigned, ResponseBody&&)>&& replyHandler) = 0;

It will be a lot easier to follow this code if Request was a struct that contained all the particulars. I do appreciate that the response is async by default, something that I had to fight against in the Objective-C http library used by safaridriver. That said, it's a bit mysterious how to signal an error just by reading this signature. Our pattern is typically to make the first completion handler argument an NSError or error string, which signals an error if non-null.

> Source/WebDriver/HTTPServer.h:54
> +    bool listen(unsigned port);

Might want to add a comment that the implementations of these are networking library-dependent.

> Source/WebDriver/Session.cpp:41
> +static const String webElementIdentifier = ASCIILiteral("element-6066-11e4-a52e-4f735466cecf");

Please link to the spec, as this is a particularly confusing thing to newcomers.

> Source/WebDriver/soup/HTTPServerSoup.cpp:40
> +        WTFLogAlways("Failed to start HTTPS server at port %u: %s", port, error->message);

Is it really https?

> Source/WebDriver/soup/HTTPServerSoup.cpp:53
> +                    soup_message_headers_append(message->response_headers, "Content-Type", responseBody.contentType.latin1().data());

This seems wrong, the data is UTF-8 but you are sending latin1. My memory says that the response is supposed to be UTF-8 unless it's an error message (4xx or 5xx), in which case it's ASCII/latin1.

-- 
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/20170712/6b6e8b77/attachment-0001.html>