[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

Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mcatanzaro at igalia.com

--- Comment #7 from Michael Catanzaro <mcatanzaro at igalia.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

Um, wow.

> Source/WebDriver/CMakeLists.txt:1
> +set_property(DIRECTORY . PROPERTY FOLDER "WebDriver")

What is this for? Did you copy it from somewhere?

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

> Source/WebDriver/CMakeLists.txt:46
> +add_dependencies(WebDriver WTF)

Is this really needed to link? It should be inherited from JSC.

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

> Source/WebDriver/HTTPServer.h:52
> +    ~HTTPServer() = default;

Why does this need to be declared?

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

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

> Source/WebDriver/Session.h:107
> +    Session(Capabilities&&);

I'd use explicit here too.

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

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

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

"to enable"

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

Ditto.

-- 
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/63e7033a/attachment.html>