Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3064012:
Base implementation of WebDriver for Chrome. This checkin includes... (Closed)
|
Created:
10 years, 5 months ago by Joe Modified:
9 years, 7 months ago CC:
chromium-reviews, Paweł Hajdan Jr., AmolKher, jleyba, jarbon_google.com Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionBase implementation of WebDriver for Chrome.
WebDriver is a tool for automating testing web applications, and in particular
to verify that they work as expected. It aims to provide a friendly API that's
easy to explore and understand, which will help make your tests easier to read
and maintain. It's not tied to any particular test framework, so it can be used
equally well with JUnit, TestNG or from a plain old "main" method. This checkin
includes all that it necessary to implement the JSON over HTTP protocol for
WebDriver along with the /session and /session/sessionID URLs. Each URL is
added under the webdriver/command directory.
To run simply run execute webdriver command, on linux you may need to add the
path to chrome to your enviroment settings. A port can be specified with the
--port option, by default webdriver will listen in on port 8080.
Note: A total refactor of my original code was done by Jason Leyba (jleyba).
All of his changes are included in this checkin
For further reference on the WebDriver remote protocol see:
http://code.google.com/p/selenium/wiki/JsonWireProtocol
BUG=none
TEST=Start webdriver then run the file webdriver_tests.py
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=56626
Patch Set 1 : '' #
Total comments: 191
Patch Set 2 : '' #
Total comments: 39
Patch Set 3 : '' #
Total comments: 44
Patch Set 4 : '' #
Total comments: 4
Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : '' #Patch Set 8 : '' #Patch Set 9 : '' #Messages
Total messages: 14 (0 generated)
Here's comments on the first few files. I'll append more in a bit. http://codereview.chromium.org/3064012/diff/52001/53001 File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/3064012/diff/52001/53001#newcode453 chrome/chrome_tests.gypi:453: '../third_party/libxml/libxml.gyp:libxml', Consider alphabetizing this list, and other lists in this file. http://codereview.chromium.org/3064012/diff/52001/53002 File chrome/test/webdriver/commands/command.cc (right): http://codereview.chromium.org/3064012/diff/52001/53002#newcode3 chrome/test/webdriver/commands/command.cc:3: // found in the LICENSE file. The bulk of your method implementations should reside in your .cc file. However, you've gone the other way. inline methods should really just be reserved for accessor/mutator methods. Anything non-trivial should be in a .cc file unless you've done the research to show that the method is called in many tight loops and inlining improves the performance in a noticeable way. http://codereview.chromium.org/3064012/diff/52001/53002#newcode43 chrome/test/webdriver/commands/command.cc:43: response->set_value(methods); // Assumes ownership ownership. http://codereview.chromium.org/3064012/diff/52001/53002#newcode46 chrome/test/webdriver/commands/command.cc:46: } } // namespace webdriver http://codereview.chromium.org/3064012/diff/52001/53003 File chrome/test/webdriver/commands/command.h (right): http://codereview.chromium.org/3064012/diff/52001/53003#newcode21 chrome/test/webdriver/commands/command.h:21: // a |Command|. Consider linking to any online documentation for the protocol here and in the Request class comments. http://codereview.chromium.org/3064012/diff/52001/53003#newcode34 chrome/test/webdriver/commands/command.h:34: inline Value* value() const { Anytime you take a pointer as an argument or return one from a function, it's good to document the contract w.r.t. that pointer. In this case, you should likely document that the returned pointer is an alias to data owned by the Response class and that the caller should not delete it. Also, you're not returning a const Value*, so it's presumed that the caller is allowed to modify the data that is pointed to. If that's not the case, please make it const. http://codereview.chromium.org/3064012/diff/52001/53003#newcode37 chrome/test/webdriver/commands/command.h:37: << "Accessing unset response value."; // Should never happen happen. Please go through the CL and check that all of your comments start with a capital letter and end with a punctuation mark of some kind. http://codereview.chromium.org/3064012/diff/52001/53003#newcode51 chrome/test/webdriver/commands/command.h:51: inline void SetField(const std::wstring& key, Value* in_value) { Declare in_value a const Value* and rename it from in_value->value. const Value* lets us know that this is an input pointer. Value* makes it seem that it's an out parameter. http://codereview.chromium.org/3064012/diff/52001/53003#newcode56 chrome/test/webdriver/commands/command.h:56: inline std::string ToJSON() const { Why not take a std::string* as an argument and set that string directly rather than allocating one on the stack and returning it? http://codereview.chromium.org/3064012/diff/52001/53003#newcode56 chrome/test/webdriver/commands/command.h:56: inline std::string ToJSON() const { ToJson would also be okay, and might be slightly more correct. But if the Chromium code base prefers JSON to Json, go with that. http://codereview.chromium.org/3064012/diff/52001/53003#newcode58 chrome/test/webdriver/commands/command.h:58: base::JSONWriter::Write(static_cast<const Value*>(&data_), false, &json); Odds are that this is not an inline call that is going to return super fast. Consider moving the implementation into the .cc file and moving the include of base/json/json_writer.h into the .cc file as well. This will reduce your header file dependencies. http://codereview.chromium.org/3064012/diff/52001/53003#newcode74 chrome/test/webdriver/commands/command.h:74: // Base class for a command mapped to an URL in the WebDriver REST API. Each a URL http://codereview.chromium.org/3064012/diff/52001/53003#newcode78 chrome/test/webdriver/commands/command.h:78: class Command { Consider moving this class into its own file. Typically non-trivial classes get their own file. http://codereview.chromium.org/3064012/diff/52001/53003#newcode81 chrome/test/webdriver/commands/command.h:81: const std::vector<std::string> path_segments, path_Segments should be passed const&, not const. http://codereview.chromium.org/3064012/diff/52001/53003#newcode113 chrome/test/webdriver/commands/command.h:113: return i < path_segments_.size() ? path_segments_.at(i) : ""; You should use path_segments_[i] instead of path_segments_.at(i), since you're doing the bounds checking. at(i) does the bounds checking you're trying to do and possibly raises an exception which you don't catch or document to be thrown. You should also verify that i >= 0. http://codereview.chromium.org/3064012/diff/52001/53003#newcode127 chrome/test/webdriver/commands/command.h:127: } } // namespace webdriver
More comments. http://codereview.chromium.org/3064012/diff/52001/53005 File chrome/test/webdriver/commands/session_command.h (right): http://codereview.chromium.org/3064012/diff/52001/53005#newcode16 chrome/test/webdriver/commands/session_command.h:16: class SessionCommand: public Command { class SessionCommand : public Command { Extra space around the : when used for inheritance. http://codereview.chromium.org/3064012/diff/52001/53005#newcode19 chrome/test/webdriver/commands/session_command.h:19: const std::vector<std::string> path_segments, Ditto on taking this guy const&, which will be forced once you change the base class. http://codereview.chromium.org/3064012/diff/52001/53005#newcode31 chrome/test/webdriver/commands/session_command.h:31: class SessionWithIDCommand: public WebDriverCommand { Ditto space before : here and everywhere else in this CL. http://codereview.chromium.org/3064012/diff/52001/53005#newcode50 chrome/test/webdriver/commands/session_command.h:50: } Ditto on documenting that this } closes the webdriver namespace here and everywhere in this CL. } // namespace webdriver http://codereview.chromium.org/3064012/diff/52001/53007 File chrome/test/webdriver/commands/webdriver_command.h (right): http://codereview.chromium.org/3064012/diff/52001/53007#newcode34 chrome/test/webdriver/commands/webdriver_command.h:34: // failure. // failure. http://codereview.chromium.org/3064012/diff/52001/53007#newcode48 chrome/test/webdriver/commands/webdriver_command.h:48: bool VerifyTabIsValid(Response* const response); Why pass Response* as const when you might set it on failure? I understand that this means that the pointer is const but what's pointed to isn't, but it's still confusing. I'd drop the const. http://codereview.chromium.org/3064012/diff/52001/53007#newcode56 chrome/test/webdriver/commands/webdriver_command.h:56: scoped_refptr<TabProxy> tab; tab_ and session_... data members should always be named with a trailing underscore. http://codereview.chromium.org/3064012/diff/52001/53008 File chrome/test/webdriver/dispatch.cc (right): http://codereview.chromium.org/3064012/diff/52001/53008#newcode9 chrome/test/webdriver/dispatch.cc:9: #include "chrome/test/webdriver/commands/command.h" nit: alphabetical ordering. http://codereview.chromium.org/3064012/diff/52001/53009 File chrome/test/webdriver/dispatch.h (right): http://codereview.chromium.org/3064012/diff/52001/53009#newcode28 chrome/test/webdriver/dispatch.h:28: void dispatch(struct mg_connection* connection, dispatch->Dispatch. http://codereview.chromium.org/3064012/diff/52001/53012 File chrome/test/webdriver/keymap.h (right): http://codereview.chromium.org/3064012/diff/52001/53012#newcode29 chrome/test/webdriver/keymap.h:29: bool shift, alt, control, command; Should only declare one member per line. Members should be named with trailing _. http://codereview.chromium.org/3064012/diff/52001/53013 File chrome/test/webdriver/server.cc (right): http://codereview.chromium.org/3064012/diff/52001/53013#newcode47 chrome/test/webdriver/server.cc:47: void set_callback(struct mg_context* ctx, const char* pattern) { SetCallback http://codereview.chromium.org/3064012/diff/52001/53013#newcode51 chrome/test/webdriver/server.cc:51: void init_callbacks(struct mg_context* ctx) { InitCallbacks http://codereview.chromium.org/3064012/diff/52001/53013#newcode90 chrome/test/webdriver/server.cc:90: // Listen on port 8080 port 8080 or port specified on command line. http://codereview.chromium.org/3064012/diff/52001/53013#newcode107 chrome/test/webdriver/server.cc:107: // we should not reach here since the service should never quit You should probably register a listener for SIGTERM and break your message loop so that people can shut your server down gracefully. Consider adding a TODO for a follow-up CL.
Please let's try and break things up more next time; this took forever to review. In any case, glad to see this coming together. http://codereview.chromium.org/3064012/diff/52001/53001 File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/3064012/diff/52001/53001#newcode434 chrome/chrome_tests.gypi:434: 'target_name': 'webdriver', Name is somewhat awkward. It implies this builds webdriver... but it does not. It builds the Chrome interface for WebDriver. Perhaps name this chrome_webdriver, or chrome_webdriver_glue, or something like that. (Is there a webdriver convention name for this? plug-in? module?) http://codereview.chromium.org/3064012/diff/52001/53002 File chrome/test/webdriver/commands/command.cc (right): http://codereview.chromium.org/3064012/diff/52001/53002#newcode18 chrome/test/webdriver/commands/command.cc:18: if (DoesPost() && method_ == "POST") { If DoesPost() et al implemented as suggested in header you can simplify this. http://codereview.chromium.org/3064012/diff/52001/53003 File chrome/test/webdriver/commands/command.h (right): http://codereview.chromium.org/3064012/diff/52001/53003#newcode22 chrome/test/webdriver/commands/command.h:22: class Response { General rule: one class per header. Move Response to new file. http://codereview.chromium.org/3064012/diff/52001/53003#newcode63 chrome/test/webdriver/commands/command.h:63: static const wchar_t* const kStatusKey_; static members don't need trailing _; they are not data members as such. http://codereview.chromium.org/3064012/diff/52001/53003#newcode80 chrome/test/webdriver/commands/command.h:80: inline Command(const std::string& method, Be clear that method here is an HTTP method like POST. Since it's passed in as a string and not an enum you probably want to confirm method is valid and (e.g.) not a typo like POTS. APIs below look like you only allow DELETE, GET, and POST. http://codereview.chromium.org/3064012/diff/52001/53003#newcode85 chrome/test/webdriver/commands/command.h:85: parameters_(parameters) {} sanity check that path segments are all cgi.escaped properly? http://codereview.chromium.org/3064012/diff/52001/53003#newcode89 chrome/test/webdriver/commands/command.h:89: // in the provided |response| object (which must be non-NULL). "... The command result, including failure to execute, ..." http://codereview.chromium.org/3064012/diff/52001/53003#newcode94 chrome/test/webdriver/commands/command.h:94: virtual bool DoesDelete() { return false; } Perhaps these could all be implemented in base class? e.g. bool DoesDelete() { return method_ == "DELETE"; } http://codereview.chromium.org/3064012/diff/52001/53003#newcode101 chrome/test/webdriver/commands/command.h:101: virtual bool Init(Response* const response) { return true; } Perhaps you should document that the init is performed by Execute() at execute time and should not be called by anyone else? http://codereview.chromium.org/3064012/diff/52001/53003#newcode105 chrome/test/webdriver/commands/command.h:105: virtual void ExecuteDelete(Response* const response) {} Document the intention is to call this from Execute() http://codereview.chromium.org/3064012/diff/52001/53003#newcode105 chrome/test/webdriver/commands/command.h:105: virtual void ExecuteDelete(Response* const response) {} Document how failure is handled (e.g. response blah blah.) http://codereview.chromium.org/3064012/diff/52001/53003#newcode117 chrome/test/webdriver/commands/command.h:117: // NULL if there is no such parameter, or if it is not a string. This method does not have the opportunity to set out to NULL. Perhaps you mean it returns the empty string if there is no key? If so, why not do so as a return value instead of passed in arg? http://codereview.chromium.org/3064012/diff/52001/53004 File chrome/test/webdriver/commands/session_command.cc (right): http://codereview.chromium.org/3064012/diff/52001/53004#newcode17 chrome/test/webdriver/commands/session_command.cc:17: void SessionCommand::ExecutePost(Response* const response) { I'm not sure I get this. Is the function just to make sure you have a valid session manager? Does that implicitly include making a connection? (I don't see anything posted to it.) Some doc might help. http://codereview.chromium.org/3064012/diff/52001/53004#newcode24 chrome/test/webdriver/commands/session_command.cc:24: stream << "http://" << session_manager->GetIPAddress() << "/session/" Might be nice for SessionManager to have a method like std::string url_string() { /* return the URL for this session and ID */ } (e.g. this code here) http://codereview.chromium.org/3064012/diff/52001/53004#newcode34 chrome/test/webdriver/commands/session_command.cc:34: void SessionWithIDCommand::ExecuteGet(Response* const response) { I don't understand the purpose of this. What does it do? E.g. "honors init contract of WebDriver"? http://codereview.chromium.org/3064012/diff/52001/53004#newcode46 chrome/test/webdriver/commands/session_command.cc:46: #elif OS_LINUX I believe ChromeOS also sets OS_LINUX to 1 so you'll never hit the OS_CHROMEOS case. http://codereview.chromium.org/3064012/diff/52001/53005 File chrome/test/webdriver/commands/session_command.h (right): http://codereview.chromium.org/3064012/diff/52001/53005#newcode16 chrome/test/webdriver/commands/session_command.h:16: class SessionCommand: public Command { all classes get a comment describing use. (What is a session command?) Add space before : http://codereview.chromium.org/3064012/diff/52001/53005#newcode25 chrome/test/webdriver/commands/session_command.h:25: virtual bool DoesPost() { return true; } What if someone passes in HEAD for method? Would this class still make sense to return true for DoesPost()? http://codereview.chromium.org/3064012/diff/52001/53005#newcode31 chrome/test/webdriver/commands/session_command.h:31: class SessionWithIDCommand: public WebDriverCommand { 1 class per header http://codereview.chromium.org/3064012/diff/52001/53005#newcode31 chrome/test/webdriver/commands/session_command.h:31: class SessionWithIDCommand: public WebDriverCommand { all classes get a comment describing use. http://codereview.chromium.org/3064012/diff/52001/53007 File chrome/test/webdriver/commands/webdriver_command.h (right): http://codereview.chromium.org/3064012/diff/52001/53007#newcode16 chrome/test/webdriver/commands/webdriver_command.h:16: class WebDriverCommand : public Command { comment for each class http://codereview.chromium.org/3064012/diff/52001/53007#newcode27 chrome/test/webdriver/commands/webdriver_command.h:27: // Tests if |dictionary| contains the |kElementDictionaryKey_|, which pipes for |arg| but not for |constants|. (and no trailing underscore). http://codereview.chromium.org/3064012/diff/52001/53007#newcode31 chrome/test/webdriver/commands/webdriver_command.h:31: // Returns the |element_id| as a |DictionaryValue| that conforms with |arg| but not for |type| (e.g. DictionaryValue). http://codereview.chromium.org/3064012/diff/52001/53007#newcode39 chrome/test/webdriver/commands/webdriver_command.h:39: virtual bool Init(Response* const response); Where does TabProxy come from? Ditto for method RequiresValidTab below. The syntax of pipes implies it's an arg. http://codereview.chromium.org/3064012/diff/52001/53007#newcode45 chrome/test/webdriver/commands/webdriver_command.h:45: // Returns whether this command has a valid |TabProxy|. Returns true on Exactly how do we get a valid tab proxy? Doc as such. (This comment seems wrong; in the code, VerifyTabIsValid appears to be the way we get/create a tab proxy in the first place.) http://codereview.chromium.org/3064012/diff/52001/53007#newcode46 chrome/test/webdriver/commands/webdriver_command.h:46: // success. Otherwise, returns false and populates the |resposne| with the resposne http://codereview.chromium.org/3064012/diff/52001/53007#newcode50 chrome/test/webdriver/commands/webdriver_command.h:50: // Waits for a new navigation if none has occurred within 500ms of What if navigation happened within 400ms? What will happen? Did we wait for it? http://codereview.chromium.org/3064012/diff/52001/53007#newcode55 chrome/test/webdriver/commands/webdriver_command.h:55: Session* session; trailing underscores on data members http://codereview.chromium.org/3064012/diff/52001/53008 File chrome/test/webdriver/dispatch.cc (right): http://codereview.chromium.org/3064012/diff/52001/53008#newcode11 chrome/test/webdriver/dispatch.cc:11: #include "base/json/json_writer.h" alphabetize headers http://codereview.chromium.org/3064012/diff/52001/53008#newcode15 chrome/test/webdriver/dispatch.cc:15: void SendHttpOk(struct mg_connection* const connection, comment on every function. For this case, a comment explaining all functions at once might be fine. http://codereview.chromium.org/3064012/diff/52001/53009 File chrome/test/webdriver/dispatch.h (right): http://codereview.chromium.org/3064012/diff/52001/53009#newcode21 chrome/test/webdriver/dispatch.h:21: void SendResponse(struct mg_connection* const connection, doc on other args. This seems an internal use function? http://codereview.chromium.org/3064012/diff/52001/53009#newcode26 chrome/test/webdriver/dispatch.h:26: // service. |CommandType| must be a subtype of |chrome::webdriver::Command|. webdriver:command (I don't think you are using chrome:: namespace) http://codereview.chromium.org/3064012/diff/52001/53009#newcode26 chrome/test/webdriver/dispatch.h:26: // service. |CommandType| must be a subtype of |chrome::webdriver::Command|. Not sure I get this. Seems like something to be implemented in the webdriver::Command base class instead of as a template. Can you detail your design or code organization to help me understand this better? http://codereview.chromium.org/3064012/diff/52001/53010 File chrome/test/webdriver/error_codes.h (right): http://codereview.chromium.org/3064012/diff/52001/53010#newcode5 chrome/test/webdriver/error_codes.h:5: #ifndef CHROME_TEST_WEBDRIVER_ERROR_CODES_H__ no double-__ at end; just one http://codereview.chromium.org/3064012/diff/52001/53010#newcode11 chrome/test/webdriver/error_codes.h:11: // http://code.google.com/p/selenium/wiki/JsonWireProtocol#Response_Status_Codes Do they provide a header we can use to keep in sync? http://codereview.chromium.org/3064012/diff/52001/53012 File chrome/test/webdriver/keymap.h (right): http://codereview.chromium.org/3064012/diff/52001/53012#newcode13 chrome/test/webdriver/keymap.h:13: // Maps the key space used by WebDriver to Chrome for Linux/Mac/Windows remove this line (duped below) http://codereview.chromium.org/3064012/diff/52001/53012#newcode26 chrome/test/webdriver/keymap.h:26: void Clear(); What does Clear() mean? "unpress all"? Seems like you're mixing semantics (e.g. modifiers are "press and hold until cleared" but other keys are "press/release"). http://codereview.chromium.org/3064012/diff/52001/53013 File chrome/test/webdriver/server.cc (right): http://codereview.chromium.org/3064012/diff/52001/53013#newcode8 chrome/test/webdriver/server.cc:8: #include <sys/time.h> << these http://codereview.chromium.org/3064012/diff/52001/53013#newcode19 chrome/test/webdriver/server.cc:19: #include "third_party/mongoose/mongoose.h" alphabetize headers http://codereview.chromium.org/3064012/diff/52001/53013#newcode38 chrome/test/webdriver/server.cc:38: case 0: // The win compiler demoands at least 1 case statement demands http://codereview.chromium.org/3064012/diff/52001/53013#newcode60 chrome/test/webdriver/server.cc:60: int main(int argc, char *argv[]) { Brief description: "main for WebDriver protocol server. Runs the mg web server to implement wire blah. Sends commands to chrome blah via automation proxy see http://dev.whatevemormro.org for more details." http://codereview.chromium.org/3064012/diff/52001/53014 File chrome/test/webdriver/session_manager.cc (right): http://codereview.chromium.org/3064012/diff/52001/53014#newcode8 chrome/test/webdriver/session_manager.cc:8: #include <unistd.h> alphabetize http://codereview.chromium.org/3064012/diff/52001/53014#newcode46 chrome/test/webdriver/session_manager.cc:46: LOG(ERROR) << "Could not get system temp path" << std::endl; Perhaps use CHECK() or NOTREACHED()? http://codereview.chromium.org/3064012/diff/52001/53014#newcode68 chrome/test/webdriver/session_manager.cc:68: bool Session::Init(base::ProcessHandle process_handle) { A little long. Consider splitting up into "wait for loads and launch app and open browser window" and "get proxies". Will be forward-looking (e.g. in case it connects to existing browser a la PyAuto integration). http://codereview.chromium.org/3064012/diff/52001/53014#newcode138 chrome/test/webdriver/session_manager.cc:138: bool Session::CreateTemporaryProfileDirectory() { scoped_object in Session structure to delete temp dir so this directory doesn't leak and run out of disk space? Or use temp dir from command line so process which launched me (e.g. python script) can delete it in case I crash? http://codereview.chromium.org/3064012/diff/52001/53014#newcode177 chrome/test/webdriver/session_manager.cc:177: bool DeleteDirectory(const char* directory) { Not called? http://codereview.chromium.org/3064012/diff/52001/53014#newcode203 chrome/test/webdriver/session_manager.cc:203: // DeleteDirectory(tmp_profile_dir_); Need to implement this http://codereview.chromium.org/3064012/diff/52001/53014#newcode228 chrome/test/webdriver/session_manager.cc:228: // in size and will add lots of noise to the logs. perhaps print first 64 chars of it with ... http://codereview.chromium.org/3064012/diff/52001/53014#newcode278 chrome/test/webdriver/session_manager.cc:278: std::map<std::string, Session*> SessionManager::map_; global objects of class type forbidden. Applies to all 4 of these objects (including std::string) http://codereview.chromium.org/3064012/diff/52001/53014#newcode305 chrome/test/webdriver/session_manager.cc:305: return std::string(""); #else? http://codereview.chromium.org/3064012/diff/52001/53014#newcode401 chrome/test/webdriver/session_manager.cc:401: const char* alternative_userdir = getenv("CHROME_UI_TESTS_USER_DATA_DIR"); having one variable name be different (incompatible) types on different platforms is a little awkward. http://codereview.chromium.org/3064012/diff/52001/53015 File chrome/test/webdriver/session_manager.h (right): http://codereview.chromium.org/3064012/diff/52001/53015#newcode26 chrome/test/webdriver/session_manager.h:26: class Session { comment for each class http://codereview.chromium.org/3064012/diff/52001/53015#newcode26 chrome/test/webdriver/session_manager.h:26: class Session { comment for every class comment for every nontrivial method ... http://codereview.chromium.org/3064012/diff/52001/53015#newcode29 chrome/test/webdriver/session_manager.h:29: typedef char ProfileDir[L_tmpnam + 1]; // +1 for \0 You are not guaranteed ::GetTempPath() uses tmpnam() so MAX_PATH might be better for POSIX. http://codereview.chromium.org/3064012/diff/52001/53016 File chrome/test/webdriver/utility_functions.cc (right): http://codereview.chromium.org/3064012/diff/52001/53016#newcode47 chrome/test/webdriver/utility_functions.cc:47: return L"LIST"; default: return L"ERROR"; http://codereview.chromium.org/3064012/diff/52001/53017 File chrome/test/webdriver/utility_functions.h (right): http://codereview.chromium.org/3064012/diff/52001/53017#newcode13 chrome/test/webdriver/utility_functions.h:13: // TODO(jleyba): This should be auto-generated along with the atoms themselves. Make reference to "webdriver atom" and briefly describe. Else the term "atom" is too generic. http://codereview.chromium.org/3064012/diff/52001/53018 File chrome/test/webdriver/webdriver_tests.py (right): http://codereview.chromium.org/3064012/diff/52001/53018#newcode9 chrome/test/webdriver/webdriver_tests.py:9: import simplejson as json not in default path. Do you need a sys.path extension for ../../third_party, or do we have a webdriver_tests.py test runner? http://codereview.chromium.org/3064012/diff/52001/53018#newcode33 chrome/test/webdriver/webdriver_tests.py:33: elif method != 'POST' and method != 'PUT': elif method not in ('POST', 'PUT'): http://codereview.chromium.org/3064012/diff/52001/53018#newcode64 chrome/test/webdriver/webdriver_tests.py:64: class WebDriverTest(unittest.TestCase): brief class description. E.g. aren't these ALL WebDriver tests? http://codereview.chromium.org/3064012/diff/52001/53018#newcode151 chrome/test/webdriver/webdriver_tests.py:151: import optparse import optparse goes at top http://codereview.chromium.org/3064012/diff/52001/53018#newcode153 chrome/test/webdriver/webdriver_tests.py:153: parser = optparse.OptionParser('%prog [options]') test implies webdriver server already running? Seems like it should launch it.
1st round of fixes I should note that with the structure we have now each new URL listed in http://code.google.com/p/selenium/wiki/JsonWireProtocol should only require 100- 200 lines of code to implement. So I will check them in one at a time to make things easier. http://codereview.chromium.org/3064012/diff/52001/53001 File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/3064012/diff/52001/53001#newcode434 chrome/chrome_tests.gypi:434: 'target_name': 'webdriver', On 2010/07/28 03:57:25, John Grabowski wrote: > Name is somewhat awkward. It implies this builds webdriver... but it does not. > It builds the Chrome interface for WebDriver. Perhaps name this > chrome_webdriver, or chrome_webdriver_glue, or something like that. > > (Is there a webdriver convention name for this? plug-in? module?) > Done. http://codereview.chromium.org/3064012/diff/52001/53001#newcode453 chrome/chrome_tests.gypi:453: '../third_party/libxml/libxml.gyp:libxml', On 2010/07/28 02:46:14, jcarollo1 wrote: > Consider alphabetizing this list, and other lists in this file. Done. http://codereview.chromium.org/3064012/diff/52001/53002 File chrome/test/webdriver/commands/command.cc (right): http://codereview.chromium.org/3064012/diff/52001/53002#newcode43 chrome/test/webdriver/commands/command.cc:43: response->set_value(methods); // Assumes ownership On 2010/07/28 02:46:14, jcarollo1 wrote: > ownership. Done. http://codereview.chromium.org/3064012/diff/52001/53002#newcode46 chrome/test/webdriver/commands/command.cc:46: } On 2010/07/28 02:46:14, jcarollo1 wrote: > } // namespace webdriver Done. http://codereview.chromium.org/3064012/diff/52001/53003 File chrome/test/webdriver/commands/command.h (right): http://codereview.chromium.org/3064012/diff/52001/53003#newcode21 chrome/test/webdriver/commands/command.h:21: // a |Command|. On 2010/07/28 02:46:14, jcarollo1 wrote: > Consider linking to any online documentation for the protocol here and in the > Request class comments. Done. http://codereview.chromium.org/3064012/diff/52001/53003#newcode22 chrome/test/webdriver/commands/command.h:22: class Response { On 2010/07/28 03:57:25, John Grabowski wrote: > General rule: one class per header. Move Response to new file. > Done. http://codereview.chromium.org/3064012/diff/52001/53003#newcode34 chrome/test/webdriver/commands/command.h:34: inline Value* value() const { On 2010/07/28 02:46:14, jcarollo1 wrote: > Anytime you take a pointer as an argument or return one from a function, it's > good to document the contract w.r.t. that pointer. In this case, you should > likely document that the returned pointer is an alias to data owned by the > Response class and that the caller should not delete it. > > Also, you're not returning a const Value*, so it's presumed that the caller is > allowed to modify the data that is pointed to. If that's not the case, please > make it const. Done. http://codereview.chromium.org/3064012/diff/52001/53003#newcode37 chrome/test/webdriver/commands/command.h:37: << "Accessing unset response value."; // Should never happen On 2010/07/28 02:46:14, jcarollo1 wrote: > happen. > > Please go through the CL and check that all of your comments start with a > capital letter and end with a punctuation mark of some kind. Done. http://codereview.chromium.org/3064012/diff/52001/53003#newcode51 chrome/test/webdriver/commands/command.h:51: inline void SetField(const std::wstring& key, Value* in_value) { I can't make it const since in the function Set DictionaryValue modifies Value and takes ownership, see base.values.h On 2010/07/28 02:46:14, jcarollo1 wrote: > Declare in_value a const Value* and rename it from in_value->value. const > Value* lets us know that this is an input pointer. Value* makes it seem that > it's an out parameter. http://codereview.chromium.org/3064012/diff/52001/53003#newcode56 chrome/test/webdriver/commands/command.h:56: inline std::string ToJSON() const { On 2010/07/28 02:46:14, jcarollo1 wrote: > ToJson would also be okay, and might be slightly more correct. But if the > Chromium code base prefers JSON to Json, go with that. Done. http://codereview.chromium.org/3064012/diff/52001/53003#newcode56 chrome/test/webdriver/commands/command.h:56: inline std::string ToJSON() const { It's simpler and not performance critical On 2010/07/28 02:46:14, jcarollo1 wrote: > Why not take a std::string* as an argument and set that string directly rather > than allocating one on the stack and returning it? http://codereview.chromium.org/3064012/diff/52001/53003#newcode58 chrome/test/webdriver/commands/command.h:58: base::JSONWriter::Write(static_cast<const Value*>(&data_), false, &json); I removed the inline and the dependencies were removed when i moved this class to it's own file On 2010/07/28 02:46:14, jcarollo1 wrote: > Odds are that this is not an inline call that is going to return super fast. > Consider moving the implementation into the .cc file and moving the include of > base/json/json_writer.h into the .cc file as well. This will reduce your header > file dependencies. http://codereview.chromium.org/3064012/diff/52001/53003#newcode63 chrome/test/webdriver/commands/command.h:63: static const wchar_t* const kStatusKey_; On 2010/07/28 03:57:25, John Grabowski wrote: > static members don't need trailing _; they are not data members as such. > Done. http://codereview.chromium.org/3064012/diff/52001/53003#newcode74 chrome/test/webdriver/commands/command.h:74: // Base class for a command mapped to an URL in the WebDriver REST API. Each On 2010/07/28 02:46:14, jcarollo1 wrote: > a URL Done. http://codereview.chromium.org/3064012/diff/52001/53003#newcode78 chrome/test/webdriver/commands/command.h:78: class Command { On 2010/07/28 02:46:14, jcarollo1 wrote: > Consider moving this class into its own file. Typically non-trivial classes get > their own file. Done. http://codereview.chromium.org/3064012/diff/52001/53003#newcode80 chrome/test/webdriver/commands/command.h:80: inline Command(const std::string& method, To do a POST you need to override the function DoesPost and ExecutePost. The method name is the URL method in your REST protocol. For example /session On 2010/07/28 03:57:25, John Grabowski wrote: > Be clear that method here is an HTTP method like POST. > Since it's passed in as a string and not an enum you probably want to confirm > method is valid and (e.g.) not a typo like POTS. > > APIs below look like you only allow DELETE, GET, and POST. > > http://codereview.chromium.org/3064012/diff/52001/53003#newcode81 chrome/test/webdriver/commands/command.h:81: const std::vector<std::string> path_segments, On 2010/07/28 02:46:14, jcarollo1 wrote: > path_Segments should be passed const&, not const. Done. http://codereview.chromium.org/3064012/diff/52001/53003#newcode89 chrome/test/webdriver/commands/command.h:89: // in the provided |response| object (which must be non-NULL). On 2010/07/28 03:57:25, John Grabowski wrote: > "... The command result, including failure to execute, ..." > Done. http://codereview.chromium.org/3064012/diff/52001/53003#newcode105 chrome/test/webdriver/commands/command.h:105: virtual void ExecuteDelete(Response* const response) {} On 2010/07/28 03:57:25, John Grabowski wrote: > Document the intention is to call this from Execute() > Done. http://codereview.chromium.org/3064012/diff/52001/53003#newcode105 chrome/test/webdriver/commands/command.h:105: virtual void ExecuteDelete(Response* const response) {} On 2010/07/28 03:57:25, John Grabowski wrote: > Document how failure is handled (e.g. response blah blah.) > > Done. http://codereview.chromium.org/3064012/diff/52001/53003#newcode113 chrome/test/webdriver/commands/command.h:113: return i < path_segments_.size() ? path_segments_.at(i) : ""; We want an exception to be thrown and the app aborted since that is a serious logical issue which we can't recover from On 2010/07/28 02:46:14, jcarollo1 wrote: > You should use path_segments_[i] instead of path_segments_.at(i), since you're > doing the bounds checking. at(i) does the bounds checking you're trying to do > and possibly raises an exception which you don't catch or document to be thrown. > You should also verify that i >= 0. http://codereview.chromium.org/3064012/diff/52001/53003#newcode127 chrome/test/webdriver/commands/command.h:127: } On 2010/07/28 02:46:14, jcarollo1 wrote: > } // namespace webdriver Done. http://codereview.chromium.org/3064012/diff/52001/53004 File chrome/test/webdriver/commands/session_command.cc (right): http://codereview.chromium.org/3064012/diff/52001/53004#newcode17 chrome/test/webdriver/commands/session_command.cc:17: void SessionCommand::ExecutePost(Response* const response) { I added a link to the doc in the header. This function is to create a browser session; so a new instance of chrome is always created On 2010/07/28 03:57:25, John Grabowski wrote: > I'm not sure I get this. > Is the function just to make sure you have a valid session manager? > Does that implicitly include making a connection? > (I don't see anything posted to it.) > Some doc might help. > http://codereview.chromium.org/3064012/diff/52001/53004#newcode24 chrome/test/webdriver/commands/session_command.cc:24: stream << "http://" << session_manager->GetIPAddress() << "/session/" I can add the function but technically only this class reports the URL for the HTTP redirect. All of the other functions require you to know the session ID before hand. On 2010/07/28 03:57:25, John Grabowski wrote: > Might be nice for SessionManager to have a method like > std::string url_string() { /* return the URL for this session and ID */ } > (e.g. this code here) > http://codereview.chromium.org/3064012/diff/52001/53004#newcode34 chrome/test/webdriver/commands/session_command.cc:34: void SessionWithIDCommand::ExecuteGet(Response* const response) { In the webdriver spec passing in the sessionID return info on the browser settings and is the only way to properly end a session On 2010/07/28 03:57:25, John Grabowski wrote: > I don't understand the purpose of this. > What does it do? > E.g. "honors init contract of WebDriver"? > http://codereview.chromium.org/3064012/diff/52001/53004#newcode46 chrome/test/webdriver/commands/session_command.cc:46: #elif OS_LINUX On 2010/07/28 03:57:25, John Grabowski wrote: > I believe ChromeOS also sets OS_LINUX to 1 so you'll never hit the OS_CHROMEOS > case. > Done. http://codereview.chromium.org/3064012/diff/52001/53004#newcode46 chrome/test/webdriver/commands/session_command.cc:46: #elif OS_LINUX On 2010/07/28 03:57:25, John Grabowski wrote: > I believe ChromeOS also sets OS_LINUX to 1 so you'll never hit the OS_CHROMEOS > case. > Done. http://codereview.chromium.org/3064012/diff/52001/53005 File chrome/test/webdriver/commands/session_command.h (right): http://codereview.chromium.org/3064012/diff/52001/53005#newcode16 chrome/test/webdriver/commands/session_command.h:16: class SessionCommand: public Command { On 2010/07/28 03:03:10, jcarollo1 wrote: > class SessionCommand : public Command { > Extra space around the : when used for inheritance. Done. http://codereview.chromium.org/3064012/diff/52001/53005#newcode16 chrome/test/webdriver/commands/session_command.h:16: class SessionCommand: public Command { On 2010/07/28 03:57:25, John Grabowski wrote: > all classes get a comment describing use. (What is a session command?) > Add space before : > > Done. http://codereview.chromium.org/3064012/diff/52001/53005#newcode19 chrome/test/webdriver/commands/session_command.h:19: const std::vector<std::string> path_segments, On 2010/07/28 03:03:10, jcarollo1 wrote: > Ditto on taking this guy const&, which will be forced once you change the base > class. Done. http://codereview.chromium.org/3064012/diff/52001/53005#newcode25 chrome/test/webdriver/commands/session_command.h:25: virtual bool DoesPost() { return true; } We dont use the HTTP protocols in the traditional sense. Basically a POST call is anything that changes or could change the state if of the browser. In this case we create a new instance so state is always changed. On 2010/07/28 03:57:25, John Grabowski wrote: > What if someone passes in HEAD for method? Would this class still make sense to > return true for DoesPost()? > http://codereview.chromium.org/3064012/diff/52001/53005#newcode31 chrome/test/webdriver/commands/session_command.h:31: class SessionWithIDCommand: public WebDriverCommand { On 2010/07/28 03:03:10, jcarollo1 wrote: > Ditto space before : here and everywhere else in this CL. Done. http://codereview.chromium.org/3064012/diff/52001/53005#newcode31 chrome/test/webdriver/commands/session_command.h:31: class SessionWithIDCommand: public WebDriverCommand { I don't mind breaking them up but this two classes are closely related to the spec and if one changes the other will need to in a very similar fashion. On 2010/07/28 03:57:25, John Grabowski wrote: > 1 class per header http://codereview.chromium.org/3064012/diff/52001/53005#newcode31 chrome/test/webdriver/commands/session_command.h:31: class SessionWithIDCommand: public WebDriverCommand { On 2010/07/28 03:57:25, John Grabowski wrote: > all classes get a comment describing use. Done. http://codereview.chromium.org/3064012/diff/52001/53005#newcode50 chrome/test/webdriver/commands/session_command.h:50: } On 2010/07/28 03:03:10, jcarollo1 wrote: > Ditto on documenting that this } closes the webdriver namespace here and > everywhere in this CL. > > } // namespace webdriver Done. http://codereview.chromium.org/3064012/diff/52001/53007 File chrome/test/webdriver/commands/webdriver_command.h (right): http://codereview.chromium.org/3064012/diff/52001/53007#newcode27 chrome/test/webdriver/commands/webdriver_command.h:27: // Tests if |dictionary| contains the |kElementDictionaryKey_|, which On 2010/07/28 03:57:25, John Grabowski wrote: > pipes for |arg| but not for |constants|. (and no trailing underscore). > Done. http://codereview.chromium.org/3064012/diff/52001/53007#newcode31 chrome/test/webdriver/commands/webdriver_command.h:31: // Returns the |element_id| as a |DictionaryValue| that conforms with On 2010/07/28 03:57:25, John Grabowski wrote: > |arg| but not for |type| (e.g. DictionaryValue). > > Done. http://codereview.chromium.org/3064012/diff/52001/53007#newcode34 chrome/test/webdriver/commands/webdriver_command.h:34: // failure. On 2010/07/28 03:03:10, jcarollo1 wrote: > // failure. Done. http://codereview.chromium.org/3064012/diff/52001/53007#newcode46 chrome/test/webdriver/commands/webdriver_command.h:46: // success. Otherwise, returns false and populates the |resposne| with the On 2010/07/28 03:57:25, John Grabowski wrote: > resposne > Done. http://codereview.chromium.org/3064012/diff/52001/53007#newcode48 chrome/test/webdriver/commands/webdriver_command.h:48: bool VerifyTabIsValid(Response* const response); On 2010/07/28 03:03:10, jcarollo1 wrote: > Why pass Response* as const when you might set it on failure? I understand that > this means that the pointer is const but what's pointed to isn't, but it's still > confusing. I'd drop the const. Done. http://codereview.chromium.org/3064012/diff/52001/53007#newcode55 chrome/test/webdriver/commands/webdriver_command.h:55: Session* session; On 2010/07/28 03:57:25, John Grabowski wrote: > trailing underscores on data members Done. http://codereview.chromium.org/3064012/diff/52001/53007#newcode56 chrome/test/webdriver/commands/webdriver_command.h:56: scoped_refptr<TabProxy> tab; On 2010/07/28 03:03:10, jcarollo1 wrote: > tab_ and session_... data members should always be named with a trailing > underscore. Done. http://codereview.chromium.org/3064012/diff/52001/53008 File chrome/test/webdriver/dispatch.cc (right): http://codereview.chromium.org/3064012/diff/52001/53008#newcode9 chrome/test/webdriver/dispatch.cc:9: #include "chrome/test/webdriver/commands/command.h" On 2010/07/28 03:03:10, jcarollo1 wrote: > nit: alphabetical ordering. Done. http://codereview.chromium.org/3064012/diff/52001/53008#newcode11 chrome/test/webdriver/dispatch.cc:11: #include "base/json/json_writer.h" On 2010/07/28 03:57:25, John Grabowski wrote: > alphabetize headers > Done. http://codereview.chromium.org/3064012/diff/52001/53008#newcode15 chrome/test/webdriver/dispatch.cc:15: void SendHttpOk(struct mg_connection* const connection, On 2010/07/28 03:57:25, John Grabowski wrote: > comment on every function. For this case, a comment explaining all functions at > once might be fine. Done. http://codereview.chromium.org/3064012/diff/52001/53009 File chrome/test/webdriver/dispatch.h (right): http://codereview.chromium.org/3064012/diff/52001/53009#newcode21 chrome/test/webdriver/dispatch.h:21: void SendResponse(struct mg_connection* const connection, On 2010/07/28 03:57:25, John Grabowski wrote: > doc on other args. This seems an internal use function? > > Done. http://codereview.chromium.org/3064012/diff/52001/53009#newcode21 chrome/test/webdriver/dispatch.h:21: void SendResponse(struct mg_connection* const connection, On 2010/07/28 03:57:25, John Grabowski wrote: > doc on other args. This seems an internal use function? > > Done. http://codereview.chromium.org/3064012/diff/52001/53009#newcode26 chrome/test/webdriver/dispatch.h:26: // service. |CommandType| must be a subtype of |chrome::webdriver::Command|. On 2010/07/28 03:57:25, John Grabowski wrote: > webdriver:command > (I don't think you are using chrome:: namespace) > Done. http://codereview.chromium.org/3064012/diff/52001/53009#newcode28 chrome/test/webdriver/dispatch.h:28: void dispatch(struct mg_connection* connection, On 2010/07/28 03:03:10, jcarollo1 wrote: > dispatch->Dispatch. Done. http://codereview.chromium.org/3064012/diff/52001/53009#newcode28 chrome/test/webdriver/dispatch.h:28: void dispatch(struct mg_connection* connection, On 2010/07/28 03:03:10, jcarollo1 wrote: > dispatch->Dispatch. Done. http://codereview.chromium.org/3064012/diff/52001/53010 File chrome/test/webdriver/error_codes.h (right): http://codereview.chromium.org/3064012/diff/52001/53010#newcode5 chrome/test/webdriver/error_codes.h:5: #ifndef CHROME_TEST_WEBDRIVER_ERROR_CODES_H__ On 2010/07/28 03:57:25, John Grabowski wrote: > no double-__ at end; just one > Done. http://codereview.chromium.org/3064012/diff/52001/53010#newcode11 chrome/test/webdriver/error_codes.h:11: // http://code.google.com/p/selenium/wiki/JsonWireProtocol#Response_Status_Codes These error are fixed and will probably not change but I will ask if it can be included in the atoms.h file or generated with it. On 2010/07/28 03:57:25, John Grabowski wrote: > Do they provide a header we can use to keep in sync? > http://codereview.chromium.org/3064012/diff/52001/53012 File chrome/test/webdriver/keymap.h (right): http://codereview.chromium.org/3064012/diff/52001/53012#newcode13 chrome/test/webdriver/keymap.h:13: // Maps the key space used by WebDriver to Chrome for Linux/Mac/Windows On 2010/07/28 03:57:25, John Grabowski wrote: > remove this line (duped below) > Done. http://codereview.chromium.org/3064012/diff/52001/53012#newcode26 chrome/test/webdriver/keymap.h:26: void Clear(); On 2010/07/28 03:57:25, John Grabowski wrote: > What does Clear() mean? "unpress all"? > Seems like you're mixing semantics (e.g. modifiers are "press and hold until > cleared" but other keys are "press/release"). > Done. http://codereview.chromium.org/3064012/diff/52001/53012#newcode29 chrome/test/webdriver/keymap.h:29: bool shift, alt, control, command; On 2010/07/28 03:03:10, jcarollo1 wrote: > Should only declare one member per line. Members should be named with trailing > _. Done. http://codereview.chromium.org/3064012/diff/52001/53013 File chrome/test/webdriver/server.cc (right): http://codereview.chromium.org/3064012/diff/52001/53013#newcode8 chrome/test/webdriver/server.cc:8: #include <sys/time.h> On 2010/07/28 03:57:25, John Grabowski wrote: > << these Done. http://codereview.chromium.org/3064012/diff/52001/53013#newcode19 chrome/test/webdriver/server.cc:19: #include "third_party/mongoose/mongoose.h" On 2010/07/28 03:57:25, John Grabowski wrote: > alphabetize headers > Done. http://codereview.chromium.org/3064012/diff/52001/53013#newcode38 chrome/test/webdriver/server.cc:38: case 0: // The win compiler demoands at least 1 case statement On 2010/07/28 03:57:25, John Grabowski wrote: > demands > Done. http://codereview.chromium.org/3064012/diff/52001/53013#newcode38 chrome/test/webdriver/server.cc:38: case 0: // The win compiler demoands at least 1 case statement On 2010/07/28 03:57:25, John Grabowski wrote: > demands > Done. http://codereview.chromium.org/3064012/diff/52001/53013#newcode47 chrome/test/webdriver/server.cc:47: void set_callback(struct mg_context* ctx, const char* pattern) { On 2010/07/28 03:03:10, jcarollo1 wrote: > > SetCallback Done. http://codereview.chromium.org/3064012/diff/52001/53013#newcode47 chrome/test/webdriver/server.cc:47: void set_callback(struct mg_context* ctx, const char* pattern) { On 2010/07/28 03:03:10, jcarollo1 wrote: > > SetCallback Done. http://codereview.chromium.org/3064012/diff/52001/53013#newcode51 chrome/test/webdriver/server.cc:51: void init_callbacks(struct mg_context* ctx) { On 2010/07/28 03:03:10, jcarollo1 wrote: > InitCallbacks Done. http://codereview.chromium.org/3064012/diff/52001/53013#newcode60 chrome/test/webdriver/server.cc:60: int main(int argc, char *argv[]) { On 2010/07/28 03:57:25, John Grabowski wrote: > Brief description: > "main for WebDriver protocol server. > Runs the mg web server to implement wire blah. > Sends commands to chrome blah via automation proxy > see http://dev.whatevemormro.org for more details." > Done. http://codereview.chromium.org/3064012/diff/52001/53013#newcode90 chrome/test/webdriver/server.cc:90: // Listen on port 8080 On 2010/07/28 03:03:10, jcarollo1 wrote: > port 8080 or port specified on command line. Done. http://codereview.chromium.org/3064012/diff/52001/53013#newcode90 chrome/test/webdriver/server.cc:90: // Listen on port 8080 On 2010/07/28 03:03:10, jcarollo1 wrote: > port 8080 or port specified on command line. Done. http://codereview.chromium.org/3064012/diff/52001/53013#newcode107 chrome/test/webdriver/server.cc:107: // we should not reach here since the service should never quit On 2010/07/28 03:03:10, jcarollo1 wrote: > You should probably register a listener for SIGTERM and break your message loop > so that people can shut your server down gracefully. Consider adding a TODO for > a follow-up CL. Done. http://codereview.chromium.org/3064012/diff/52001/53013#newcode107 chrome/test/webdriver/server.cc:107: // we should not reach here since the service should never quit On 2010/07/28 03:03:10, jcarollo1 wrote: > You should probably register a listener for SIGTERM and break your message loop > so that people can shut your server down gracefully. Consider adding a TODO for > a follow-up CL. Done. http://codereview.chromium.org/3064012/diff/52001/53014 File chrome/test/webdriver/session_manager.cc (right): http://codereview.chromium.org/3064012/diff/52001/53014#newcode8 chrome/test/webdriver/session_manager.cc:8: #include <unistd.h> On 2010/07/28 03:57:25, John Grabowski wrote: > alphabetize > Done. http://codereview.chromium.org/3064012/diff/52001/53014#newcode46 chrome/test/webdriver/session_manager.cc:46: LOG(ERROR) << "Could not get system temp path" << std::endl; On 2010/07/28 03:57:25, John Grabowski wrote: > Perhaps use CHECK() or NOTREACHED()? > Done. http://codereview.chromium.org/3064012/diff/52001/53014#newcode228 chrome/test/webdriver/session_manager.cc:228: // in size and will add lots of noise to the logs. I think we should log everything since disk space, especially when in KB, is not really an issue. On 2010/07/28 03:57:25, John Grabowski wrote: > perhaps print first 64 chars of it with ... > http://codereview.chromium.org/3064012/diff/52001/53014#newcode305 chrome/test/webdriver/session_manager.cc:305: return std::string(""); I use the #endif as a fall through On 2010/07/28 03:57:25, John Grabowski wrote: > #else? > http://codereview.chromium.org/3064012/diff/52001/53014#newcode401 chrome/test/webdriver/session_manager.cc:401: const char* alternative_userdir = getenv("CHROME_UI_TESTS_USER_DATA_DIR"); On 2010/07/28 03:57:25, John Grabowski wrote: > having one variable name be different (incompatible) types on different > platforms is a little awkward. > Done. http://codereview.chromium.org/3064012/diff/52001/53015 File chrome/test/webdriver/session_manager.h (right): http://codereview.chromium.org/3064012/diff/52001/53015#newcode26 chrome/test/webdriver/session_manager.h:26: class Session { On 2010/07/28 03:57:25, John Grabowski wrote: > comment for each class > Done. http://codereview.chromium.org/3064012/diff/52001/53015#newcode26 chrome/test/webdriver/session_manager.h:26: class Session { On 2010/07/28 03:57:25, John Grabowski wrote: > comment for every class > comment for every nontrivial method > ... > Done. http://codereview.chromium.org/3064012/diff/52001/53016 File chrome/test/webdriver/utility_functions.cc (right): http://codereview.chromium.org/3064012/diff/52001/53016#newcode47 chrome/test/webdriver/utility_functions.cc:47: return L"LIST"; On 2010/07/28 03:57:25, John Grabowski wrote: > default: > return L"ERROR"; > Done. http://codereview.chromium.org/3064012/diff/52001/53017 File chrome/test/webdriver/utility_functions.h (right): http://codereview.chromium.org/3064012/diff/52001/53017#newcode13 chrome/test/webdriver/utility_functions.h:13: // TODO(jleyba): This should be auto-generated along with the atoms themselves. On 2010/07/28 03:57:25, John Grabowski wrote: > Make reference to "webdriver atom" and briefly describe. > Else the term "atom" is too generic. > Done.
http://codereview.chromium.org/3064012/diff/52001/53003 File chrome/test/webdriver/commands/command.h (right): http://codereview.chromium.org/3064012/diff/52001/53003#newcode80 chrome/test/webdriver/commands/command.h:80: inline Command(const std::string& method, On 2010/07/29 00:58:31, Joe wrote: > To do a POST you need to override the function DoesPost and ExecutePost. The > method name is the URL method in your REST protocol. For example /session > > On 2010/07/28 03:57:25, John Grabowski wrote: > > Be clear that method here is an HTTP method like POST. > > Since it's passed in as a string and not an enum you probably want to confirm > > method is valid and (e.g.) not a typo like POTS. > > > > APIs below look like you only allow DELETE, GET, and POST. > > > > > > I don't think that's true in the code. For example, in command.cc: void Command::Execute(Response* const response) { if (DoesPost() && method_ == "POST") { This implies the method is "POST", not "/session". Either your comment is wrong or the code is wrong. In either case, the correct answer needs to be documented. (And if method_ really is POST et al it should be confirmed correct on init.)
I've taken only a cursory look. 1. It appears to me that a huge bunch of utility functionality could be reused from UITestBase (in chrome/test/ui/ui_test.h). This would improve consistency with the other assumptions (like timeouts, cmd line arguments passed to chrome, etc). Besides, You'd get things like: cross-platform functions to create/delete profile dir, kill chrome (when it becomes stubborn), for free. 2. It did not compile for me (I was on linux). third_party/mongoose files missing 3. It would be nice to link to a reference doc somewhere about how to compile and run chromedriver. Maybe somewhere on dev.chromium.org http://codereview.chromium.org/3064012/diff/23004/39004 File chrome/test/webdriver/commands/command.cc (right): http://codereview.chromium.org/3064012/diff/23004/39004#newcode22 chrome/test/webdriver/commands/command.cc:22: nit: blank line not needed http://codereview.chromium.org/3064012/diff/23004/39004#newcode27 chrome/test/webdriver/commands/command.cc:27: nit: blank line not needed http://codereview.chromium.org/3064012/diff/23004/39004#newcode32 chrome/test/webdriver/commands/command.cc:32: nit: blank line not needed http://codereview.chromium.org/3064012/diff/23004/39004#newcode45 chrome/test/webdriver/commands/command.cc:45: } nit: need blank line after this http://codereview.chromium.org/3064012/diff/23004/39005 File chrome/test/webdriver/commands/command.h (right): http://codereview.chromium.org/3064012/diff/23004/39005#newcode75 chrome/test/webdriver/commands/command.h:75: }; nit: need blank line after this http://codereview.chromium.org/3064012/diff/23004/39006 File chrome/test/webdriver/commands/response.h (right): http://codereview.chromium.org/3064012/diff/23004/39006#newcode14 chrome/test/webdriver/commands/response.h:14: namespace webdriver { nit: need blank line after this http://codereview.chromium.org/3064012/diff/23004/39006#newcode15 chrome/test/webdriver/commands/response.h:15: // A simple class that encapsulates the information describing the response to "Encapsulates the information describing..." (It's evident that it's a class) http://codereview.chromium.org/3064012/diff/23004/39006#newcode71 chrome/test/webdriver/commands/response.h:71: }; nit: need blank line after this. (Please fix this in all files. Not reporting anymore) http://codereview.chromium.org/3064012/diff/23004/39007 File chrome/test/webdriver/commands/session_command.cc (right): http://codereview.chromium.org/3064012/diff/23004/39007#newcode37 chrome/test/webdriver/commands/session_command.cc:37: temp_value->SetString(std::wstring(L"browserName"), You don't need to std::wstring() it. L"browserName" is good enough. http://codereview.chromium.org/3064012/diff/23004/39007#newcode39 chrome/test/webdriver/commands/session_command.cc:39: temp_value->SetString(std::wstring(L"version"), same here, and below http://codereview.chromium.org/3064012/diff/23004/39008 File chrome/test/webdriver/commands/session_command.h (right): http://codereview.chromium.org/3064012/diff/23004/39008#newcode16 chrome/test/webdriver/commands/session_command.h:16: // Create a new session. The desired capabilities should be specified Could you briefly describe here what a session is? http://codereview.chromium.org/3064012/diff/23004/39009 File chrome/test/webdriver/commands/webdriver_command.cc (right): http://codereview.chromium.org/3064012/diff/23004/39009#newcode7 chrome/test/webdriver/commands/webdriver_command.cc:7: #include <string> nit: need blank line after this http://codereview.chromium.org/3064012/diff/23004/39009#newcode11 chrome/test/webdriver/commands/webdriver_command.cc:11: #include "base/values.h" not in order http://codereview.chromium.org/3064012/diff/23004/39009#newcode17 chrome/test/webdriver/commands/webdriver_command.cc:17: #include "chrome/test/automation/automation_proxy.h" Several of these includes are not used in this file http://codereview.chromium.org/3064012/diff/23004/39009#newcode32 chrome/test/webdriver/commands/webdriver_command.cc:32: return dictionary->Get(kElementDictionaryKey, &element_id) && std::string element_id; return dictionary->GetString(kElementDictionaryKey, &element_id); http://codereview.chromium.org/3064012/diff/23004/39009#newcode52 chrome/test/webdriver/commands/webdriver_command.cc:52: LOG(INFO) << "Fetching session: " << session_id; This sounds more like LOG(DEBUG) http://codereview.chromium.org/3064012/diff/23004/39009#newcode54 chrome/test/webdriver/commands/webdriver_command.cc:54: if (session_ == NULL) { if (!session_) http://codereview.chromium.org/3064012/diff/23004/39010 File chrome/test/webdriver/commands/webdriver_command.h (right): http://codereview.chromium.org/3064012/diff/23004/39010#newcode50 chrome/test/webdriver/commands/webdriver_command.h:50: // Waits for a new navigation if none has occurred within 500ms of Why do we care if a previous navigation has happened within 500ms? What is 500ms? How long does it wait? http://codereview.chromium.org/3064012/diff/23004/39012 File chrome/test/webdriver/dispatch.h (right): http://codereview.chromium.org/3064012/diff/23004/39012#newcode10 chrome/test/webdriver/dispatch.h:10: #include <vector> need blank line after this http://codereview.chromium.org/3064012/diff/23004/39012#newcode11 chrome/test/webdriver/dispatch.h:11: #include "third_party/mongoose/mongoose.h" not in order http://codereview.chromium.org/3064012/diff/23004/39012#newcode44 chrome/test/webdriver/dispatch.h:44: LOG(INFO) << "...parsing request body"; Looks like LOG(DEBUG) to me http://codereview.chromium.org/3064012/diff/23004/39015 File chrome/test/webdriver/keymap.h (right): http://codereview.chromium.org/3064012/diff/23004/39015#newcode14 chrome/test/webdriver/keymap.h:14: // Maps the key space used by WebDriver to Chrome for Linux/Mac/Windows Should end with '.' http://codereview.chromium.org/3064012/diff/23004/39016 File chrome/test/webdriver/server.cc (right): http://codereview.chromium.org/3064012/diff/23004/39016#newcode14 chrome/test/webdriver/server.cc:14: #include <iostream> not in order http://codereview.chromium.org/3064012/diff/23004/39016#newcode20 chrome/test/webdriver/server.cc:20: #include "chrome/test/webdriver/commands/session_command.h" not in order http://codereview.chromium.org/3064012/diff/23004/39016#newcode29 chrome/test/webdriver/server.cc:29: // Make sure we have ho zombies from CGIs comments should end in '.' http://codereview.chromium.org/3064012/diff/23004/39017 File chrome/test/webdriver/session_manager.cc (right): http://codereview.chromium.org/3064012/diff/23004/39017#newcode27 chrome/test/webdriver/session_manager.cc:27: #include "chrome/test/automation/tab_proxy.h" not in order http://codereview.chromium.org/3064012/diff/23004/39017#newcode81 chrome/test/webdriver/session_manager.cc:81: AutomationLaunchResult result = proxy_->WaitForAppLaunch(); Have you considered using ui_test.h? Several things you do here (like this line) are available in UITestBase, and can be reused. Besides, it does a lot of other things like set pass options like --no-first-run, disable error dialogs, set timeouts. There are functions to create unique temporary directory. http://codereview.chromium.org/3064012/diff/23004/39017#newcode184 chrome/test/webdriver/session_manager.cc:184: bool DeleteDirectory(const char* directory) { use base::DieFileDie? http://codereview.chromium.org/3064012/diff/23004/39017#newcode210 chrome/test/webdriver/session_manager.cc:210: // DeleteDirectory(tmp_profile_dir_); why is this commented out? http://codereview.chromium.org/3064012/diff/23004/39017#newcode222 chrome/test/webdriver/session_manager.cc:222: std::wstring jscript = L"window.domAutomationController.send("; A giant StringPrintf would look neater http://codereview.chromium.org/3064012/diff/23004/39017#newcode411 chrome/test/webdriver/session_manager.cc:411: FilePath command = These are already done in UITestBase. Why not reuse? http://codereview.chromium.org/3064012/diff/23004/39018 File chrome/test/webdriver/session_manager.h (right): http://codereview.chromium.org/3064012/diff/23004/39018#newcode9 chrome/test/webdriver/session_manager.h:9: #include <sys/stat.h> not in order http://codereview.chromium.org/3064012/diff/23004/39018#newcode11 chrome/test/webdriver/session_manager.h:11: #include <string> nit: need blank line after this http://codereview.chromium.org/3064012/diff/23004/39018#newcode16 chrome/test/webdriver/session_manager.h:16: #include "base/singleton.h" not in order http://codereview.chromium.org/3064012/diff/23004/39018#newcode66 chrome/test/webdriver/session_manager.h:66: inline int implicit_wait() { return implicit_wait_; } what is this? http://codereview.chromium.org/3064012/diff/23004/39019 File chrome/test/webdriver/utility_functions.cc (right): http://codereview.chromium.org/3064012/diff/23004/39019#newcode30 chrome/test/webdriver/utility_functions.cc:30: std::wstring print_valuetype(Value::ValueType e) { misnomer. it's not really printing anythin http://codereview.chromium.org/3064012/diff/23004/39021 File chrome/test/webdriver/webdriver_tests.py (right): http://codereview.chromium.org/3064012/diff/23004/39021#newcode4 chrome/test/webdriver/webdriver_tests.py:4: # author: Jason Leyba Chromium convention is to not include individual author names http://codereview.chromium.org/3064012/diff/23004/39021#newcode25 chrome/test/webdriver/webdriver_tests.py:25: Arguments: s/Arguments/Args http://codereview.chromium.org/3064012/diff/23004/39021#newcode49 chrome/test/webdriver/webdriver_tests.py:49: Arguments: s/Arguments/Args
Note: I am a significant change in how commands are executed and removed passing in the method string http://codereview.chromium.org/3064012/diff/52001/53002 File chrome/test/webdriver/commands/command.cc (right): http://codereview.chromium.org/3064012/diff/52001/53002#newcode18 chrome/test/webdriver/commands/command.cc:18: if (DoesPost() && method_ == "POST") { On 2010/07/28 03:57:25, John Grabowski wrote: > If DoesPost() et al implemented as suggested in header you can simplify this. Done. http://codereview.chromium.org/3064012/diff/52001/53003 File chrome/test/webdriver/commands/command.h (right): http://codereview.chromium.org/3064012/diff/52001/53003#newcode94 chrome/test/webdriver/commands/command.h:94: virtual bool DoesDelete() { return false; } The class that inherits this class must override DoesDelete to be true. I got rid of method_ On 2010/07/28 03:57:25, John Grabowski wrote: > Perhaps these could all be implemented in base class? > e.g. > bool DoesDelete() { return method_ == "DELETE"; } > http://codereview.chromium.org/3064012/diff/52001/53003#newcode117 chrome/test/webdriver/commands/command.h:117: // NULL if there is no such parameter, or if it is not a string. I follow the syntax of GetStringASCII in class DictionaryValue. If that function returned false I wanted to pass it back We also do a check with the return value of GetStringASCII in class DictionaryValue. On 2010/07/28 03:57:25, John Grabowski wrote: > This method does not have the opportunity to set out to NULL. Perhaps you mean > it returns the empty string if there is no key? If so, why not do so as a > return value instead of passed in arg? > http://codereview.chromium.org/3064012/diff/52001/53007 File chrome/test/webdriver/commands/webdriver_command.h (right): http://codereview.chromium.org/3064012/diff/52001/53007#newcode16 chrome/test/webdriver/commands/webdriver_command.h:16: class WebDriverCommand : public Command { On 2010/07/28 03:57:25, John Grabowski wrote: > comment for each class Done. http://codereview.chromium.org/3064012/diff/52001/53014 File chrome/test/webdriver/session_manager.cc (right): http://codereview.chromium.org/3064012/diff/52001/53014#newcode68 chrome/test/webdriver/session_manager.cc:68: bool Session::Init(base::ProcessHandle process_handle) { On 2010/07/28 03:57:25, John Grabowski wrote: > A little long. Consider splitting up into "wait for loads and launch app and > open browser window" and "get proxies". Will be forward-looking (e.g. in case > it connects to existing browser a la PyAuto integration). > Done. http://codereview.chromium.org/3064012/diff/52001/53014#newcode177 chrome/test/webdriver/session_manager.cc:177: bool DeleteDirectory(const char* directory) { fixed On 2010/07/28 03:57:25, John Grabowski wrote: > Not called? http://codereview.chromium.org/3064012/diff/52001/53014#newcode203 chrome/test/webdriver/session_manager.cc:203: // DeleteDirectory(tmp_profile_dir_); fixed On 2010/07/28 03:57:25, John Grabowski wrote: > Need to implement this > http://codereview.chromium.org/3064012/diff/52001/53014#newcode278 chrome/test/webdriver/session_manager.cc:278: std::map<std::string, Session*> SessionManager::map_; On 2010/07/28 03:57:25, John Grabowski wrote: > global objects of class type forbidden. > Applies to all 4 of these objects (including std::string) > > Done. http://codereview.chromium.org/3064012/diff/52001/53018 File chrome/test/webdriver/webdriver_tests.py (right): http://codereview.chromium.org/3064012/diff/52001/53018#newcode9 chrome/test/webdriver/webdriver_tests.py:9: import simplejson as json The system path variable should be properly set when the test runs. PyAuto does the same import so I will check to make sure we run in the same environment. On 2010/07/28 03:57:25, John Grabowski wrote: > not in default path. Do you need a sys.path extension for ../../third_party, or > do we have a webdriver_tests.py test runner? http://codereview.chromium.org/3064012/diff/52001/53018#newcode33 chrome/test/webdriver/webdriver_tests.py:33: elif method != 'POST' and method != 'PUT': On 2010/07/28 03:57:25, John Grabowski wrote: > elif method not in ('POST', 'PUT'): > Done. http://codereview.chromium.org/3064012/diff/52001/53018#newcode64 chrome/test/webdriver/webdriver_tests.py:64: class WebDriverTest(unittest.TestCase): On 2010/07/28 03:57:25, John Grabowski wrote: > brief class description. E.g. aren't these ALL WebDriver tests? Done. http://codereview.chromium.org/3064012/diff/52001/53018#newcode151 chrome/test/webdriver/webdriver_tests.py:151: import optparse On 2010/07/28 03:57:25, John Grabowski wrote: > import optparse goes at top > Done. http://codereview.chromium.org/3064012/diff/52001/53018#newcode153 chrome/test/webdriver/webdriver_tests.py:153: parser = optparse.OptionParser('%prog [options]') On 2010/07/28 03:57:25, John Grabowski wrote: > test implies webdriver server already running? > Seems like it should launch it. > Done.
Glad to see this making progress. Are you ready for re-review? jrg On Tue, Aug 3, 2010 at 10:31 AM, <jmikhail@google.com> wrote: > Note: I am a significant change in how commands are executed and removed > passing > in the method string > > > http://codereview.chromium.org/3064012/diff/52001/53002 > > File chrome/test/webdriver/commands/command.cc (right): > > http://codereview.chromium.org/3064012/diff/52001/53002#newcode18 > chrome/test/webdriver/commands/command.cc:18: if (DoesPost() && method_ > == "POST") { > On 2010/07/28 03:57:25, John Grabowski wrote: > >> If DoesPost() et al implemented as suggested in header you can >> > simplify this. > > Done. > > http://codereview.chromium.org/3064012/diff/52001/53003 > > File chrome/test/webdriver/commands/command.h (right): > > http://codereview.chromium.org/3064012/diff/52001/53003#newcode94 > chrome/test/webdriver/commands/command.h:94: virtual bool DoesDelete() { > return false; } > The class that inherits this class must override DoesDelete to be true. > I got rid of method_ > > > On 2010/07/28 03:57:25, John Grabowski wrote: > >> Perhaps these could all be implemented in base class? >> e.g. >> bool DoesDelete() { return method_ == "DELETE"; } >> > > > http://codereview.chromium.org/3064012/diff/52001/53003#newcode117 > chrome/test/webdriver/commands/command.h:117: // NULL if there is no > such parameter, or if it is not a string. > I follow the syntax of GetStringASCII in class DictionaryValue. If that > function returned false I wanted to pass it back > > We also do a check with the return value of GetStringASCII in class > DictionaryValue. > > > On 2010/07/28 03:57:25, John Grabowski wrote: > >> This method does not have the opportunity to set out to NULL. Perhaps >> > you mean > >> it returns the empty string if there is no key? If so, why not do so >> > as a > >> return value instead of passed in arg? >> > > > http://codereview.chromium.org/3064012/diff/52001/53007 > > File chrome/test/webdriver/commands/webdriver_command.h (right): > > http://codereview.chromium.org/3064012/diff/52001/53007#newcode16 > chrome/test/webdriver/commands/webdriver_command.h:16: class > WebDriverCommand : public Command { > On 2010/07/28 03:57:25, John Grabowski wrote: > >> comment for each class >> > > Done. > > http://codereview.chromium.org/3064012/diff/52001/53014 > > File chrome/test/webdriver/session_manager.cc (right): > > http://codereview.chromium.org/3064012/diff/52001/53014#newcode68 > chrome/test/webdriver/session_manager.cc:68: bool > Session::Init(base::ProcessHandle process_handle) { > On 2010/07/28 03:57:25, John Grabowski wrote: > >> A little long. Consider splitting up into "wait for loads and launch >> > app and > >> open browser window" and "get proxies". Will be forward-looking (e.g. >> > in case > >> it connects to existing browser a la PyAuto integration). >> > > > Done. > > http://codereview.chromium.org/3064012/diff/52001/53014#newcode177 > chrome/test/webdriver/session_manager.cc:177: bool DeleteDirectory(const > char* directory) { > fixed > > On 2010/07/28 03:57:25, John Grabowski wrote: > >> Not called? >> > > http://codereview.chromium.org/3064012/diff/52001/53014#newcode203 > chrome/test/webdriver/session_manager.cc:203: // > DeleteDirectory(tmp_profile_dir_); > fixed > > On 2010/07/28 03:57:25, John Grabowski wrote: > >> Need to implement this >> > > > http://codereview.chromium.org/3064012/diff/52001/53014#newcode278 > chrome/test/webdriver/session_manager.cc:278: std::map<std::string, > Session*> SessionManager::map_; > On 2010/07/28 03:57:25, John Grabowski wrote: > >> global objects of class type forbidden. >> Applies to all 4 of these objects (including std::string) >> > > > > Done. > > http://codereview.chromium.org/3064012/diff/52001/53018 > > File chrome/test/webdriver/webdriver_tests.py (right): > > http://codereview.chromium.org/3064012/diff/52001/53018#newcode9 > chrome/test/webdriver/webdriver_tests.py:9: import simplejson as json > The system path variable should be properly set when the test runs. > PyAuto does the same import so I will check to make sure we run in the > same environment. > > > On 2010/07/28 03:57:25, John Grabowski wrote: > >> not in default path. Do you need a sys.path extension for >> > ../../third_party, or > >> do we have a webdriver_tests.py test runner? >> > > http://codereview.chromium.org/3064012/diff/52001/53018#newcode33 > chrome/test/webdriver/webdriver_tests.py:33: elif method != 'POST' and > method != 'PUT': > On 2010/07/28 03:57:25, John Grabowski wrote: > >> elif method not in ('POST', 'PUT'): >> > > > Done. > > > http://codereview.chromium.org/3064012/diff/52001/53018#newcode64 > chrome/test/webdriver/webdriver_tests.py:64: class > WebDriverTest(unittest.TestCase): > On 2010/07/28 03:57:25, John Grabowski wrote: > >> brief class description. E.g. aren't these ALL WebDriver tests? >> > > Done. > > http://codereview.chromium.org/3064012/diff/52001/53018#newcode151 > chrome/test/webdriver/webdriver_tests.py:151: import optparse > > On 2010/07/28 03:57:25, John Grabowski wrote: > >> import optparse goes at top >> > > > Done. > > > http://codereview.chromium.org/3064012/diff/52001/53018#newcode153 > chrome/test/webdriver/webdriver_tests.py:153: parser = > optparse.OptionParser('%prog [options]') > On 2010/07/28 03:57:25, John Grabowski wrote: > >> test implies webdriver server already running? >> Seems like it should launch it. >> > > > Done. > > > http://codereview.chromium.org/3064012/show >
http://codereview.chromium.org/3064012/diff/41003/14026 File chrome/test/webdriver/commands/session_command.cc (right): http://codereview.chromium.org/3064012/diff/41003/14026#newcode54 chrome/test/webdriver/commands/session_command.cc:54: temp_value->SetBoolean(std::wstring(L"javascriptEnabled"), true); Is this because Chrome cannot be run with javascript disabled using WebDriver? I though you could set that option when creating an instance of WebDriver? http://codereview.chromium.org/3064012/diff/41003/14026#newcode61 chrome/test/webdriver/commands/session_command.cc:61: Singleton<SessionManager>::get()->Delete(session_->id()); Is this guaranteed to be Idempotent? If I call delete the second time will the get() return a valid pointer or will it NullRef?
http://codereview.chromium.org/3064012/diff/41003/14026 File chrome/test/webdriver/commands/session_command.cc (right): http://codereview.chromium.org/3064012/diff/41003/14026#newcode54 chrome/test/webdriver/commands/session_command.cc:54: temp_value->SetBoolean(std::wstring(L"javascriptEnabled"), true); We currently require javascript to always run in this version On 2010/08/03 22:36:51, amolk wrote: > Is this because Chrome cannot be run with javascript disabled using WebDriver? I > though you could set that option when creating an instance of WebDriver? http://codereview.chromium.org/3064012/diff/41003/14026#newcode61 chrome/test/webdriver/commands/session_command.cc:61: Singleton<SessionManager>::get()->Delete(session_->id()); the second call will fail since the session ID is removed on delete. So on the second call you will get a HTTP Not Found URL. On 2010/08/03 22:36:51, amolk wrote: > Is this guaranteed to be Idempotent? If I call delete the second time will the > get() return a valid pointer or will it NullRef?
Looking a lot better. Most of my remaining comments relates to comments or naming. http://codereview.chromium.org/3064012/diff/52001/53004 File chrome/test/webdriver/commands/session_command.cc (right): http://codereview.chromium.org/3064012/diff/52001/53004#newcode17 chrome/test/webdriver/commands/session_command.cc:17: void SessionCommand::ExecutePost(Response* const response) { On 2010/07/29 00:58:31, Joe wrote: > I added a link to the doc in the header. Not nearly enough. > This function is to create a browser > session; so a new instance of chrome is always created Say that explicitly in the header. It isn't clear if "create session" launches Chrome or simply attaches to a running instance. http://codereview.chromium.org/3064012/diff/52001/53007 File chrome/test/webdriver/commands/webdriver_command.h (right): http://codereview.chromium.org/3064012/diff/52001/53007#newcode50 chrome/test/webdriver/commands/webdriver_command.h:50: // Waits for a new navigation if none has occurred within 500ms of On 2010/07/28 03:57:25, John Grabowski wrote: > What if navigation happened within 400ms? What will happen? Did we wait for > it? Plz answer. http://codereview.chromium.org/3064012/diff/52001/53010 File chrome/test/webdriver/error_codes.h (right): http://codereview.chromium.org/3064012/diff/52001/53010#newcode11 chrome/test/webdriver/error_codes.h:11: // http://code.google.com/p/selenium/wiki/JsonWireProtocol#Response_Status_Codes On 2010/07/29 00:58:31, Joe wrote: > These error are fixed and will probably not change but I will ask if it can be > included in the atoms.h file or generated with it. > On 2010/07/28 03:57:25, John Grabowski wrote: > > Do they provide a header we can use to keep in sync? > > > > If you think it's very stable then we're probably fine. But would be nice to be in atoms.h to be sure. (I won't let that hold up this CL.) http://codereview.chromium.org/3064012/diff/52001/53014 File chrome/test/webdriver/session_manager.cc (right): http://codereview.chromium.org/3064012/diff/52001/53014#newcode138 chrome/test/webdriver/session_manager.cc:138: bool Session::CreateTemporaryProfileDirectory() { On 2010/07/28 03:57:25, John Grabowski wrote: > scoped_object in Session structure to delete temp dir so this directory doesn't > leak and run out of disk space? Or use temp dir from command line so process > which launched me (e.g. python script) can delete it in case I crash? > plz consider this don't want to leave temp directories around especially if this gets run continuously on bots http://codereview.chromium.org/3064012/diff/52001/53015 File chrome/test/webdriver/session_manager.h (right): http://codereview.chromium.org/3064012/diff/52001/53015#newcode29 chrome/test/webdriver/session_manager.h:29: typedef char ProfileDir[L_tmpnam + 1]; // +1 for \0 On 2010/07/28 03:57:25, John Grabowski wrote: > You are not guaranteed ::GetTempPath() uses tmpnam() so MAX_PATH might be better > for POSIX. Pls fix. Use MAX_PATH http://codereview.chromium.org/3064012/diff/41003/14022 File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/3064012/diff/41003/14022#newcode435 chrome/chrome_tests.gypi:435: 'target_name': 'chromedriver', Add comment saying what chromedriver is for. E.g. # chrome compoent for WebDriver support. http://blah/blah It might get confused with chromebot. http://codereview.chromium.org/3064012/diff/41003/14023 File chrome/test/webdriver/commands/command.cc (right): http://codereview.chromium.org/3064012/diff/41003/14023#newcode9 chrome/test/webdriver/commands/command.cc:9: const wchar_t* const Response::kStatusKey = L"status"; Do we have a convention for this in chrome? On the Mac side it might be const wchar_t* const --> wchar_t* const but I'm not sure for the C++ side. Is worth confirmation. http://codereview.chromium.org/3064012/diff/41003/14024 File chrome/test/webdriver/commands/command.h (right): http://codereview.chromium.org/3064012/diff/41003/14024#newcode60 chrome/test/webdriver/commands/command.h:60: // NULL if there is no such parameter, or if it is not a string. s/NULL/false/g http://codereview.chromium.org/3064012/diff/41003/14025 File chrome/test/webdriver/commands/response.h (right): http://codereview.chromium.org/3064012/diff/41003/14025#newcode17 chrome/test/webdriver/commands/response.h:17: // a |Command|. See: Be just a little more specific. E.g. "response always a json dict" http://codereview.chromium.org/3064012/diff/41003/14025#newcode35 chrome/test/webdriver/commands/response.h:35: LOG_IF(WARNING, data_.Get(kValueKey, &out)) LOG_IF_NOT? http://codereview.chromium.org/3064012/diff/41003/14027 File chrome/test/webdriver/commands/session_command.h (right): http://codereview.chromium.org/3064012/diff/41003/14027#newcode19 chrome/test/webdriver/commands/session_command.h:19: class SessionCommand : public Command { I'm a bit wary of the naming here. In the CS context, a session is a communication channel or conversation. Commands would go across a session. It seems a bit awkward for a Session to be a Command. Perhaps this should be called CreateSessionCommand? Why do you not have the functional equivalent of a DestroySessionCommand? http://codereview.chromium.org/3064012/diff/41003/14027#newcode37 chrome/test/webdriver/commands/session_command.h:37: class SessionWithIDCommand : public WebDriverCommand { If placed in this file I would expect SessionWithIDCommand to create a session. But it does not. Can we move it into a different file? Given that it returns session capabilities, perhaps it should be called SessionCapabilitiesCommand? Looks like this is the only way to destroy a session. Not sure "delete session" is logically the same as "get capabilities" unless it's called something like GetOrSetSessionStateCommand. http://codereview.chromium.org/3064012/diff/41003/14028 File chrome/test/webdriver/commands/webdriver_command.cc (right): http://codereview.chromium.org/3064012/diff/41003/14028#newcode67 chrome/test/webdriver/commands/webdriver_command.cc:67: tab_ = session_->ActivateTab(); It isn't clear why you save a copy of tab_; you don't appear to use it anywhere else. http://codereview.chromium.org/3064012/diff/41003/14029 File chrome/test/webdriver/commands/webdriver_command.h (right): http://codereview.chromium.org/3064012/diff/41003/14029#newcode18 chrome/test/webdriver/commands/webdriver_command.h:18: // and are to be supporte for all browsers and platforms should inhert supporte --> supported http://codereview.chromium.org/3064012/diff/41003/14029#newcode19 chrome/test/webdriver/commands/webdriver_command.h:19: // this class. For all other cases the Command class should be used. Your comment just isn't clear enough. Can you give examples of commands which should be WebDriverCommands, and commands which should be just "Commands", and why? http://codereview.chromium.org/3064012/diff/41003/14029#newcode24 chrome/test/webdriver/commands/webdriver_command.h:24: : Command(path_segments, parameters) {} You should init/zero member pointer session_ in your constructor. http://codereview.chromium.org/3064012/diff/41003/14030 File chrome/test/webdriver/dispatch.cc (right): http://codereview.chromium.org/3064012/diff/41003/14030#newcode133 chrome/test/webdriver/dispatch.cc:133: void DispatchCommand(Command* const command, const std::string& method, This looks like the central loop/funnel for chromedriver. A little doc on how we get here might be nice. http://codereview.chromium.org/3064012/diff/41003/14031 File chrome/test/webdriver/dispatch.h (right): http://codereview.chromium.org/3064012/diff/41003/14031#newcode31 chrome/test/webdriver/dispatch.h:31: void DispatchCommand(Command* const command, const std::string& method, comment on each function http://codereview.chromium.org/3064012/diff/41003/14034 File chrome/test/webdriver/keymap.h (right): http://codereview.chromium.org/3064012/diff/41003/14034#newcode26 chrome/test/webdriver/keymap.h:26: void Clear(); Perhaps rename ClearModifiers() http://codereview.chromium.org/3064012/diff/41003/14038 File chrome/test/webdriver/session_manager.cc (right): http://codereview.chromium.org/3064012/diff/41003/14038#newcode121 chrome/test/webdriver/session_manager.cc:121: id += text[random() % (sizeof text - 1)]; It's unlikely but there is no way to guarantee 2 session IDs in a row are unique. (Like the lottery you may just get lucky.) Perhaps you can simply have an incrementing counter? http://codereview.chromium.org/3064012/diff/41003/14039 File chrome/test/webdriver/session_manager.h (right): http://codereview.chromium.org/3064012/diff/41003/14039#newcode18 chrome/test/webdriver/session_manager.h:18: class SessionManager { What does it mean to have 2 sessions? Does that mean we're running 2 chromes? http://codereview.chromium.org/3064012/diff/41003/14039#newcode36 chrome/test/webdriver/session_manager.h:36: Lock session_generation; Trailing _ for these 3 What do these mean? Why does a session manager have an addr and port? I understand if a session does, but not the session manager. http://codereview.chromium.org/3064012/diff/41003/14042 File chrome/test/webdriver/webdriver_tests.py (right): http://codereview.chromium.org/3064012/diff/41003/14042#newcode1 chrome/test/webdriver/webdriver_tests.py:1: #!/usr/bin/python I like this file a lot. http://codereview.chromium.org/3064012/diff/41003/14042#newcode111 chrome/test/webdriver/webdriver_tests.py:111: class WebDriverTest(unittest.TestCase): class WebDriverSessionlessTest
I am also writing up 2 docs to publish to the chromium website. One will be how to setup and run chromedriver with a simple hello world app. Another will be detailed instructions on how the internals if chromedriver work and how to add a command with an example. http://codereview.chromium.org/3064012/diff/52001/53004 File chrome/test/webdriver/commands/session_command.cc (right): http://codereview.chromium.org/3064012/diff/52001/53004#newcode17 chrome/test/webdriver/commands/session_command.cc:17: void SessionCommand::ExecutePost(Response* const response) { On 2010/08/05 03:01:17, John Grabowski wrote: > On 2010/07/29 00:58:31, Joe wrote: > > I added a link to the doc in the header. > > Not nearly enough. > > > This function is to create a browser > > session; so a new instance of chrome is always created > > Say that explicitly in the header. It isn't clear if "create session" launches > Chrome or simply attaches to a running instance. > Done. http://codereview.chromium.org/3064012/diff/52001/53007 File chrome/test/webdriver/commands/webdriver_command.h (right): http://codereview.chromium.org/3064012/diff/52001/53007#newcode50 chrome/test/webdriver/commands/webdriver_command.h:50: // Waits for a new navigation if none has occurred within 500ms of The code for this function was pulled out for this 1st checkin but i forgot to remove it. So I am removing it now On 2010/08/05 03:01:17, John Grabowski wrote: > On 2010/07/28 03:57:25, John Grabowski wrote: > > What if navigation happened within 400ms? What will happen? Did we wait for > > it? > > Plz answer. > http://codereview.chromium.org/3064012/diff/52001/53010 File chrome/test/webdriver/error_codes.h (right): http://codereview.chromium.org/3064012/diff/52001/53010#newcode11 chrome/test/webdriver/error_codes.h:11: // http://code.google.com/p/selenium/wiki/JsonWireProtocol#Response_Status_Codes On 2010/08/05 03:01:17, John Grabowski wrote: > On 2010/07/29 00:58:31, Joe wrote: > > These error are fixed and will probably not change but I will ask if it can be > > included in the atoms.h file or generated with it. > > On 2010/07/28 03:57:25, John Grabowski wrote: > > > Do they provide a header we can use to keep in sync? > > > > > > > > > If you think it's very stable then we're probably fine. > But would be nice to be in atoms.h to be sure. > (I won't let that hold up this CL.) > > Done. http://codereview.chromium.org/3064012/diff/52001/53014 File chrome/test/webdriver/session_manager.cc (right): http://codereview.chromium.org/3064012/diff/52001/53014#newcode138 chrome/test/webdriver/session_manager.cc:138: bool Session::CreateTemporaryProfileDirectory() { The directory will be deleted when the session is closed see Session::Terminate(). I have to keep the directory until then since I won't know when the user is done until the user sends in the delete command for the session to end. On 2010/08/05 03:01:17, John Grabowski wrote: > On 2010/07/28 03:57:25, John Grabowski wrote: > > scoped_object in Session structure to delete temp dir so this directory > doesn't > > leak and run out of disk space? Or use temp dir from command line so process > > which launched me (e.g. python script) can delete it in case I crash? > > > > plz consider this > don't want to leave temp directories around especially if this gets run > continuously on bots > > http://codereview.chromium.org/3064012/diff/52001/53015 File chrome/test/webdriver/session_manager.h (right): http://codereview.chromium.org/3064012/diff/52001/53015#newcode29 chrome/test/webdriver/session_manager.h:29: typedef char ProfileDir[L_tmpnam + 1]; // +1 for \0 GetTempPath is only used on windows, for POSIX we use mkdtemp. See Session::CreateTemporaryProfileDirectory On 2010/08/05 03:01:17, John Grabowski wrote: > On 2010/07/28 03:57:25, John Grabowski wrote: > > You are not guaranteed ::GetTempPath() uses tmpnam() so MAX_PATH might be > better > > for POSIX. > > Pls fix. Use MAX_PATH > > http://codereview.chromium.org/3064012/diff/41003/14022 File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/3064012/diff/41003/14022#newcode435 chrome/chrome_tests.gypi:435: 'target_name': 'chromedriver', On 2010/08/05 03:01:17, John Grabowski wrote: > Add comment saying what chromedriver is for. > E.g. > # chrome compoent for WebDriver support. http://blah/blah > > It might get confused with chromebot. > Done. http://codereview.chromium.org/3064012/diff/41003/14023 File chrome/test/webdriver/commands/command.cc (right): http://codereview.chromium.org/3064012/diff/41003/14023#newcode9 chrome/test/webdriver/commands/command.cc:9: const wchar_t* const Response::kStatusKey = L"status"; I've seen const wchar_t* const in other files; for example, base/win_util.cc. But I have also seen constants listed as const wchar_t foo[] = L"foo", so I don't think there is a convention. On 2010/08/05 03:01:17, John Grabowski wrote: > Do we have a convention for this in chrome? > On the Mac side it might be > const wchar_t* const --> wchar_t* const > but I'm not sure for the C++ side. > Is worth confirmation. > > http://codereview.chromium.org/3064012/diff/41003/14024 File chrome/test/webdriver/commands/command.h (right): http://codereview.chromium.org/3064012/diff/41003/14024#newcode60 chrome/test/webdriver/commands/command.h:60: // NULL if there is no such parameter, or if it is not a string. On 2010/08/05 03:01:17, John Grabowski wrote: > s/NULL/false/g > Done. http://codereview.chromium.org/3064012/diff/41003/14025 File chrome/test/webdriver/commands/response.h (right): http://codereview.chromium.org/3064012/diff/41003/14025#newcode17 chrome/test/webdriver/commands/response.h:17: // a |Command|. See: On 2010/08/05 03:01:17, John Grabowski wrote: > Be just a little more specific. E.g. "response always a json dict" > > Done. http://codereview.chromium.org/3064012/diff/41003/14025#newcode35 chrome/test/webdriver/commands/response.h:35: LOG_IF(WARNING, data_.Get(kValueKey, &out)) On 2010/08/05 03:01:17, John Grabowski wrote: > LOG_IF_NOT? > Done. http://codereview.chromium.org/3064012/diff/41003/14027 File chrome/test/webdriver/commands/session_command.h (right): http://codereview.chromium.org/3064012/diff/41003/14027#newcode19 chrome/test/webdriver/commands/session_command.h:19: class SessionCommand : public Command { I renamed the class http://codereview.chromium.org/3064012/diff/41003/14027#newcode37 chrome/test/webdriver/commands/session_command.h:37: class SessionWithIDCommand : public WebDriverCommand { I think you are right that "delete session" is not logically the same as "get capabilities" but that WebDriver spec require that this be so and all of the other browsers implement it in this way so it might be too late to change the spec On 2010/08/05 03:01:17, John Grabowski wrote: > If placed in this file I would expect SessionWithIDCommand to create a session. > But it does not. Can we move it into a different file? > > Given that it returns session capabilities, perhaps it should be called > SessionCapabilitiesCommand? > > Looks like this is the only way to destroy a session. > Not sure "delete session" is logically the same as "get capabilities" unless > it's called something like GetOrSetSessionStateCommand. > http://codereview.chromium.org/3064012/diff/41003/14028 File chrome/test/webdriver/commands/webdriver_command.cc (right): http://codereview.chromium.org/3064012/diff/41003/14028#newcode67 chrome/test/webdriver/commands/webdriver_command.cc:67: tab_ = session_->ActivateTab(); TabProxy is used in most of the future webdriver comands On 2010/08/05 03:01:17, John Grabowski wrote: > It isn't clear why you save a copy of tab_; you don't appear to use it anywhere > else. > http://codereview.chromium.org/3064012/diff/41003/14029 File chrome/test/webdriver/commands/webdriver_command.h (right): http://codereview.chromium.org/3064012/diff/41003/14029#newcode18 chrome/test/webdriver/commands/webdriver_command.h:18: // and are to be supporte for all browsers and platforms should inhert On 2010/08/05 03:01:17, John Grabowski wrote: > supporte --> supported > Done. http://codereview.chromium.org/3064012/diff/41003/14029#newcode19 chrome/test/webdriver/commands/webdriver_command.h:19: // this class. For all other cases the Command class should be used. On 2010/08/05 03:01:17, John Grabowski wrote: > Your comment just isn't clear enough. > Can you give examples of commands which should be WebDriverCommands, and > commands which should be just "Commands", and why? > Done. http://codereview.chromium.org/3064012/diff/41003/14029#newcode24 chrome/test/webdriver/commands/webdriver_command.h:24: : Command(path_segments, parameters) {} On 2010/08/05 03:01:17, John Grabowski wrote: > You should init/zero member pointer session_ in your constructor. > Done. http://codereview.chromium.org/3064012/diff/41003/14030 File chrome/test/webdriver/dispatch.cc (right): http://codereview.chromium.org/3064012/diff/41003/14030#newcode133 chrome/test/webdriver/dispatch.cc:133: void DispatchCommand(Command* const command, const std::string& method, On 2010/08/05 03:01:17, John Grabowski wrote: > This looks like the central loop/funnel for chromedriver. > A little doc on how we get here might be nice. > Done. http://codereview.chromium.org/3064012/diff/41003/14031 File chrome/test/webdriver/dispatch.h (right): http://codereview.chromium.org/3064012/diff/41003/14031#newcode31 chrome/test/webdriver/dispatch.h:31: void DispatchCommand(Command* const command, const std::string& method, On 2010/08/05 03:01:17, John Grabowski wrote: > comment on each function > Done. http://codereview.chromium.org/3064012/diff/41003/14034 File chrome/test/webdriver/keymap.h (right): http://codereview.chromium.org/3064012/diff/41003/14034#newcode26 chrome/test/webdriver/keymap.h:26: void Clear(); On 2010/08/05 03:01:17, John Grabowski wrote: > Perhaps rename ClearModifiers() > Done. http://codereview.chromium.org/3064012/diff/41003/14038 File chrome/test/webdriver/session_manager.cc (right): http://codereview.chromium.org/3064012/diff/41003/14038#newcode121 chrome/test/webdriver/session_manager.cc:121: id += text[random() % (sizeof text - 1)]; I added a counter and appended that to the session ID On 2010/08/05 03:01:17, John Grabowski wrote: > It's unlikely but there is no way to guarantee 2 session IDs in a row are > unique. (Like the lottery you may just get lucky.) Perhaps you can simply have > an incrementing counter? > http://codereview.chromium.org/3064012/diff/41003/14039 File chrome/test/webdriver/session_manager.h (right): http://codereview.chromium.org/3064012/diff/41003/14039#newcode18 chrome/test/webdriver/session_manager.h:18: class SessionManager { With the webdriver the user is allowed multiple instances of the browser on the same machine. So 2 sessions open would mean you would have 2 instances of chrome running under their own profiles. On 2010/08/05 03:01:17, John Grabowski wrote: > What does it mean to have 2 sessions? Does that mean we're running 2 chromes? > http://codereview.chromium.org/3064012/diff/41003/14039#newcode36 chrome/test/webdriver/session_manager.h:36: Lock session_generation; Session manager keeps track of all of the session that are currently running on the machine under test. We record the address and port for the HTTP 303 See other redirect. We save the IP and Port of the machine chromedriver is running on since we send a HTTP 303, see other, after a successful creation of a session, ie: http://172.22.41.105:8080/session/DFSSE453CV588 On 2010/08/05 03:01:17, John Grabowski wrote: > Trailing _ for these 3 > > What do these mean? Why does a session manager have an addr and port? I > understand if a session does, but not the session manager. > http://codereview.chromium.org/3064012/diff/41003/14039#newcode36 chrome/test/webdriver/session_manager.h:36: Lock session_generation; fixed to use _ On 2010/08/05 03:01:17, John Grabowski wrote: > Trailing _ for these 3 > > What do these mean? Why does a session manager have an addr and port? I > understand if a session does, but not the session manager. > Done. http://codereview.chromium.org/3064012/diff/41003/14042 File chrome/test/webdriver/webdriver_tests.py (right): http://codereview.chromium.org/3064012/diff/41003/14042#newcode111 chrome/test/webdriver/webdriver_tests.py:111: class WebDriverTest(unittest.TestCase): On 2010/08/05 03:01:17, John Grabowski wrote: > class WebDriverSessionlessTest Done.
Following up on the discussion we had offline about reusing as much code as possible from the ui-test infrastructure. I'm marking some of the methods that could potentially be re-used. This list is not exhaustive. In chrome/test/ui/ui_test.h: - UITestBase::SetUp - UITestBase::TearDown - UITestBase::InitializeTimeouts - UITestBase::LaunchBrowserAndServer - UITestBase::CloseBrowserAndServer - UITestBase::QuitBrowser - UITestBase::CleanupAppProcesses - UITestBase::TerminateBrowser - UITestBase::LaunchBrowser Ideally it'd make sense to derive from UITestBase. This is precisely what PyAuto does. See chrome/test/pyautolib/pyautolib.h In chrome/test/ui/ui_test_suite.h: - UITestSuite::Initialize - UITestSuite::Shutdown Like we talked, these can go in a followup CL.
LGTM. Land it! Really great to see this develop. Please remember to look at the routines Nirnimesh pointed you at (e.g. UITestBase::SetUp) as follow-up to see if we can reuse a bunch of code make PyAuto/ChromeDriver integration easier. http://codereview.chromium.org/3064012/diff/41003/14039 File chrome/test/webdriver/session_manager.h (right): http://codereview.chromium.org/3064012/diff/41003/14039#newcode18 chrome/test/webdriver/session_manager.h:18: class SessionManager { On 2010/08/10 01:05:07, Joe wrote: > With the webdriver the user is allowed multiple instances of the browser on the > same machine. So 2 sessions open would mean you would have 2 instances of > chrome running under their own profiles. > > On 2010/08/05 03:01:17, John Grabowski wrote: > > What does it mean to have 2 sessions? Does that mean we're running 2 chromes? > > > > Plz add comment to that effect in code http://codereview.chromium.org/3064012/diff/41003/14039#newcode36 chrome/test/webdriver/session_manager.h:36: Lock session_generation; On 2010/08/10 01:05:07, Joe wrote: > Session manager keeps track of all of the session that are currently running on > the machine under test. We record the address and port for the HTTP 303 See > other redirect. > > We save the IP and Port of the machine chromedriver is running on since we send > a HTTP 303, see other, after a successful creation of a session, ie: > http://172.22.41.105:8080/session/DFSSE453CV588 > > On 2010/08/05 03:01:17, John Grabowski wrote: > > Trailing _ for these 3 > > > > What do these mean? Why does a session manager have an addr and port? I > > understand if a session does, but not the session manager. > > > > Very helpful. Please add this comment in the code. http://codereview.chromium.org/3064012/diff/76001/77001 File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/3064012/diff/76001/77001#newcode442 chrome/chrome_tests.gypi:442: # wire protcol. A description of the WebDriver and examples can "Chromedriver is a stand-alone process that implements the WebDriver wire protocol. It talks to Chrome over the automation proxy". Be clear this isn't compiled into Chrome. http://codereview.chromium.org/3064012/diff/76001/77005 File chrome/test/webdriver/commands/create_session.h (right): http://codereview.chromium.org/3064012/diff/76001/77005#newcode16 chrome/test/webdriver/commands/create_session.h:16: // Create a new session which is a new instance of the chrome browser with no Excellent; great clarity. http://codereview.chromium.org/3064012/diff/76001/77008 File chrome/test/webdriver/commands/session_with_id.h (right): http://codereview.chromium.org/3064012/diff/76001/77008#newcode23 chrome/test/webdriver/commands/session_with_id.h:23: class SessionWithID : public WebDriverCommand { To be honest, I still dislike the name of this class. It is a command for querying or closing the session. It isn't a "session object" per se. This I'd expect it to be called ProbeOrCloseSession. But we've talked about this so if you still like it I'll let it go. http://codereview.chromium.org/3064012/diff/76001/77010 File chrome/test/webdriver/commands/webdriver_command.h (right): http://codereview.chromium.org/3064012/diff/76001/77010#newcode20 chrome/test/webdriver/commands/webdriver_command.h:20: // browser, such a transfering a file, inhert from the Command class excellent; makes things much more clear. It is unclear to me why you'd want to xfer a file to the webdriver process, but that isn't relevant here. |
