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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jul 17 04:40:56 PDT 2017


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

--- Comment #30 from Carlos Garcia Campos <cgarcia at igalia.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..

Yes, I copy pasted this from InspectorBackendDispatcher.cpp

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

That's what I understood from your comment in previous review:

"For every error in this switch except ServerError (which here is correctly interpreted as a failed command), safaridriver lumps these together as an HTTP 500 / "unknown error" and propagates the underlying error in non-production builds."

So, the equivalent of non-production build in our case is the developer mode build.

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

hmm, what about using Handler instead of Client then? It's the one handling the requests in the end.

>> Source/WebDriver/HTTPServer.h:33
>> +#include <wtf/glib/GRefPtr.h>
> 
> I think you could use a forward declaration.

I'm not sure it's possible with GRefPtr.

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

Right, I'll remove it.

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

Great idea.

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

I'm not familiar with all the inspector generated code. Do we already have code generator for this? or do we have to add it? Are FrontendDispatchers what we want here, or that's actually part of the backend implementation? For now, I could get rid of the CommandResponse by simply passing both the result and error as parameters to the lambda.

The this, protectedThis is just a "trick" to avoid having to use protectedThis->m_foo everywhere inside the lambda body, improving the readability by just using m_foo.

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

Yes, I agree. I need to understand how we are going to do the generated code to fix this, though.

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

Yes, this is part of the session creation, to ensure we have a valid top level browsing context.

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

I guess with the generated code we will just do something like:

automationNavigateBrowsingContext(m_toplevelBrowsingContext, url, [] (RefPtr<InspectorValue>&& result) { });

And the generated code will handle the parameters and send the message. I guess our host here is similar to what the frontend router is in generated frontend dispatchers?

>> Source/WebDriver/Session.cpp:130
>> +        m_browsingContext = emptyString();
> 
> Please make this an optional<String>, then the line can me m_browsingContext.clear()

Ok, I guess this will be Automation::BrowsingContext with generated code?

>> Source/WebDriver/WebDriverService.cpp:227
>> +    auto lowerCaseMethod = method.convertToASCIILowercase();
> 
> Does W3C spec support HEAD or OPTIONS requests? Safaridriver did these for legacy protocol.

No, I don't think so.

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

:-D I took this idea from inspector backend dispatchers.

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

Found the problem, it was my fault. Selenium checks if status is either the legacy numeric id, or the w3c error name. The problem is that for some reason I'm returning "asynchronous script timeout" instead if "script timeout" which is what the spec says and what selenium correctly expects. So, I'll remove this hack :-)

>> Source/WebDriver/WebDriverService.cpp:320
>> +            responseObject->setString(ASCIILiteral("error"), result.errorString());
> 
> It seems like this should be written as result.errorName()?

You mean errorString() should be renamed as errorName()? I think you suggested both options in previous review. I used String in the end, because I think of it as the string representation of the error, and it contains more than 1 word (I'm not sure why, but I would expect a single word from a name). But anyway, I don't mind using one or the other.

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

Yes, that's part of the previous hack, that ended up being my fault and it will be removed :-)

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

hmm, that was not supported at all in the selenium version I was using, I've just pulled the git repo, it's now version 3.4.4, and now I see it actually uses firstMatch, alwaysMatch. I'll check it in detail.

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

The idea is that webkitgtk:browserOptions is optional, but if present it should contain all the webkitgtk specific capabilities, so webkitgtk based drivers should include those. I'll read again the capabilities mess in the spec in any case.

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

Indeed.

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

This is something I still don't know. I tried to make this implementation as close as possible to what the spec says, but maybe we should indeed limit the sessions to 1. I don't think we will allow to reuse an existing browser instance, though. since we don't target s specific browser we will encourage browser developers to add an automation mode that uses an ephemeral session and private profile.

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

Yes, I think so, if elementId is missing all they should return invalid argument. Actually, I had this in every method and then I moved to a helper to avoid duplicated code.

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

Ah, ok.

-- 
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/20170717/74d0fbd7/attachment-0001.html>


More information about the webkit-unassigned mailing list