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

Thu Jul 13 02:59:57 PDT 2017

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

--- Comment #14 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to Brian Burg from comment #6)
> Comment on attachment 315232 [details]
> 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.

Ok.

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

Ideally, it should only depend on WTF, I had to link to JSC only because of InspectorValues, and I don't consider it depends on WebKit2 either, it's only using static resources that are located under WebKit2 directory because that's how safari needs it. But I agree we can calrify all this in the ChangeLog.

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

Ok, I'll rework it.

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

Ok.

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

It makes sense, I'll do the same.

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

hmm, I guess the will create a real dependency on WebKit2, right? If it's only a matter of including a header with enum definitions then it could be ok, but I don't think we want to link to WebKit2.

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

I tried to optimize here and avoid String copies, but it isn't worth it all.

> > Source/WebDriver/CommandResult.cpp:104
> > +    switch (m_status) {
> 
> Please reference the table in §6.6.

Ok.

> > Source/WebDriver/CommandResult.cpp:128
> > +String CommandResult::error() const
> 
> NIt: errorString() or errorMessage()

I'll use errorString() for the string representation of an error code and errorMessage() for the optional error description.

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

I agree, I'll add Request and renamed ResponseBody as Response including also the http status code. This was we always sent a Request and reply with a Response.

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

No, it's a typo or a copy-paste error, I don't remember :-)

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

This is not the data, but the "Content-Type" HTTP header, but you are right, we always pass utf8 to soup. It doesn't make any difference in this case, because we are always receiving an ASCII literal here "application/json; charset=utf-8".

-- 
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/20170713/a494cfef/attachment-0001.html>