[webkit-reviews] review denied: [Bug 166682] Add initial implementation of WebDriver process to run the HTTP server : [Attachment 315417] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 166682: Add initial implementation of WebDriver process to run the HTTP
server
https://bugs.webkit.org/show_bug.cgi?id=166682

Attachment 315417: Patch

https://bugs.webkit.org/attachment.cgi?id=315417&action=review




--- Comment #29 from Brian Burg <bburg at apple.com> ---
Comment on attachment 315417
  --> https://bugs.webkit.org/attachment.cgi?id=315417
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315417&action=review

Overall this looks quite good. My remaining concerns are about capability
handling, and current behavior seems wrong so marking r- for that part. Please
post another patch and I'll sign off.

I didn't spend as much time looking over Session command implementations as
they are kinda mechanical and will change a lot when generated code is adopted.
Let me know if you want specific feedback about some of them.

> Source/WebDriver/Session.cpp:98
> +    m_host->startAutomationSession(m_id, [this, protectedThis =
makeRef(*this), completionHandler = WTFMove(completionHandler)]() mutable {

I was kinda expecting this call to happen externally to the Session
abstraction. I guess it's okay this way, since you don't make the session
active unless this completes normally.

> Source/WebDriver/Session.cpp:122
> +    RefPtr<InspectorObject> parameters = InspectorObject::create();

To reiterate: the medium term goal should be to remove details about the
protocol encoding from the session commands, like this one. It will make things
a lot less boilerplate.

> Source/WebDriver/Session.cpp:130
> +	   m_browsingContext = emptyString();

Please make this an optional<String>, then the line can me
m_browsingContext.clear()

> Source/WebDriver/WebDriverService.cpp:619
> +String WebDriverService::findElementOrCompleteWithError(InspectorObject&
parameters, Function<void (CommandResult&&)>& completionHandler)

Should be a static method. I'm ambivalent about doing the completion handlers
here vs. inline. Are you sure that the same errors are expected in all callers?

> Source/WebDriver/WebDriverService.cpp:629
> +bool
WebDriverService::findStrategyAndSelectorOrCompleteWithError(InspectorObject&
parameters, Function<void (CommandResult&&)>& completionHandler, String&
strategy, String& selector)

Should be a static method.

> Source/WebDriver/WebDriverService.cpp:827
> +    auto session = findSessionOrCompleteWithError(*parameters,
completionHandler);

I meant to add comments with the section numbers in the implementations here,
so it's easy to cross-reference the error conditions. Right now it's in the
method table, which is something but not particularly useful for code
inspection.

> Source/WebDriver/WebDriverService.cpp:876
> +bool
WebDriverService::findScriptAndArgumentsOrCompleteWithError(InspectorObject&
parameters, Function<void (CommandResult&&)>& completionHandler, String&
script, RefPtr<InspectorArray>& arguments)

This could (should?) be a static method.

> Source/WebDriver/glib/SessionHostGlib.cpp:1
> +/*

You'll probably want a GTK reviewer to look over the dbus/glib parts of the
patch. It looks OK to me, but it's rather obscure :)

> Source/cmake/WebKitFeatures.cmake:185
> +    WEBKIT_OPTION_DEFINE(ENABLE_WEBDRIVER "Whether to enable WebDriver
compilation" PRIVATE OFF)

Nit: "Toggle building the WebDriver REST API service" or similar.