The WebDriver process runs the HTTP server that processes the commands sent by clients and redirects them to the browser.
Do you plan to reuse existing WebDriver implementations around WebKit? There are some for QtWebKit and WPE (fork): Qt: https://github.com/cisco-open-source/qtwebdriver WPE: https://github.com/Metrological/webdriver We were about to modify the WPE webdriver fork to remove the message queues IPC (WebDriver <-> WPEProxy), and instead talk directly to the inspector. This would have been compatible with WebKitGTK as well. Though, do we really need to switch to REMOTE_INSPECTOR for this?
No, I plan to write a WebKit web driver process that runs the HTTP inspector and converts the HTTP commands into inspector commands that are forwarded to browser using the remote inspector, like the Apple ports do.
Created attachment 315232 [details] Patch Note that this is just an initial implementation. There are several commands not yet implemented and known issues to solve. However, around 80% of the selenium python tests pass already. We'll work on implementing the missing commands, and fixing bugs on top of this initial impl.
Attachment 315232 [details] did not pass style-queue: ERROR: Source/WebDriver/WebDriver.h:65: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:66: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:67: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:68: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:69: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:70: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:71: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:72: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:73: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:74: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:75: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:76: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:77: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:78: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:79: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:80: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:81: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:82: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:83: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:84: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:85: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:86: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:87: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:89: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:90: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:91: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:92: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:93: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:94: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:95: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:96: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:97: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:99: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:100: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:102: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:103: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:104: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:105: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:107: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.h:108: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:69: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:70: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:71: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:72: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:74: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:75: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:76: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:77: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:78: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:79: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:80: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:81: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:82: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:83: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:84: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:85: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:86: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:87: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:89: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:90: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:91: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:92: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:93: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:94: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:95: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:96: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:97: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:99: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:100: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:101: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:102: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:123: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:125: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:151: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:157: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:163: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:179: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:186: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:197: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:204: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverMain.cpp:28: You should add a blank line after implementation file's own header. [build/include_order] [4] ERROR: Source/WebDriver/WebDriverMain.cpp:28: You should not add a blank line before implementation file's own header. [build/include_order] [4] ERROR: Source/WebDriver/HTTPServer.h:46: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/config.h:31: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebDriver/Session.cpp:66: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:79: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:99: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:115: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:167: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:185: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:205: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:233: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:252: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:271: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:290: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:320: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:348: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:358: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:372: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:410: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:464: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:494: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:522: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:544: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:572: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:635: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:688: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:762: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:798: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:834: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:869: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:890: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:925: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:960: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:996: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:1017: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:1164: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:1236: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:1287: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:1339: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:1388: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:198: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:229: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:255: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:272: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:296: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:316: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:355: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:370: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:376: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:382: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:388: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:394: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:400: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:406: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:412: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:433: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:439: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:460: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:466: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:481: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:487: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:502: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:508: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:518: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:531: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:544: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:557: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:574: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:591: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:604: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:617: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDr iver/WebDriver.cpp:630: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:643: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:656: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:669: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:688: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:701: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:714: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:752: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:765: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:778: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriver.cpp:792: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/glib/SessionGlib.cpp:99: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/glib/SessionGlib.cpp:110: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/glib/SessionGlib.cpp:125: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/glib/SessionGlib.cpp:200: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 171 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 315232 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315232&action=review Wow, that's a bit of code. > Source/WebDriver/CommandResult.cpp:60 > + case -32700: // ParseError > + case -32600: // InvalidRequest > + case -32601: // MethodNotFound Where are these numbers defined? > Source/WebDriver/CommandResult.cpp:125 > + return 200; 500? > Source/WebDriver/Session.cpp:1046 > + case 0xE001U: How can String::operator[] return values greater than 0xFFFF? And what are these values?
Comment on attachment 315232 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315232&action=review General comments: It would be useful for you to state up front that namespace WebDriver is for HTTP service code only. You should also say what it depends on (it seems to link against JavaScriptCore and WTF and depend on files from WebKit2). > Source/WebDriver/CommandResult.cpp:36 > + : m_status(status) I think "status" is the wrong name here for the type and the member. How about std::optional<ErrorCode>? This matches what the spec calls it, in section 6.6. > Source/WebDriver/CommandResult.cpp:58 > + case -32700: // ParseError Please use an enum rather than obscure numbers, and add a note that these are defined in JSON-RPC 2.0, Section 5.1. safaridriver calls this the ProtocolErrorCode enum to differentiate from command-specific error codes (those defined in §6.6). > Source/WebDriver/CommandResult.cpp:63 > + m_errorMessage = errorMessage; Since these JSON-RPC calls to the backend are entirely generated by the web server, I don't think it makes sense to report this to the outside world as an invalid argument. It is always a programming error internal to the web service and/or the UIProcess backend. If the REST API call provided a bogus argument, then that should be checked before sending out the command to the backend. 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. This is the only error code from §6.6 that makes sense for errors we can't attribute to the user. > Source/WebDriver/CommandResult.cpp:68 > + if (errorMessage == "WindowNotFound") If you have the right generators turned on, you can instead make this automated by splitting at the first ';' if found, then calling Inspector::fromProtocolString<WDProtocolAutomationErrorMessage>(firstPart) to get an enum that matches those defined in Automation.json's "ErrorMessage" type. EDIT: oops, these helpers are only generated for the ObjC "frontend" used by safaridriver. You could easily extend this to generate enum helpers for the C++ "frontend". > Source/WebDriver/CommandResult.cpp:74 > + else if (errorMessage.startsWith("JavaScriptError;")) { I see you figured out my hacky encoding. ;-) > Source/WebDriver/CommandResult.cpp:77 > + m_errorMessage = errorMessage.substring(position + 1); You should extract the string manipulation code out to the top and use std::optional for m_errorMessage rather than an empty string. > Source/WebDriver/CommandResult.cpp:104 > + switch (m_status) { Please reference the table in §6.6. > Source/WebDriver/CommandResult.cpp:128 > +String CommandResult::error() const NIt: errorString() or errorMessage() > Source/WebDriver/CommandResult.h:40 > + Ok, Please add cross reference to the spec section that defines the status codes. > Source/WebDriver/HTTPServer.h:46 > + virtual void handleRequest(const String& method, const String& path, const char* data, size_t length, Function<void (unsigned, ResponseBody&&)>&& replyHandler) = 0; It will be a lot easier to follow this code if Request was a struct that contained all the particulars. I do appreciate that the response is async by default, something that I had to fight against in the Objective-C http library used by safaridriver. That said, it's a bit mysterious how to signal an error just by reading this signature. Our pattern is typically to make the first completion handler argument an NSError or error string, which signals an error if non-null. > Source/WebDriver/HTTPServer.h:54 > + bool listen(unsigned port); Might want to add a comment that the implementations of these are networking library-dependent. > Source/WebDriver/Session.cpp:41 > +static const String webElementIdentifier = ASCIILiteral("element-6066-11e4-a52e-4f735466cecf"); Please link to the spec, as this is a particularly confusing thing to newcomers. > Source/WebDriver/soup/HTTPServerSoup.cpp:40 > + WTFLogAlways("Failed to start HTTPS server at port %u: %s", port, error->message); Is it really https? > Source/WebDriver/soup/HTTPServerSoup.cpp:53 > + soup_message_headers_append(message->response_headers, "Content-Type", responseBody.contentType.latin1().data()); This seems wrong, the data is UTF-8 but you are sending latin1. My memory says that the response is supposed to be UTF-8 unless it's an error message (4xx or 5xx), in which case it's ASCII/latin1.
Comment on attachment 315232 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315232&action=review Um, wow. > Source/WebDriver/CMakeLists.txt:1 > +set_property(DIRECTORY . PROPERTY FOLDER "WebDriver") What is this for? Did you copy it from somewhere? > Source/WebDriver/CMakeLists.txt:29 > +set(WebDriver_SCRIPTS > + ${WEBKIT2_DIR}/UIProcess/Automation/atoms/ElementAttribute.js > + ${WEBKIT2_DIR}/UIProcess/Automation/atoms/ElementDisplayed.js > + ${WEBKIT2_DIR}/UIProcess/Automation/atoms/FindNodes.js > + ${WEBKIT2_DIR}/UIProcess/Automation/atoms/FormElementClear.js > + ${WEBKIT2_DIR}/UIProcess/Automation/atoms/FormSubmit.js > +) This is unusual layering...? > Source/WebDriver/CMakeLists.txt:46 > +add_dependencies(WebDriver WTF) Is this really needed to link? It should be inherited from JSC. > Source/WebDriver/CommandResult.h:54 > + explicit CommandResult(Status, RefPtr<Inspector::InspectorValue>&& = nullptr); Good use of explicit! > Source/WebDriver/HTTPServer.h:51 > + HTTPServer(HTTPRequestClient&); explicit. Don't want HTTPRequestClients magically turning into HTTPServers by mistake. > Source/WebDriver/HTTPServer.h:52 > + ~HTTPServer() = default; Why does this need to be declared? > Source/WebDriver/Session.cpp:117 > +long Session::sendCommandToBackend(const String& command, RefPtr<InspectorObject>&& parameters, Function<void (CommandResponse&&)>&& responseHandler) > +{ > + static long lastSequenceID = 0; long, really? Why not int64_t? Or uint64_t? >> Source/WebDriver/Session.cpp:1046 >> + case 0xE001U: > > How can String::operator[] return values greater than 0xFFFF? And what are these values? Why do you think 0xE001 is greater than 0xFFFF? :) A link to the spec would be helpful, though, yes. > Source/WebDriver/Session.h:107 > + Session(Capabilities&&); I'd use explicit here too. >> Source/WebDriver/soup/HTTPServerSoup.cpp:40 >> + WTFLogAlways("Failed to start HTTPS server at port %u: %s", port, error->message); > > Is it really https? Nope, that would require passing SOUP_SERVER_LISTEN_HTTPS for the third argument. Anyway, it's a local socket, so that would be pointless. The log message should be fixed. > Source/cmake/OptionsGTK.cmake:140 > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_WEBDRIVER PUBLIC ON) Does it depend on ENABLE_MINIBROWSER? That would require declaring a dependency between the options in OptionsGTK.cmake, and having a chat about whether to turn ENABLE_MINIBROWSER on by default or disable ENABLE_WEBDRIVER by default. I'm uncertain which would be more preferable. Am I missing something here? > Source/cmake/OptionsGTK.cmake:279 > + message(FATAL_ERROR "GLib 2.40 is required to enabled WebDriver support.") "to enable" > Source/cmake/OptionsGTK.cmake:282 > + message(FATAL_ERROR "libsoup 2.48 is required to enabled WebDriver support.") Ditto.
Comment on attachment 315232 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315232&action=review Still have a few more comments, will try to write those up later today. > Source/CMakeLists.txt:41 > +if (ENABLE_WEBDRIVER) You might want to name this ENABLE_WEBDRIVER_SERVER or ENABLE_WEBDRIVER_SERVICE. Otherwise people may think it controls the support in the engine proper, but that's actually behind ENABLE_REMOTE_AUTOMATION. > Source/WebDriver/HTTPServer.h:43 > + String data; Similar to reasons below, it will be easier to deal with the entire response if you include the HTTP code as part of the Response object. Currently these are bundled but the HTTP status code is not, for reasons I don't understand. > Source/WebDriver/Session.h:177 > + }; As I said above, you can get these enums generated by the protocol generator without much additional work. This is especially useful for error codes which are likely to be expanded over time. > Source/WebDriver/WebDriver.cpp:30 > +#include <inspector/InspectorValues.h> Maybe we should start moving InspectorValues to wtf project so that you don't need to link in JSC. > Source/WebDriver/WebDriver.cpp:38 > +WebDriver::WebDriver() I think the purpose of this class would be more obvious if this were named WebDriverService, HTTPService, or the like. > Source/WebDriver/WebDriver.cpp:107 > + { Method::Delete, "/session/$sessionID", &WebDriver::deleteSession }, A note on how safaridriver arranges the method table: Since our server was written over a year ago before W3C started converging, we designed it to support multiple REST API command sets. The HTTP service is configured with a command dialect (selenium vs w3c) at launch time. So, we have two dialects, each with a method such as - (void)_handlePostSessionWindow:(RouteRequest *)request response:(RouteResponse *)response Which corresponds to POST /session/:sessionId/window And inside the endpoint-specific method, after doing argument validation it calls into a dialect-independent implementation that does the JSON-RPC call to the remote over the Automation protocol. If you don't intend to support the legacy protocol, then this level of indirection is not strictly necessary. Just thought you might find it interesting to know. Even without the need for indirection due to dialects, you may find it worthwhile to stick to naming the entry methods after the URL template and HTTP method. If you know that a specific endpoint is failing, then it is trivial to start looking in the right method by prepping the URL template .Whereas in this patch, you need to know where this super-important vtable is to make any progress. Another way to get the same utility during debugging (which we also do) is to add comments at the header of each endpoint implementation and include a link to the spec section. You might also want to write in a comment the HTTP method, the URL template, and expected errors. > Source/WebDriver/WebDriver.cpp:145 > + { Method::Post, "/session/$sessionID/execute_async", &WebDriver::executeAsyncScript }, Unexpected dupe? > Source/WebDriver/WebDriver.cpp:280 > + Capabilities capabilities(WTFMove(desiredCapabilities)); I highly prefer you didn't entangle InspectorObject (soon to be JSON::Object) with the implementations of these auxillary classes. Instead you can use a helper method to individually parse and validate capabilities. That will also let you get rid of m_isValid in Capabilities class... > Source/WebDriver/WebDriver.cpp:282 > + completionHandler(CommandResult(CommandResult::Status::InvalidArgument)); ... and provide an actually useful error message here, since you'd know exactly which capability was invalid. BTW, per §8.1, this should return "session not created" if something goes wrong. In general, capability processing is something that will get really messy really quick. Often you may need some context from the system (not available in Capabilities, but available from the main web service class) to determine whether they capability can be matched or not. > Source/WebDriver/WebDriver.h:54 > + enum class Method { Get, Post, Delete }; > + typedef void (WebDriver::*CommandHandler)(RefPtr<Inspector::InspectorObject>&&, Function<void (CommandResult&&)>&&); Nit: HTTPMethod > Source/WebDriver/WebDriver.h:62 > + static std::optional<Method> toCommandMethod(const String& method); No reason for this to be a header declaration. > Source/WebDriver/WebDriver.h:63 > + static bool findCommand(const String& method, const String& path, CommandHandler*, HashMap<String, String>& parameters); Ditto. > Source/WebDriver/glib/SessionGlib.cpp:127 > + m_cancellable = adoptGRef(g_cancellable_new()); Heh, there's a lot of fun stuff here, a lot of which I don't understand due to being GTK/dbus API. In safaridriver we eventually extracted all remote connection bootstrap code into a separate class called SessionHost. That way, the Session class can assume it has a valid connection and only really cares about processing commands. It only uses the session host to send out messages and close the session. We don't even vend out the Session class to the web service (i.e., WebDriver class here) until it's been fully initialized so that it can't be used in an uninitialized state. Not sure how relevant here, but such a design also better isolates the platform-specific peering/bootstrap code from cross-platform bits. Due to sandboxing reasons, we also could not rely on the assumption that the browser is a subprocess of the web service, so we use a fully async state machine to keep track of the browser coming up and going down. If you really can assume it will be a subprocess, and you d on't need to connect to an existing browser, then it's understandable that your connection code is more straightforward w.r.t. asynchrony. > Source/WebDriver/glib/SessionGlib.cpp:152 > + connectToBrowser(std::make_unique<ConnectToBrowserAsyncData>(this, WTFMove(dbusAddress), m_cancellable.get(), WTFMove(completionHandler))); You should think about how you want to report errors that happen when launching/connecting to the browser, preferably out to the HTTP response so users can read the actual error. > Source/WebDriver/gtk/CapabilitiesGtk.cpp:35 > +bool Capabilities::parsePlatformCapabilities(RefPtr<InspectorObject>&& desiredCapabilities) As I suggested above, this makes more sense to be as a platform method of WebDriverService rather than a pseudo-factory method. Trust me, it will be a lot easier down the line when Capabilities is just a string-to-string map that's been merged and normalized to something that's meaningful to the webdriver service implementation. > Source/WebDriver/gtk/CapabilitiesGtk.cpp:38 > + if (!desiredCapabilities->getObject(ASCIILiteral("browserOptions"), browserOptions)) { Per §6.7, I believe that extension capabilities are supposed to have a prefix, like so: "webkitgtk:browserOptions" : { ... }
(In reply to Michael Catanzaro from comment #7) > Comment on attachment 315232 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=315232&action=review > > > Source/WebDriver/CMakeLists.txt:29 > > +set(WebDriver_SCRIPTS > > + ${WEBKIT2_DIR}/UIProcess/Automation/atoms/ElementAttribute.js > > + ${WEBKIT2_DIR}/UIProcess/Automation/atoms/ElementDisplayed.js > > + ${WEBKIT2_DIR}/UIProcess/Automation/atoms/FindNodes.js > > + ${WEBKIT2_DIR}/UIProcess/Automation/atoms/FormElementClear.js > > + ${WEBKIT2_DIR}/UIProcess/Automation/atoms/FormSubmit.js > > +) > > This is unusual layering...? WebKit2 is where most of WebDriver support in WebKit is implemented. And, for safaridriver and webkitgtkdriver to share the same atoms and work in Apple's wonky production build system, the atoms need to be copyable as framework private headers. In any case, WebDriver should be building after WebKit2. We build safaridriver as a dependent of Safari itself due to some shared code.
Is this intended to replace or merge with or be used by safaridriver in some distant future, or will they always be separate implementations?
(In reply to Michael Catanzaro from comment #10) > Is this intended to replace or merge with or be used by safaridriver in some > distant future, or will they always be separate implementations? I wouldn't rule it out, but it seems unlikely given how many integrations with system frameworks we have already. Most of the hard work (atoms and code in WebKit2) is shared.
Wow, thank you guys for quick review (it's not a small patch) and all the comments.
(In reply to Alex Christensen from comment #5) > Comment on attachment 315232 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=315232&action=review > > Wow, that's a bit of code. Indeed. > > Source/WebDriver/CommandResult.cpp:60 > > + case -32700: // ParseError > > + case -32600: // InvalidRequest > > + case -32601: // MethodNotFound > > Where are these numbers defined? JSON-RPC 2.0, I copied them from BackendDispatcher::sendPendingErrors(). > > Source/WebDriver/CommandResult.cpp:125 > > + return 200; > > 500? I think it doesn't really matter, this is mostly to silence GCC. > > Source/WebDriver/Session.cpp:1046 > > + case 0xE001U: > > How can String::operator[] return values greater than 0xFFFF? And what are > these values? Copied the values from selenium, see https://github.com/SeleniumHQ/selenium/blob/master/cpp/webdriver-interactions/keycodes.h
(In reply to Brian Burg from comment #6) > Comment on attachment 315232 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=315232&action=review > > General comments: > > It would be useful for you to state up front that namespace WebDriver is for > HTTP service code only. Ok. > You should also say what it depends on (it seems to link against > JavaScriptCore and WTF and depend on files from WebKit2). Ideally, it should only depend on WTF, I had to link to JSC only because of InspectorValues, and I don't consider it depends on WebKit2 either, it's only using static resources that are located under WebKit2 directory because that's how safari needs it. But I agree we can calrify all this in the ChangeLog. > > Source/WebDriver/CommandResult.cpp:36 > > + : m_status(status) > > I think "status" is the wrong name here for the type and the member. How > about std::optional<ErrorCode>? This matches what the spec calls it, in > section 6.6. Ok, I'll rework it. > > Source/WebDriver/CommandResult.cpp:58 > > + case -32700: // ParseError > > Please use an enum rather than obscure numbers, and add a note that these > are defined in JSON-RPC 2.0, Section 5.1. > > safaridriver calls this the ProtocolErrorCode enum to differentiate from > command-specific error codes (those defined in §6.6). Ok. > > Source/WebDriver/CommandResult.cpp:63 > > + m_errorMessage = errorMessage; > > Since these JSON-RPC calls to the backend are entirely generated by the web > server, I don't think it makes sense to report this to the outside world as > an invalid argument. It is always a programming error internal to the web > service and/or the UIProcess backend. If the REST API call provided a bogus > argument, then that should be checked before sending out the command to the > backend. > > 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. This is the only error code from §6.6 that makes > sense for errors we can't attribute to the user. It makes sense, I'll do the same. > > Source/WebDriver/CommandResult.cpp:68 > > + if (errorMessage == "WindowNotFound") > > If you have the right generators turned on, you can instead make this > automated by splitting at the first ';' if found, then calling > Inspector::fromProtocolString<WDProtocolAutomationErrorMessage>(firstPart) > to get an enum that matches those defined in Automation.json's > "ErrorMessage" type. > > EDIT: oops, these helpers are only generated for the ObjC "frontend" used by > safaridriver. You could easily extend this to generate enum helpers for the > C++ "frontend". hmm, I guess the will create a real dependency on WebKit2, right? If it's only a matter of including a header with enum definitions then it could be ok, but I don't think we want to link to WebKit2. > > Source/WebDriver/CommandResult.cpp:74 > > + else if (errorMessage.startsWith("JavaScriptError;")) { > > I see you figured out my hacky encoding. ;-) > > > Source/WebDriver/CommandResult.cpp:77 > > + m_errorMessage = errorMessage.substring(position + 1); > > You should extract the string manipulation code out to the top and use > std::optional for m_errorMessage rather than an empty string. I tried to optimize here and avoid String copies, but it isn't worth it all. > > Source/WebDriver/CommandResult.cpp:104 > > + switch (m_status) { > > Please reference the table in §6.6. Ok. > > Source/WebDriver/CommandResult.cpp:128 > > +String CommandResult::error() const > > NIt: errorString() or errorMessage() I'll use errorString() for the string representation of an error code and errorMessage() for the optional error description. > > Source/WebDriver/CommandResult.h:40 > > + Ok, > > Please add cross reference to the spec section that defines the status codes. > > > Source/WebDriver/HTTPServer.h:46 > > + virtual void handleRequest(const String& method, const String& path, const char* data, size_t length, Function<void (unsigned, ResponseBody&&)>&& replyHandler) = 0; > > It will be a lot easier to follow this code if Request was a struct that > contained all the particulars. I do appreciate that the response is async by > default, something that I had to fight against in the Objective-C http > library used by safaridriver. That said, it's a bit mysterious how to signal > an error just by reading this signature. Our pattern is typically to make > the first completion handler argument an NSError or error string, which > signals an error if non-null. I agree, I'll add Request and renamed ResponseBody as Response including also the http status code. This was we always sent a Request and reply with a Response. > > Source/WebDriver/HTTPServer.h:54 > > + bool listen(unsigned port); > > Might want to add a comment that the implementations of these are networking > library-dependent. > > > Source/WebDriver/Session.cpp:41 > > +static const String webElementIdentifier = ASCIILiteral("element-6066-11e4-a52e-4f735466cecf"); > > Please link to the spec, as this is a particularly confusing thing to > newcomers. > > > Source/WebDriver/soup/HTTPServerSoup.cpp:40 > > + WTFLogAlways("Failed to start HTTPS server at port %u: %s", port, error->message); > > Is it really https? No, it's a typo or a copy-paste error, I don't remember :-) > > Source/WebDriver/soup/HTTPServerSoup.cpp:53 > > + soup_message_headers_append(message->response_headers, "Content-Type", responseBody.contentType.latin1().data()); > > This seems wrong, the data is UTF-8 but you are sending latin1. My memory > says that the response is supposed to be UTF-8 unless it's an error message > (4xx or 5xx), in which case it's ASCII/latin1. This is not the data, but the "Content-Type" HTTP header, but you are right, we always pass utf8 to soup. It doesn't make any difference in this case, because we are always receiving an ASCII literal here "application/json; charset=utf-8".
(In reply to Michael Catanzaro from comment #7) > Comment on attachment 315232 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=315232&action=review > > Um, wow. > > > Source/WebDriver/CMakeLists.txt:1 > > +set_property(DIRECTORY . PROPERTY FOLDER "WebDriver") > > What is this for? Did you copy it from somewhere? I have no idea to be honest, I guess I copied it from other wk cmake files. > > Source/WebDriver/CMakeLists.txt:29 > > +set(WebDriver_SCRIPTS > > + ${WEBKIT2_DIR}/UIProcess/Automation/atoms/ElementAttribute.js > > + ${WEBKIT2_DIR}/UIProcess/Automation/atoms/ElementDisplayed.js > > + ${WEBKIT2_DIR}/UIProcess/Automation/atoms/FindNodes.js > > + ${WEBKIT2_DIR}/UIProcess/Automation/atoms/FormElementClear.js > > + ${WEBKIT2_DIR}/UIProcess/Automation/atoms/FormSubmit.js > > +) > > This is unusual layering...? These are there only because safari needs it, they are static resources we build with WebDriver namespace as part of the web driver process, so in practice this is part of web driver for us. > > Source/WebDriver/CMakeLists.txt:46 > > +add_dependencies(WebDriver WTF) > > Is this really needed to link? It should be inherited from JSC. I started it with WTF as the only dependency, then I had to add JCS because of InspectorValues, and I forgot to update this. I guess it's not needed in any case, since the dep is already implicit as you say. > > Source/WebDriver/CommandResult.h:54 > > + explicit CommandResult(Status, RefPtr<Inspector::InspectorValue>&& = nullptr); > > Good use of explicit! > > > Source/WebDriver/HTTPServer.h:51 > > + HTTPServer(HTTPRequestClient&); > > explicit. Don't want HTTPRequestClients magically turning into HTTPServers > by mistake. Right. > > Source/WebDriver/HTTPServer.h:52 > > + ~HTTPServer() = default; > > Why does this need to be declared? I think in the past we always had to define destructors when using smart pointers, I don't know if that's still true, but I keep doing it, even using = default. If it's no longer needed, I can simply remove it. > > Source/WebDriver/Session.cpp:117 > > +long Session::sendCommandToBackend(const String& command, RefPtr<InspectorObject>&& parameters, Function<void (CommandResponse&&)>&& responseHandler) > > +{ > > + static long lastSequenceID = 0; > > long, really? Why not int64_t? Or uint64_t? I used long because that's what IdentifiersFactory use. > >> Source/WebDriver/Session.cpp:1046 > >> + case 0xE001U: > > > > How can String::operator[] return values greater than 0xFFFF? And what are these values? > > Why do you think 0xE001 is greater than 0xFFFF? :) > > A link to the spec would be helpful, though, yes. Agree, I'll add a link to https://www.w3.org/TR/webdriver/#keyboard-actions > > Source/WebDriver/Session.h:107 > > + Session(Capabilities&&); > > I'd use explicit here too. It's private constructor, I guess it can be used implicitly? > >> Source/WebDriver/soup/HTTPServerSoup.cpp:40 > >> + WTFLogAlways("Failed to start HTTPS server at port %u: %s", port, error->message); > > > > Is it really https? > > Nope, that would require passing SOUP_SERVER_LISTEN_HTTPS for the third > argument. Anyway, it's a local socket, so that would be pointless. The log > message should be fixed. Right, I'll fix it. > > Source/cmake/OptionsGTK.cmake:140 > > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_WEBDRIVER PUBLIC ON) > > Does it depend on ENABLE_MINIBROWSER? That would require declaring a > dependency between the options in OptionsGTK.cmake, and having a chat about > whether to turn ENABLE_MINIBROWSER on by default or disable ENABLE_WEBDRIVER > by default. I'm uncertain which would be more preferable. > > Am I missing something here? Yes. It doesn't depend on any browser, actually. The selenium bindings are the ones specifying the browser to use. The idea is to create a generic WebKitGTK driver with browser specific options, including the binary path and command line arguments, passed as capabilities to the driver server. Then Epiphany, Midori, etc. can easily add their own driver to selenium on top of the generic one just by providing the browser options. You can also use the generic driver directly and provide the options you want. MiniBrowser is just the default browser used only by the GTK+ port when browser options are not provided by selenium driver. If MB is not installed, it will fail to launch the browser and session could not be created error will be generated. So, there's no dependency. > > Source/cmake/OptionsGTK.cmake:279 > > + message(FATAL_ERROR "GLib 2.40 is required to enabled WebDriver support.") > > "to enable" Oops. > > Source/cmake/OptionsGTK.cmake:282 > > + message(FATAL_ERROR "libsoup 2.48 is required to enabled WebDriver support.") > > Ditto. Oops.
(In reply to Brian Burg from comment #8) > Comment on attachment 315232 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=315232&action=review > > Still have a few more comments, will try to write those up later today. Great, I'm, already updating things. > > Source/CMakeLists.txt:41 > > +if (ENABLE_WEBDRIVER) > > You might want to name this ENABLE_WEBDRIVER_SERVER or > ENABLE_WEBDRIVER_SERVICE. Otherwise people may think it controls the support > in the engine proper, but that's actually behind ENABLE_REMOTE_AUTOMATION. I didn't do that because we already have that distinction, we use WebAutomation for the support to control the browser, and it's enabled unconditionally anyway. I tried to follow what other browsers and selenium seem to do, which is referring to the server/service as the driver (chromedriver, firefoxdriver, safaridriver, etc.) > > Source/WebDriver/HTTPServer.h:43 > > + String data; > > Similar to reasons below, it will be easier to deal with the entire response > if you include the HTTP code as part of the Response object. Currently these > are bundled but the HTTP status code is not, for reasons I don't understand. Me neither :-P I don't remember why I did it that way. Anyway, as I said before I've added Request and renamed ResponseBody as Response including the status code as you suggests. > > Source/WebDriver/Session.h:177 > > + }; > > As I said above, you can get these enums generated by the protocol generator > without much additional work. This is especially useful for error codes > which are likely to be expanded over time. Yes, I'm afraid of adding a real dependency on WebKit2. > > Source/WebDriver/WebDriver.cpp:30 > > +#include <inspector/InspectorValues.h> > > Maybe we should start moving InspectorValues to wtf project so that you > don't need to link in JSC. Indeed, ideally WebDriver process should only link to WTF. Do you plan to do it, or do you want me to give it a try? Maybe it could be done in two steps, since InspectorValues are used in several places and even generated code, first we add WTF::JSON and make InspectorValues use it as a simple wrapper, and then we can incrementally replace all uses of InspectorValues with WTF::JSON without having to do it in a single huge patch. > > Source/WebDriver/WebDriver.cpp:38 > > +WebDriver::WebDriver() > > I think the purpose of this class would be more obvious if this were named > WebDriverService, HTTPService, or the like. I always think of WebDriver as the service, but I'm fine with using WebDriverService. > > Source/WebDriver/WebDriver.cpp:107 > > + { Method::Delete, "/session/$sessionID", &WebDriver::deleteSession }, > > A note on how safaridriver arranges the method table: > > Since our server was written over a year ago before W3C started converging, > we designed it to support multiple REST API command sets. The HTTP service > is configured with a command dialect (selenium vs w3c) at launch time. So, > we have two dialects, each with a method such as > > - (void)_handlePostSessionWindow:(RouteRequest *)request > response:(RouteResponse *)response > > Which corresponds to > > POST /session/:sessionId/window > > And inside the endpoint-specific method, after doing argument validation it > calls into a dialect-independent implementation that does the JSON-RPC call > to the remote over the Automation protocol. > If you don't intend to support the legacy protocol, then this level of > indirection is not strictly necessary. Just thought you might find it > interesting to know. My initial idea is to not support legacy stuff in this new implementation, unless it's really needed. So, for now, and for the sake of simplicity, I've focused on making it w3c compliant only. > Even without the need for indirection due to dialects, you may find it > worthwhile to stick to naming the entry methods after the URL template and > HTTP method. I used the spec terminology for the methods, which is think is more decriptive, so that the names match the spec sections. > If you know that a specific endpoint is failing, then it is trivial to start > looking in the right method by prepping the URL template > .Whereas in this patch, you need to know where this super-important vtable > is to make any progress. hmm, yes, or check the spec to know it. > Another way to get the same utility during debugging (which we also do) is > to add comments at the header of each endpoint implementation and > include a link to the spec section. You might also want to write in a > comment the HTTP method, the URL template, and expected errors. I'll definitely do it. > > Source/WebDriver/WebDriver.cpp:145 > > + { Method::Post, "/session/$sessionID/execute_async", &WebDriver::executeAsyncScript }, > > Unexpected dupe? Not really, this is because selenium (at least the version I'm currently using), defines EXECUTE_ASYNC_SCRIPT with execute_async instead of execute/async. I guess it's a bug, but I'm not sure. I'll add a comment, and I'll check current selenium or file a bug report. > > Source/WebDriver/WebDriver.cpp:280 > > + Capabilities capabilities(WTFMove(desiredCapabilities)); > > I highly prefer you didn't entangle InspectorObject (soon to be > JSON::Object) with the implementations of these auxillary classes. Instead > you can use a helper method to individually parse and validate capabilities. > That will also let you get rid of m_isValid in Capabilities class... > > > Source/WebDriver/WebDriver.cpp:282 > > + completionHandler(CommandResult(CommandResult::Status::InvalidArgument)); > > ... and provide an actually useful error message here, since you'd know > exactly which capability was invalid. Ok, then we can make Capabilities a simple struct, instead of a class. > BTW, per §8.1, this should return "session not created" if something goes > wrong. Right! > In general, capability processing is something that will get really messy > really quick. Often you may need some context from the system (not available > in Capabilities, but available from the main web service class) to determine > whether they capability can be matched or not. > > > Source/WebDriver/WebDriver.h:54 > > + enum class Method { Get, Post, Delete }; > > + typedef void (WebDriver::*CommandHandler)(RefPtr<Inspector::InspectorObject>&&, Function<void (CommandResult&&)>&&); > > Nit: HTTPMethod > > > Source/WebDriver/WebDriver.h:62 > > + static std::optional<Method> toCommandMethod(const String& method); > > No reason for this to be a header declaration. HTTPMethod enum is private. > > Source/WebDriver/WebDriver.h:63 > > + static bool findCommand(const String& method, const String& path, CommandHandler*, HashMap<String, String>& parameters); > > Ditto. HTTPMethod enum and Command struct and the commands array are private too. > > Source/WebDriver/glib/SessionGlib.cpp:127 > > + m_cancellable = adoptGRef(g_cancellable_new()); > > Heh, there's a lot of fun stuff here, a lot of which I don't understand due > to being GTK/dbus API. > > In safaridriver we eventually extracted all remote connection bootstrap code > into a separate class called SessionHost. That way, the Session class can > assume it has a valid connection and only really cares about processing > commands. It only uses the session host to send out messages and close the > session. We don't even vend out the Session class to the web service (i.e., > WebDriver class here) until it's been fully initialized so that it can't be > used in an uninitialized state. hmm, I never liked that part of the code either, I'll try to add the SessionHost class. > Not sure how relevant here, but such a design also better isolates the > platform-specific peering/bootstrap code from cross-platform bits. > > Due to sandboxing reasons, we also could not rely on the assumption that the > browser is a subprocess of the web service, so we use a fully async state > machine to keep track of the browser coming up and going down. If you really > can assume it will be a subprocess, and you d on't need to connect to an > existing browser, then it's understandable that your connection code is more > straightforward w.r.t. asynchrony. > > > Source/WebDriver/glib/SessionGlib.cpp:152 > > + connectToBrowser(std::make_unique<ConnectToBrowserAsyncData>(this, WTFMove(dbusAddress), m_cancellable.get(), WTFMove(completionHandler))); > > You should think about how you want to report errors that happen when > launching/connecting to the browser, preferably out to the HTTP response so > users can read the actual error. > > > Source/WebDriver/gtk/CapabilitiesGtk.cpp:35 > > +bool Capabilities::parsePlatformCapabilities(RefPtr<InspectorObject>&& desiredCapabilities) > > As I suggested above, this makes more sense to be as a platform method of > WebDriverService rather than a pseudo-factory method. Trust me, it will be a > lot easier down the line when Capabilities is just a string-to-string map > that's been merged and normalized to something that's meaningful to the > webdriver service implementation. Sure, I already reworked it. > > Source/WebDriver/gtk/CapabilitiesGtk.cpp:38 > > + if (!desiredCapabilities->getObject(ASCIILiteral("browserOptions"), browserOptions)) { > > Per §6.7, I believe that extension capabilities are supposed to have a > prefix, like so: > > "webkitgtk:browserOptions" : { ... } Added webkitgtk prefix.
Created attachment 315346 [details] Patch Updated patch. I think I've addressed most of the comments. I haven't added the comments pointing to the spec in the command definitions yet, but I haven't forgotten, it's just that I haven't had time and I wanted to submit a new patch to get feedback for all other updates. I'll continue tomorrow.
Attachment 315346 [details] did not pass style-queue: ERROR: Source/WebDriver/SessionHost.cpp:47: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:58: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:59: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:65: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:81: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:83: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:93: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:97: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:63: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:64: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:65: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:66: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:68: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:69: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:70: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:71: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:72: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:73: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:74: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:75: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:76: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:77: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:78: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:79: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:80: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:81: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:82: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:83: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:84: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:85: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:86: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:87: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:89: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:90: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:91: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:92: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:93: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:94: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:95: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:96: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:136: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:152: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:66: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:67: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:68: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:69: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:70: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:71: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:72: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:73: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:74: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:75: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:76: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:77: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:78: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:79: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:80: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:81: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:82: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:83: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:84: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:85: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:86: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:87: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:89: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:90: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:91: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:92: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:93: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:94: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:95: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:96: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:97: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:99: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:100: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:101: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:103: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:104: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:105: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:106: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:107: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:108: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:110: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:111: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/HTTPServer.h:53: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/config.h:31: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebDriver/glib/SessionHostGlib.cpp:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/glib/SessionHostGlib.cpp:104: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/glib/SessionHostGlib.cpp:115: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/glib/SessionHostGlib.cpp:130: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/glib/SessionHostGlib.cpp:205: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/glib/SessionHostGlib.cpp:222: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:60: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:79: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:95: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:115: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:135: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:163: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:182: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:201: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:220: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:250: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:278: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:288: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:302: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:340: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:394: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:424: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:452: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:474: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:502: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:565: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:618: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:692: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:728: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:764: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:799: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:820: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:855: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:890: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:926: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:947: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:1096: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:1168: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:1219: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:1271: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:1320: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:200: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:231: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:257: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:274: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:291: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:332: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:352: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:391: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:406: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:412: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:418: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:424: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:430: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:436: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:442: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:448: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:469: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:475: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:496: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:502: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:517: Extra space before ( in function call [whitespace /parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:523: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:538: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:544: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:554: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:567: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:580: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:593: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:610: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:627: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:640: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:653: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:666: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:679: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:692: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:705: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:724: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:737: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:750: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:788: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:801: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:814: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:828: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/gtk/WebDriverServiceGtk.cpp:37: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 175 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 315348 [details] Patch
Attachment 315348 [details] did not pass style-queue: ERROR: Source/WebDriver/SessionHost.cpp:47: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:58: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:59: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:65: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:81: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:83: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:93: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:97: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:63: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:64: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:65: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:66: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:68: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:69: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:70: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:71: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:72: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:73: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:74: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:75: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:76: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:77: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:78: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:79: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:80: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:81: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:82: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:83: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:84: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:85: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:86: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:87: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:89: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:90: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:91: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:92: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:93: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:94: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:95: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:96: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:136: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:152: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:66: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:67: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:68: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:69: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:70: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:71: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:72: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:73: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:74: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:75: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:76: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:77: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:78: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:79: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:80: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:81: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:82: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:83: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:84: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:85: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:86: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:87: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:89: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:90: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:91: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:92: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:93: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:94: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:95: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:96: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:97: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:99: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:100: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:101: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:103: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:104: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:105: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:106: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:107: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:108: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:110: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:111: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/HTTPServer.h:53: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/config.h:31: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebDriver/glib/SessionHostGlib.cpp:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/glib/SessionHostGlib.cpp:104: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/glib/SessionHostGlib.cpp:115: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/glib/SessionHostGlib.cpp:130: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/glib/SessionHostGlib.cpp:205: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/glib/SessionHostGlib.cpp:222: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:60: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:79: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:95: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:115: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:135: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:163: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:182: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:201: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:220: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:250: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:278: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:288: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:302: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:340: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:394: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:424: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:452: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:474: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:502: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:565: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:618: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:692: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:728: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:764: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:799: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:820: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:855: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:890: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:926: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:947: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:1096: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:1168: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:1219: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:1271: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:1320: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:200: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:231: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:257: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:274: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:291: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:332: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:352: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:391: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:406: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:412: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:418: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:424: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:430: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:436: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:442: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:448: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:469: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:475: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:496: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:502: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:517: Extra space before ( in function call [whitespace /parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:523: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:538: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:544: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:554: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:567: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:580: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:593: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:610: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:627: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:640: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:653: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:666: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:679: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:692: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:705: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:724: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:737: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:750: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:788: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:801: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:814: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:828: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/gtk/WebDriverServiceGtk.cpp:37: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 175 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to Carlos Garcia Campos from comment #16) > (In reply to Brian Burg from comment #8) > > Comment on attachment 315232 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=315232&action=review > > > > Still have a few more comments, will try to write those up later today. > > Great, I'm, already updating things. Exciting! > > > Source/CMakeLists.txt:41 > > > +if (ENABLE_WEBDRIVER) > > > > You might want to name this ENABLE_WEBDRIVER_SERVER or > > ENABLE_WEBDRIVER_SERVICE. Otherwise people may think it controls the support > > in the engine proper, but that's actually behind ENABLE_REMOTE_AUTOMATION. > > I didn't do that because we already have that distinction, we use > WebAutomation for the support to control the browser, and it's enabled > unconditionally anyway. I tried to follow what other browsers and selenium > seem to do, which is referring to the server/service as the driver > (chromedriver, firefoxdriver, safaridriver, etc.) Should we name the executable 'webkitdriver', then, if this is not specific to a particular browser? I could imagine each GTK-based browser shipping a wrapper with a name like midoridriver, which is really just calling into webkitdriver with some presets. I think it's fine to call the executable webkitdriver. But internal to its implementation, the web server/service part is fairly distinct from the command line parsing, talking to the remote, the concept of a "session", and other things not related to handling REST API requests. That's why I advocate that it have a more specific name. :-) > > > Source/WebDriver/HTTPServer.h:43 > > > + String data; > > > > Similar to reasons below, it will be easier to deal with the entire response > > if you include the HTTP code as part of the Response object. Currently these > > are bundled but the HTTP status code is not, for reasons I don't understand. > > Me neither :-P I don't remember why I did it that way. Anyway, as I said > before I've added Request and renamed ResponseBody as Response including the > status code as you suggests. > > > > Source/WebDriver/Session.h:177 > > > + }; > > > > As I said above, you can get these enums generated by the protocol generator > > without much additional work. This is especially useful for error codes > > which are likely to be expanded over time. > > Yes, I'm afraid of adding a real dependency on WebKit2. IIRC, safaridriver copies headers out of WK2 but doesn't link against it. The generated "frontend" code is compiled in safaridriver's framework, not in WebKit2's framework (the "backend"). > > > Source/WebDriver/WebDriver.cpp:30 > > > +#include <inspector/InspectorValues.h> > > > > Maybe we should start moving InspectorValues to wtf project so that you > > don't need to link in JSC. > > Indeed, ideally WebDriver process should only link to WTF. Do you plan to do > it, or do you want me to give it a try? Maybe it could be done in two steps, > since InspectorValues are used in several places and even generated code, > first we add WTF::JSON and make InspectorValues use it as a simple wrapper, > and then we can incrementally replace all uses of InspectorValues with > WTF::JSON without having to do it in a single huge patch. I have some patches to clean up InspectorValues so that they can be moved around easily (say, to WTF) and otherwise improved (say, to be more spec compliant). But don't wait for that- it's kind of tedious to push through and land, and my machines don't have much spare build time right now. > > And inside the endpoint-specific method, after doing argument validation it > > calls into a dialect-independent implementation that does the JSON-RPC call > > to the remote over the Automation protocol. > > If you don't intend to support the legacy protocol, then this level of > > indirection is not strictly necessary. Just thought you might find it > > interesting to know. > > My initial idea is to not support legacy stuff in this new implementation, > unless it's really needed. So, for now, and for the sake of simplicity, I've > focused on making it w3c compliant only. That's fair! > > > Even without the need for indirection due to dialects, you may find it > > worthwhile to stick to naming the entry methods after the URL template and > > HTTP method. > > I used the spec terminology for the methods, which is think is more > decriptive, so that the names match the spec sections. OK, that's fair too.
(In reply to Brian Burg from comment #21) > (In reply to Carlos Garcia Campos from comment #16) > > (In reply to Brian Burg from comment #8) > > > Comment on attachment 315232 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=315232&action=review > > > > > > Still have a few more comments, will try to write those up later today. > > > > Great, I'm, already updating things. > > Exciting! > > > > > Source/CMakeLists.txt:41 > > > > +if (ENABLE_WEBDRIVER) > > > > > > You might want to name this ENABLE_WEBDRIVER_SERVER or > > > ENABLE_WEBDRIVER_SERVICE. Otherwise people may think it controls the support > > > in the engine proper, but that's actually behind ENABLE_REMOTE_AUTOMATION. > > > > I didn't do that because we already have that distinction, we use > > WebAutomation for the support to control the browser, and it's enabled > > unconditionally anyway. I tried to follow what other browsers and selenium > > seem to do, which is referring to the server/service as the driver > > (chromedriver, firefoxdriver, safaridriver, etc.) > > Should we name the executable 'webkitdriver', then, if this is not specific > to a particular browser? I could imagine each GTK-based browser shipping a > wrapper with a name like midoridriver, which is really just calling into > webkitdriver with some presets. No, the exec name was just an example, the point is that other browser refer to the driver as the service/server process. In GTK+ port the process is WebKitWebDriver. WebKitGTK+ based browsers are not expected to provide their own driver, but use the WebKitGTK+ one. They are expected to add a driver implementation in selenium, on top of the WebKitGTK one, that simply passes the browser options that will be included in the capabilities. > I think it's fine to call the executable webkitdriver. But internal to its > implementation, the web server/service part is fairly distinct from the > command line parsing, talking to the remote, the concept of a "session", and > other things not related to handling REST API requests. That's why I > advocate that it have a more specific name. :-) Sure, I already renamed WebDriver class as WebDriverService, see the latest patch attached here. > > > > Source/WebDriver/HTTPServer.h:43 > > > > + String data; > > > > > > Similar to reasons below, it will be easier to deal with the entire response > > > if you include the HTTP code as part of the Response object. Currently these > > > are bundled but the HTTP status code is not, for reasons I don't understand. > > > > Me neither :-P I don't remember why I did it that way. Anyway, as I said > > before I've added Request and renamed ResponseBody as Response including the > > status code as you suggests. > > > > > > Source/WebDriver/Session.h:177 > > > > + }; > > > > > > As I said above, you can get these enums generated by the protocol generator > > > without much additional work. This is especially useful for error codes > > > which are likely to be expanded over time. > > > > Yes, I'm afraid of adding a real dependency on WebKit2. > > IIRC, safaridriver copies headers out of WK2 but doesn't link against it. > The generated "frontend" code is compiled in safaridriver's framework, not > in WebKit2's framework (the "backend"). Ok, that sounds better, I'll look at it in more detail. > > > > Source/WebDriver/WebDriver.cpp:30 > > > > +#include <inspector/InspectorValues.h> > > > > > > Maybe we should start moving InspectorValues to wtf project so that you > > > don't need to link in JSC. > > > > Indeed, ideally WebDriver process should only link to WTF. Do you plan to do > > it, or do you want me to give it a try? Maybe it could be done in two steps, > > since InspectorValues are used in several places and even generated code, > > first we add WTF::JSON and make InspectorValues use it as a simple wrapper, > > and then we can incrementally replace all uses of InspectorValues with > > WTF::JSON without having to do it in a single huge patch. > > I have some patches to clean up InspectorValues so that they can be moved > around easily (say, to WTF) and otherwise improved (say, to be more spec > compliant). But don't wait for that- it's kind of tedious to push through > and land, and my machines don't have much spare build time right now. Ok, great. > > > And inside the endpoint-specific method, after doing argument validation it > > > calls into a dialect-independent implementation that does the JSON-RPC call > > > to the remote over the Automation protocol. > > > If you don't intend to support the legacy protocol, then this level of > > > indirection is not strictly necessary. Just thought you might find it > > > interesting to know. > > > > My initial idea is to not support legacy stuff in this new implementation, > > unless it's really needed. So, for now, and for the sake of simplicity, I've > > focused on making it w3c compliant only. > > That's fair! > > > > > > Even without the need for indirection due to dialects, you may find it > > > worthwhile to stick to naming the entry methods after the URL template and > > > HTTP method. > > > > I used the spec terminology for the methods, which is think is more > > decriptive, so that the names match the spec sections. > > OK, that's fair too.
(In reply to Carlos Garcia Campos from comment #22) > > > > > > > > As I said above, you can get these enums generated by the protocol generator > > > > without much additional work. This is especially useful for error codes > > > > which are likely to be expanded over time. > > > > > > Yes, I'm afraid of adding a real dependency on WebKit2. > > > > IIRC, safaridriver copies headers out of WK2 but doesn't link against it. > > The generated "frontend" code is compiled in safaridriver's framework, not > > in WebKit2's framework (the "backend"). > > Ok, that sounds better, I'll look at it in more detail. First we need to add support for generating the frontend C++ code, currently generate-inspector-protocol-bindings.py --frontend only generates Objc code. So, I'll leave it for follow up patches.
Created attachment 315417 [details] Patch
Attachment 315417 [details] did not pass style-queue: ERROR: Source/WebDriver/SessionHost.cpp:47: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:58: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:59: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:65: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:81: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:83: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:93: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:97: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:63: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:64: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:65: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:66: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:68: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:69: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:70: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:71: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:72: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:73: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:74: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:75: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:76: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:77: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:78: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:79: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:80: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:81: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:82: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:83: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:84: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:85: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:86: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:87: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:89: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:90: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:91: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:92: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:93: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:94: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:95: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:96: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:136: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:152: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:66: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:67: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:68: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:69: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:70: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:71: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:72: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:73: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:74: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:75: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:76: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:77: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:78: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:79: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:80: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:81: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:82: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:83: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:84: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:85: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:86: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:87: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:89: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:90: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:91: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:92: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:93: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:94: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:95: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:96: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:97: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:99: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:100: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:101: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:103: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:104: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:105: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:106: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:107: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:108: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:110: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:111: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/HTTPServer.h:53: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/config.h:31: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebDriver/glib/SessionHostGlib.cpp:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/glib/SessionHostGlib.cpp:104: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/glib/SessionHostGlib.cpp:115: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/glib/SessionHostGlib.cpp:130: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/glib/SessionHostGlib.cpp:205: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/glib/SessionHostGlib.cpp:222: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:60: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:79: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:95: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:115: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:135: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:163: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:182: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:201: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:220: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:250: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:278: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:288: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:302: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:340: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:394: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:424: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:452: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:474: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:502: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:565: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:618: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:692: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:737: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:773: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:808: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:829: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:864: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:899: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:935: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:956: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:1105: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:1177: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:1228: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:1280: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:1329: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:275: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:306: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:332: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:349: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:366: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:407: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:427: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:466: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:481: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:487: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:493: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:499: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:505: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:511: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:517: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:523: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:544: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:550: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:571: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:577: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:592: Extra space before ( in function call [whitespace /parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:598: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:613: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:619: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:629: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:642: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:655: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:668: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:685: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:702: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:715: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:728: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:741: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:754: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:767: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:780: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:799: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:812: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:825: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:863: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:876: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:889: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:903: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/gtk/WebDriverServiceGtk.cpp:37: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 175 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 315417 [details] Patch Attachment 315417 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4119179 New failing tests: storage/websql/execute-sql-rowsAffected.html
Created attachment 315420 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Comment on attachment 315417 [details] 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.
Comment on attachment 315417 [details] 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.
Comment on attachment 315417 [details] 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.
Comment on attachment 315417 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315417&action=review >>> 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. I've read the spec again and now I remember why I didn't pay much attention to capabilities, not only because it's indeed a mess, but because selenium doesn't seem to use the returned capabilities at all, they are just stored in the driver. There aren't tests checking capabilities either. >> 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. These are supposed to be the resolved (or merged) capabilities, the idea is that the session host receives the desired capabilities, but changes them to the actual ones. It just happens that for now they are the same ones, since I'm only using the capabilities as a way to pass the browser binary path and arguments. What do you mean by populate the capabilities from the host? I understood that you preferred that session (and session host I guess) don't have to deal with the inspector objects, that's why I added this here. I added Session::capabilities as a wrapper of SessionHost::capabilities just for convenience.
Created attachment 315670 [details] Patch I've addressed most of the comments, hopefully everything except the parts I have doubts.
Attachment 315670 [details] did not pass style-queue: ERROR: Source/WebDriver/SessionHost.cpp:47: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:58: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:59: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:65: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:81: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:83: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:93: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/SessionHost.h:97: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:69: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:70: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:71: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:72: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:74: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:75: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:76: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:77: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:78: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:79: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:80: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:81: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:82: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:83: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:84: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:85: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:86: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:87: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:89: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:90: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:91: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:92: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:93: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:94: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:95: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:96: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:97: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:99: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:100: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:101: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:102: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:132: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:136: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.h:152: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:66: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:67: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:68: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:69: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:70: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:71: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:72: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:73: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:74: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:75: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:76: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:77: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:78: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:79: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:80: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:81: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:82: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:83: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:84: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:85: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:86: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:87: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:88: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:89: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:90: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:91: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:92: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:93: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:94: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:95: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:96: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:97: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:99: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:100: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:101: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:103: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:104: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:105: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:107: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.h:108: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/HTTPServer.h:52: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/config.h:31: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebDriver/glib/SessionHostGlib.cpp:98: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/glib/SessionHostGlib.cpp:104: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/glib/SessionHostGlib.cpp:115: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/glib/SessionHostGlib.cpp:130: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/glib/SessionHostGlib.cpp:205: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/glib/SessionHostGlib.cpp:222: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:60: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:79: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:90: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:110: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:130: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:158: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:177: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:196: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:215: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:245: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:273: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:283: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:297: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:335: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:389: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:419: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:447: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:469: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:497: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:560: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:613: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:687: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:732: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:768: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:803: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:824: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:859: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:894: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:930: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:951: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:1100: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:1172: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:1223: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:1275: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/Session.cpp:1324: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:207: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:238: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:254: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:273: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:290: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:342: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:364: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:399: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:416: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:424: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:432: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:440: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:448: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:456: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:464: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:470: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:491: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:497: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:518: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:526: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:543: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:551: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:568: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:576: Extra space before ( in function call [w hitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:586: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:599: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:614: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:629: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:648: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:667: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:682: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:703: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:718: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:733: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:748: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:763: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:778: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:793: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:808: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:848: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:861: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:874: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/WebDriverService.cpp:890: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebDriver/gtk/WebDriverServiceGtk.cpp:37: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 172 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.
(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. > >> 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. > > >> 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. > > 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. > >> 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. > > >> 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. > >> 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!
Comment on attachment 315670 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315670&action=review r=me ! > Source/WebDriver/Session.cpp:63 > + completionHandler(CommandResult::success()); Much better!
Comment on attachment 315670 [details] Patch Attachment 315670 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4136360 New failing tests: imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html
Created attachment 315680 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
(In reply to Build Bot from comment #36) > Comment on attachment 315670 [details] > Patch > > Attachment 315670 [details] did not pass ios-sim-ews (ios-simulator-wk2): > Output: http://webkit-queues.webkit.org/results/4136360 > > New failing tests: > imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html I think this is unrelated, likely caused by bug 174354
(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!
Committed r219605: <http://trac.webkit.org/changeset/219605>
Committed r219608: <http://trac.webkit.org/changeset/219608>
Committed r220584: <http://trac.webkit.org/changeset/220584>