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

No, the exec name was just an example, the point is that other browser refer to the driver as the service/server process. In GTK+ port the process is WebKitWebDriver. WebKitGTK+ based browsers are not expected to provide their own driver, but use the WebKitGTK+ one. They are expected to add a driver implementation in selenium, on top of the WebKitGTK one, that simply passes the browser options that will be included in the capabilities.

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

Sure, I already renamed WebDriver class as WebDriverService, see the latest patch attached here.

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

Ok, that sounds better, I'll look at it in more detail.

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

Ok, great.

> > > 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/20170714/31a7bcea/attachment.html>