[Webkit-unassigned] [Bug 166682] Add initial implementation of WebDriver process to run the HTTP server

Fri Jul 14 14:56:11 PDT 2017

https://bugs.webkit.org/show_bug.cgi?id=166682

--- Comment #28 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

> Source/WebDriver/CommandResult.cpp:37
> +    ParseError = -32700,

The sorting struck me as odd, though I guess it is consistent..

> Source/WebDriver/CommandResult.cpp:74
> +#if ENABLE(DEVELOPER_MODE)

Hmm, is there any reason to not vend a detailed error message for everyone? Hiding this will make it really hard to diagnose bug reports from users.

In this vein, safaridriver has the ability to dump entire protocol traces, both HTTP and Automation protocol. You might want to add that eventually, it has been super useful since many users are running strange setups that I am unable to easily reproduce.

> Source/WebDriver/HTTPServer.cpp:31
> +HTTPServer::HTTPServer(HTTPRequestClient& requestClient)

Note: this C++ idiom of calling a delegate a client is kind of unfortunate in this instance, where the code is implementing part of the HTTP server routing but is nonetheless named like an HTTP client's method.

> Source/WebDriver/HTTPServer.h:33
> +#include <wtf/glib/GRefPtr.h>

I think you could use a forward declaration.

> Source/WebDriver/HTTPServer.h:34
> +typedef struct _SoupMessage SoupMessage;

Not used?

> Source/WebDriver/HTTPServer.h:36
> +#endif

OK

> Source/WebDriver/HTTPServer.h:49
> +        unsigned statusCode;

{ 0 }

> Source/WebDriver/Session.cpp:57
> +    return m_host->capabilities();

Hmm, this seems a bit strange. More comments below in SessionHost.

> Source/WebDriver/Session.cpp:63
> +        completionHandler(CommandResult(nullptr));

This would read better if you made factory methods like CommandResult::success(object = nullptr), CommandResult::fail(code, message = nullopt). I can't tell just by reading this whether it failed or succeeded.

> Source/WebDriver/Session.cpp:69
> +    m_host->sendCommandToBackend(ASCIILiteral("closeBrowsingContext"), WTFMove(parameters), [this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)](SessionHost::CommandResponse&& response) {

Heh, C++14 is getting hard to read. This will look nicer with generated interfaces.

BTW, maybe I'm just not familiar, but why do you need to capture this and protectedThis? If the latter is just to keep |this| from being destroyed, could it move out of the capture list?

> Source/WebDriver/Session.cpp:70
> +        if (response.isError) {

I think this case is duplicated everywhere and could be factored out into generated code. It would also let you get rid of the CommandResponse struct, which is confusingly similar to CommandResult but actually a struct.

> Source/WebDriver/SessionHost.cpp:47
> +long SessionHost::sendCommandToBackend(const String& command, RefPtr<InspectorObject>&& parameters, Function<void (CommandResponse&&)>&& responseHandler)

The send and receive methods here will be unnecessary once generated code is adopted. But it looks fine as is.

> Source/WebDriver/WebDriverService.cpp:227
> +    auto lowerCaseMethod = method.convertToASCIILowercase();

Does W3C spec support HEAD or OPTIONS requests? Safaridriver did these for legacy protocol.

> Source/WebDriver/WebDriverService.cpp:272
> +    return false;

Very clean, nice.

> Source/WebDriver/WebDriverService.cpp:301
> +    ((*this).*handler)(WTFMove(parametersObject), [this, replyHandler = WTFMove(replyHandler)](CommandResult&& result) mutable {

My brain exploded :D

> Source/WebDriver/WebDriverService.cpp:309
> +    // FIXME: Selenium expects a legacy response for script timeout.

Nit: amend comment to [...], but W3C does [blah blah].

It should be possible for the Selenium libraries to detect whether you support W3C or Selenium dialect and adjust accordingly. Perhaps the tests are not quite updated yet? You could raise a bug with Selenium if a test in their suite should be fixed.

> Source/WebDriver/WebDriverService.cpp:315
> +            responseObject->setInteger(ASCIILiteral("status"), 28);

Please use an enum for the legacy status codes.

> Source/WebDriver/WebDriverService.cpp:320
> +            responseObject->setString(ASCIILiteral("error"), result.errorString());

It seems like this should be written as result.errorName()?

> Source/WebDriver/WebDriverService.cpp:329
> +    replyHandler({ isScriptTimeout ? 200 : result.httpStatusCode(), responseObject->toJSONString().utf8(), ASCIILiteral("application/json; charset=utf-8") });

Huh? I don't understand this conditional.  In general, you should link to the spec for odd cases.

EDIT: I'm assuming that this is the legacy behavior warned about in the FIXME. You could extract this into a method to compute the status code and put the hack(s) in there.

> Source/WebDriver/WebDriverService.cpp:332
> +bool WebDriverService::parseCapabilities(InspectorObject& desiredCapabilities, Capabilities& capabilities, Function<void (CommandResult&&)>& completionHandler)

This does not match the latest W3C draft spec, so you'll need to improve this eventually (i.e., handle firstMatch, alwaysMatch).

> Source/WebDriver/WebDriverService.cpp:334
> +    if (!desiredCapabilities.getString(ASCIILiteral("browserName"), capabilities.browserName)) {

I found it weird, and probably wrong, that this requires these three capabilities to be specified by the REST API client in order to do anything. Typically no capabilities are required by other drivers.

The semantics of capabilities are a bit strange, especially now in W3C spec, but basically an endpoint should ignore capabilities it does not understand or that don't control anything. If the server can interpret the capability and do something with it, then it is possible for it to reject a capability value as an invalid argument. Since these capabilities aren't being used to match a particular browser, it seems like a mistake to make them required. If you start rejecting the session when these capabilities don't match the current system, then it makes more sense to validate them here or elsewhere.

> Source/WebDriver/WebDriverService.cpp:346
> +    return parsePlatformCapabilities(desiredCapabilities, capabilities, completionHandler);

Nit: platformParseCapabilities?

> Source/WebDriver/WebDriverService.cpp:378
> +    auto sessionHost = std::make_unique<SessionHost>(WTFMove(capabilities));

Do you expect to automate more than one browser instance per machine concurrently? If you want to only run one instance, then you probably want to share the host among all sessions and not allow another session request to go through if there is already an active session. Similarly, the current design might not work well if you want to reuse an already open browser instance. Then again, that might not be important if your browser is more profile-based than Safari.

(Design sidenote: safaridriver keeps one session host object per connected machine, such as Mac, simulator, device. Since by policy, we can't run multiple browsers at the same time on one machine, the host is what mediates requests to pair the service-side session with the browser-side session.)

> Source/WebDriver/WebDriverService.cpp:398
> +            capabilities->setString(ASCIILiteral("browserName"), session->capabilities().browserName);

I think this is where you would use the resolved capabilities (from requested capabilities), and also populate the capabilities from the session host directly, as it can query these platform things.

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