[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 #8 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

Still have a few more comments, will try to write those up later today.

> Source/CMakeLists.txt:41
> +if (ENABLE_WEBDRIVER)

You might want to name this ENABLE_WEBDRIVER_SERVER or ENABLE_WEBDRIVER_SERVICE. Otherwise people may think it controls the support in the engine proper, but that's actually behind ENABLE_REMOTE_AUTOMATION.

> Source/WebDriver/HTTPServer.h:43
> +        String data;

Similar to reasons below, it will be easier to deal with the entire response if you include the HTTP code as part of the Response object. Currently these are bundled but the HTTP status code is not, for reasons I don't understand.

> Source/WebDriver/Session.h:177
> +    };

As I said above, you can get these enums generated by the protocol generator without much additional work. This is especially useful for error codes which are likely to be expanded over time.

> Source/WebDriver/WebDriver.cpp:30
> +#include <inspector/InspectorValues.h>

Maybe we should start moving InspectorValues to wtf project so that you don't need to link in JSC.

> Source/WebDriver/WebDriver.cpp:38
> +WebDriver::WebDriver()

I think the purpose of this class would be more obvious if this were named WebDriverService, HTTPService, or the like.

> Source/WebDriver/WebDriver.cpp:107
> +    { Method::Delete, "/session/$sessionID", &WebDriver::deleteSession },

A note on how safaridriver arranges the method table:

Since our server was written over a year ago before W3C started converging, we designed it to support multiple REST API command sets. The HTTP service is configured with a command dialect (selenium vs w3c) at launch time. So, we have two dialects, each with a method such as

- (void)_handlePostSessionWindow:(RouteRequest *)request response:(RouteResponse *)response

Which corresponds to 

POST /session/:sessionId/window

And inside the endpoint-specific method, after doing argument validation it calls into a dialect-independent implementation that does the JSON-RPC call to the remote over the Automation protocol.
If you don't intend to support the legacy protocol, then this level of indirection is not strictly necessary. Just thought you might find it interesting to know.

Even without the need for indirection due to dialects, you may find it worthwhile to stick to naming the entry methods after the URL template and HTTP method.
If you know that a specific endpoint is failing, then it is trivial to start looking in the right method by prepping the URL template
.Whereas in this patch, you need to know where this super-important vtable is to make any progress.

Another way to get the same utility during debugging (which we also do) is to add comments at the header of each endpoint implementation and
include a link to the spec section. You might also want to write in a comment the HTTP method, the URL template, and expected errors.

> Source/WebDriver/WebDriver.cpp:145
> +    { Method::Post, "/session/$sessionID/execute_async", &WebDriver::executeAsyncScript },

Unexpected dupe?

> Source/WebDriver/WebDriver.cpp:280
> +    Capabilities capabilities(WTFMove(desiredCapabilities));

I highly prefer you didn't entangle InspectorObject (soon to be JSON::Object) with the implementations of these auxillary classes. Instead you can use a helper method to individually parse and validate capabilities. That will also let you get rid of m_isValid in Capabilities class...

> Source/WebDriver/WebDriver.cpp:282
> +        completionHandler(CommandResult(CommandResult::Status::InvalidArgument));

... and provide an actually useful error message here, since you'd know exactly which capability was invalid.

BTW, per §8.1, this should return "session not created" if something goes wrong.

In general, capability processing is something that will get really messy really quick. Often you may need some context from the system (not available in Capabilities, but available from the main web service class) to determine whether they capability can be matched or not.

> Source/WebDriver/WebDriver.h:54
> +    enum class Method { Get, Post, Delete };
> +    typedef void (WebDriver::*CommandHandler)(RefPtr<Inspector::InspectorObject>&&, Function<void (CommandResult&&)>&&);

Nit: HTTPMethod

> Source/WebDriver/WebDriver.h:62
> +    static std::optional<Method> toCommandMethod(const String& method);

No reason for this to be a header declaration.

> Source/WebDriver/WebDriver.h:63
> +    static bool findCommand(const String& method, const String& path, CommandHandler*, HashMap<String, String>& parameters);

Ditto.

> Source/WebDriver/glib/SessionGlib.cpp:127
> +    m_cancellable = adoptGRef(g_cancellable_new());

Heh, there's a lot of fun stuff here, a lot of which I don't understand due to being GTK/dbus API.

In safaridriver we eventually extracted all remote connection bootstrap code into a separate class called SessionHost. That way, the Session class can assume it has a valid connection and only really cares about processing commands. It only uses the session host to send out messages and close the session. We don't even vend out the Session class to the web service (i.e., WebDriver class here) until it's been fully initialized so that it can't be used in an uninitialized state.

Not sure how relevant here, but such a design also better isolates the platform-specific peering/bootstrap code from cross-platform bits.

Due to sandboxing reasons, we also could not rely on the assumption that the browser is a subprocess of the web service, so we use a fully async state machine to keep track of the browser coming up and going down. If you really can assume it will be a subprocess, and you d on't need to connect to an existing browser, then it's understandable that your connection code is more straightforward w.r.t. asynchrony.

> Source/WebDriver/glib/SessionGlib.cpp:152
> +    connectToBrowser(std::make_unique<ConnectToBrowserAsyncData>(this, WTFMove(dbusAddress), m_cancellable.get(), WTFMove(completionHandler)));

You should think about how you want to report errors that happen when launching/connecting to the browser, preferably out to the HTTP response so users can read the actual error.

> Source/WebDriver/gtk/CapabilitiesGtk.cpp:35
> +bool Capabilities::parsePlatformCapabilities(RefPtr<InspectorObject>&& desiredCapabilities)

As I suggested above, this makes more sense to be as a platform method of WebDriverService rather than a pseudo-factory method. Trust me, it will be a lot easier down the line when Capabilities is just a string-to-string map that's been merged and normalized to something that's meaningful to the webdriver service implementation.

> Source/WebDriver/gtk/CapabilitiesGtk.cpp:38
> +    if (!desiredCapabilities->getObject(ASCIILiteral("browserOptions"), browserOptions)) {

Per §6.7, I believe that extension capabilities are supposed to have a prefix, like so:

"webkitgtk:browserOptions" : { ... }

-- 
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/422e235c/attachment.html>