[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

Brian Burg <bburg at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #315417|review?                     |review-
              Flags|                            |

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

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