[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 #21 from Brian Burg <bburg at apple.com> ---
(In reply to Carlos Garcia Campos from comment #16)
> (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.

Exciting!

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

Should we name the executable 'webkitdriver', then, if this is not specific to a particular browser? I could imagine each GTK-based browser shipping a wrapper with a name like midoridriver, which is really just calling into webkitdriver with some presets.

I think it's fine to call the executable webkitdriver. But internal to its implementation, the web server/service part is fairly distinct from the command line parsing, talking to the remote, the concept of a "session", and other things not related to handling REST API requests. That's why I advocate that it have a more specific name. :-)

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

IIRC, safaridriver copies headers out of WK2 but doesn't link against it. The generated "frontend" code is compiled in safaridriver's framework, not in WebKit2's framework (the "backend").

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

I have some patches to clean up InspectorValues so that they can be moved around easily (say, to WTF) and otherwise improved (say, to be more spec compliant). But don't wait for that- it's kind of tedious to push through and land, and my machines don't have much spare build time right now.

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

That's fair!

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

OK, that's fair too.

-- 
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/89aa1fde/attachment.html>