Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
613163004:
[chromedriver] setting browser default download directory (Closed)
|
Created:
6 years, 2 months ago by andrewcheng Modified:
6 years, 2 months ago Reviewers:
samuong Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[chromedriver] setting browser default download directory
BUG=chromedriver:783
Committed: https://crrev.com/e361733704a9d522ea6a49f64e380ef7bc7794ad
Cr-Commit-Position: refs/heads/master@{#300531}
Patch Set 1 #
Total comments: 16
Patch Set 2 : codes review fixed #Patch Set 3 : minor change - word spelling #Patch Set 4 : Modified the codes based on the code review #Patch Set 5 : in progress #Patch Set 6 : add test testDownloadDirectoryOverridesExistingPreferences #
Total comments: 22
Patch Set 7 : Modified codes based on reviewer's comments - please ignore this #Patch Set 8 : Modified codes based on reviewer's comments #
Total comments: 36
Patch Set 9 : Modified codes based on reviewer's comments #
Total comments: 18
Patch Set 10 : Modified codes based on reviewer's comments #
Total comments: 21
Patch Set 11 : Modified codes based on reviewer's comments - please ignore this #Patch Set 12 : Modified codes based on reviewer's comments - please ignore this #Patch Set 13 : Modified codes based on reviewer's comments - please ignore this #Patch Set 14 : Modified codes based on reviewer's comments #Patch Set 15 : Modified codes based on reviewer's comments #Patch Set 16 : for window compiler - (std::string) #Patch Set 17 : for window compiler - base::FilePath::StringType #Patch Set 18 : static_cast<base::FilePath::StringType> #Patch Set 19 : use GetSwitchValueNative #
Total comments: 1
Patch Set 20 : minor change for code standard #
Messages
Total messages: 30 (8 generated)
samuong@chromium.org changed reviewers: + samuong@chromium.org
Thanks for looking into this issue. I've got a few comments below. Also please delete any of the trailing whitespace that Eclipse adds, as the presubmit checks will test for this and block the commit if it finds any. https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cap... File chrome/test/chromedriver/capabilities.cc (right): https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cap... chrome/test/chromedriver/capabilities.cc:566: // Could you move this out to a separate function and put it in the parser_map? See lines 568-570 below for examples on how to do this. https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cli... File chrome/test/chromedriver/client/chromedriver.py (right): https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cli... chrome/test/chromedriver/client/chromedriver.py:128: # 'prefs': {'download': {'default_directory': '/usr/local/google/home/andrewcheng/xx', "directory_upgrade": 'true'}}, delete this line? https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cli... File chrome/test/chromedriver/client/run_py_tests.py (right): https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cli... chrome/test/chromedriver/client/run_py_tests.py:144: download_dir = tempfile.mkdtemp(dir='/tmp') This is going to be used by all tests, which is not necessary. Could you create a subclass class of ChromeDriverBaseTest (maybe call it ChromeDriverDownloadTest?) which contains just the testFileDownload() test, and only set the download_dir for that class? https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cli... chrome/test/chromedriver/client/run_py_tests.py:214: self._driver.Load(self.GetHttpUrlForFile('/chromedriver/download.html')) Before clicking, please also add a check that ensures that the file is *not* in the download directory. https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cli... chrome/test/chromedriver/client/run_py_tests.py:216: time.sleep(0.5) I'm a bit worried that this is going to be flaky, and also this test will always take >0.5 seconds. Could you change this to a loop (with a timeout) that waits until the file exists? https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cli... chrome/test/chromedriver/client/run_py_tests.py:217: download_name = ChromeDriverBaseTest.download_dir+"/a_red_dot.png"; minor style nit: please put spaces around the + We try to follow the Google Python Style Guide (http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Whitespace) with a few exceptions (http://www.chromium.org/chromium-os/python-style-guidelines). https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cli... chrome/test/chromedriver/client/run_py_tests.py:218: self.assertTrue(os.path.isfile(download_name), "Test user prefer download directory failed!") Another style nit: please keep lines within 80 characters. The style guide sets a maximum width of 80 characters and there are presubmits which will block the commit if code exceeds this. https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cli... chrome/test/chromedriver/client/run_py_tests.py:220: os.removedirs(ChromeDriverBaseTest.download_dir) If we add another test that uses the download directory, and it runs after this test, it will fail because the directory does not exist. Please move this into the tearDown() function (and call tempfile.mkdtemp() in the setUp() function).
On 2014/10/01 20:22:13, samuong wrote: > Thanks for looking into this issue. I've got a few comments below. > > Also please delete any of the trailing whitespace that Eclipse adds, as the > presubmit checks will test for this and block the commit if it finds any. > > https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cap... > File chrome/test/chromedriver/capabilities.cc (right): > > https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cap... > chrome/test/chromedriver/capabilities.cc:566: // > Could you move this out to a separate function and put it in the parser_map? See > lines 568-570 below for examples on how to do this. > > https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cli... > File chrome/test/chromedriver/client/chromedriver.py (right): > > https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cli... > chrome/test/chromedriver/client/chromedriver.py:128: # 'prefs': {'download': > {'default_directory': '/usr/local/google/home/andrewcheng/xx', > "directory_upgrade": 'true'}}, > delete this line? > > https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cli... > File chrome/test/chromedriver/client/run_py_tests.py (right): > > https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cli... > chrome/test/chromedriver/client/run_py_tests.py:144: download_dir = > tempfile.mkdtemp(dir='/tmp') > This is going to be used by all tests, which is not necessary. > > Could you create a subclass class of ChromeDriverBaseTest (maybe call it > ChromeDriverDownloadTest?) which contains just the testFileDownload() test, and > only set the download_dir for that class? > > https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cli... > chrome/test/chromedriver/client/run_py_tests.py:214: > self._driver.Load(self.GetHttpUrlForFile('/chromedriver/download.html')) > Before clicking, please also add a check that ensures that the file is *not* in > the download directory. > > https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cli... > chrome/test/chromedriver/client/run_py_tests.py:216: time.sleep(0.5) > I'm a bit worried that this is going to be flaky, and also this test will always > take >0.5 seconds. Could you change this to a loop (with a timeout) that waits > until the file exists? > > https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cli... > chrome/test/chromedriver/client/run_py_tests.py:217: download_name = > ChromeDriverBaseTest.download_dir+"/a_red_dot.png"; > minor style nit: please put spaces around the + > > We try to follow the Google Python Style Guide > (http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Whitespace) with > a few exceptions (http://www.chromium.org/chromium-os/python-style-guidelines). > > https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cli... > chrome/test/chromedriver/client/run_py_tests.py:218: > self.assertTrue(os.path.isfile(download_name), "Test user prefer download > directory failed!") > Another style nit: please keep lines within 80 characters. The style guide sets > a maximum width of 80 characters and there are presubmits which will block the > commit if code exceeds this. > > https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cli... > chrome/test/chromedriver/client/run_py_tests.py:220: > os.removedirs(ChromeDriverBaseTest.download_dir) > If we add another test that uses the download directory, and it runs after this > test, it will fail because the directory does not exist. Please move this into > the tearDown() function (and call tempfile.mkdtemp() in the setUp() function). As discussed in person, let's make the following changes: - revert changes to base/files/scoped_temp_dir.* - if the user provides a custom user data dir, don't assign it to the ScopedTempDir - remove superfluous comments and fix coding style issues - move download_dir from ChromeDriverBaseTest to your subclass Thanks!
Modified the codes based on code review on Oct. 8 https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cap... File chrome/test/chromedriver/capabilities.cc (right): https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cap... chrome/test/chromedriver/capabilities.cc:566: // On 2014/10/01 20:22:12, samuong wrote: > Could you move this out to a separate function and put it in the parser_map? See > lines 568-570 below for examples on how to do this. Done. https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cli... File chrome/test/chromedriver/client/run_py_tests.py (right): https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cli... chrome/test/chromedriver/client/run_py_tests.py:144: download_dir = tempfile.mkdtemp(dir='/tmp') On 2014/10/01 20:22:12, samuong wrote: > This is going to be used by all tests, which is not necessary. > > Could you create a subclass class of ChromeDriverBaseTest (maybe call it > ChromeDriverDownloadTest?) which contains just the testFileDownload() test, and > only set the download_dir for that class? def setUp(self): if self._testMethodName == 'testFileDownLoad': self.download_dir = tempfile.mkdtemp(dir='/tmp') else: self.download_dir = None Done. https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cli... chrome/test/chromedriver/client/run_py_tests.py:214: self._driver.Load(self.GetHttpUrlForFile('/chromedriver/download.html')) On 2014/10/01 20:22:12, samuong wrote: > Before clicking, please also add a check that ensures that the file is *not* in > the download directory. Done. https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cli... chrome/test/chromedriver/client/run_py_tests.py:214: self._driver.Load(self.GetHttpUrlForFile('/chromedriver/download.html')) On 2014/10/01 20:22:12, samuong wrote: > Before clicking, please also add a check that ensures that the file is *not* in > the download directory. This is a random generated dir - very unlikely, a download dir will exist. However, we can put a check as following, in case it does exist self.assertFalse(os.path.isfile(download_name), "Test user prefer download directory failed! - download file already exist") https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cli... chrome/test/chromedriver/client/run_py_tests.py:216: time.sleep(0.5) On 2014/10/01 20:22:13, samuong wrote: > I'm a bit worried that this is going to be flaky, and also this test will always > take >0.5 seconds. Could you change this to a loop (with a timeout) that waits > until the file exists? download_name = ChromeDriverBaseTest.download_dir + "/a_red_dot.png"; timeout = time.time() + 60*1 # 1 minutes from now while True: time.sleep(0.1) if os.path.isfile(download_name) or time.time() > timeout: break # self.assertTrue(os.path.isfile(download_name), "Test user prefer download di rectory failed!") Done. https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cli... chrome/test/chromedriver/client/run_py_tests.py:217: download_name = ChromeDriverBaseTest.download_dir+"/a_red_dot.png"; On 2014/10/01 20:22:12, samuong wrote: > minor style nit: please put spaces around the + > > We try to follow the Google Python Style Guide > (http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Whitespace) with > a few exceptions (http://www.chromium.org/chromium-os/python-style-guidelines). Done. https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cli... chrome/test/chromedriver/client/run_py_tests.py:218: self.assertTrue(os.path.isfile(download_name), "Test user prefer download directory failed!") On 2014/10/01 20:22:12, samuong wrote: > Another style nit: please keep lines within 80 characters. The style guide sets > a maximum width of 80 characters and there are presubmits which will block the > commit if code exceeds this. Done. https://codereview.chromium.org/613163004/diff/1/chrome/test/chromedriver/cli... chrome/test/chromedriver/client/run_py_tests.py:220: os.removedirs(ChromeDriverBaseTest.download_dir) On 2014/10/01 20:22:12, samuong wrote: > If we add another test that uses the download directory, and it runs after this > test, it will fail because the directory does not exist. Please move this into > the tearDown() function (and call tempfile.mkdtemp() in the setUp() function). Done. def tearDown(self): try: if self.download_dir: os.removedirs(self.download_dir) self.download_dir = None
For some reason run_py_tests.py has been moved into the
chrome/test/chromedriver/client directory, but it should be in
chrome/test/chromedriver/test, otherwise the tester buildbots won't be able to
find it. Please move this file into the correct location and reupload.
Please set
.desiredCapabilities.chromeOptions.prefs.download.default_directory=XXX rather
than .desiredCapabilities.prefs.download.default_directory (i.e. put it under
chromeOptions). This means that the changes to capabilities.cc are no longer
necessary.
We've decided that issue 783 is not a bug, but we still want to add a test
(which is already here - good). For 611, we need both the bugfix (which is here,
also good) but please also add some tests for 611:
def testDownloadDirectoryCanBeSet(self):
(same as the test you've already added)
def testDownloadDirectoryOverridesExistingPreferences(self):
1. create a temp user data directory
2. create a file inside the temp directory ($tmpdir/Default/Preferences)
3. write a json object to it (e.g. '{"test": "this shouldn't be changed",
"download": {"default_directory": "/old/download/directory"}}')
4. launch chrome with --user-data-dir=$tmpdir, and set download directory to
"/new/download/directory"
5. re-read the $tmpdir/Default/Preferences file
6. check that download directory is "/new/download/directory" and "test" is
unchanged
Please remove any extraneous comments and whitespace (type "git diff
origin/master" to get the full diff against the original code).
https://codereview.chromium.org/613163004/diff/110001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome/chrome_desktop_impl.cc (right): https://codereview.chromium.org/613163004/diff/110001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/chrome_desktop_impl.cc:81: } Since you're not changing anything here, please delete the braces and restore the indenting. That way, the version history will stay clean and tools like git-blame will be more useful. https://codereview.chromium.org/613163004/diff/110001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome_launcher.cc (right): https://codereview.chromium.org/613163004/diff/110001/chrome/test/chromedrive... chrome/test/chromedriver/chrome_launcher.cc:131: base::FilePath userDataDirPath; For local variable names in C++, we use underscores rather than camelCase (see http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Variable_Names). Please try to stay as consistent as you can with existing code. https://codereview.chromium.org/613163004/diff/110001/chrome/test/chromedrive... chrome/test/chromedriver/chrome_launcher.cc:140: // More style nitpicking: don't use empty comments like this, just use blank lines, and stay consistent with existing code. https://codereview.chromium.org/613163004/diff/110001/chrome/test/chromedrive... chrome/test/chromedriver/chrome_launcher.cc:141: command.AppendArg("data:,"); Existing user data directories might have a home page set, and the test could expect chrome to open this home page when it launches. So we should only add "data:," to the command line if there is no user-data-dir specified. https://codereview.chromium.org/613163004/diff/110001/chrome/test/chromedrive... chrome/test/chromedriver/chrome_launcher.cc:770: Status mergeExistPrefFile(const base::DictionaryValue* custom_prefs, For function names in C++ like this, we use CamelCase and start the function name with a capital letter (see http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names). https://codereview.chromium.org/613163004/diff/110001/chrome/test/chromedrive... chrome/test/chromedriver/chrome_launcher.cc:828: return status; C++ code should be indented with two spaces, not four (see http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Spaces_vs._Tabs). Please make sure you stay consistent with the existing coding style. https://codereview.chromium.org/613163004/diff/110001/chrome/test/chromedrive... File chrome/test/chromedriver/client/chromedriver.py (right): https://codereview.chromium.org/613163004/diff/110001/chrome/test/chromedrive... chrome/test/chromedriver/client/chromedriver.py:123: # Can we get rid of these kinds of comments? As I mentioned earlier, please type "git diff origin/master" and review your changes before uploading, and delete any temporary comments like this. https://codereview.chromium.org/613163004/diff/110001/chrome/test/chromedrive... File chrome/test/chromedriver/client/run_py_tests.py (right): https://codereview.chromium.org/613163004/diff/110001/chrome/test/chromedrive... chrome/test/chromedriver/client/run_py_tests.py:1: #!/usr/bin/env python This file is still in the wrong location. Could you please move it back to chrome/test/chromedriver/test? The command to do this is: git mv -f chrome/test/chromedriver/client/run_py_tests.py chrome/test/chromedriver/test Next time you do a "git cl upload", please check under "Patch Set 7" that this file is under chrome/test/chromedriver/test. https://codereview.chromium.org/613163004/diff/110001/chrome/test/chromedrive... chrome/test/chromedriver/client/run_py_tests.py:207: self.download_dir = tempfile.mkdtemp(dir='/tmp') By default, tempfile.mkdtemp() will use dir='/tmp', so you don't need to specify this explicitly. Just call "tempfile.mkdtemp()". https://codereview.chromium.org/613163004/diff/110001/chrome/test/chromedrive... chrome/test/chromedriver/client/run_py_tests.py:218: pass If and when this fails, let's print out a proper error message rather than just silently swallowing the exception. https://codereview.chromium.org/613163004/diff/110001/chrome/test/chromedrive... chrome/test/chromedriver/client/run_py_tests.py:226: download_name = self.download_dir + "/a_red_dot.png"; Windows uses "\" as the directory separator, so you can use os.path.join instead of doing this manually: download_name = os.path.join(self.download_dir, 'a_red_dot.png') https://codereview.chromium.org/613163004/diff/110001/chrome/test/chromedrive... chrome/test/chromedriver/client/run_py_tests.py:227: self.assertFalse(os.path.isfile(download_name), If there's a directory called "a_red_dot.png" then this will cause the download to fail. So let's use "os.path.exists" instead of "os.path.isfile". https://codereview.chromium.org/613163004/diff/110001/chrome/test/chromedrive... chrome/test/chromedriver/client/run_py_tests.py:233: timeout = time.time() + 60*1 # 1 minutes from now How about we call this variable "deadline" instead of "timeout"? A timeout is a relative time (e.g. 60 seconds) whereas a deadline is an absolute time (e.g. now + 60 seconds). This looks like a deadline not a timeout. https://codereview.chromium.org/613163004/diff/110001/chrome/test/chromedrive... chrome/test/chromedriver/client/run_py_tests.py:789: userDataDir='/usr/local/google/home/andrewcheng/zz' Please don't hardcode your home directory into test cases like this. This is going to break when running on everyone else's computers, as well as on the tester bots. Use tempfile.mkdtemp() to create a temporary directory to store this in. You'll also need to add the following line so that the subdirectory that contains the Preferences file exists before the test tries to write to it: os.mkdir(os.path.join(userDataDir, 'Default')) https://codereview.chromium.org/613163004/diff/110001/chrome/test/chromedrive... chrome/test/chromedriver/client/run_py_tests.py:790: defPrefFile = userDataDir+'/Default/Preferences' For both defPrefFile and userDataDir, use names with underscores rather than camelCase (see http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Naming). Also please put spaces around binary operators such as '=' and '+' (see http://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Whitespace). You don't need to read and memorize the style guide from top to bottom, but please at least look in the file that you're editing and stay as consistent with the existing style as you can. https://codereview.chromium.org/613163004/diff/110001/chrome/test/chromedrive... chrome/test/chromedriver/client/run_py_tests.py:791: # Also for consistency with the existing style in this file, use a blank line instead of an empty comment. https://codereview.chromium.org/613163004/diff/110001/chrome/test/chromedrive... chrome/test/chromedriver/client/run_py_tests.py:797: data['download'] = test_dir How about we change these lines above so that data is declared as below? data = { 'test': 'this should not be changed', 'download': { 'directory_upgrade': 'true', 'default_directory': '/old/download/directory' } } I think this will be easier to read since the indenting makes the nesting clearer. https://codereview.chromium.org/613163004/diff/110001/chrome/test/chromedrive... chrome/test/chromedriver/client/run_py_tests.py:804: download_dir="/usr/local/google/home/andrewcheng/zz") You're setting the user data directory to the same directory as the download directory. Please create two separate temporary directories at the start of the test and use different directories for these two things. https://codereview.chromium.org/613163004/diff/110001/chrome/test/chromedrive... chrome/test/chromedriver/client/run_py_tests.py:806: with open(defPrefFile) as inFile: data = json.load(inFile) Please break the above line up into two lines: with open(defPrefFile) as inFile: data = json.load(inFile) https://codereview.chromium.org/613163004/diff/110001/chrome/test/chromedrive... chrome/test/chromedriver/client/run_py_tests.py:807: self.assertEqual(data['test'], 'this should not be changed', Before doing this assert, please also check that the preferences file contains a 'test' key: self.assertTrue('test' in data, 'existing preference has been deleted') https://codereview.chromium.org/613163004/diff/110001/chrome/test/chromedrive... chrome/test/chromedriver/client/run_py_tests.py:808: "test data has changed") "test data has changed" doesn't really tell anyone what has gone wrong here. How about we change this to the following? "existing preference has been changed to %s" % data['test'] https://codereview.chromium.org/613163004/diff/110001/chrome/test/data/chrome... File chrome/test/data/chromedriver/download.html (right): https://codereview.chromium.org/613163004/diff/110001/chrome/test/data/chrome... chrome/test/data/chromedriver/download.html:4: <a id='red-dot' href="anchor_download_test.png" download='a_red_dot.png'>Download Red Dot!</a> This file is in "chrome/test/data/chromedriver", and contains a relative link to "anchor_download_test.png", so there should also be a file called "chrome/test/data/chromedriver/anchor_download_test.png". You might just need to do a "git add" before resubmitting.
Hi Sam, Please review it. Thanks, Andrew
On 2014/10/16 18:08:41, andrewcheng wrote: > Hi Sam, > > Please review it. > > Thanks, > Andrew Thanks Andrew, this is looking a lot better. Most of this is just style nitpicking, but please also refactor the end-to-end tests so that your download-related tests are in a single class, and it also seems wrong to call PrepareUserDataDir() unconditionally. See the comments for more details.
https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome/chrome_desktop_impl.cc (right): https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/chrome_desktop_impl.cc:93: LOG(WARNING) << " and " << extension_dir.value(); This error message is awkwardly worded. If the user data dir is not valid, it will say "and" even though it only prints out one directory. How about we change it to: LOG(WARNING) << "chrome quit unexpectedly, leaving behind temporary directories for debugging:"; if (user_data_dir_.IsValid()) LOG(WARNING) << "chrome user data directory: " << user_data_dir.value(); if (extension_dir.IsValid()) LOG(WARNING) << "chromedriver automation extension directory: " << extension_dir.value(); https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome_launcher.cc (right): https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/chrome_launcher.cc:144: user_data_dir_path , capabilities.prefs.get(), I've just noticed that if you unconditionally call PrepareUserDataDir, it will overwrite files such as "Local State" (see line 827). We don't want to do this, and I think the best way to handle this is to move this back to where it was, and do the merge only if there is a prefs object in the capabilities dictionary. https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/chrome_launcher.cc:147: return status; This is indented too far in. https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/chrome_launcher.cc:157: extension_bg_pages); Reindent lines 154-157 so that they line up with the opening parenthesis on line 153. In general, when breaking a function call over multiple lines, you can either indent four spaces, or indent up to the opening parenthesis: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Calls This applies to both C++ as well as Python, and please fix this in other occurrences in this file as well as the other files in your CL. https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... File chrome/test/chromedriver/client/chromedriver.py (right): https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/client/chromedriver.py:64: android_package=None, Reflow lines 63-70, or move download_dir to the end of the argument list. https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/client/chromedriver.py:122: default_dir['directory_upgrade'] = 'true' Does it work if you don't set directory_upgrade to true? https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... File chrome/test/chromedriver/test/run_py_tests.py (right): https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:168: driver = chromedriver.ChromeDriver(server_url, download_dir, Put each parameter on a separate line. https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:206: if self._testMethodName == 'testFileDownLoad': As we discussed earlier, I don't think this is a very good way to set up the download directory. Let's move testFileDownLoad into the ChromeDownloadDirTest class so that all the download-related tests are together. Please revert ChromeDriverTest.setUp() and ChromeDriverTest.tearDown() and see my comment under ChromeDownloadDirTest for some pointers on how to do this. https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:228: "Test prefer download directory failed! - download file already exist") I think you were right in your previous comment that this assertion is unnecessary, since it comes right after the temporary directory is created. Let's just remove it. https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:233: timeout = time.time() + 60*1 # 1 minutes from now I think these two comments state the obvious, and are not really necessary. Also, don't forget to put spaces before and after the '*'. Also, weren't we going to rename 'timeout' to 'deadline'? https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:240: "Test user prefer download directory failed!") I don't think this sentence is grammatically correct. How about we change it to 'Failed to download file'? https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:785: class ChromeDownloadDirTest(ChromeDriverBaseTest): Add the following functions here to set up the download directory: def setUp(self): self._http_server = webserver.WebServer(chrome_paths.GetTestData()) self._download_dir = tempfile.mkdtemp() self._driver = self.CreateDriver(download_dir=self._download_dir) def tearDown(self): os.rmdir(self._download_dir) self._http_server.Shutdown() ChromeDriverBaseTest.tearDown(self) https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:787: # test existing prefence profile - check setting if it is correct Use a docstring here rather than a regular comment (see the other lines that start and end with three double-quotes """ for examples). https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:790: tmp_download_dir = tempfile.mkdtemp() If you use the setUp method I mentioned above, you won't need to do this. https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:793: def_pref_file = os.path.join(sub_dir, 'Preferences') How about we call this "prefs_file_path"? https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:795: data = { "data" is not a very descriptive name, how about we call it "prefs"? https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:798: 'directory_upgrade': 'true', As I mentioned somewhere else in this review, I think 'directory_upgrade' might not need to be specified. If it isn't needed, please remove it. https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:803: with open(def_pref_file, 'w') as outfile: "outfile" is not very descriptive, how about we call this "prefs_file"? https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:810: with open(def_pref_file) as infile: Same as "outfile" - how about we call this "prefs_file"? https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:813: "The test data is drop - should stay without change") I don't think this error message is grammatically correct. How about we change it to something like: 'Existing preference was unexpectedly overridden'? https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:817: 'default_directory is not set') This is a bit misleading. The default_directory value will be set, but might not be set to the correct value. How about we change it to the following? 'Download directory preference was not updated to match capabilities' https://codereview.chromium.org/613163004/diff/150001/chrome/test/data/chrome... File chrome/test/data/chromedriver/download.html (right): https://codereview.chromium.org/613163004/diff/150001/chrome/test/data/chrome... chrome/test/data/chromedriver/download.html:2: <head><title>Download Test for <a download></title></head> Since this is no longer a test for <a download>, let's change the title to: <title>Test Page for ChromeDownloadDirTest</title>
Thanks Andrew, this looks good. There's just one cleanup issue that we need to resolve in run_py_tests.py, and otherwise I think it's just minor stylistic nitpicking. Also, could you go through and either mark all the previous comments (in the previous patchsets) as "Done", or mention if you're not doing them (and why)? Once that's done I think this would be ready to commit. https://codereview.chromium.org/613163004/diff/170001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome/chrome_desktop_impl.cc (right): https://codereview.chromium.org/613163004/diff/170001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/chrome_desktop_impl.cc:89: // can you please make sure to delete these comments? https://codereview.chromium.org/613163004/diff/170001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome_launcher.cc (right): https://codereview.chromium.org/613163004/diff/170001/chrome/test/chromedrive... chrome/test/chromedriver/chrome_launcher.cc:145: capabilities.local_state.get()); fix indent and whitespace https://codereview.chromium.org/613163004/diff/170001/chrome/test/chromedrive... chrome/test/chromedriver/chrome_launcher.cc:157: extension_bg_pages); fix indent https://codereview.chromium.org/613163004/diff/170001/chrome/test/chromedrive... File chrome/test/chromedriver/client/chromedriver.py (right): https://codereview.chromium.org/613163004/diff/170001/chrome/test/chromedrive... chrome/test/chromedriver/client/chromedriver.py:64: android_package=None, remove line break https://codereview.chromium.org/613163004/diff/170001/chrome/test/chromedrive... File chrome/test/chromedriver/test/run_py_tests.py (right): https://codereview.chromium.org/613163004/diff/170001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:757: self.download_dir = tempfile.mkdtemp() let's either put this in setUp() or a try-finally block, otherwise it might not get cleaned up if the test fails, e.g.: try: download_dir = tempfile.mkdtemp() #do the actual test... finally: shutil.rmtree(download_dir) you should do the same thing with user_data_dir and download_dir in your other test https://codereview.chromium.org/613163004/diff/170001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:771: "Failed to download file!") you could fit this whole assertion on a single line https://codereview.chromium.org/613163004/diff/170001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:778: self.assertRaises(chromedriver.UnknownError) i think we can delete this try-except block if you do the try-finally thing i mentioned above, let me know if i'm missing something https://codereview.chromium.org/613163004/diff/170001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:780: """ test existing prefence profile - check setting if it is correct """ unlike javadoc and doxygen, python docstrings go inside the function https://codereview.chromium.org/613163004/diff/170001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:786: prefs_file_path = os.path.join(sub_dir, 'Preferences') don't forget to clean all this up after the test finishes (see my previous comment for how to do this using a try-finally block)
Hi Sam, Please review it. Thanks, Andrew https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... File chrome/test/chromedriver/client/chromedriver.py (right): https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/client/chromedriver.py:122: default_dir['directory_upgrade'] = 'true' On 2014/10/16 20:23:40, samuong wrote: > Does it work if you don't set directory_upgrade to true? you have comment in other place - we may not need this. I removed it now. https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... File chrome/test/chromedriver/test/run_py_tests.py (right): https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:168: driver = chromedriver.ChromeDriver(server_url, download_dir, On 2014/10/16 20:23:41, samuong wrote: > Put each parameter on a separate line. Done. https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:228: "Test prefer download directory failed! - download file already exist") On 2014/10/16 20:23:40, samuong wrote: > I think you were right in your previous comment that this assertion is > unnecessary, since it comes right after the temporary directory is created. > Let's just remove it. Done. https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:233: timeout = time.time() + 60*1 # 1 minutes from now On 2014/10/16 20:23:40, samuong wrote: > I think these two comments state the obvious, and are not really necessary. > > Also, don't forget to put spaces before and after the '*'. > > Also, weren't we going to rename 'timeout' to 'deadline'? another place also has timeout def _WaitForNewWindow(self, old_handles): Line 254 timeout = time.time() + 20 while time.time() < timeout: should we change it as well - consist https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:240: "Test user prefer download directory failed!") On 2014/10/16 20:23:40, samuong wrote: > I don't think this sentence is grammatically correct. How about we change it to > 'Failed to download file'? good idea https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:787: # test existing prefence profile - check setting if it is correct On 2014/10/16 20:23:41, samuong wrote: > Use a docstring here rather than a regular comment (see the other lines that > start and end with three double-quotes """ for examples). Done. https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:790: tmp_download_dir = tempfile.mkdtemp() On 2014/10/16 20:23:40, samuong wrote: > If you use the setUp method I mentioned above, you won't need to do this. not sure how this work, i don't touch for now https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:793: def_pref_file = os.path.join(sub_dir, 'Preferences') On 2014/10/16 20:23:41, samuong wrote: > How about we call this "prefs_file_path"? Done. https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:795: data = { On 2014/10/16 20:23:41, samuong wrote: > "data" is not a very descriptive name, how about we call it "prefs"? Done. https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:798: 'directory_upgrade': 'true', On 2014/10/16 20:23:40, samuong wrote: > As I mentioned somewhere else in this review, I think 'directory_upgrade' might > not need to be specified. If it isn't needed, please remove it. Done. https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:803: with open(def_pref_file, 'w') as outfile: On 2014/10/16 20:23:41, samuong wrote: > "outfile" is not very descriptive, how about we call this "prefs_file"? this is generic use for short term - as temp, so that we know it is for writing. (as documentation) we have with open(prefs_file_path, 'w') as outfile: this is easier to read https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:810: with open(def_pref_file) as infile: On 2014/10/16 20:23:40, samuong wrote: > Same as "outfile" - how about we call this "prefs_file"? same comment provide in outfile https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:813: "The test data is drop - should stay without change") On 2014/10/16 20:23:41, samuong wrote: > I don't think this error message is grammatically correct. How about we change > it to something like: 'Existing preference was unexpectedly overridden'? it will not be set to other value - only disappear. may be we need better description than "The test data is drop - should stay without change" I changed to "The test data disappear - it should stay without change" for now https://codereview.chromium.org/613163004/diff/150001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:817: 'default_directory is not set') On 2014/10/16 20:23:40, samuong wrote: > This is a bit misleading. The default_directory value will be set, but might not > be set to the correct value. How about we change it to the following? > > 'Download directory preference was not updated to match capabilities' Done. https://codereview.chromium.org/613163004/diff/170001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome/chrome_desktop_impl.cc (right): https://codereview.chromium.org/613163004/diff/170001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/chrome_desktop_impl.cc:89: // On 2014/10/17 21:52:25, samuong wrote: > can you please make sure to delete these comments? Done. https://codereview.chromium.org/613163004/diff/170001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome_launcher.cc (right): https://codereview.chromium.org/613163004/diff/170001/chrome/test/chromedrive... chrome/test/chromedriver/chrome_launcher.cc:145: capabilities.local_state.get()); On 2014/10/17 21:52:25, samuong wrote: > fix indent and whitespace Done. https://codereview.chromium.org/613163004/diff/170001/chrome/test/chromedrive... chrome/test/chromedriver/chrome_launcher.cc:157: extension_bg_pages); On 2014/10/17 21:52:25, samuong wrote: > fix indent Done. https://codereview.chromium.org/613163004/diff/170001/chrome/test/chromedrive... File chrome/test/chromedriver/client/chromedriver.py (right): https://codereview.chromium.org/613163004/diff/170001/chrome/test/chromedrive... chrome/test/chromedriver/client/chromedriver.py:64: android_package=None, On 2014/10/17 21:52:25, samuong wrote: > remove line break Done. https://codereview.chromium.org/613163004/diff/170001/chrome/test/chromedrive... File chrome/test/chromedriver/test/run_py_tests.py (right): https://codereview.chromium.org/613163004/diff/170001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:757: self.download_dir = tempfile.mkdtemp() On 2014/10/17 21:52:25, samuong wrote: > let's either put this in setUp() or a try-finally block, otherwise it might not > get cleaned up if the test fails, e.g.: > > try: > download_dir = tempfile.mkdtemp() > #do the actual test... > finally: > shutil.rmtree(download_dir) > > you should do the same thing with user_data_dir and download_dir in your other > test Done. https://codereview.chromium.org/613163004/diff/170001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:771: "Failed to download file!") On 2014/10/17 21:52:25, samuong wrote: > you could fit this whole assertion on a single line Done. https://codereview.chromium.org/613163004/diff/170001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:778: self.assertRaises(chromedriver.UnknownError) On 2014/10/17 21:52:25, samuong wrote: > i think we can delete this try-except block if you do the try-finally thing i > mentioned above, let me know if i'm missing something Done. https://codereview.chromium.org/613163004/diff/170001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:780: """ test existing prefence profile - check setting if it is correct """ On 2014/10/17 21:52:25, samuong wrote: > unlike javadoc and doxygen, python docstrings go inside the function Done. https://codereview.chromium.org/613163004/diff/170001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:786: prefs_file_path = os.path.join(sub_dir, 'Preferences') On 2014/10/17 21:52:25, samuong wrote: > don't forget to clean all this up after the test finishes (see my previous > comment for how to do this using a try-finally block) Done.
Just a few more things. Please mark them as "done" or reply if you don't think think you should follow any of these comments. https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome_launcher.cc (right): https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... chrome/test/chromedriver/chrome_launcher.cc:146: capabilities.local_state.get()); please fix indenting for arguments - they should either be lined up to the opening parenthesis or indented by 4 spaces https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... chrome/test/chromedriver/chrome_launcher.cc:148: return status; please fix indenting for "return status;" (it should only be indented by 4 spaces, not 6) https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... chrome/test/chromedriver/chrome_launcher.cc:783: default_dir.Append(chrome::kPreferencesFilename); fix indenting (should be 6 spaces, not 9) https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... chrome/test/chromedriver/chrome_launcher.cc:788: preferences = std::string(kPreferences); why not just do "preferences = kPreferences;"? https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... chrome/test/chromedriver/chrome_launcher.cc:799: user_data_dir.Append(chrome::kLocalStateFilename); fix indenting (should be 6 spaces, not 8) https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... chrome/test/chromedriver/chrome_launcher.cc:804: local_state = std::string(kLocalState); similar to above, why not just do "local_state = kLocalState;"? https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... File chrome/test/chromedriver/client/chromedriver.py (right): https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... chrome/test/chromedriver/client/chromedriver.py:123: options['prefs'] = download_prefs Just in case prefs already exists in some form (e.g. via experimental_options) maybe we should be careful about not clobbering it. How about we change this to the following? if download_dir: if 'prefs' not in options: options['prefs'] = {} if 'download' not in options['prefs']: options['prefs']['download'] = {} options['prefs']['download']['default_directory'] = download_dir https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... File chrome/test/chromedriver/test/run_py_tests.py (right): https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:763: '/chromedriver/download.html') Ideally I'd prefer that this class doesn't use anything from ChromeDriverTest, but that can be addressed in a separate commit. But for this CL, if you're going to use ChromeDriverTest's _http_server, why not use ChromeDriverTest.GetHttpUrlForFile()? https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:766: deadline = time.time() + 60 * 1 # 1 minutes from now are you ok with removing these two comments? https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:797: download_dir = tmp_download_dir) Fix indenting. One way you could write this is like this: driver = self.CreateDriver( chrome_switches=['user-data-dir=' + user_data_dir], download_dir = tmp_download_dir) https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:802: self.assertEqual(prefs['test'], 'this should not be changed', for these assertEqual functions, the convention is to have it as "assertEqual(expected_value, actual_value, message)", so please swap the first two arguments around https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:803: "Existing preference was unexpectedly overridden") also please fix the indentation so that this lines up with the opening parenthesis
please review it https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome_launcher.cc (right): https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... chrome/test/chromedriver/chrome_launcher.cc:146: capabilities.local_state.get()); On 2014/10/17 23:30:34, samuong wrote: > please fix indenting for arguments - they should either be lined up to the > opening parenthesis or indented by 4 spaces Done. https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... chrome/test/chromedriver/chrome_launcher.cc:148: return status; On 2014/10/17 23:30:34, samuong wrote: > please fix indenting for "return status;" (it should only be indented by 4 > spaces, not 6) Done. https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... chrome/test/chromedriver/chrome_launcher.cc:783: default_dir.Append(chrome::kPreferencesFilename); On 2014/10/17 23:30:35, samuong wrote: > fix indenting (should be 6 spaces, not 9) Done. https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... chrome/test/chromedriver/chrome_launcher.cc:788: preferences = std::string(kPreferences); On 2014/10/17 23:30:34, samuong wrote: > why not just do "preferences = kPreferences;"? Done. https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... chrome/test/chromedriver/chrome_launcher.cc:799: user_data_dir.Append(chrome::kLocalStateFilename); On 2014/10/17 23:30:34, samuong wrote: > fix indenting (should be 6 spaces, not 8) Done. https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... chrome/test/chromedriver/chrome_launcher.cc:804: local_state = std::string(kLocalState); On 2014/10/17 23:30:35, samuong wrote: > similar to above, why not just do "local_state = kLocalState;"? Done. https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... File chrome/test/chromedriver/client/chromedriver.py (right): https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... chrome/test/chromedriver/client/chromedriver.py:123: options['prefs'] = download_prefs On 2014/10/17 23:30:35, samuong wrote: > Just in case prefs already exists in some form (e.g. via experimental_options) > maybe we should be careful about not clobbering it. How about we change this to > the following? > > if download_dir: > if 'prefs' not in options: > options['prefs'] = {} > if 'download' not in options['prefs']: > options['prefs']['download'] = {} > options['prefs']['download']['default_directory'] = download_dir Done. https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... File chrome/test/chromedriver/test/run_py_tests.py (right): https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:763: '/chromedriver/download.html') On 2014/10/17 23:30:35, samuong wrote: > Ideally I'd prefer that this class doesn't use anything from ChromeDriverTest, > but that can be addressed in a separate commit. But for this CL, if you're going > to use ChromeDriverTest's _http_server, why not use > ChromeDriverTest.GetHttpUrlForFile()? Done. https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:766: deadline = time.time() + 60 * 1 # 1 minutes from now On 2014/10/17 23:30:35, samuong wrote: > are you ok with removing these two comments? certainly no problem to me Done.
On 2014/10/18 00:45:46, andrewcheng wrote: > please review it > > https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... > File chrome/test/chromedriver/chrome_launcher.cc (right): > > https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... > chrome/test/chromedriver/chrome_launcher.cc:146: > capabilities.local_state.get()); > On 2014/10/17 23:30:34, samuong wrote: > > please fix indenting for arguments - they should either be lined up to the > > opening parenthesis or indented by 4 spaces > > Done. > > https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... > chrome/test/chromedriver/chrome_launcher.cc:148: return status; > On 2014/10/17 23:30:34, samuong wrote: > > please fix indenting for "return status;" (it should only be indented by 4 > > spaces, not 6) > > Done. > > https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... > chrome/test/chromedriver/chrome_launcher.cc:783: > default_dir.Append(chrome::kPreferencesFilename); > On 2014/10/17 23:30:35, samuong wrote: > > fix indenting (should be 6 spaces, not 9) > > Done. > > https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... > chrome/test/chromedriver/chrome_launcher.cc:788: preferences = > std::string(kPreferences); > On 2014/10/17 23:30:34, samuong wrote: > > why not just do "preferences = kPreferences;"? > > Done. > > https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... > chrome/test/chromedriver/chrome_launcher.cc:799: > user_data_dir.Append(chrome::kLocalStateFilename); > On 2014/10/17 23:30:34, samuong wrote: > > fix indenting (should be 6 spaces, not 8) > > Done. > > https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... > chrome/test/chromedriver/chrome_launcher.cc:804: local_state = > std::string(kLocalState); > On 2014/10/17 23:30:35, samuong wrote: > > similar to above, why not just do "local_state = kLocalState;"? > > Done. > > https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... > File chrome/test/chromedriver/client/chromedriver.py (right): > > https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... > chrome/test/chromedriver/client/chromedriver.py:123: options['prefs'] = > download_prefs > On 2014/10/17 23:30:35, samuong wrote: > > Just in case prefs already exists in some form (e.g. via experimental_options) > > maybe we should be careful about not clobbering it. How about we change this > to > > the following? > > > > if download_dir: > > if 'prefs' not in options: > > options['prefs'] = {} > > if 'download' not in options['prefs']: > > options['prefs']['download'] = {} > > options['prefs']['download']['default_directory'] = download_dir > > Done. > > https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... > File chrome/test/chromedriver/test/run_py_tests.py (right): > > https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... > chrome/test/chromedriver/test/run_py_tests.py:763: > '/chromedriver/download.html') > On 2014/10/17 23:30:35, samuong wrote: > > Ideally I'd prefer that this class doesn't use anything from ChromeDriverTest, > > but that can be addressed in a separate commit. But for this CL, if you're > going > > to use ChromeDriverTest's _http_server, why not use > > ChromeDriverTest.GetHttpUrlForFile()? > > Done. > > https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... > chrome/test/chromedriver/test/run_py_tests.py:766: deadline = time.time() + 60 * > 1 # 1 minutes from now > On 2014/10/17 23:30:35, samuong wrote: > > are you ok with removing these two comments? > > certainly no problem to me > > Done. It looks like this only file that has been updated is chrome_launcher.cc. Did you forget to "git add" the other files?
On 2014/10/18 00:49:43, samuong wrote: > On 2014/10/18 00:45:46, andrewcheng wrote: > > please review it > > > > > https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... > > File chrome/test/chromedriver/chrome_launcher.cc (right): > > > > > https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... > > chrome/test/chromedriver/chrome_launcher.cc:146: > > capabilities.local_state.get()); > > On 2014/10/17 23:30:34, samuong wrote: > > > please fix indenting for arguments - they should either be lined up to the > > > opening parenthesis or indented by 4 spaces > > > > Done. > > > > > https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... > > chrome/test/chromedriver/chrome_launcher.cc:148: return status; > > On 2014/10/17 23:30:34, samuong wrote: > > > please fix indenting for "return status;" (it should only be indented by 4 > > > spaces, not 6) > > > > Done. > > > > > https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... > > chrome/test/chromedriver/chrome_launcher.cc:783: > > default_dir.Append(chrome::kPreferencesFilename); > > On 2014/10/17 23:30:35, samuong wrote: > > > fix indenting (should be 6 spaces, not 9) > > > > Done. > > > > > https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... > > chrome/test/chromedriver/chrome_launcher.cc:788: preferences = > > std::string(kPreferences); > > On 2014/10/17 23:30:34, samuong wrote: > > > why not just do "preferences = kPreferences;"? > > > > Done. > > > > > https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... > > chrome/test/chromedriver/chrome_launcher.cc:799: > > user_data_dir.Append(chrome::kLocalStateFilename); > > On 2014/10/17 23:30:34, samuong wrote: > > > fix indenting (should be 6 spaces, not 8) > > > > Done. > > > > > https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... > > chrome/test/chromedriver/chrome_launcher.cc:804: local_state = > > std::string(kLocalState); > > On 2014/10/17 23:30:35, samuong wrote: > > > similar to above, why not just do "local_state = kLocalState;"? > > > > Done. > > > > > https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... > > File chrome/test/chromedriver/client/chromedriver.py (right): > > > > > https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... > > chrome/test/chromedriver/client/chromedriver.py:123: options['prefs'] = > > download_prefs > > On 2014/10/17 23:30:35, samuong wrote: > > > Just in case prefs already exists in some form (e.g. via > experimental_options) > > > maybe we should be careful about not clobbering it. How about we change this > > to > > > the following? > > > > > > if download_dir: > > > if 'prefs' not in options: > > > options['prefs'] = {} > > > if 'download' not in options['prefs']: > > > options['prefs']['download'] = {} > > > options['prefs']['download']['default_directory'] = download_dir > > > > Done. > > > > > https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... > > File chrome/test/chromedriver/test/run_py_tests.py (right): > > > > > https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... > > chrome/test/chromedriver/test/run_py_tests.py:763: > > '/chromedriver/download.html') > > On 2014/10/17 23:30:35, samuong wrote: > > > Ideally I'd prefer that this class doesn't use anything from > ChromeDriverTest, > > > but that can be addressed in a separate commit. But for this CL, if you're > > going > > > to use ChromeDriverTest's _http_server, why not use > > > ChromeDriverTest.GetHttpUrlForFile()? > > > > Done. > > > > > https://codereview.chromium.org/613163004/diff/190001/chrome/test/chromedrive... > > chrome/test/chromedriver/test/run_py_tests.py:766: deadline = time.time() + 60 > * > > 1 # 1 minutes from now > > On 2014/10/17 23:30:35, samuong wrote: > > > are you ok with removing these two comments? > > > > certainly no problem to me > > > > Done. > > It looks like this only file that has been updated is chrome_launcher.cc. Did > you forget to "git add" the other files? lgtm
The CQ bit was checked by samuong@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/613163004/290001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
Hi Sam, Build is completed. Andrew
lgtm, with one nit https://codereview.chromium.org/613163004/diff/370001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome_launcher.cc (right): https://codereview.chromium.org/613163004/diff/370001/chrome/test/chromedrive... chrome/test/chromedriver/chrome_launcher.cc:140: GetSwitchValueNative("user-data-dir")); can you put the "switches." on line 140? ie: user_data_dir_path = base::FilePath( switches.GetSwitchValueNative("user-data-dir"));
The CQ bit was checked by andrewcheng@google.com
The CQ bit was unchecked by andrewcheng@google.com
The CQ bit was checked by andrewcheng@google.com
The CQ bit was unchecked by andrewcheng@google.com
The CQ bit was checked by andrewcheng@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/613163004/390001
Message was sent while issue was closed.
Committed patchset #20 (id:390001)
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/e361733704a9d522ea6a49f64e380ef7bc7794ad Cr-Commit-Position: refs/heads/master@{#300531} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
