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

Great, I'm, already updating things.

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

I didn't do that because we already have that distinction, we use WebAutomation for the support to control the browser, and it's enabled unconditionally anyway. I tried to follow what other browsers and selenium seem to do, which is referring to the server/service as the driver (chromedriver, firefoxdriver, safaridriver, etc.)

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

Me neither :-P I don't remember why I did it that way. Anyway, as I said before I've added Request and renamed ResponseBody as Response including the status code as you suggests.

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

Yes, I'm afraid of adding a real dependency on WebKit2.

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

Indeed, ideally WebDriver process should only link to WTF. Do you plan to do it, or do you want me to give it a try? Maybe it could be done in two steps, since InspectorValues are used in several places and even generated code, first we add WTF::JSON and make InspectorValues use it as a simple wrapper, and then we can incrementally replace all uses of InspectorValues with WTF::JSON without having to do it in a single huge patch.

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

I always think of WebDriver as the service, but I'm fine with using WebDriverService.

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

My initial idea is to not support legacy stuff in this new implementation, unless it's really needed. So, for now, and for the sake of simplicity, I've focused on making it w3c compliant only.

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

I used the spec terminology for the methods, which is think is more decriptive, so that the names match the spec sections.

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

hmm, yes, or check the spec to know it.

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

I'll definitely do it.

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

Not really, this is because selenium (at least the version I'm currently using), defines EXECUTE_ASYNC_SCRIPT with execute_async instead of execute/async. I guess it's a bug, but I'm not sure. I'll add a comment, and I'll check current selenium or file a bug report.

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

Ok, then we can make Capabilities a simple struct, instead of a class.

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

Right!

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

HTTPMethod enum is private.

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

HTTPMethod enum and Command struct and the commands array are private too.

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

hmm, I never liked that part of the code either, I'll try to add the SessionHost class.

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

Sure, I already reworked it.

> > 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" : { ... }

Added webkitgtk prefix.

-- 
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/3d9f24de/attachment-0001.html>