[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 #15 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to Michael Catanzaro from comment #7)
> Comment on attachment 315232 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315232&action=review
> 
> Um, wow.
> 
> > Source/WebDriver/CMakeLists.txt:1
> > +set_property(DIRECTORY . PROPERTY FOLDER "WebDriver")
> 
> What is this for? Did you copy it from somewhere?

I have no idea to be honest, I guess I copied it from other wk cmake files.

> > Source/WebDriver/CMakeLists.txt:29
> > +set(WebDriver_SCRIPTS
> > +    ${WEBKIT2_DIR}/UIProcess/Automation/atoms/ElementAttribute.js
> > +    ${WEBKIT2_DIR}/UIProcess/Automation/atoms/ElementDisplayed.js
> > +    ${WEBKIT2_DIR}/UIProcess/Automation/atoms/FindNodes.js
> > +    ${WEBKIT2_DIR}/UIProcess/Automation/atoms/FormElementClear.js
> > +    ${WEBKIT2_DIR}/UIProcess/Automation/atoms/FormSubmit.js
> > +)
> 
> This is unusual layering...?

These are there only because safari needs it, they are static resources we build  with WebDriver namespace as part of the web driver process, so in practice this is part of web driver for us.

> > Source/WebDriver/CMakeLists.txt:46
> > +add_dependencies(WebDriver WTF)
> 
> Is this really needed to link? It should be inherited from JSC.

I started it with WTF as the only dependency, then I had to add JCS because of InspectorValues, and I forgot to update this. I guess it's not needed in any case, since the dep is already implicit as you say.

> > Source/WebDriver/CommandResult.h:54
> > +    explicit CommandResult(Status, RefPtr<Inspector::InspectorValue>&& = nullptr);
> 
> Good use of explicit!
> 
> > Source/WebDriver/HTTPServer.h:51
> > +    HTTPServer(HTTPRequestClient&);
> 
> explicit. Don't want HTTPRequestClients magically turning into HTTPServers
> by mistake.

Right.

> > Source/WebDriver/HTTPServer.h:52
> > +    ~HTTPServer() = default;
> 
> Why does this need to be declared?

I think in the past we always had to define destructors when using smart pointers, I don't know if that's still true, but I keep doing it, even using = default. If it's no longer needed, I can simply remove it.

> > Source/WebDriver/Session.cpp:117
> > +long Session::sendCommandToBackend(const String& command, RefPtr<InspectorObject>&& parameters, Function<void (CommandResponse&&)>&& responseHandler)
> > +{
> > +    static long lastSequenceID = 0;
> 
> long, really? Why not int64_t? Or uint64_t?

I used long because that's what IdentifiersFactory use.

> >> Source/WebDriver/Session.cpp:1046
> >> +    case 0xE001U:
> > 
> > How can String::operator[] return values greater than 0xFFFF?  And what are these values?
> 
> Why do you think 0xE001 is greater than 0xFFFF? :)
> 
> A link to the spec would be helpful, though, yes.

Agree, I'll add a link to https://www.w3.org/TR/webdriver/#keyboard-actions

> > Source/WebDriver/Session.h:107
> > +    Session(Capabilities&&);
> 
> I'd use explicit here too.

It's private constructor, I guess it can be used implicitly?

> >> Source/WebDriver/soup/HTTPServerSoup.cpp:40
> >> +        WTFLogAlways("Failed to start HTTPS server at port %u: %s", port, error->message);
> > 
> > Is it really https?
> 
> Nope, that would require passing SOUP_SERVER_LISTEN_HTTPS for the third
> argument. Anyway, it's a local socket, so that would be pointless. The log
> message should be fixed.

Right, I'll fix it.

> > Source/cmake/OptionsGTK.cmake:140
> > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_WEBDRIVER PUBLIC ON)
> 
> Does it depend on ENABLE_MINIBROWSER? That would require declaring a
> dependency between the options in OptionsGTK.cmake, and having a chat about
> whether to turn ENABLE_MINIBROWSER on by default or disable ENABLE_WEBDRIVER
> by default. I'm uncertain which would be more preferable.
> 
> Am I missing something here?

Yes. It doesn't depend on any browser, actually. The selenium bindings are the ones specifying the browser to use. The idea is to create a generic WebKitGTK driver with browser specific options, including the binary path and command line arguments, passed as capabilities to the driver server. Then Epiphany, Midori, etc. can easily add their own driver to selenium on top of the generic one just by providing the browser options. You can also use the generic driver directly and provide the options you want. MiniBrowser is just the default browser used only by the GTK+ port when browser options are not provided by selenium driver. If MB is not installed, it will fail to launch the browser and session could not be created error will be generated. So, there's no dependency.

> > Source/cmake/OptionsGTK.cmake:279
> > +        message(FATAL_ERROR "GLib 2.40 is required to enabled WebDriver support.")
> 
> "to enable"

Oops.

> > Source/cmake/OptionsGTK.cmake:282
> > +        message(FATAL_ERROR "libsoup 2.48 is required to enabled WebDriver support.")
> 
> Ditto.

Oops.

-- 
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/85033f80/attachment-0001.html>