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

Mon Jul 17 23:40:48 PDT 2017

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

--- Comment #39 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to Brian Burg from comment #34)
> (In reply to Carlos Garcia Campos from comment #30)
> > Comment on attachment 315417 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=315417&action=review
> > 
> > >> 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.
> 
> We can get away with this, because there exists the other opt-in method for
> getting full details in the case of reporting a bug. If you don't have that
> capability available, then it might be better to propagate the error details
> by default.

Yes, I removed it.

> > >> 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.
> 
> That sounds better to me.

Cool, Handler is already used in the patch.

> > 
> > >> 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.
> 
> Yeah, this would technically be the "frontend" and the WebKit2 side is the
> "backend". It should really use service/client terminology, but it's been
> using frontend/backend since forever. :)
> 
> There currently exists ObjC and C++ generated code for the backend. There is
> ObjC and JavaScript generated code for the frontend. Hmm, so maybe this
> might be a bit more work than I thought. I could take a stab at this,
> provided I can get this code to build on Mac for testing. Let's table this
> idea for now.

Ok, let me know if I can help, I think it will be better to switch to the generated code before implementing more commands.

> > 
> > 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: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?
> 
> Pretty much. I'll try to get this working for you in a bit, after this
> version lands.

Thanks! Again, let me know if I can help :-)

> > >> 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?
> 
> Yeah.
> 
> > >> 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 :-)
> 
> Hehe, good to know.
> 
> > 
> > >> 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.
> 
> Oh, I must have forgotten my contradictory feedback =) errorString() is fine
> then.
> 
> > >> 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.
> 
> Not necessary to do this in the first patch, but yeah do look into it.

I did part of this, at least now I get the capabilities from alwaysMatch. I could also remove several FIXMEs of commands that selenium was using the legacy ones, but now in 3.4.4 they use the w3c ones. I had to rewrite the setTimeouts too, because selenium now follows the spec.

> > 
> > >> 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.
> 
> Because particular capabilities are almost completely unspecified, you'll
> want to write your own capabilities tests. Safaridriver has some to test
> things like "automatic inspection" and platform / version requests.
> 
> > 
> > >> 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.
> 
> Okay. It will really depend on the browser capabilities. For example, Safari
> can launch a new private window, but can't use profiles/launch multiple
> instances. But, it's fairly easy for safaridriver to enumerate open browsers
> and check if they can make an automation session.

The opposite here for epiphany, you can easily run multiple private instances with difference profiles, but it's not easy to enumerate the running instances. I renamed launchBrowser to connectToBrowser (that in our case we have to launch it first).

> > >> 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.
> 
> 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/20170718/5c81255a/attachment-0001.html>