Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6973052:
When the download folder does not exist, change the download folder to a user's "Downloads" (Closed)
|
Created:
9 years, 7 months ago by haraken1 Modified:
9 years, 4 months ago Reviewers:
Peter Kasting, csilv, Randy Smith (Not in Mondays), chrome-ui-review, Glen Murphy, commit-bot: I haz the power, eroman, Paweł Hajdan Jr., brakowski CC:
chromium-reviews, rdsmith+dwatch_chromium.org Visibility:
Public. |
DescriptionWhen the download folder does not exist, change the download folder to a user's "Downloads" (or something depending on the platform)
- Set the download folder to Foo. You can download a PDF file to Foo without being asked anything (if you do not choose "Ask where to save each file before downloading"). Then delete Foo. Now, if you try to download the PDF file, then a file chooser dialog is displayed suggesting your "Downloads" folder.
- Set the download folder to Foo. Save an HTML to Bar by right-clicking and choosing "Save as ..." on the tab. If you try to save the HTML, the file chooser dialog is displayed suggesting Bar. Then delete Bar. Now, if you try to save the HTML, the file chooser dialog is displayed suggesting Foo. Then delete Foo. If you try to save the HTML, the file chooser dialog is displayed suggesting your "Downloads" folder.
- Set the download folder to Foo. Observe that Foo is displayed as the download location in the preferences page. Then delete Foo and reload the preferences page. Observe that your "Downloads" folder is displayed as the download location in the preferences page. If you are opening multiple preferences pages, then this change is automatically reflected to all the preferences pages.
BUG=32552
TEST=SavePageBrowserTest.SavedFolder1,SavePageBrowserTest.SavedFolder2,SavePageBrowserTest.SavedFolder3,DownloadTest.DownloadedFolder,DownloadManagerTest.StartDownload,BaseFileTest.*,DownloadFileTest.RenameFileFinal
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91861
Patch Set 1 #Patch Set 2 : Correct typo #
Total comments: 19
Patch Set 3 : Change "My Documents" to "Downloads" #Patch Set 4 : Added SavePageBrowserTest.SavedDirectory #Patch Set 5 : Correct typo #
Total comments: 26
Patch Set 6 : Added DownloadTest.DownloadedDirectory #Patch Set 7 : Correct typo #
Total comments: 43
Patch Set 8 : Remove overloading of GetSaveInfo() and misc #Patch Set 9 : Correct typo #
Total comments: 11
Patch Set 10 : Add SavePageTest.SavedFolder4 and DownloadTest.DownloadedFolder2 #
Total comments: 4
Patch Set 11 : Add PathServiceWrapper and DownloadUtilTest.OverridePathService #Patch Set 12 : Remove PathServiceWrapper and add DefaultDownloadDirectory #
Total comments: 23
Patch Set 13 : Add ChooseSavableDirectory() and ScopedDefaultDownloadDirectory #
Total comments: 19
Patch Set 14 : Overrides the user's "Downloads" folder in DownloadPrefs #
Total comments: 23
Patch Set 15 : Reflected Pawel's comments #
Total comments: 7
Patch Set 16 : Remove DownloadTest.DownloadFolder2 and SavePageBrowserTest.SaveFolder4 #
Total comments: 3
Patch Set 17 : Revert DownloadManagerTest.StartDownload back to the original #Patch Set 18 : Give up overriding #
Total comments: 2
Patch Set 19 : Correct nits #Patch Set 20 : Just rebased #Patch Set 21 : Rebased with the latest repository #Patch Set 22 : Fix save_page_browsertest.cc for Windows build #Patch Set 23 : Fixed string conversion for Windows filepath #Patch Set 24 : Delete xxxxx.crdownload before downloading to xxxxx to the default download folder #Patch Set 25 : Use a temporal file with a unique name in download tests #Patch Set 26 : Use ScopedTempDir for temporarily generated test data #Patch Set 27 : Added URLRequestMockHTTPJob.test_dir_ and URLRequestMockHTTPJob.temp_dir_ #
Total comments: 6
Patch Set 28 : Call filter->AddHostnameHandler from browser_tests #Patch Set 29 : Redirect "http://mock.testfile.http/<random path>" to "chrome/test/data/a.htm" #
Total comments: 1
Patch Set 30 : Added FILE_PATH_LITERAL(".crdownload") #Patch Set 31 : Removed the change in url_request_mock_http_job.h #
Total comments: 12
Patch Set 32 : Applied Pawel's comments #Messages
Total messages: 98 (0 generated)
I am trying to fix the bug 32552. Would you take a look at it? One question to Pawel: In download_manager.cc, I (simply) removed your following comment in order to fix the bug. 343 // Make sure the default download directory exists. 344 // TODO(phajdan.jr): only create the directory when we're sure the user 345 // is going to save there and not to another directory of his choice. 346 file_util::CreateDirectory(default_path); Specifically, what is the situation that you are referring to as "when we're sure the user is going to save there and not to another directory of his choice"?
I'm not very familiar with the WebUI advanced settings thing. Please make sure to include someone who knows it as a reviewer. http://codereview.chromium.org/6973052/diff/2001/chrome/browser/download/down... File chrome/browser/download/download_manager.cc (left): http://codereview.chromium.org/6973052/diff/2001/chrome/browser/download/down... chrome/browser/download/download_manager.cc:344: // TODO(phajdan.jr): only create the directory when we're sure the user By this comment I meant that in DownloadManager::CheckIfSuggestedPathExists we're still not sure whether |default_path| will be the final destination directory. Some users are annoyed when we create the default directory even if they choose a custom location for each download, so this TODO was put here to remove that annoyance. Some more context might be present in the svn log / svn blame of those lines. http://codereview.chromium.org/6973052/diff/2001/chrome/browser/download/save... File chrome/browser/download/save_package.cc (right): http://codereview.chromium.org/6973052/diff/2001/chrome/browser/download/save... chrome/browser/download/save_package.cc:1282: PathService::Get(chrome::DIR_USER_DOCUMENTS, &save_dir); Please check the return value. http://codereview.chromium.org/6973052/diff/2001/chrome/browser/download/save... chrome/browser/download/save_package.cc:1283: else Now this is a double-negation. Please put the "true" branch first, i.e. if (file_util::DirectoryExists(...)) { save_dir = download_save_dir; } else { ... } http://codereview.chromium.org/6973052/diff/2001/chrome/browser/ui/webui/opti... File chrome/browser/ui/webui/options/advanced_options_handler.cc (right): http://codereview.chromium.org/6973052/diff/2001/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/advanced_options_handler.cc:196: default_download_location_.GetValue()); nit: Is the indentation correct here? I think it should rather be +4 from the beginning of the previous line. http://codereview.chromium.org/6973052/diff/2001/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/advanced_options_handler.cc:671: PathService::Get(chrome::DIR_USER_DOCUMENTS, &new_path); Check the return value. http://codereview.chromium.org/6973052/diff/2001/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/advanced_options_handler.cc:679: OnDownloadPathChanged(const FilePath path) { nit: Why not "const FilePath&", here and above? http://codereview.chromium.org/6973052/diff/2001/chrome/browser/ui/webui/opti... File chrome/browser/ui/webui/options/advanced_options_handler.h (right): http://codereview.chromium.org/6973052/diff/2001/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/advanced_options_handler.h:27: : public base::RefCountedThreadSafe<DownloadPathChecker> { nit: Shouldn't this be indented 4 spaces rather than 2? http://codereview.chromium.org/6973052/diff/2001/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/advanced_options_handler.h:39: ~DownloadPathChecker() {} nit: Could you put the dtor implementation in the .cc file?
A couple of high-level issues: * The bug as written advocates changing the default downloads location to ~/Downloads, not ~/My Documents. Since it says that's what Safari and Firefox do, it's probably an important point. I think you want GetUserDownloadsDirectory/chrome::DIR_DEFAULT_DOWNLOADS. Does that fit your understanding? * The bug as written is an OS-X bug and I'm guessing you're not doing development on OS-X. I personally think that this problem will exist on all platforms, so we should change that to an OS-All bug, but we should make sure that we fix it on all platforms on which it's happening at the same time, which I'm not sure you're in a position to do at the moment. http://codereview.chromium.org/6973052/diff/2001/chrome/browser/download/down... File chrome/browser/download/download_manager.cc (left): http://codereview.chromium.org/6973052/diff/2001/chrome/browser/download/down... chrome/browser/download/download_manager.cc:344: // TODO(phajdan.jr): only create the directory when we're sure the user On 2011/05/13 09:16:22, Paweł Hajdan Jr. wrote: > By this comment I meant that in DownloadManager::CheckIfSuggestedPathExists > we're still not sure whether |default_path| will be the final destination > directory. > > Some users are annoyed when we create the default directory even if they choose > a custom location for each download, so this TODO was put here to remove that > annoyance. Some more context might be present in the svn log / svn blame of > those lines. Pawel: I'm trying to think about how I'd ideally fix this problem. My sense is that if the user chooses a custom location, the file chooser dialog will make sure that we have the parents to that location. This CL would deal with having a correct default location. We still have to worry about races with someone deleting directories at just the right time, but we always have to worry about that, and it's unlikely enough that I think just failing the download is fine. So I think if a fix for this issue is implemented, we can in fact get rid of your extra CreateDirectory() here. Do you check me on that logic?
> * The bug as written advocates changing the default downloads location to > ~/Downloads, not ~/My Documents. Since it says that's what Safari and > Firefox > do, it's probably an important point. I think you want > GetUserDownloadsDirectory/chrome::DIR_DEFAULT_DOWNLOADS. Does that fit your > understanding? Oops!! You are right. I fixed it. > * The bug as written is an OS-X bug and I'm guessing you're not doing > development on OS-X. I personally think that this problem will exist on all > platforms, so we should change that to an OS-All bug, but we should make > sure > that we fix it on all platforms on which it's happening at the same time, > which > I'm not sure you're in a position to do at the moment. Yes, this is a bug on all platforms. Would you please change the label of this bug (http://code.google.com/p/chromium/issues/detail?id=32552) to OS-All (since I do not have a permission to change it)? Fortunately, I guess that this bug fix will require no per-platform modifications. Of course, I will check the behavior for all platforms as soon as possible (maybe within this week). http://codereview.chromium.org/6973052/diff/2001/chrome/browser/download/down... File chrome/browser/download/download_manager.cc (left): http://codereview.chromium.org/6973052/diff/2001/chrome/browser/download/down... chrome/browser/download/download_manager.cc:344: // TODO(phajdan.jr): only create the directory when we're sure the user I would like to wait Pawel's reply. http://codereview.chromium.org/6973052/diff/2001/chrome/browser/download/save... File chrome/browser/download/save_package.cc (right): http://codereview.chromium.org/6973052/diff/2001/chrome/browser/download/save... chrome/browser/download/save_package.cc:1282: PathService::Get(chrome::DIR_USER_DOCUMENTS, &save_dir); On 2011/05/13 09:16:22, Paweł Hajdan Jr. wrote: > Please check the return value. Done. http://codereview.chromium.org/6973052/diff/2001/chrome/browser/download/save... chrome/browser/download/save_package.cc:1283: else On 2011/05/13 09:16:22, Paweł Hajdan Jr. wrote: > Now this is a double-negation. Please put the "true" branch first, i.e. > > if (file_util::DirectoryExists(...)) { > save_dir = download_save_dir; > } else { > ... > } Done. http://codereview.chromium.org/6973052/diff/2001/chrome/browser/ui/webui/opti... File chrome/browser/ui/webui/options/advanced_options_handler.cc (right): http://codereview.chromium.org/6973052/diff/2001/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/advanced_options_handler.cc:196: default_download_location_.GetValue()); On 2011/05/13 09:16:22, Paweł Hajdan Jr. wrote: > nit: Is the indentation correct here? I think it should rather be +4 from the > beginning of the previous line. Done. http://codereview.chromium.org/6973052/diff/2001/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/advanced_options_handler.cc:671: PathService::Get(chrome::DIR_USER_DOCUMENTS, &new_path); On 2011/05/13 09:16:22, Paweł Hajdan Jr. wrote: > Check the return value. Done. http://codereview.chromium.org/6973052/diff/2001/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/advanced_options_handler.cc:679: OnDownloadPathChanged(const FilePath path) { On 2011/05/13 09:16:22, Paweł Hajdan Jr. wrote: > nit: Why not "const FilePath&", here and above? Done. http://codereview.chromium.org/6973052/diff/2001/chrome/browser/ui/webui/opti... File chrome/browser/ui/webui/options/advanced_options_handler.h (right): http://codereview.chromium.org/6973052/diff/2001/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/advanced_options_handler.h:27: : public base::RefCountedThreadSafe<DownloadPathChecker> { On 2011/05/13 09:16:22, Paweł Hajdan Jr. wrote: > nit: Shouldn't this be indented 4 spaces rather than 2? Done. http://codereview.chromium.org/6973052/diff/2001/chrome/browser/ui/webui/opti... chrome/browser/ui/webui/options/advanced_options_handler.h:39: ~DownloadPathChecker() {} On 2011/05/13 09:16:22, Paweł Hajdan Jr. wrote: > nit: Could you put the dtor implementation in the .cc file? Done.
http://codereview.chromium.org/6973052/diff/2001/chrome/browser/download/down... File chrome/browser/download/download_manager.cc (left): http://codereview.chromium.org/6973052/diff/2001/chrome/browser/download/down... chrome/browser/download/download_manager.cc:344: // TODO(phajdan.jr): only create the directory when we're sure the user On 2011/05/16 21:31:05, rdsmith wrote: > Pawel: I'm trying to think about how I'd ideally fix this problem. My sense is > that if the user chooses a custom location, the file chooser dialog will make > sure that we have the parents to that location. This CL would deal with having > a correct default location. We still have to worry about races with someone > deleting directories at just the right time, but we always have to worry about > that, and it's unlikely enough that I think just failing the download is fine. > So I think if a fix for this issue is implemented, we can in fact get rid of > your extra CreateDirectory() here. Do you check me on that logic? Yeah, generally the TODO is about removing this code. I'm not sure whether the chooser dialog is entirely correct. Ideally we should write some tests for all scenarios that users reported as buggy.
Adding csilv as an OWNER and apparent contributor for reviewing advanced_options_handler.*. haraken: Could you also add tests for the functionality you're changing? I'm not sure how to add tests for advanced_options_handler.* (possibly csilv can advise) but for the save package code, could you make sure the directories "fail over" in the way you expect? Probably adding to save_page_browsertest.cc is the way to go; you can look at download_browsertest.cc for creation of temporary directories that don't last longer than the current browser test. (I don't think there are any tests implied for downloads by this CL, since the judgement is that the case that Pawel's create directory was for "shouldn't really happen" any more, and I'm not sure how to test for that. But Pawel may have other perspectives/suggestions on tests to write.)
I am very sorry for this late reply... I added SavePageBrowserTest.SavedDirectory, which checks the directory where an HTML is saved. Would you take a look at it? http://codereview.chromium.org/6973052/diff/2001/chrome/browser/download/down... File chrome/browser/download/download_manager.cc (left): http://codereview.chromium.org/6973052/diff/2001/chrome/browser/download/down... chrome/browser/download/download_manager.cc:344: // TODO(phajdan.jr): only create the directory when we're sure the user Finally, I removed these codes.
http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (left): http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:374: file_util::CreateDirectory(default_path); Please also add a test for a case where we're downloading a file (not a web page) and the save directory doesn't exist. http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... File chrome/browser/download/save_package.cc (right): http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1276: void SavePackage::GetSaveInfo(const FilePath& website_save_dir, This duplicates most of SavePackage::GetSaveInfo(). Can we avoid that? http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1315: file_util::CreateDirectory(save_dir); This call can still fail (for example because of a race, or just some error). Are you sure we should just ignore such failure? http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... File chrome/browser/download/save_package.h (right): http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... chrome/browser/download/save_package.h:128: void GetSaveInfo(const FilePath& website_save_dir, I think that a comment should get added. Why do we have two different GetSaveInfos? And why this *Get method doesn't return anything? (That last one is really a question about existing code, but we should fix that while modifying this code). http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... chrome/browser/download/save_package.h:130: SavePackageType save_type); nit: Add empty line below. http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... File chrome/browser/download/save_page_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:156: ScopedTempDir *website_save_temp_dir = new ScopedTempDir(); Why are all of those pointers? Can't you just stack-allocate all of them? It seems it doesn't matter much to delete them before the end of the test. If that's not possible, switch to scoped_ptr. Also, the star is on the wrong side. The right style is always "ScopedTempDir* ". http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:164: FilePath website_save_dir = website_save_temp_dir->path(); nit: Does it make sense to have temporary variables for those? http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:173: PathService::Override(chrome::DIR_DEFAULT_DOWNLOADS, default_save_dir); PathService::Override is _extremely_ error-prone. Could you find a solution that doesn't use it? http://codereview.chromium.org/6973052/diff/15001/chrome/browser/ui/webui/opt... File chrome/browser/ui/webui/options/advanced_options_handler.cc (right): http://codereview.chromium.org/6973052/diff/15001/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/advanced_options_handler.cc:678: file_util::CreateDirectory(new_path); What if this fails?
Thank you for the review. I added DownloadTest.DownloadedDirectory for testing the behavior when a default download folder does not exist. http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (left): http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:374: file_util::CreateDirectory(default_path); I added DownloadTest.DownloadedDirectory. http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... File chrome/browser/download/save_package.cc (right): http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1276: void SavePackage::GetSaveInfo(const FilePath& website_save_dir, I also first thought that I should avoid this duplication, but gave up because this GetSaveInfo(...) is just for tests and is doing something special in it (e.g. ignore the return value of GetSaveDirPreference()). I am afraid that if we remove this duplication, then the original logic of GetSaveInfo() may become hard to follow. However, if you think we should remove the duplication, I will remove it. http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1315: file_util::CreateDirectory(save_dir); I just added an error message when CreateDirectory fails. I think that ignoring this error may be acceptable here, since in any way we cannot avoid the situation that we fail to save a file because the targeted directory does not exist. For example, even if |save_dir| exists here, the |save_dir| may be removed when a file is actually saved to the directory. http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... File chrome/browser/download/save_package.h (right): http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... chrome/browser/download/save_package.h:128: void GetSaveInfo(const FilePath& website_save_dir, On 2011/05/30 14:15:49, Paweł Hajdan Jr. wrote: > I think that a comment should get added. Why do we have two different > GetSaveInfos? And why this *Get method doesn't return anything? (That last one > is really a question about existing code, but we should fix that while modifying > this code). Done. http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... chrome/browser/download/save_package.h:130: SavePackageType save_type); On 2011/05/30 14:15:49, Paweł Hajdan Jr. wrote: > nit: Add empty line below. Done. http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... File chrome/browser/download/save_page_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:156: ScopedTempDir *website_save_temp_dir = new ScopedTempDir(); On 2011/05/30 14:15:49, Paweł Hajdan Jr. wrote: > Why are all of those pointers? Can't you just stack-allocate all of them? It > seems it doesn't matter much to delete them before the end of the test. If > that's not possible, switch to scoped_ptr. > > Also, the star is on the wrong side. The right style is always "ScopedTempDir* > ". Done. http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:164: FilePath website_save_dir = website_save_temp_dir->path(); I feel that having these variables will make the code clearer. (Note: these variables are not literally "temporary" variables but the variables for temporal directories.) http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:173: PathService::Override(chrome::DIR_DEFAULT_DOWNLOADS, default_save_dir); I see. I removed PathService::Override and instead I used the real user's "Downloads" directory for the test. But is it OK to use the real user's "Downloads" directory for tests? http://codereview.chromium.org/6973052/diff/15001/chrome/browser/ui/webui/opt... File chrome/browser/ui/webui/options/advanced_options_handler.cc (right): http://codereview.chromium.org/6973052/diff/15001/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options/advanced_options_handler.cc:678: file_util::CreateDirectory(new_path); For the reason that I wrote above, I just added an error message when it fails.
http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... File chrome/browser/download/save_package.cc (right): http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1276: void SavePackage::GetSaveInfo(const FilePath& website_save_dir, On 2011/05/31 12:41:31, haraken wrote: > I also first thought that I should avoid this duplication, but gave up because > this GetSaveInfo(...) is just for tests and is doing something special in it > (e.g. ignore the return value of GetSaveDirPreference()). I am afraid that if we > remove this duplication, then the original logic of GetSaveInfo() may become > hard to follow. However, if you think we should remove the duplication, I will > remove it. Why do we need to have this special variant for tests? http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... File chrome/browser/download/save_page_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:164: FilePath website_save_dir = website_save_temp_dir->path(); On 2011/05/31 12:41:31, haraken wrote: > I feel that having these variables will make the code clearer. (Note: these > variables are not literally "temporary" variables but the variables for temporal > directories.) I don't see how they help. The length of the expression is roughly the same, and a value of those variables might change, while ScopedTempDir generally stays the same. http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:173: PathService::Override(chrome::DIR_DEFAULT_DOWNLOADS, default_save_dir); On 2011/05/31 12:41:31, haraken wrote: > I see. I removed PathService::Override and instead I used the real user's > "Downloads" directory for the test. But is it OK to use the real user's > "Downloads" directory for tests? I think some existing tests use the real Downloads directory, but in this case... why don't you just Set the pref instead of overriding it? You should be able to set it to a non-existent directory, and that'd make the tests much simpler. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:818: // Delete the default folder for downloaded files. Do we only need to do the setup above to get the file name? It's weird to download something first and then delete some things. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:826: PathService::Get(chrome::DIR_DEFAULT_DOWNLOADS, &default_downloads_path); Please check the return value. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:832: ui_test_utils::NavigateToURLWithDisposition( Why do we have a second navigation to |url|? By "first" I mean DownloadAndWait on line 823. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:378: PathService::Get(chrome::DIR_DEFAULT_DOWNLOADS, &state.suggested_path); Oh, this code didn't check return value of PathService::Get. Could you do that now? http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/dow... chrome/browser/download/download_manager.h:89: virtual void SelectFileDialogDisplayed(int32 id, FilePath& path) {} Make sure to explain the meaning of |path|. It seems it's the suggested path, not the finally chosen path. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... File chrome/browser/download/save_page_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:163: PathService::Get(chrome::DIR_DEFAULT_DOWNLOADS, &default_save_dir); Check the return value. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:168: title = UTF16ToASCII( nit: Why not initialize title when declaring it then? Also, please put blank lines in places that results in logical blocks. full_file_name and title are more related to this block rather than the block above. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:184: // Delete the default folder for saving HTML. This is weird. Why don't we just pass a nonexistent folder at the beginning? http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:204: ASSERT_TRUE(download_save_temp_dir.Delete()); Why do we explicitly do that here? http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:223: ASSERT_TRUE(file_util::Delete(full_file_name, false)); Why do we explicitly do that here?
LGTM on advanced_options_handler.*
Some duplication with Pawel's comments--I'm not liking the extra pure testing methods either, and hoping as I understand better what you're doing, we can find another way :-}. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:828: EXPECT_FALSE(file_util::PathExists(file_path)); Would it be also appropriate to confirm that we didn't create the downloads directory? http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... File chrome/browser/download/save_package.cc (right): http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1274: void SavePackage::GetSaveInfo(const FilePath& website_save_dir, It's a minor point, but it would seem relatively easy to share code between this function and the variation without arguments. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1279: GetSaveDirPreference(prefs); Could you document the side effect for which you're calling this function? Naively, it looks like a function that wouldn't have side effects. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1280: prefs->SetInteger(prefs::kSaveFileType, save_type); This looks like a global state change that isn't documented/suggested in the header file comment. Could you at least document why we're doing that here? (I might want to ask you to separate the global state change out from this function after I understand better its purpose.) http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... File chrome/browser/download/save_package.h (right): http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... chrome/browser/download/save_package.h:138: SavePackageType save_type); The C++ style guide discourages overloading; see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl.... http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... File chrome/browser/download/save_page_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:146: IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SavedDirectory) { I know it's not consistent with the rest of the file, but could you put a sentence or two comment before this test describing what it's testing? http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:184: // Delete the default folder for saving HTML. On 2011/05/31 17:15:53, Paweł Hajdan Jr. wrote: > This is weird. Why don't we just pass a nonexistent folder at the beginning? Is there a way to create a unique temp dir that doesn't exist? I'd think that'd require creating a new version of CreateUniqueTempDir() that (like that function) figured out where it could put a temp dir that wasn't already being used, but (unlike that function) didn't create it. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/ui/download/... File chrome/browser/ui/download/download_tab_helper.cc (right): http://codereview.chromium.org/6973052/diff/20012/chrome/browser/ui/download/... chrome/browser/ui/download/download_tab_helper.cc:81: return tab_contents()->GetTitle(); This makes me nervous. By name and placement, this routine is parallel to DownloadTabHelper::SavePage, but it looks to be doing a couple of things noticeably differently: * SetShouldPromptUser(false): I'm inclined to suggest this be done directly from the test. * Not calling Init() * Calling GetSaveInfo() directly. I'd suggest that you try to make this as parallel to SavePage() as possible, to reduce confusion, and that you document the differences in behavior in the header file. (In my ideal world, we'd find some way to test this functionality without creating two new methods, one of which can't be made private, but I'm not understanding the test well enough to suggest a way to do that :-{.) http://codereview.chromium.org/6973052/diff/20012/chrome/browser/ui/download/... File chrome/browser/ui/download/download_tab_helper.h (right): http://codereview.chromium.org/6973052/diff/20012/chrome/browser/ui/download/... chrome/browser/ui/download/download_tab_helper.h:46: SavePackage::SavePackageType save_type); In general, I feel better about methods that are only for use by tests being private, with an explicit FRIEND_TEST_ALL_PREFIXES() to allow access by that one test.
http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... File chrome/browser/download/save_page_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:184: // Delete the default folder for saving HTML. On 2011/05/31 23:03:10, rdsmith wrote: > On 2011/05/31 17:15:53, Paweł Hajdan Jr. wrote: > > This is weird. Why don't we just pass a nonexistent folder at the beginning? > > Is there a way to create a unique temp dir that doesn't exist? I'd think that'd > require creating a new version of CreateUniqueTempDir() that (like that > function) figured out where it could put a temp dir that wasn't already being > used, but (unlike that function) didn't create it. I'd say just pass some dir name like /this/dir/does/not/exist/cyuwvuvcudrandomness and assert that it indeed doesn't exist.
http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... File chrome/browser/download/save_package.cc (right): http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1276: void SavePackage::GetSaveInfo(const FilePath& website_save_dir, On 2011/05/31 17:15:53, Paweł Hajdan Jr. wrote: > On 2011/05/31 12:41:31, haraken wrote: > > I also first thought that I should avoid this duplication, but gave up because > > this GetSaveInfo(...) is just for tests and is doing something special in it > > (e.g. ignore the return value of GetSaveDirPreference()). I am afraid that if > we > > remove this duplication, then the original logic of GetSaveInfo() may become > > hard to follow. However, if you think we should remove the duplication, I will > > remove it. > > Why do we need to have this special variant for tests? Done. http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... File chrome/browser/download/save_page_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:164: FilePath website_save_dir = website_save_temp_dir->path(); On 2011/05/31 17:15:53, Paweł Hajdan Jr. wrote: > On 2011/05/31 12:41:31, haraken wrote: > > I feel that having these variables will make the code clearer. (Note: these > > variables are not literally "temporary" variables but the variables for > temporal > > directories.) > > I don't see how they help. The length of the expression is roughly the same, and > a value of those variables might change, while ScopedTempDir generally stays the > same. Done. http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:173: PathService::Override(chrome::DIR_DEFAULT_DOWNLOADS, default_save_dir); I added a new method PathService::Set(int key, const FilePath& path), and used it here. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:818: // Delete the default folder for downloaded files. On 2011/05/31 17:15:53, Paweł Hajdan Jr. wrote: > Do we only need to do the setup above to get the file name? It's weird to > download something first and then delete some things. Done. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:826: PathService::Get(chrome::DIR_DEFAULT_DOWNLOADS, &default_downloads_path); On 2011/05/31 17:15:53, Paweł Hajdan Jr. wrote: > Please check the return value. Done. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:828: EXPECT_FALSE(file_util::PathExists(file_path)); On 2011/05/31 23:03:10, rdsmith wrote: > Would it be also appropriate to confirm that we didn't create the downloads > directory? Done. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:832: ui_test_utils::NavigateToURLWithDisposition( I cannot yet say the exact reason but this NavigateToURLWithDisposition() is required. I confirmed that if I remove this NavigateToURLWithDisposition(), then a file select dialog remains displayed and this test does not proceed any more. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:378: PathService::Get(chrome::DIR_DEFAULT_DOWNLOADS, &state.suggested_path); On 2011/05/31 17:15:53, Paweł Hajdan Jr. wrote: > Oh, this code didn't check return value of PathService::Get. Could you do that > now? Done. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/dow... chrome/browser/download/download_manager.h:89: virtual void SelectFileDialogDisplayed(int32 id, FilePath& path) {} On 2011/05/31 17:15:53, Paweł Hajdan Jr. wrote: > Make sure to explain the meaning of |path|. It seems it's the suggested path, > not the finally chosen path. Done. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... File chrome/browser/download/save_package.cc (right): http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1274: void SavePackage::GetSaveInfo(const FilePath& website_save_dir, I removed this GetSaveInfo(). Instead, I added DownloadTabHelper::ChangeSaveDirectoryPrefs() and DownloadTabHelper::RestoreSaveDirectoryPrefs(). As I commented in download_tab_helper.h, if we want to change the default folder preferences used for some tests, then (1) call ChangeSaveDirectoryPrefs() to change the preferences (2) call GetSaveInfo(), which will use the prefs that we set in (1) (3) call RestoreSaveDirectoryPrefs() to restore the preferences http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1279: GetSaveDirPreference(prefs); This is now unnecessary since I removed this code. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1280: prefs->SetInteger(prefs::kSaveFileType, save_type); This is now unnecessary since I removed this code. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... File chrome/browser/download/save_package.h (right): http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... chrome/browser/download/save_package.h:138: SavePackageType save_type); On 2011/05/31 23:03:10, rdsmith wrote: > The C++ style guide discourages overloading; see > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl.... Done. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... File chrome/browser/download/save_page_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:146: IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SavedDirectory) { I added a detailed comment for the test. Also, for clarity, I split this test into three tests, SaveToDirectoryForHTMLPage, SaveToDirectoryForDownloadedFiles and SaveToUsersDownloadsFolder. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:163: PathService::Get(chrome::DIR_DEFAULT_DOWNLOADS, &default_save_dir); On 2011/05/31 17:15:53, Paweł Hajdan Jr. wrote: > Check the return value. Done. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:168: title = UTF16ToASCII( On 2011/05/31 17:15:53, Paweł Hajdan Jr. wrote: > nit: Why not initialize title when declaring it then? > > Also, please put blank lines in places that results in logical blocks. > full_file_name and title are more related to this block rather than the block > above. Done. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:184: // Delete the default folder for saving HTML. On 2011/06/01 07:08:09, Paweł Hajdan Jr. wrote: > On 2011/05/31 23:03:10, rdsmith wrote: > > On 2011/05/31 17:15:53, Paweł Hajdan Jr. wrote: > > > This is weird. Why don't we just pass a nonexistent folder at the beginning? > > > > Is there a way to create a unique temp dir that doesn't exist? I'd think > that'd > > require creating a new version of CreateUniqueTempDir() that (like that > > function) figured out where it could put a temp dir that wasn't already being > > used, but (unlike that function) didn't create it. > > I'd say just pass some dir name like > /this/dir/does/not/exist/cyuwvuvcudrandomness and assert that it indeed doesn't > exist. Done. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:204: ASSERT_TRUE(download_save_temp_dir.Delete()); I removed it. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:223: ASSERT_TRUE(file_util::Delete(full_file_name, false)); On 2011/05/31 17:15:53, Paweł Hajdan Jr. wrote: > Why do we explicitly do that here? Done. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/ui/download/... File chrome/browser/ui/download/download_tab_helper.cc (right): http://codereview.chromium.org/6973052/diff/20012/chrome/browser/ui/download/... chrome/browser/ui/download/download_tab_helper.cc:81: return tab_contents()->GetTitle(); > I'd suggest that you try to make this as parallel to SavePage() as possible, to > reduce confusion, and that you document the differences in behavior I agree. Now I revised this SavePageToProperDirectory() to SavePageWithougDialog(), which is more parallel to the SavePage(), and added a comment about their differences. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/ui/download/... File chrome/browser/ui/download/download_tab_helper.h (right): http://codereview.chromium.org/6973052/diff/20012/chrome/browser/ui/download/... chrome/browser/ui/download/download_tab_helper.h:46: SavePackage::SavePackageType save_type); If we simply make these methods private and add FRIEND_TEST_ALL_PREFIXES(), then we cannot compile save_page_browsertest.cc. because SavePageTest.* belongs to empty namespace: In save_page_browsertest.cc: namespace { IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveToFolderForHTMLPage) { ...; } IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveToFolderForDownloadedFiles) { ...; } } http://codereview.chromium.org/6973052/diff/20012/chrome/browser/ui/download/... chrome/browser/ui/download/download_tab_helper.h:46: SavePackage::SavePackageType save_type); If we simply make these methods private and add FRIEND_TEST_ALL_PREFIXES(), then we cannot compile save_page_browsertest.cc because SavePageTest.* belongs to an empty namespace. save_page_browsertest.cc: namespace { IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveToFolderForHTMLPage) { ...; } IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveToFolderForDownloadedFiles) { ...; } IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveToUsersDownloadsFolder) { ...; } } Thus, we cannot specify the above tests simply by writing FRIEND_TEST_ALL_PREFIXES() as follows. download_tab_helper.h: class DownloadTabHelper : public TabContentsObserver { private: FRIEND_TEST_ALL_PREFIXES(SavePageBrowserTest, SaveToFolderForHTMLPage); FRIEND_TEST_ALL_PREFIXES(SavePageBrowserTest, SaveToFolderForDownloadedFiles); FRIEND_TEST_ALL_PREFIXES(SavePageBrowserTest, SaveToUsersDownloadsFolder); ...; } The first possible approach is to leave these methods public as I write. The second approach is to eliminate the empty namespace (namespace { ... }) from save_page_browsertest.cc. The third approach is to name the namespace of save_page_browsertest.cc (like namespace savepage { ... }) and use the namespace to specify the tests (like FRIEND_TEST_ALL_PREFIXES(savepage::SavePageBrowserTest, SaveToFolderForHTMLPage)). Which one would be best?
http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... File chrome/browser/download/save_page_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:173: PathService::Override(chrome::DIR_DEFAULT_DOWNLOADS, default_save_dir); On 2011/05/31 17:15:53, Paweł Hajdan Jr. wrote: > On 2011/05/31 12:41:31, haraken wrote: > > I see. I removed PathService::Override and instead I used the real user's > > "Downloads" directory for the test. But is it OK to use the real user's > > "Downloads" directory for tests? > > I think some existing tests use the real Downloads directory, but in this > case... why don't you just Set the pref instead of overriding it? You should be > able to set it to a non-existent directory, and that'd make the tests much > simpler. Pawel: I'll go along with this if you really think it's the better choice, but why is this better than using Override? I can't offhand think of any gotchas for Override that won't apply to Set, and Set involves creating a new method on a very central class. Enlighten me as to why this is the better path? http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:832: ui_test_utils::NavigateToURLWithDisposition( On 2011/06/02 09:13:22, haraken wrote: > I cannot yet say the exact reason but this NavigateToURLWithDisposition() is > required. I confirmed that if I remove this NavigateToURLWithDisposition(), then > a file select dialog remains displayed and this test does not proceed any more. Haraken: Let's figure out why the test isn't behaving properly; I'm a big believer in "If you don't understand it, it's dangerous" :-}. I notice that the observer isn't being used correctly; it needs to be created before doing the download that you want to track and WaitForFinished() called afterwards, to resolve race conditions; it only waits for downloads that were started after its creation. Maybe this is the problem? http://codereview.chromium.org/6973052/diff/20012/chrome/browser/ui/download/... File chrome/browser/ui/download/download_tab_helper.h (right): http://codereview.chromium.org/6973052/diff/20012/chrome/browser/ui/download/... chrome/browser/ui/download/download_tab_helper.h:46: SavePackage::SavePackageType save_type); On 2011/06/02 09:13:22, haraken wrote: > Which one would be best? What I've always done is move the tests (but not any helper code) out of the empty namespace. If there's no helper code, that may mean eliminating the namespace completely, but more often it means ending the scope early. See download_browsertest.cc for an example. http://codereview.chromium.org/6973052/diff/27017/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6973052/diff/27017/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:379: chrome::DIR_DEFAULT_DOWNLOADS, &state.suggested_path)) { Are we testing this case? http://codereview.chromium.org/6973052/diff/27017/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:380: LOG(ERROR) << "Cannot find the user's \"Downloads\" folder."; I'm inclined to suggest you make this a VLOG rather than a LOG(ERROR). It is an uncommon case, but it's not really broken from the user's perspective--they may have some very good (or bad, but still their choice) reason to delete the default downloads directory, and I don't think we want to dump stuff to the error output on release builds because of it. It's rarely a good idea to do non-VLOG release build logging. http://codereview.chromium.org/6973052/diff/27017/chrome/browser/download/sav... File chrome/browser/download/save_page_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/27017/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:249: // The user's "Downloads" folder exists. Should we also test the fourth case where none of them exist? http://codereview.chromium.org/6973052/diff/27017/chrome/browser/ui/download/... File chrome/browser/ui/download/download_tab_helper.cc (left): http://codereview.chromium.org/6973052/diff/27017/chrome/browser/ui/download/... chrome/browser/ui/download/download_tab_helper.cc:59: // file name sanitation and extension / mime checking. Why delete this comment? I think it's still valid (though arguably it should be in the header file.) http://codereview.chromium.org/6973052/diff/27017/chrome/browser/ui/download/... File chrome/browser/ui/download/download_tab_helper.cc (right): http://codereview.chromium.org/6973052/diff/27017/chrome/browser/ui/download/... chrome/browser/ui/download/download_tab_helper.cc:123: prefs->SetInteger(prefs::kSaveFileType, save_type); It looks to me as if everything in this function could be done in a test routine helper function rather than in main code. Am I missing something? If not, I'd prefer that--to the extent we can, I think it's good to keep the main header files/code free of code that's only used for testing. http://codereview.chromium.org/6973052/diff/27017/chrome/browser/ui/download/... File chrome/browser/ui/download/download_tab_helper.h (right): http://codereview.chromium.org/6973052/diff/27017/chrome/browser/ui/download/... chrome/browser/ui/download/download_tab_helper.h:56: string16 SavePageWithoutDialog(); I still don't see a comment describing the difference in functionality between this and SavePage...?
http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... File chrome/browser/download/save_page_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:173: PathService::Override(chrome::DIR_DEFAULT_DOWNLOADS, default_save_dir); On 2011/06/02 19:13:57, rdsmith wrote: > On 2011/05/31 17:15:53, Paweł Hajdan Jr. wrote: > > On 2011/05/31 12:41:31, haraken wrote: > > > I see. I removed PathService::Override and instead I used the real user's > > > "Downloads" directory for the test. But is it OK to use the real user's > > > "Downloads" directory for tests? > > > > I think some existing tests use the real Downloads directory, but in this > > case... why don't you just Set the pref instead of overriding it? You should > be > > able to set it to a non-existent directory, and that'd make the tests much > > simpler. > > Pawel: I'll go along with this if you really think it's the better choice, but > why is this better than using Override? I can't offhand think of any gotchas > for Override that won't apply to Set, and Set involves creating a new method on > a very central class. Enlighten me as to why this is the better path? No, Set is not the right solution. Please don't touch PathService at all. Just change the pref's value in the user's profile. I'm pretty sure the downloads directory is a preference.
On 2011/06/02 19:31:18, Paweł Hajdan Jr. wrote: > http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... > File chrome/browser/download/save_page_browsertest.cc (right): > > http://codereview.chromium.org/6973052/diff/15001/chrome/browser/download/sav... > chrome/browser/download/save_page_browsertest.cc:173: > PathService::Override(chrome::DIR_DEFAULT_DOWNLOADS, default_save_dir); > On 2011/06/02 19:13:57, rdsmith wrote: > > On 2011/05/31 17:15:53, Paweł Hajdan Jr. wrote: > > > On 2011/05/31 12:41:31, haraken wrote: > > > > I see. I removed PathService::Override and instead I used the real user's > > > > "Downloads" directory for the test. But is it OK to use the real user's > > > > "Downloads" directory for tests? > > > > > > I think some existing tests use the real Downloads directory, but in this > > > case... why don't you just Set the pref instead of overriding it? You should > > be > > > able to set it to a non-existent directory, and that'd make the tests much > > > simpler. > > > > Pawel: I'll go along with this if you really think it's the better choice, but > > why is this better than using Override? I can't offhand think of any gotchas > > for Override that won't apply to Set, and Set involves creating a new method > on > > a very central class. Enlighten me as to why this is the better path? > > No, Set is not the right solution. Please don't touch PathService at all. Just > change the pref's value in the user's profile. I'm pretty sure the downloads > directory is a preference. If I understand the code correctly, there's what's in the user preferences, and then, if we can't get at that, there's what the path service provides (which is system set, not controllable by user preferences). There are places in this patch where we fall back from one to the other. I.e. I think he has to do something on PathService to test the case when the code is accessing path service rather than user prefs.
> If I understand the code correctly, there's what's in the user preferences, and > then, if we can't get at that, there's what the path service provides (which is > system set, not controllable by user preferences). There are places in this > patch where we fall back from one to the other. I.e. I think he has to do > something on PathService to test the case when the code is accessing path > service rather than user prefs. Randy is right. Somehow we have to change the PathService's value (unless we allow tests to use the user's real "Downloads" folder). > Pawel: I'll go along with this if you really think it's the better choice, but > why is this better than using Override? I can't offhand think of any gotchas > for Override that won't apply to Set, and Set involves creating a new method on > a very central class. Enlighten me as to why this is the better path? Whether our current PathService::Set() is good or bad, I think that we anyway need another version of PathService::Override(), since PathService::Override() creates the specified path if the path does not exist. http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/20012/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:832: ui_test_utils::NavigateToURLWithDisposition( Ahh, I got it. Thank you. After all, for simplicity, I added the check of the suggested path to DownloadAndWait(). http://codereview.chromium.org/6973052/diff/20012/chrome/browser/ui/download/... File chrome/browser/ui/download/download_tab_helper.h (right): http://codereview.chromium.org/6973052/diff/20012/chrome/browser/ui/download/... chrome/browser/ui/download/download_tab_helper.h:46: SavePackage::SavePackageType save_type); On 2011/06/02 19:13:57, rdsmith wrote: > On 2011/06/02 09:13:22, haraken wrote: > > Which one would be best? > > What I've always done is move the tests (but not any helper code) out of the > empty namespace. If there's no helper code, that may mean eliminating the > namespace completely, but more often it means ending the scope early. See > download_browsertest.cc for an example. > Done. http://codereview.chromium.org/6973052/diff/27017/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6973052/diff/27017/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:380: LOG(ERROR) << "Cannot find the user's \"Downloads\" folder."; On 2011/06/02 19:13:57, rdsmith wrote: > I'm inclined to suggest you make this a VLOG rather than a LOG(ERROR). It is an > uncommon case, but it's not really broken from the user's perspective--they may > have some very good (or bad, but still their choice) reason to delete the > default downloads directory, and I don't think we want to dump stuff to the > error output on release builds because of it. It's rarely a good idea to do > non-VLOG release build logging. Done. http://codereview.chromium.org/6973052/diff/27017/chrome/browser/download/sav... File chrome/browser/download/save_page_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/27017/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:249: // The user's "Downloads" folder exists. I added a new test, SavePageBrowserTest.SavedFolder4. Also, I added a new test, DownloadTest.DownloadedFolder2, which checks the similar thing for downloading files. http://codereview.chromium.org/6973052/diff/27017/chrome/browser/ui/download/... File chrome/browser/ui/download/download_tab_helper.cc (left): http://codereview.chromium.org/6973052/diff/27017/chrome/browser/ui/download/... chrome/browser/ui/download/download_tab_helper.cc:59: // file name sanitation and extension / mime checking. Oops. I restored this comment download_tab_helper.h. http://codereview.chromium.org/6973052/diff/27017/chrome/browser/ui/download/... File chrome/browser/ui/download/download_tab_helper.cc (right): http://codereview.chromium.org/6973052/diff/27017/chrome/browser/ui/download/... chrome/browser/ui/download/download_tab_helper.cc:123: prefs->SetInteger(prefs::kSaveFileType, save_type); Makes sense. Done. http://codereview.chromium.org/6973052/diff/27017/chrome/browser/ui/download/... File chrome/browser/ui/download/download_tab_helper.h (right): http://codereview.chromium.org/6973052/diff/27017/chrome/browser/ui/download/... chrome/browser/ui/download/download_tab_helper.h:56: string16 SavePageWithoutDialog(); I wrote the difference here and download_tab_helper.cc (about implementation details). Also, I changed the method name to SavePageBasedOnDefaultPrefs().
We should really avoid modifying PathService. If there are fallbacks on the directories both ways, please introduce another layer of indirection, so that we can just flip some value in chrome/browser/downloads (that'd say "don't ask PathService, here's the path you should use"). There should be no modifications to the PathService at all.
On 2011/06/03 09:12:47, Paweł Hajdan Jr. wrote: > We should really avoid modifying PathService. If there are fallbacks on the > directories both ways, please introduce another layer of indirection, so that we > can just flip some value in chrome/browser/downloads (that'd say "don't ask > PathService, here's the path you should use"). There should be no modifications > to the PathService at all. What's wrong with using Override and then deleting the directory that was created? I'd imagine that's what Override is *for*. I have a bias against adding yet more layers of indirection to an already indirection ladden stack (adding another layer of indirection to test all the layers of indirection about it--joy) but I'll go along if you really really don't want Override used. But tell me why so that I don't continue to go through life confused :-}.
On 2011/06/03 16:21:02, rdsmith wrote: > What's wrong with using Override and then deleting the directory that was > created? I'd imagine that's what Override is *for*. I'd love to remove ::Override. It's quite tricky. I hit many problems with http://codereview.chromium.org/2805100 (it has been reverted once or twice). > I have a bias against > adding yet more layers of indirection to an already indirection ladden stack > (adding another layer of indirection to test all the layers of indirection about > it--joy) but I'll go along if you really really don't want Override used. But > tell me why so that I don't continue to go through life confused :-}. Right, agreed that the "another layer" could possibly make it more complicated. But it _might_ make it simpler in the future, I have some ideas. In my opinion it's still going to be easier to debug than Override. Another note - we're in a separate process here, so most of the arguments against Override do not apply. However, I'd like to discourage its use in all code, possibly allowing removal of ::Override in the future, and also making it less likely that someone will misuse Override in code that doesn't run out-of-process.
Makes sense. I withdraw my question. On 2011/06/03 17:44:27, Paweł Hajdan Jr. wrote: > On 2011/06/03 16:21:02, rdsmith wrote: > > What's wrong with using Override and then deleting the directory that was > > created? I'd imagine that's what Override is *for*. > > I'd love to remove ::Override. It's quite tricky. I hit many problems with > http://codereview.chromium.org/2805100 (it has been reverted once or twice). > > > I have a bias against > > adding yet more layers of indirection to an already indirection ladden stack > > (adding another layer of indirection to test all the layers of indirection > about > > it--joy) but I'll go along if you really really don't want Override used. But > > tell me why so that I don't continue to go through life confused :-}. > > Right, agreed that the "another layer" could possibly make it more complicated. > But it _might_ make it simpler in the future, I have some ideas. In my opinion > it's still going to be easier to debug than Override. > > Another note - we're in a separate process here, so most of the arguments > against Override do not apply. However, I'd like to discourage its use in all > code, possibly allowing removal of ::Override in the future, and also making it > less likely that someone will misuse Override in code that doesn't run > out-of-process.
I am not sure where we should add the indirection layer for PathService, but I added PathServiceWrapper to download_util.cc, which provides static methods for overriding the paths provided by PathService. Also, I added the test for PathServiceWrapper as DownloadUtilTest.OverridePathService. I modified save_page_browsertest.cc and download_browsertest.cc so that they use PathServiceWrapper.
Publishing comments on PS10; will take a look at PS11 soon and publish comments no that. I'd offhand rather have something that was downloads system specific than was called PathServiceWrapper (as the latter implies that it's of general utility, and I have no interest in wrapping all uses of PathService) but I haven't yet looked at the code. I'm hoping Pawel will suggest a cleaner solution to allow this testing :-}. http://codereview.chromium.org/6973052/diff/33001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/33001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:555: expected_suggested_path)); I'd suggest (i.e. I don't feel very strongly) that you expand the contents of CheckSelectFileDialogState out here inline into a couple of separate EXPECT_* calls. I have two reasons for the suggestion: * If a function's only used in one place and I don't see likely future uses of it elsewhere, it can produce simpler code to have all the code in the same place. In this case I think it does, since you don't need to remember/know the definition of CheckSelectFileDialogState. * Multiple EXPECT statements mean that if there's a test failure, you get a narrower pointer to what actually failed in the failure status. http://codereview.chromium.org/6973052/diff/33001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:562: const FilePath& expected_suggested_path) { I'm inclined to say you should make a new variant of DownloadAndWait rather than expand the current one. Most of the calls to this just get a null FilePath(), so for them it's just extra work. DownloadAndWaitWithDisposition() is complex enough so duplicating it for caller convenience probably doesn't make sense, but my judgement goes the other way for DownloadAndWait().
I'd suggest using download_util.cc GetDefaultDownloadsDirectory and finding a way to override it. Make sure to put warnings in about the override can only be used on contexts (e.g. testing) in which you're sure you won't be racing on other threads with the reading or other overriding.
> I'd suggest using download_util.cc GetDefaultDownloadsDirectory and finding a > way to override it. Make sure to put warnings in about the override can only be > used on contexts (e.g. testing) in which you're sure you won't be racing on > other threads with the reading or other overriding. I added the functionality of overriding a default download folder to class DefaultDownloadDirectory. I also wrote the warnings on download_util.h. http://codereview.chromium.org/6973052/diff/33001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/33001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:555: expected_suggested_path)); On 2011/06/06 15:31:29, rdsmith wrote: > I'd suggest (i.e. I don't feel very strongly) that you expand the contents of > CheckSelectFileDialogState out here inline into a couple of separate EXPECT_* > calls. I have two reasons for the suggestion: > * If a function's only used in one place and I don't see likely future uses of > it elsewhere, it can produce simpler code to have all the code in the same > place. In this case I think it does, since you don't need to remember/know the > definition of CheckSelectFileDialogState. > * Multiple EXPECT statements mean that if there's a test failure, you get a > narrower pointer to what actually failed in the failure status. Done. http://codereview.chromium.org/6973052/diff/33001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:562: const FilePath& expected_suggested_path) { I made DownloadAndWaitWithoutDialog() and DownloadAndWaitWithDialog().
http://codereview.chromium.org/6973052/diff/42001/base/path_service.h File base/path_service.h (right): http://codereview.chromium.org/6973052/diff/42001/base/path_service.h#newcode33 base/path_service.h:33: static void Set(int key, const FilePath& path); Please remove all modifications to base/ http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:841: download_util::DefaultDownloadDirectory::UnOverride(); I think this should be handled by a scoped object (the ASSERTs can fail and so on). http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:388: if (!file_util::CreateDirectory(state.suggested_path.DirName())) nit: Please use braces {} for multi-line "if" body. http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:389: LOG(ERROR) << "Failed to create " << Logging the error is fine, but shouldn't we exit at this point and propagate the error? I'd like to avoid downloads mysteriously failing with the only error message being in the logs. http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... File chrome/browser/download/download_util.cc (right): http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... chrome/browser/download/download_util.cc:221: void DefaultDownloadDirectory::Override(const FilePath& override_path) { Now the problem is that we have prefs::kDownloadDefaultDirectory that should be updated too. How about moving this entire logic to DownloadPrefs? I that we generally avoid adding new code to download_util. http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/sav... File chrome/browser/download/save_package.cc (right): http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1273: void SavePackage::CreateDirectoryOnFileThread( Interesting, this code seems much duplicated between the save package and downloaded file. Is it possible to unify those two, or share as much as possible?
http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:551: void DownloadAndWaitWithoutDialog(Browser* browser, const GURL& url) { I think that this is the common case, so I'd leave it named DownloadAndWait() for simplicity of test writing (and to some extent reading). http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:389: LOG(ERROR) << "Failed to create " << On 2011/06/08 09:50:23, Paweł Hajdan Jr. wrote: > Logging the error is fine, but shouldn't we exit at this point and propagate the > error? I'd like to avoid downloads mysteriously failing with the only error > message being in the logs. I'm inclined to think that the right behavior is to make sure that the user is prompted for the file location and not worry about it beyond that. Ideally (and maybe actually, based on the file selection dialog on the different systems) the file selection dialog won't allow the user to pick a location that can't be written to. This is a corner case, and seeding the UI with a bad location doesn't strike me as a horrible way to deal, given that we tried really hard and couldn't find a good location. http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... File chrome/browser/download/download_util.cc (left): http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... chrome/browser/download/download_util.cc:212: NOTREACHED(); This may generate disagreement from you or Pawel, but I'd rather leave in the NOTREACHED()'s and not return success/failure (i.e. assert that we'll always succeed). Otherwise we have to figure out ways to propagate an error up for an event we believe will never happen. http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... File chrome/browser/download/download_util.cc (right): http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... chrome/browser/download/download_util.cc:221: void DefaultDownloadDirectory::Override(const FilePath& override_path) { On 2011/06/08 09:50:23, Paweł Hajdan Jr. wrote: > Now the problem is that we have prefs::kDownloadDefaultDirectory that should be > updated too. How about moving this entire logic to DownloadPrefs? I that we > generally avoid adding new code to download_util. I agree about not adding more logic to download_util.cc, but DownloadPrefs isn't right--DownloadPrefs is just a cache/layer on top of the preferences service, and adding stuff unrelated to the preferences service to it would just pollute the interface. Maybe just move DefaultDownloadDirectory to its own file?
http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... File chrome/browser/download/download_util.cc (left): http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... chrome/browser/download/download_util.cc:212: NOTREACHED(); On 2011/06/08 22:31:18, rdsmith wrote: > This may generate disagreement from you or Pawel, but I'd rather leave in the > NOTREACHED()'s and not return success/failure (i.e. assert that we'll always > succeed). Otherwise we have to figure out ways to propagate an error up for an > event we believe will never happen. I'm fine with the NOTREACHED in that case. http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... File chrome/browser/download/download_util.cc (right): http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... chrome/browser/download/download_util.cc:221: void DefaultDownloadDirectory::Override(const FilePath& override_path) { On 2011/06/08 22:31:18, rdsmith wrote: > On 2011/06/08 09:50:23, Paweł Hajdan Jr. wrote: > > Now the problem is that we have prefs::kDownloadDefaultDirectory that should > be > > updated too. How about moving this entire logic to DownloadPrefs? I that we > > generally avoid adding new code to download_util. > > I agree about not adding more logic to download_util.cc, but DownloadPrefs isn't > right--DownloadPrefs is just a cache/layer on top of the preferences service, > and adding stuff unrelated to the preferences service to it would just pollute > the interface. Maybe just move DefaultDownloadDirectory to its own file? I added DownloadPrefs with that kind of code in mind too. Having a thin wrapper is not ideal, and having the tiniest pieces in their own files is not ideal either. My idea is that DownloadPrefs is for everything that deals with the prefs, and this _will_ deal with the prefs.
http://codereview.chromium.org/6973052/diff/42001/base/path_service.h File base/path_service.h (right): http://codereview.chromium.org/6973052/diff/42001/base/path_service.h#newcode33 base/path_service.h:33: static void Set(int key, const FilePath& path); On 2011/06/08 09:50:23, Paweł Hajdan Jr. wrote: > Please remove all modifications to base/ Done. http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:551: void DownloadAndWaitWithoutDialog(Browser* browser, const GURL& url) { On 2011/06/08 22:31:18, rdsmith wrote: > I think that this is the common case, so I'd leave it named DownloadAndWait() > for simplicity of test writing (and to some extent reading). Done. http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:841: download_util::DefaultDownloadDirectory::UnOverride(); Done. I created a new class ScopedDefaultDownloadDirectory and used a scoped object of the class. http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:388: if (!file_util::CreateDirectory(state.suggested_path.DirName())) On 2011/06/08 09:50:23, Paweł Hajdan Jr. wrote: > nit: Please use braces {} for multi-line "if" body. Done. http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:389: LOG(ERROR) << "Failed to create " << I would like to agree with Randy's idea, also considering the fact that we cannot anyway avoid the "downloads mysteriously failing" situation, since even if the download folder exists here, the download folder may be removed when a file is actually downloaded to the folder. In conclusion, I made no modification here for now. http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:414: path = dir.Append(file_name); I changed this dir.Append(file_name) to state.suggested_path.DirName().Append(file_name) in the latest patch. I think it is correct, but would you please look at it? http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... File chrome/browser/download/download_util.cc (left): http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... chrome/browser/download/download_util.cc:212: NOTREACHED(); Done. I left NOTREACHED() here and changed the signature of "bool DefaultDownloadDirectory::Get(FilePath* path)" to "const FilePath DefaultDownloadDirectory::Get()". http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... File chrome/browser/download/download_util.cc (right): http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... chrome/browser/download/download_util.cc:221: void DefaultDownloadDirectory::Override(const FilePath& override_path) { > My idea is that DownloadPrefs is for everything that deals with the prefs, and > this _will_ deal with the prefs. I am sorry but I am a bit confused. First of all, my understanding is that user's preferences and PathService are entirely different things. So, in the beginning, I cannot see why we have to update prefs::kDownloadDefaultDirectory when we want to update the user's "Downloads" folder provided by PathService. I guess that an overriding function for user's preferences and an overriding function for PathService should be prepared separately. Currently, the former is PrefService::SetFilePath() and the latter is DefaultDownloadDirectory::Override(). Would you please tell me your idea more in detail? (I made no modification yet for now. ) http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/sav... File chrome/browser/download/save_package.cc (right): http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1273: void SavePackage::CreateDirectoryOnFileThread( I collected up the duplicated code into ChooseSavableDirectory() in download_util.cc, although I am not sure whether download_util.cc is a good place or not. If it is not suitable, please tell me the right place.
I'm sorry if some of my comments or ideas are still unclear. We can schedule a chat to resolve it more quickly. http://codereview.chromium.org/6973052/diff/51001/chrome/browser/download/dow... File chrome/browser/download/download_util.cc (right): http://codereview.chromium.org/6973052/diff/51001/chrome/browser/download/dow... chrome/browser/download/download_util.cc:215: DefaultDownloadDirectory::override_path_ = override_path; We should still update the pref here, as said before. It has a default value in DownloadPrefs based on DefaultDownloadDirectory::Get, and we should keep those in sync to avoid weirdness. http://codereview.chromium.org/6973052/diff/51001/chrome/browser/download/dow... File chrome/browser/download/download_util.h (right): http://codereview.chromium.org/6973052/diff/51001/chrome/browser/download/dow... chrome/browser/download/download_util.h:58: // WARNING: In order to override the default download folder, we must use nit: Please remove the part about PathService::Override, it's really weird to list things we shouldn't be using here. There are lots of them. http://codereview.chromium.org/6973052/diff/51001/chrome/browser/download/dow... chrome/browser/download/download_util.h:62: class ScopedDefaultDownloadDirectory { Could you move this class to a special file only for testing? http://codereview.chromium.org/6973052/diff/51001/chrome/browser/download/dow... chrome/browser/download/download_util.h:70: void Override(const FilePath& override_path); I think the ctor should take the FilePath parameter, and that there should be no other public methods. Much harder to misuse. http://codereview.chromium.org/6973052/diff/51001/chrome/browser/download/dow... chrome/browser/download/download_util.h:74: }; And DISALLOW_COPY_AND_ASSIGN please.
Getting closer. Sorry this is taking so long, haraken .... http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... File chrome/browser/download/download_util.cc (right): http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... chrome/browser/download/download_util.cc:221: void DefaultDownloadDirectory::Override(const FilePath& override_path) { On 2011/06/09 09:07:45, Paweł Hajdan Jr. wrote: > On 2011/06/08 22:31:18, rdsmith wrote: > > On 2011/06/08 09:50:23, Paweł Hajdan Jr. wrote: > > > Now the problem is that we have prefs::kDownloadDefaultDirectory that should > > be > > > updated too. How about moving this entire logic to DownloadPrefs? I that we > > > generally avoid adding new code to download_util. > > > > I agree about not adding more logic to download_util.cc, but DownloadPrefs > isn't > > right--DownloadPrefs is just a cache/layer on top of the preferences service, > > and adding stuff unrelated to the preferences service to it would just pollute > > the interface. Maybe just move DefaultDownloadDirectory to its own file? > > I added DownloadPrefs with that kind of code in mind too. Having a thin wrapper > is not ideal, and having the tiniest pieces in their own files is not ideal > either. > > My idea is that DownloadPrefs is for everything that deals with the prefs, and > this _will_ deal with the prefs. Ok, I'm willing to accept this. It's not ideal, but it's ok, and I don't see a better plan (sorry about the tepid endorsement :-}). http://codereview.chromium.org/6973052/diff/51001/chrome/browser/download/dow... File chrome/browser/download/download_util.h (right): http://codereview.chromium.org/6973052/diff/51001/chrome/browser/download/dow... chrome/browser/download/download_util.h:73: void UnOverride(); I'd get rid of Override and UnOverride and just have the constructor and destructor do work--having the *Overrides seems like a more complicated interface than you need. http://codereview.chromium.org/6973052/diff/51001/chrome/browser/download/dow... chrome/browser/download/download_util.h:76: class DefaultDownloadDirectory { This can be used from multiple threads *unless* ScopedDefaultDownloadDirectory is being used, in which case it's only valid to use it on the same thread as ScopedDefaultDownloadDirectory (or otherwise be certain that you're not racing). That's a fairly complex and non-obvious thread safety restriction. Could you either document it here or (preferred) make sure that this function is only called on one thread and document that that's it's thread? http://codereview.chromium.org/6973052/diff/51001/chrome/browser/download/dow... chrome/browser/download/download_util.h:96: FilePath* save_dir); I'm not sure I see a reason for this to be in download_util, and if we can have it somewhere else, that's my preference. I'd suggest making it a static method on DownloadManager if it's needed outside of DownloadManager, and a static function in download_manager.cc if it's not. http://codereview.chromium.org/6973052/diff/51001/chrome/browser/download/sav... File chrome/browser/download/save_package.cc (right): http://codereview.chromium.org/6973052/diff/51001/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1282: download_util::DefaultDownloadDirectory::Get(); (Reference to other comment.) This is the current location in this CL on which DefaultDownloadsDirectory::Get() is referenced off the UI thread. Maybe reference it on the UI thread and pass it across in the PostTask?
Thank you very much for the long discussion. I am learning much from this review. http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... File chrome/browser/download/download_util.cc (right): http://codereview.chromium.org/6973052/diff/42001/chrome/browser/download/dow... chrome/browser/download/download_util.cc:221: void DefaultDownloadDirectory::Override(const FilePath& override_path) { I moved the functionality of DefaultDownloadDirectory into DownloadPrefs as follows: - DownloadPrefs::GetDefaultDownloadDirectory() : Returns the user's default "Downloads" folder that may be being overridden. - DownloadPrefs::OverrideDefaultDownloadDirectory() : Overrides the user's default "Downloads" folder. - DownloadPrefs::UnOverrideDefaultDownloadDirectory() : Invalidates the overriding. In particular, DownloadPrefs::OverrideDefaultDownloadDirectory() updates not only the path provided by PathService but also the path managed by PrefService and |download_path_|. http://codereview.chromium.org/6973052/diff/51001/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6973052/diff/51001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:377: FilePath(), download_save_dir, default_downloads_dir, &save_dir); Note: This was wrong... If |state.prompt_user_for_save_location| is true before here, we must not overwrite it here. http://codereview.chromium.org/6973052/diff/51001/chrome/browser/download/dow... File chrome/browser/download/download_util.cc (right): http://codereview.chromium.org/6973052/diff/51001/chrome/browser/download/dow... chrome/browser/download/download_util.cc:215: DefaultDownloadDirectory::override_path_ = override_path; I see. Now, DownloadPrefs::OverrideDefaultDownloadDirectory() updates those values. http://codereview.chromium.org/6973052/diff/51001/chrome/browser/download/dow... File chrome/browser/download/download_util.h (right): http://codereview.chromium.org/6973052/diff/51001/chrome/browser/download/dow... chrome/browser/download/download_util.h:58: // WARNING: In order to override the default download folder, we must use On 2011/06/09 19:10:38, Paweł Hajdan Jr. wrote: > nit: Please remove the part about PathService::Override, it's really weird to > list things we shouldn't be using here. There are lots of them. Done. http://codereview.chromium.org/6973052/diff/51001/chrome/browser/download/dow... chrome/browser/download/download_util.h:62: class ScopedDefaultDownloadDirectory { I moved it to download_test_util.h, download_test_util.cc and download_test_util_unittest.cc. http://codereview.chromium.org/6973052/diff/51001/chrome/browser/download/dow... chrome/browser/download/download_util.h:70: void Override(const FilePath& override_path); Done. Now, ScopedDefaultDownloadDirectory has only a constructor and a destructor. http://codereview.chromium.org/6973052/diff/51001/chrome/browser/download/dow... chrome/browser/download/download_util.h:73: void UnOverride(); On 2011/06/10 20:58:53, rdsmith wrote: > I'd get rid of Override and UnOverride and just have the constructor and > destructor do work--having the *Overrides seems like a more complicated > interface than you need. Done. http://codereview.chromium.org/6973052/diff/51001/chrome/browser/download/dow... chrome/browser/download/download_util.h:74: }; On 2011/06/09 19:10:38, Paweł Hajdan Jr. wrote: > And DISALLOW_COPY_AND_ASSIGN please. Done. http://codereview.chromium.org/6973052/diff/51001/chrome/browser/download/dow... chrome/browser/download/download_util.h:76: class DefaultDownloadDirectory { - I wrote "DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI))" at the head of DownloadPrefs::OverrideDefaultDownloadDirectory() and DownloadPrefs::UnOverrideDefaultDownloadDirectory(). - I added a comment in download_prefs.h that says those methods can be used only through ScopedDefaultDownloadDirectory. - I added a comment in download_test_util.h that says ScopedDefaultDownloadDirectory can be used only from the UI thread. http://codereview.chromium.org/6973052/diff/51001/chrome/browser/download/dow... chrome/browser/download/download_util.h:96: FilePath* save_dir); Done. I made it a static method of DownloadManager, since SavePackage::CreateDirectoryOnFileThread requires the method. http://codereview.chromium.org/6973052/diff/51001/chrome/browser/download/sav... File chrome/browser/download/save_package.cc (right): http://codereview.chromium.org/6973052/diff/51001/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1282: download_util::DefaultDownloadDirectory::Get(); On 2011/06/10 20:58:53, rdsmith wrote: > (Reference to other comment.) This is the current location in this CL on which > DefaultDownloadsDirectory::Get() is referenced off the UI thread. Maybe > reference it on the UI thread and pass it across in the PostTask? Done. http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:847: IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadFolder2) { This test fails in this patch. Please see my comment in SavePageBrowserTest.SaveFolder4. http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/sav... File chrome/browser/download/save_page_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:393: IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveFolder4) { IMPORTANT: This test fails in this patch by the following reason. A downloaded file is renamed .org.chromium.xxxx -> yyyy.crdownload -> yyyy, in this order. The problem here is where .org.chromium.xxxx is created. In the previous patch, .org.chromium.xxxx is created on the user's "Downloads" folder provided by PathService. Specifically, SaveFileManager::StartSave() calls BaseFile::Initialize(), and then BaseFile::Initialize() creates .org.chromium.xxxx on the "Downloads" folder provided by PathService, not on the "Downloads" folder being overridden by ScopedDefaultDownloadDirectory. Thus, this creation definitely succeeds even if the overridden "Downloads" folder does not exist (this is the thing that we are going to test in this test). However, in this patch, I modified so that SaveFileManager::StartSave() uses the folder provided by DownloadPrefs::GetDefaultDownloadDirectory(), which returns the overridden "Downloads" folder (if it is being overridden). In this way, SaveFileManager::StartSave() tries to create .org.chromium.xxxx on the non-existent overriden "Downloads" folder and thus it fails... I think that the modification that SaveFileManager::StartSave() uses the folder provided by DownloadPrefs::GetDefaultDownloadDirectory() is definitely right, because If we use the folder provided by PathService as before, we cannot test what we want to test here (i.e. what happens if the "Downloads" folder does not exist). Anyway, what is important is to decide what *should* happen if the "Downloads" folder does not exist. I guess that creating the "Downloads" folder and trying to create .org.chromium.xxxx on the created folder may be a good approach, but what do you think? The same problem happens in DownloadTest.DownloadFolder2, which also fails with this patch. (I feel that this patch becomes too large... So if possible, I would like to just remove SavePageBrowserTest.SaveFolder4 and DownloadTest.DownloadFolder2 from this patch, and then tackle this problem in detail as an another patch.)
> (I feel that this patch becomes too large... So if possible, I would like to > just remove SavePageBrowserTest.SaveFolder4 and DownloadTest.DownloadFolder2 > from this patch, and then tackle this problem in detail as an another patch.) Sorry, please ignore the above comment. Within this patch, we must handle the case where the user's "Downloads" folder does not exist when .org.chromium.xxxx is about to be created. Otherwise, Chromium will suddenly crash when it starts to save something without the user's "Downloads" folder.
http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/bas... File chrome/browser/download/base_file.cc (right): http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/bas... chrome/browser/download/base_file.cc:59: if (!file_util::CreateTemporaryFileInDir(save_path, &full_path_)) nit: Instead of having a nested "if", how about adding && !file_util::Create.... to the previous "if"? http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/bas... File chrome/browser/download/base_file.h (right): http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/bas... chrome/browser/download/base_file.h:36: bool Initialize(bool calculate_hash, const FilePath& save_path); nit: Please document |save_path|. http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/dow... chrome/browser/download/download_manager.h:262: static bool ChooseSavableDirectory(const FilePath& website_save_dir, Let's avoid having static methods in DownloadManager. Could you move this to download_util, or, if Randy doesn't like it, find a different place for it? http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/dow... File chrome/browser/download/download_prefs.cc (right): http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/dow... chrome/browser/download/download_prefs.cc:168: download_path_.SetValue(override_path); Aren't those two lines (SetValue and SetFilePath) modifying the same pref? I think just SetValue should be sufficient. http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/dow... chrome/browser/download/download_prefs.cc:178: bool overridden = !override_path_.empty(); I think we can always assume (DCHECK if needed) that when UnOverride is called override_path_ is not empty. http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/dow... chrome/browser/download/download_prefs.cc:181: if (overridden) { For symmetry, shouldn't we only reset the pref to default if currently it == override_path_ ? http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/dow... File chrome/browser/download/download_prefs.h (right): http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/dow... chrome/browser/download/download_prefs.h:13: #include "chrome/browser/download/download_test_util.h" This #include shouldn't be here. I don't think it's needed. http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/dow... chrome/browser/download/download_prefs.h:70: FilePath override_path_; nit: override_default_download_directory_? "path" is too vague.
I wrote: > http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/sav... > File chrome/browser/download/save_page_browsertest.cc (right): > > http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/sav... > chrome/browser/download/save_page_browsertest.cc:393: > IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveFolder4) { > IMPORTANT: This test fails in this patch by the following reason. > > A downloaded file is renamed .org.chromium.xxxx -> yyyy.crdownload -> yyyy, in > this order. The problem here is where .org.chromium.xxxx is created. In the > previous patch, .org.chromium.xxxx is created on the user's "Downloads" folder > provided by PathService. Specifically, SaveFileManager::StartSave() calls > BaseFile::Initialize(), and then BaseFile::Initialize() creates > .org.chromium.xxxx on the "Downloads" folder provided by PathService, not on the > "Downloads" folder being overridden by ScopedDefaultDownloadDirectory. Thus, > this creation definitely succeeds even if the overridden "Downloads" folder does > not exist (this is the thing that we are going to test in this test). > > However, in this patch, I modified so that SaveFileManager::StartSave() uses the > folder provided by DownloadPrefs::GetDefaultDownloadDirectory(), which returns > the overridden "Downloads" folder (if it is being overridden). In this way, > SaveFileManager::StartSave() tries to create .org.chromium.xxxx on the > non-existent overriden "Downloads" folder and thus it fails... I think that the > modification that SaveFileManager::StartSave() uses the folder provided by > DownloadPrefs::GetDefaultDownloadDirectory() is definitely right, because If we > use the folder provided by PathService as before, we cannot test what we want to > test here (i.e. what happens if the "Downloads" folder does not exist). > > Anyway, what is important is to decide what *should* happen if the "Downloads" > folder does not exist. I guess that creating the "Downloads" folder and trying > to create .org.chromium.xxxx on the created folder may be a good approach, but > what do you think? The same problem happens in DownloadTest.DownloadFolder2, > which also fails with this patch. Pawel: Do you have any idea about the above? I found your TODO comment in save_file_manager.cc. (This is the place where the above problem occurs.) save_file_manager.cc: 237: // TODO(phajdan.jr): We should check the return value and handle errors here. 238: // No need to calculate hash. 239: save_file->Initialize(false, info->default_download_dir); http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/bas... File chrome/browser/download/base_file.cc (right): http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/bas... chrome/browser/download/base_file.cc:59: if (!file_util::CreateTemporaryFileInDir(save_path, &full_path_)) On 2011/06/15 09:31:33, Paweł Hajdan Jr. wrote: > nit: Instead of having a nested "if", how about adding && !file_util::Create.... > to the previous "if"? Done. http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/bas... File chrome/browser/download/base_file.h (right): http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/bas... chrome/browser/download/base_file.h:36: bool Initialize(bool calculate_hash, const FilePath& save_path); On 2011/06/15 09:31:33, Paweł Hajdan Jr. wrote: > nit: Please document |save_path|. Done. http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/dow... chrome/browser/download/download_manager.h:262: static bool ChooseSavableDirectory(const FilePath& website_save_dir, Actually, I also prefer download_util.cc to download_manager.cc, although I am not sure if download_util.cc is the best place. I would like to wait for Randy's opinion. (No change for now.) http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/dow... File chrome/browser/download/download_prefs.cc (right): http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/dow... chrome/browser/download/download_prefs.cc:168: download_path_.SetValue(override_path); Done. Just SetValue() is OK. http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/dow... chrome/browser/download/download_prefs.cc:178: bool overridden = !override_path_.empty(); On 2011/06/15 09:31:33, Paweł Hajdan Jr. wrote: > I think we can always assume (DCHECK if needed) that when UnOverride is called > override_path_ is not empty. Done. http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/dow... chrome/browser/download/download_prefs.cc:181: if (overridden) { Done, although I am not sure if it is the fix you intended. (Maybe, do you mean "prefs->RegisterFilePathPref(prefs::kDownloadDefaultDirectory, default_download_dir, PrefService::UNSYNCABLE_PREF);" here?) http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/dow... File chrome/browser/download/download_prefs.h (right): http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/dow... chrome/browser/download/download_prefs.h:13: #include "chrome/browser/download/download_test_util.h" Done. Instead, I added "namespace download_test_util {class ScopedDefaultDownloadDirectory;}". http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/dow... chrome/browser/download/download_prefs.h:70: FilePath override_path_; I renamed it to |override_default_download_dir_|.
High level comment: I'm struggling with the effects of putting in the shim rather than using PathService. By my count, there are now eight functions (BaseFile::Initialize, DownloadFileManager::CreateDownloadFile, DownloadManager::CheckIfSuggestedPathExists, SaveFileManager::OnSaveURL, ResourceDispatcherHost::BeginSaveFile, SavePackage::CreateDirectoryOnFileThread, SaveFileCreateInfo, SaveFileResourceHandle) scattered through the system that take another argument, which is the default download folder that is usually what we get from pathservice, all in the name of testing. This is at least partially because of my request that we make GetDefaultDownloadsDirectory() thread-specific, but wow did it have effects beyond what I expected when I made the suggestion. I'd like to at least explore some different alternatives. haraken, Pawel, what do you think of the following options: * Don't test the final fallback code path of the default downloads directory being non-existent or non-writeable (or test it manually). * Put the default downloads directory (and the override) on the File thread (which I believe will substantially reduce the number of functions that need an extra argument, but please correct me if this isn't the case). I could be convinced to accept the current proposal, but it makes me uncomfortable and I'd like to find a better way. Other comments follow. http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/dow... chrome/browser/download/download_manager.h:262: static bool ChooseSavableDirectory(const FilePath& website_save_dir, On 2011/06/15 09:31:33, Paweł Hajdan Jr. wrote: > Let's avoid having static methods in DownloadManager. Could you move this to > download_util, or, if Randy doesn't like it, find a different place for it? Pawel: Why do you prefer not to have static methods on DownloadManager? The argument I know of against static methods is that they can't be mocked by inheriting the class--I think that's a valid argument, but if we're comparing download_util.cc to putting it on DownloadManager, it doesn't apply. So I presume there's some argument against static methods I'm missing? I feel like DownloadManager is the right place for this functionality...? http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/sav... File chrome/browser/download/save_page_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:393: IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveFolder4) { On 2011/06/14 11:10:05, haraken wrote: > IMPORTANT: This test fails in this patch by the following reason. > > A downloaded file is renamed .org.chromium.xxxx -> yyyy.crdownload -> yyyy, in > this order. The problem here is where .org.chromium.xxxx is created. In the > previous patch, .org.chromium.xxxx is created on the user's "Downloads" folder > provided by PathService. Specifically, SaveFileManager::StartSave() calls > BaseFile::Initialize(), and then BaseFile::Initialize() creates > .org.chromium.xxxx on the "Downloads" folder provided by PathService, not on the > "Downloads" folder being overridden by ScopedDefaultDownloadDirectory. Thus, > this creation definitely succeeds even if the overridden "Downloads" folder does > not exist (this is the thing that we are going to test in this test). > > However, in this patch, I modified so that SaveFileManager::StartSave() uses the > folder provided by DownloadPrefs::GetDefaultDownloadDirectory(), which returns > the overridden "Downloads" folder (if it is being overridden). In this way, > SaveFileManager::StartSave() tries to create .org.chromium.xxxx on the > non-existent overriden "Downloads" folder and thus it fails... I think that the > modification that SaveFileManager::StartSave() uses the folder provided by > DownloadPrefs::GetDefaultDownloadDirectory() is definitely right, because If we > use the folder provided by PathService as before, we cannot test what we want to > test here (i.e. what happens if the "Downloads" folder does not exist). > > Anyway, what is important is to decide what *should* happen if the "Downloads" > folder does not exist. I guess that creating the "Downloads" folder and trying > to create .org.chromium.xxxx on the created folder may be a good approach, but > what do you think? The same problem happens in DownloadTest.DownloadFolder2, > which also fails with this patch. I'm torn. That's fine behavior, but there will be circumstances under which we cannot create that directory, so we'll need to handle that case as well. And I think the case where the default downloads directory doesn't exist is rare enough that I don't want to add yet another conditional in the code to try to create it and then surface the error if we don't. So my inclination is to say that we should fail the download/save page in this situation. Ideally we'd report that error back to the user, but if there's not an easy way to do that I'd be ok with filing a bug indicating that we need to find a way to report the error back to the user and fail the download/save page in a not very informative way. http://codereview.chromium.org/6973052/diff/67001/chrome/browser/download/dow... File chrome/browser/download/download_manager_unittest.cc (right): http://codereview.chromium.org/6973052/diff/67001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:127: true, }, It looks like we no longer have any cases in which we're not expecting a save_as dialog; why is that? I'm sure it's something obvious I'm not getting, but I wouldn't have thought this patch would change that behavior, and the fact that we have argues that there's a code path we used to test that we're not testing anymore. Can you enlighten me? http://codereview.chromium.org/6973052/diff/67001/chrome/browser/download/dow... File chrome/browser/download/download_prefs.cc (right): http://codereview.chromium.org/6973052/diff/67001/chrome/browser/download/dow... chrome/browser/download/download_prefs.cc:162: void DownloadPrefs::OverrideDefaultDownloadDirectory( I wince a bit at the behavior of these functions when you have nested instances of ScopedDefaultDownloadDirectory--it basically doesn't work (the external instance will have the original value, not the new value, after destruction of the internal instance). There's no need for nested functionality, but could you comment indicating that it doesn't work? (Or, if you feel moved, make it work by creating a linked list/stack of ScopedDefaultDownloadDirectories that remove themselves on destruction and returning the most recent one that was pushed on the stack. It's overkill, and you don't need to, it's just the behavior the current interface implies to me.)
Randy: I think you're right about the added complexity for testing. I think we should avoid it. I'm fine with not covering some paths with automated tests if that'd simplify the normal code. I'm not sure what do you mean by "Put the default downloads directory (and the override) on the File thread". If I haven't answered some remaining questions, I defer to Randy. I have no idea about the test failures. http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/dow... chrome/browser/download/download_manager.h:262: static bool ChooseSavableDirectory(const FilePath& website_save_dir, On 2011/06/15 19:48:59, rdsmith wrote: > On 2011/06/15 09:31:33, Paweł Hajdan Jr. wrote: > > Let's avoid having static methods in DownloadManager. Could you move this to > > download_util, or, if Randy doesn't like it, find a different place for it? > > Pawel: Why do you prefer not to have static methods on DownloadManager? The > argument I know of against static methods is that they can't be mocked by > inheriting the class--I think that's a valid argument, but if we're comparing > download_util.cc to putting it on DownloadManager, it doesn't apply. So I > presume there's some argument against static methods I'm missing? I feel like > DownloadManager is the right place for this functionality...? No, actually I'm fine with static methods per se. However, I'd like to keep any code that doesn't _have_ to be in DownloadManager out of DownloadManager. The class is unfortunately named ("Manager"), and there is a tendency to put anything related to downloads inside it. http://codereview.chromium.org/6973052/diff/67001/chrome/browser/download/dow... File chrome/browser/download/download_prefs.cc (right): http://codereview.chromium.org/6973052/diff/67001/chrome/browser/download/dow... chrome/browser/download/download_prefs.cc:162: void DownloadPrefs::OverrideDefaultDownloadDirectory( On 2011/06/15 19:48:59, rdsmith wrote: > I wince a bit at the behavior of these functions when you have nested instances > of ScopedDefaultDownloadDirectory--it basically doesn't work (the external > instance will have the original value, not the new value, after destruction of > the internal instance). There's no need for nested functionality, but could you > comment indicating that it doesn't work? (Or, if you feel moved, make it work > by creating a linked list/stack of ScopedDefaultDownloadDirectories that remove > themselves on destruction and returning the most recent one that was pushed on > the stack. It's overkill, and you don't need to, it's just the behavior the > current interface implies to me.) A linked list/stack just for this is obviously an overkill. However, I think we should prevent potential misuse by signaling failure when someone tries to override an already overridden directory.
On 2011/06/16 17:41:31, Paweł Hajdan Jr. wrote:
> Randy: I think you're right about the added complexity for testing. I think we
> should avoid it.
>
> I'm fine with not covering some paths with automated tests if that'd simplify
> the normal code.
>
> I'm not sure what do you mean by "Put the default downloads directory (and the
> override) on the File thread".
The complexity in the current code is mostly getting the value to where it's
used, and it's usually used on the file thread. So if we made the class/method
that provides/changes the default value only accessible on the file thread, that
would probably reduce the code complexity. But I'm also good with just leaving
that out of automated tests (but I would like to see it tested manually in that
case).
> If I haven't answered some remaining questions, I defer to Randy. I have no
idea
> about the test failures.
I think I've responded to those questions, but if I haven't, please ping me
again.
> > Pawel: Why do you prefer not to have static methods on DownloadManager? The
> > argument I know of against static methods is that they can't be mocked by
> > inheriting the class--I think that's a valid argument, but if we're
comparing
> > download_util.cc to putting it on DownloadManager, it doesn't apply. So I
> > presume there's some argument against static methods I'm missing? I feel
like
> > DownloadManager is the right place for this functionality...?
>
> No, actually I'm fine with static methods per se. However, I'd like to keep
any
> code that doesn't _have_ to be in DownloadManager out of DownloadManager. The
> class is unfortunately named ("Manager"), and there is a tendency to put
> anything related to downloads inside it.
Ah, ok. You're quite right, that's why I was suggesting putting it there. For
now, I'll accept download_util, and think in the back of my mind how I want to
conceptualize DownloadManager (Though "DownloadItem holder and
receiver/dispatcher of all DownloadItem targeted messages" does jump pretty
quickly to mind).
On 2011/06/16 18:11:00, rdsmith wrote: > The complexity in the current code is mostly getting the value to where it's > used, and it's usually used on the file thread. So if we made the class/method > that provides/changes the default value only accessible on the file thread, that > would probably reduce the code complexity. But I'm also good with just leaving > that out of automated tests (but I would like to see it tested manually in that > case). I see. I'm fine with both options, but after some thought leaving it out of automated testing and using manual testing sounds slightly better to me. > Ah, ok. You're quite right, that's why I was suggesting putting it there. For > now, I'll accept download_util, and think in the back of my mind how I want to > conceptualize DownloadManager (Though "DownloadItem holder and > receiver/dispatcher of all DownloadItem targeted messages" does jump pretty > quickly to mind). Agreed.
Randy wrote: > High level comment: I'm struggling with the effects of putting in the shim > rather than using PathService. By my count, there are now eight functions > (BaseFile::Initialize, DownloadFileManager::CreateDownloadFile, > DownloadManager::CheckIfSuggestedPathExists, SaveFileManager::OnSaveURL, > ResourceDispatcherHost::BeginSaveFile, SavePackage::CreateDirectoryOnFileThread, > SaveFileCreateInfo, SaveFileResourceHandle) scattered through the system that > take another argument, which is the default download folder that is usually what > we get from pathservice, all in the name of testing. This is at least partially > because of my request that we make GetDefaultDownloadsDirectory() > thread-specific, but wow did it have effects beyond what I expected when I made > the suggestion. I'd like to at least explore some different alternatives. > haraken, Pawel, what do you think of the following options: > > * Put the default downloads directory (and the override) on the File thread > (which I believe will substantially reduce the number of functions that need an > extra argument, but please correct me if this isn't the case). Randy wrote: > The complexity in the current code is mostly getting the value to where it's > used, and it's usually used on the file thread. So if we made the class/method > that provides/changes the default value only accessible on the file thread, that > would probably reduce the code complexity. But I'm also good with just leaving > that out of automated tests (but I would like to see it tested manually in that > case). I am sorry but I am a bit confused... Your ideas are as follows. Right? [Idea1] Make methods that provide and override the path of the user's "Downloads" folder. The methods must be accessible only by the FILE thread. [Idea2] Remove all the codes about overriding (i.e. revert this patch back to the previous code that uses PathService). Remove DownloadTest.DownloadFolder2 and SavePageBrowserTest.SaveFolder4. In other tests, just use the user's real "Downloads" directory (because we no longer have a way to override it). However, I am not sure how [Idea1] is possible. As Pawel indicated, we have to override not only the value provided by PathService but also the value of download preferences in order to make those values consistent. In order to override download preferences, we have to access the object of DownloadPrefs in a way like 'browser()->profile()->GetDownloadManager()->download_prefs()'. However, such kind of access to DownloadPrefs' object is allowed only by the UI thread, which means that only the UI thread can override the value. With this observation, I am not sure how we can make a method that overrides the user's "Downloads" folder by the FILE thread. (Just a comment: You may say that "there are now eight functions that take another argument (default_download_dir), all in the name of testing", but I feel that it may not be only for testing. Currently, BaseFile::Initialize() can create a file only under the user's "Downloads" folder, but are there any expectation that we may want to create a file of BaseFile under another folder (not for testing)? For exmaple, in the case where the user's "Downloads" folder does not exist and we fail to create it.) http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/dow... chrome/browser/download/download_manager.h:262: static bool ChooseSavableDirectory(const FilePath& website_save_dir, Randy: > Ah, ok. You're quite right, that's why I was suggesting putting it there. > For now, I'll accept download_util, and think in the back of my mind how I want to > conceptualize DownloadManager (Though "DownloadItem holder and > receiver/dispatcher of all DownloadItem targeted messages" does jump pretty > quickly to mind). In conclusion, I placed ChooseSavableDirectory() in download_util.cc. http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/sav... File chrome/browser/download/save_page_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/60001/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:393: IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveFolder4) { I removed SavePageBrowserTest.SaveFolder4 and DownloadTest.DownloadFolder2. http://codereview.chromium.org/6973052/diff/67001/chrome/browser/download/dow... File chrome/browser/download/download_manager_unittest.cc (right): http://codereview.chromium.org/6973052/diff/67001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:127: true, }, This change is required in this patch (if we need to test |expected_save_as|). This patch changes the download behavior so that the select file dialog appears if the default download folder does not exist. In DownloadManagerTest.StartDownload, since we set the default download folder to a non-existent folder by 'prefs->SetFilePath(prefs::kDownloadDefaultDirectory, FilePath())', the select file dialog always appears, whether |save_as| and |prompt_for_download| is true or false. On the other hand, in the previous code, we silently created the default download folder if it does not exist, and then determined whether we should show the select file dialog or not, depending on the value of |save_as| and |prompt_for_download|. However, anyway I agree that it is no longer useful to test |expected_save_as| because it must be always true. I would like to remove |expected_save_as| after hearing your opinion. http://codereview.chromium.org/6973052/diff/67001/chrome/browser/download/dow... File chrome/browser/download/download_prefs.cc (right): http://codereview.chromium.org/6973052/diff/67001/chrome/browser/download/dow... chrome/browser/download/download_prefs.cc:162: void DownloadPrefs::OverrideDefaultDownloadDirectory( I documented "Duplicate overriding is not allowed." at ScopedDefaultDownloadDirectory() in download_test_util.h. I also put DCHECK() here to check that the default download folder is not doubly overridden. http://codereview.chromium.org/6973052/diff/73001/chrome/browser/download/dow... File chrome/browser/download/download_item.cc (right): http://codereview.chromium.org/6973052/diff/73001/chrome/browser/download/dow... chrome/browser/download/download_item.cc:545: In fact, DownloadManagerTest.StartDownload fails at AssertQueueStateConsistent() for the following reason: (1) TEST_F(DownloadManagerTest, StartDownload) calls ~DownloadManagerTest() in the end. (2) ~DownloadManagerTest() calls DownloadManager::Shutdown(). (3) DownloadManager::Shutdown() calls DownloadItem::Delete() for (not all but) some downloads in the test. (4) DownloadItem::Delete() calls DownloadItem::Remove(). (5) In DownloadItem::Remove(), state_ is IN_PROGRESS but its download id is not yet registered to DownloadManager::in_progress_, because the registration is done only at DownloadManager::ContinueDownloadWithPath() but DownloadManager::ContinueDownloadWithPath() is never called in the scenario above. (6) DownloadManager::AssertQueueStateConsistent() is called and 'CHECK(ContainsKey(in_progress_, download->id()) == download->state() == DownloadItem::IN_PROGRESS))' in DownloadManager::AssertQueueStateConsistent() fails. The reason why the previous code does not cause this problem is that (fortunately) the previous DownloadManagerTest.StartDownload always called DownloadManager::ContinueDownloadWithPath() for all the downloads that entered the call path I described above. Here, whether DownloadManager::ContinueDownloadWithPath() is called or not is determined by kStartDownloadCases[i].expected_save_as in DownloadManagerTest.StartDownload. Since we had to change the value of kStartDownloadCases[i].expected_save_as from false to true (See the comments in download_manager_unittest.cc for more details for this change), this problem appeared.
Wow, this is turning into complex work based on a simple idea. To answer your questions, haraken: * You understood my ideas correctly, and I hadn't thought about the UI Thread<->DownloadPrefs()<->default download directory connection. That does make things harder. * There are indeed cases in which we don't want Initialize() to create the file in the DEFAULT_DOWNLOADS_DIRECTORY; in fact, we have a bug out on that (http://crbug.com/62099, which we may end up fixing by accident as part of this patch). You didn't remove any of the extra default directory arguments that I was concerned about; was that because you were waiting for confirming you understood what I meant, or does the need to pass something extra to Initialize() mean that we need all eight still? http://codereview.chromium.org/6973052/diff/67001/chrome/browser/download/dow... File chrome/browser/download/download_manager_unittest.cc (right): http://codereview.chromium.org/6973052/diff/67001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:127: true, }, On 2011/06/22 18:01:58, haraken wrote: > However, anyway I agree that it is no longer useful to test |expected_save_as| > because it must be always true. I would like to remove |expected_save_as| after > hearing your opinion. I still feel like we're not testing code that we used to test that's important to test (the logic for when to prompt or not when we have a directory to write to), and I'd like to go back to testing it. Based on your description, I think the issue is that we silently create non-existent directories for the default download directory. Could you change the call to set the default download directory to a ScopedTempDir and see if you can revert the expectations back to their original? http://codereview.chromium.org/6973052/diff/73001/chrome/browser/download/dow... File chrome/browser/download/download_item.cc (right): http://codereview.chromium.org/6973052/diff/73001/chrome/browser/download/dow... chrome/browser/download/download_item.cc:545: On 2011/06/22 18:01:58, haraken wrote: > In fact, DownloadManagerTest.StartDownload fails at AssertQueueStateConsistent() > for the following reason: > > (1) TEST_F(DownloadManagerTest, StartDownload) calls ~DownloadManagerTest() in > the end. > (2) ~DownloadManagerTest() calls DownloadManager::Shutdown(). > (3) DownloadManager::Shutdown() calls DownloadItem::Delete() for (not all but) > some downloads in the test. > (4) DownloadItem::Delete() calls DownloadItem::Remove(). > (5) In DownloadItem::Remove(), state_ is IN_PROGRESS but its download id is not > yet registered to DownloadManager::in_progress_, because the registration is > done only at DownloadManager::ContinueDownloadWithPath() but > DownloadManager::ContinueDownloadWithPath() is never called in the scenario > above. > (6) DownloadManager::AssertQueueStateConsistent() is called and > 'CHECK(ContainsKey(in_progress_, download->id()) == download->state() == > DownloadItem::IN_PROGRESS))' in DownloadManager::AssertQueueStateConsistent() > fails. > > The reason why the previous code does not cause this problem is that > (fortunately) the previous DownloadManagerTest.StartDownload always called > DownloadManager::ContinueDownloadWithPath() for all the downloads that entered > the call path I described above. Here, whether > DownloadManager::ContinueDownloadWithPath() is called or not is determined by > kStartDownloadCases[i].expected_save_as in DownloadManagerTest.StartDownload. > Since we had to change the value of kStartDownloadCases[i].expected_save_as from > false to true (See the comments in download_manager_unittest.cc for more details > for this change), this problem appeared. Based on your description, it sounds as if if we can change the test so that we can revert the expect_save_as change in download_manager_unittest.cc, this problem will go away, so I'll assume that that'll work until you tell me differently. Please do *not* remove the AssertQueueStateConsistent calls; they represent invariants in the download system during normal operation that we should make sure the tests respect.
> * You understood my ideas correctly, and I hadn't thought about the UI > Thread<->DownloadPrefs()<->default download directory connection. That does > make things harder. > > You didn't remove any of the extra default directory arguments that I was > concerned about; was that because you were waiting for confirming you understood > what I meant, or does the need to pass something extra to Initialize() mean that > we need all eight still? Sorry for the confusion. I have not yet changed the code because I would like to wait for our consensus. In summary, possible options seem to be the followings: [1] Remove all the codes about overriding (i.e. revert this patch back to the previous code that uses PathService). In tests, just use the user's real "Downloads" folder. Since we cannot override the user's "Downloads" folder from the FILE thread, if you do not want to allow those extra arguments, I think that the only possible way is just to remove all the codes about overriding. [2] Allow the extra default_download_dir arguments and leave the code as it is(, at least in this patch). Both options are OK for me. Waiting for your opinions. > * There are indeed cases in which we don't want Initialize() to create the file > in the DEFAULT_DOWNLOADS_DIRECTORY; in fact, we have a bug out on that > (http://crbug.com/62099, which we may end up fixing by accident as part of this > patch). Thank you for the information. I guess that whether we should allow the extra default_download_dir argument of BaseFile::Initialize() should be discussed as another patch, anyway. http://codereview.chromium.org/6973052/diff/67001/chrome/browser/download/dow... File chrome/browser/download/download_manager_unittest.cc (right): http://codereview.chromium.org/6973052/diff/67001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:127: true, }, Ah, it make a great sense to me. I set the default download folder to ScopedTempDir and then reverted this test back to the original. http://codereview.chromium.org/6973052/diff/73001/chrome/browser/download/dow... File chrome/browser/download/download_item.cc (right): http://codereview.chromium.org/6973052/diff/73001/chrome/browser/download/dow... chrome/browser/download/download_item.cc:545: I reverted this code back to the original, since I could revert DownloadManagerTest.StartDownload to the original and thus this problem disappeared. Just an out-of-curiosity question: Aren't there any practical cases that tracks the call path I described above and thus the invariant collapses? i.e.: (1) DownloadManager::Shutdown() is called in the condition that StartDownload() is already called but ContinueDownloadWithPath() is not yet called. (2) The download is dangerous and thus the DownloadManager::Shutdown() calls DownloadItem::Delete(). (3) The problem I described occurs.
On 2011/06/24 01:57:24, haraken wrote: > > * You understood my ideas correctly, and I hadn't thought about the UI > > Thread<->DownloadPrefs()<->default download directory connection. That does > > make things harder. > > > > You didn't remove any of the extra default directory arguments that I was > > concerned about; was that because you were waiting for confirming you > understood > > what I meant, or does the need to pass something extra to Initialize() mean > that > > we need all eight still? > > Sorry for the confusion. I have not yet changed the code because I would like to > wait for our consensus. > > In summary, possible options seem to be the followings: > > [1] Remove all the codes about overriding (i.e. revert this patch back to the > previous code that uses PathService). In tests, just use the user's real > "Downloads" folder. Since we cannot override the user's "Downloads" folder from > the FILE thread, if you do not want to allow those extra arguments, I think that > the only possible way is just to remove all the codes about overriding. > > [2] Allow the extra default_download_dir arguments and leave the code as it is(, > at least in this patch). I think I'd prefer option [1]. I'm tempted to explore breaking the link that Pawel requested between DownloadPrefs and the whether we get the DefaultDownloadDirectory from, but there may not be a good way to do that, and we've already gone down a pretty large rathole in this patch already. So can we go with option [1], with the caveat that if you find if necessary to keep a large chunk of the extra arguments anyway, I may want to explore other choices. > > Both options are OK for me. Waiting for your opinions. > > > > * There are indeed cases in which we don't want Initialize() to create the > file > > in the DEFAULT_DOWNLOADS_DIRECTORY; in fact, we have a bug out on that > > (http://crbug.com/62099, which we may end up fixing by accident as part of > this > > patch). > > Thank you for the information. I guess that whether we should allow the extra > default_download_dir argument of BaseFile::Initialize() should be discussed as > another patch, anyway. Yeah, that would be my inclination. > Just an out-of-curiosity question: Aren't there any practical cases that tracks > the call path I described above and thus the invariant collapses? i.e.: > > (1) DownloadManager::Shutdown() is called in the condition that StartDownload() > is already called but ContinueDownloadWithPath() is not yet called. > (2) The download is dangerous and thus the DownloadManager::Shutdown() calls > DownloadItem::Delete(). > (3) The problem I described occurs. That's quite true--Mea Culpa. This is partially because active_downloads_ and in_progress_ overlap in funcitonality, and we intend to delete in_progress_, but I shouldn't really have the in_progress_ <=> IN_PROGRESS check there. I'll fix it on my next round of changes to the invariant checks. (One of the points of AssertQueueStateConsistent() is to flush out areas of the downloads code that don't match our beliefs about invariants, so I don't feel too bad about this one. But you're still quite right.)
I removed the codes about overriding the user's "Downloads" folder. Would you please take a look at it?
I think this is good; LGTM. Pawel? http://codereview.chromium.org/6973052/diff/93001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/93001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:960: // Download the file and wait. We do not expect the select file dialog. nit: Given the change in signature, we don't need the second sentence of the comment anymore. Applies throughout file to DownloadAndWait calls.
Randy: Thank you very much! I would like to wait for Pawel's review. http://codereview.chromium.org/6973052/diff/93001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/93001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:960: // Download the file and wait. We do not expect the select file dialog. On 2011/06/28 17:32:44, rdsmith wrote: > nit: Given the change in signature, we don't need the second sentence of the > comment anymore. Applies throughout file to DownloadAndWait calls. Done.
Pawel: Would you please take a look?
haraken: Pawel's on break. I just did another full review with as good an attention to detail as I could muster, and I think you're good. Go ahead and commit on my LGTM.
Try job failure for 6973052-106001 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Sorry for the Windows build failure... I rebased with the latest repository and made a little change to save_page_browsertest.cc: - std::string => FilePath::StringType - Replace "if (browser()->SupportsWindowFeature(Browser::FEATURE_DOWNLOADSHELF)) EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());" with "CheckDownloadUI(full_file_name);" I confirmed that the build succeeds on all of Linux, Mac and Windows. Would you please take a look the change in save_page_browsertest.cc. On 2011/07/07 02:28:30, I haz the power (commit-bot) wrote: > Try job failure for 6973052-106001 (retry) on win for step "compile" (clobber > build). > It's a second try, previously, step "compile" failed. > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
LGTM.
Try job failure for 6973052-108001 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
I am sorry again... I fixed string conversions in save_page_browsertest.cc so that it passes Windows build. I confirmed that not only chrome but also browser_tests are built successfully at least in my Windows environment. On 2011/07/07 16:16:01, I haz the power (commit-bot) wrote: > Try job failure for 6973052-108001 (retry) on win for step "compile" (clobber > build). > It's a second try, previously, step "compile" failed. > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Change committed as 91861
The commit r91861 was reverted back because of the failure of browser_tests on Mac and Windows. I will re-commit after fixing it. On 2011/07/08 16:38:30, I haz the power (commit-bot) wrote: > Change committed as 91861
> The commit r91861 was reverted back because of the failure of browser_tests on > Mac and Windows. I will re-commit after fixing it. I guess that I could fix the problem, finally... I confirmed that the latest patch passes on the try-bot. The reason why the previous patch passed browser_tests in my local environment (all of Windows, Mac and Linux) but did not pass on the try-bot or build-bot is as follows: - The try-bot and build-bot was reporting the failure on DownloadTest.DownloadFolder. Specifically, when the test tried to download a file into the default download folder, the suggested path in the select file dialog did not correspond to the path we expect. I confirmed in the try-bot that if the test tried to download "foo.txt", then the suggested path became not "foo.txt" but "foo (23).txt" or so. - This is because "foo.txt.crdownload" existed at the default download folder of the try bot, maybe because an execution in the past crashed without deleting "foo.txt.crdownload". - In my local environment, the test was passing successfully since "foo.txt.crdownload" did not exist. Thus, I added the code in DownloadTest.DownloadFolder and SavePageBrowserTest.SaveFolder3 to delete xxxxx.crdownload before trying to download xxxxx into the default download folder. With this change, I confirmed that all tests pass on the try-bot. I do not yet confirm that the latest patch passes also on the build-bot, but I hope that it will work fine. (I am suspecting that the build-bot also has xxxxx.crdownload in his default download folder.) In addition, I rebased the patch with the latest revision. There seems to be a big change in download codes (e.g. remove TabContentsWrapper, move base_file.* and save_package.* from chrome/browser/ to content/browser/, etc...), and thus I had to manually merge codes here and there. Randy: Would you please take a look at it again? Thank you for your long long long support.
[If Pawel's got a moment to chime back in, I'd appreciate hearing from him with his "global tests health" hat on.] So I think this basically makes sense. The only problem with it is one that we all agreed on several patch sets back, but this surfaces more strongly: We're making a global change to the test environment in these tests in deleting existing files. For contrast, I think of best practices for tests to create temporary directories that will be deleted at the end of the tests, but we agreed that that would require very invasive changes to many parts of the system to do that for the final fallback location, and so decided not to do it. But this means that we may be deleting files in the final fallback location. Specifically, if we run browser tests in parallel (which Charles Lee, cc'd, is working on) and some other browser test is downloading files of the same name into that directory, they could step on each other. So I'd like to request a couple of changes to minimize the chances of this happening: * Change the tests you've written that use the PathService directory to use a different name than any other tests use. * Comment those tests at the top as changing global state and give a sketch as to why. * At the end of those tests, make sure to clean up the created files (just keeping things neat--I don't think this is necessary for reducing breakage). Over time we'll modify any tests that aren't using testing that final fallback directory (which I suspect will be any tests other than yours) to create a temporary directory and use that. That'll mean that the only possible collisions your test will have will be with itself, which is why I'm ccing Charles, since I want him to know that these tests colliding with themselves is a known issue that should be ok. Pawel, does this pass muster with you?
And also, thank you very much for persevering on this complex messy change in a section of the code that's undergoing a lot of transformation!
I took an extremely brief look, I guess it's fine.
> So I'd like to request a couple of changes to minimize the chances of this > happening: > * Change the tests you've written that use the PathService directory to use a > different name than any other tests use. > * Comment those tests at the top as changing global state and give a sketch as > to why. > * At the end of those tests, make sure to clean up the created files (just > keeping things neat--I don't think this is necessary for reducing breakage). I fixed them. Also, I rebased with the latest revision. Randy: would you please take a look again?
On 2011/07/25 22:50:32, Paweł Hajdan Jr. wrote: > I took an extremely brief look, I guess it's fine. Sorry, Pawel, just wanting to make sure you actually made a judgement call on one specific part of it: Creating a new file in DIR_TEST_DATA (and cleaning it up afterwards) is ok? It's a wonderful idea in the sense that I think it reduces the chances of collisions as low as it's possible to do, but I've always thought of that directory as read-only for tests. Beyond that, this change LGTM.
Oops, sorry, no, creating temp files in DIR_TEST_DATA is bad and may make the test flaky. What's preventing usage of ScopedTempDir?
On 2011/07/28 17:11:13, Paweł Hajdan Jr. wrote: > Oops, sorry, no, creating temp files in DIR_TEST_DATA is bad and may make the > test flaky. What's preventing usage of ScopedTempDir? The place to which URLRequestMockHTTPJob::GetMockUrl() redirects is DIR_TEST_DATA. I suppose we could hack URLRequestMockHTTPJob to come from somewhere else, but I'm having a bit of a "tug on a string" feeling about all this. How would each of you feel about simply discarding these tests (only the ones that test the final fallback of the cascade) and say that we've tested that manually, but can't come up with a way to test them automatically that doesn't potentially increase flake? An alternative that I like but would raise eyebrows would be to check in the last-but-one (before writing into DIR_TEST_DATA) patchset with the tests disabled and a comment that running them automatically is problematic, but that they can be run manually to test the functionality and are expected to pass. Having a push button manual test is worse than having an automatic test but still better than having a complex manual test, I figure.
> Oops, sorry, no, creating temp files in DIR_TEST_DATA is bad and may make the > test flaky. What's preventing usage of ScopedTempDir? I added URLRequestMockHTTPJob::AddUrlOfTempDirHandler() (, which is called by SetUrlRequestMocksEnabled()) and URLRequestMockHTTPJob::GetMockUrlOfTempDir(), and then used them to use ScopedTempDir in the download tests. Pawel, Randy: How about this?
Sorry, I found that the latest patch is wrong. Please wait a moment for the review... On 2011/07/29 01:42:24, haraken wrote: > > Oops, sorry, no, creating temp files in DIR_TEST_DATA is bad and may make the > > test flaky. What's preventing usage of ScopedTempDir? > > I added URLRequestMockHTTPJob::AddUrlOfTempDirHandler() (, which is called by > SetUrlRequestMocksEnabled()) and URLRequestMockHTTPJob::GetMockUrlOfTempDir(), > and then used them to use ScopedTempDir in the download tests. > > Pawel, Randy: How about this?
> Sorry, I found that the latest patch is wrong. Please wait a moment for the > review... I fixed it and revised the patch set. Pawel, Randy: Would you please take a look? On 2011/07/29 04:31:02, haraken wrote: > Sorry, I found that the latest patch is wrong. Please wait a moment for the > review... > > On 2011/07/29 01:42:24, haraken wrote: > > > Oops, sorry, no, creating temp files in DIR_TEST_DATA is bad and may make > the > > > test flaky. What's preventing usage of ScopedTempDir? > > > > I added URLRequestMockHTTPJob::AddUrlOfTempDirHandler() (, which is called by > > SetUrlRequestMocksEnabled()) and URLRequestMockHTTPJob::GetMockUrlOfTempDir(), > > and then used them to use ScopedTempDir in the download tests. > > > > Pawel, Randy: How about this?
Sorry, I'm confused: is this change committed or not? What are the old/new bits? Thank you for the follow-up on this, but I'm just not very enthusiastic about re-reviewing a large CL ~20 times or so.
On 2011/07/29 18:08:40, Paweł Hajdan Jr. wrote: > Sorry, I'm confused: is this change committed or not? What are the old/new bits? > > Thank you for the follow-up on this, but I'm just not very enthusiastic about > re-reviewing a large CL ~20 times or so. Pawel: Do you not use the "change from PS N" feature? It's the only thing that preserves my sanity (what little sanity I have :-}) given the number of reviews that come through. As far as I'm concerned, you're welcome to leave this up to me, and I'll flag specific things I think I need your input on (as I did with DIR_TEST_DATA writing).
Eric: I want to check with you on the change to url_request_mock_http_job that's going on as part of this CL. It adds a handler that can serve files out of the system temporary directory as well as DIR_TEST_DATA. I think this is ok, but I'm not sure if there are any security implications (e.g. is the mock server active in production?). I'm happy to do the detailed review, but I wanted to get a high-level architectural reaction from you. haraken-san: I apologize, but I'm going to be out on vacation Monday/Tuesday next week, and leaving for home fairly soon today, so I won't be able to give the next round of review for a while. If the above change is ok with Eric, I don't think it'll take us very much longer to iterate to completion, but it'll have to wait until Wednesday. I hope that doesn't cause you too much grief (I figure since this CL has dragged on this long, a couple more days isn't too big a deal :-}).
> Eric: I want to check with you on the change to url_request_mock_http_job that's > going on as part of this CL. It adds a handler that can serve files out of the > system temporary directory as well as DIR_TEST_DATA. I think this is ok, but > I'm not sure if there are any security implications (e.g. is the mock server > active in production?). I'm happy to do the detailed review, but I wanted to > get a high-level architectural reaction from you. Eric: Would you please take a look at the change for url_request_mock_http_job.h and url_request_mock_http_job.cc? (You can find the use cases of the change in SavePageBrowserTest.SaveFolder3 and DownloadTest.DownloadFolder.) We need to store temporarily generated test data in /tmp directory and then use it in browser_tests through url_request_mock_http_job.
I didn't notice the question in my inbox until Randy pinged me today :). Taking a look, will answer shortly...
The mock handler for the system temp dir doesn't seem necessary or even desirable. Correct me if I have misunderstood, but here is how I see things: It looks like the code is creating a temporary file (with a unique name) in the system temp directory, for the sole purpose of being able to use URLRequestMockHTTPJob() to map http://mock.temp.http/<PATH> to the file $TEMP/$PATH. This is done so that when the download is triggered, it will have a unique filename. That seems like overkill. Rather, what you can do is register a URL request hook which directly maps ALL URL paths under some host to a FIXED content (or echos the path if you prefer). This way you can still construct random paths which gives you random filenames on download, however without needing back it by a real file. http://codereview.chromium.org/6973052/diff/144001/content/browser/net/url_re... File content/browser/net/url_request_mock_http_job.cc (right): http://codereview.chromium.org/6973052/diff/144001/content/browser/net/url_re... content/browser/net/url_request_mock_http_job.cc:74: url.append("/"); note you can also use GURL::ReplaceComponents() to write the different components of a URL. http://codereview.chromium.org/6973052/diff/144001/content/browser/net/url_re... File content/browser/net/url_request_mock_http_job.h (right): http://codereview.chromium.org/6973052/diff/144001/content/browser/net/url_re... content/browser/net/url_request_mock_http_job.h:27: static net::URLRequest::ProtocolFactory FactoryForMockUrlOfTempDir; Our style disallows non-POD static initializers. Looks like there is already a precedent here, but ideally we shouldn't have this. http://codereview.chromium.org/6973052/diff/144001/content/browser/net/url_re... content/browser/net/url_request_mock_http_job.h:59: static FilePath test_dir_; See earlier comment --> no static initializers please! (they slow down startup). (Ideally this file wouldn't be compiled into release chrome ... but it is, so the usual rules apply).
On 2011/08/04 20:09:44, eroman wrote: > The mock handler for the system temp dir doesn't seem necessary or even > desirable. > > Correct me if I have misunderstood, but here is how I see things: > > It looks like the code is creating a temporary file (with a unique name) in the > system temp directory, for the sole purpose of being able to use > URLRequestMockHTTPJob() to map http://mock.temp.http/%3CPATH> to the file > $TEMP/$PATH. This is done so that when the download is triggered, it will have a > unique filename. > > That seems like overkill. > > Rather, what you can do is register a URL request hook which directly maps ALL > URL paths under some host to a FIXED content (or echos the path if you prefer). > This way you can still construct random paths which gives you random filenames > on download, however without needing back it by a real file. Oh, that's a nice idea. Correct me if I'm wrong, but that basically requires creating a new url request job handler parallel to URLRequestMockHttpJob, correct? Feel free to stay on the CL as a reviewer or not as you wish--I feel competent to drive it from here. (Having said that, Haraken, I'm heading out on vacation this weekend for 1 1/2 weeks. OTOH, when I get back I'm happy to do the change Eric suggests if it looks daunting; I've messed around a fair amount in that code.)
> Correct me if I'm wrong, but that basically requires creating a new url request job > handler parallel to URLRequestMockHttpJob, correct? Correct. The one which maps test directory can be left as-is, and a new one introduced with the mirroring behavior. Or alternately, rather than adding a client-side hook, you could build this logic into testserver (net/tools/testserver/testserver.py). Although if you want less flaky tests, using the testserver probably isn't the way to go :-) On Thu, Aug 4, 2011 at 1:17 PM, <rdsmith@chromium.org> wrote: > On 2011/08/04 20:09:44, eroman wrote: >> >> The mock handler for the system temp dir doesn't seem necessary or even >> desirable. > >> Correct me if I have misunderstood, but here is how I see things: > >> It looks like the code is creating a temporary file (with a unique name) >> in > > the >> >> system temp directory, for the sole purpose of being able to use >> URLRequestMockHTTPJob() to map http://mock.temp.http/%3CPATH> to the file >> $TEMP/$PATH. This is done so that when the download is triggered, it will >> have > > a >> >> unique filename. > >> That seems like overkill. > >> Rather, what you can do is register a URL request hook which directly maps >> ALL >> URL paths under some host to a FIXED content (or echos the path if you > > prefer). >> >> This way you can still construct random paths which gives you random >> filenames >> on download, however without needing back it by a real file. > > Oh, that's a nice idea. Correct me if I'm wrong, but that basically > requires > creating a new url request job handler parallel to URLRequestMockHttpJob, > correct? > > Feel free to stay on the CL as a reviewer or not as you wish--I feel > competent > to drive it from here. > > (Having said that, Haraken, I'm heading out on vacation this weekend for 1 > 1/2 > weeks. OTOH, when I get back I'm happy to do the change Eric suggests if it > looks daunting; I've messed around a fair amount in that code.) > > > > > > http://codereview.chromium.org/6973052/ >
On 2011/08/04 21:35:33, eroman wrote: > > Correct me if I'm wrong, but that basically requires creating a new url > request job > > handler parallel to URLRequestMockHttpJob, correct? > > Correct. The one which maps test directory can be left as-is, and a > new one introduced with the mirroring behavior. > > Or alternately, rather than adding a client-side hook, you could build > this logic into testserver (net/tools/testserver/testserver.py). > Although if you want less flaky tests, using the testserver probably > isn't the way to go :-) Yeah, there might be situations in which I'd reach for using an external server, but this ain't one :-}. > > On Thu, Aug 4, 2011 at 1:17 PM, <mailto:rdsmith@chromium.org> wrote: > > On 2011/08/04 20:09:44, eroman wrote: > >> > >> The mock handler for the system temp dir doesn't seem necessary or even > >> desirable. > > > >> Correct me if I have misunderstood, but here is how I see things: > > > >> It looks like the code is creating a temporary file (with a unique name) > >> in > > > > the > >> > >> system temp directory, for the sole purpose of being able to use > >> URLRequestMockHTTPJob() to map http://mock.temp.http/%253CPATH> to the file > >> $TEMP/$PATH. This is done so that when the download is triggered, it will > >> have > > > > a > >> > >> unique filename. > > > >> That seems like overkill. > > > >> Rather, what you can do is register a URL request hook which directly maps > >> ALL > >> URL paths under some host to a FIXED content (or echos the path if you > > > > prefer). > >> > >> This way you can still construct random paths which gives you random > >> filenames > >> on download, however without needing back it by a real file. > > > > Oh, that's a nice idea. Correct me if I'm wrong, but that basically > > requires > > creating a new url request job handler parallel to URLRequestMockHttpJob, > > correct? > > > > Feel free to stay on the CL as a reviewer or not as you wish--I feel > > competent > > to drive it from here. > > > > (Having said that, Haraken, I'm heading out on vacation this weekend for 1 > > 1/2 > > weeks. OTOH, when I get back I'm happy to do the change Eric suggests if it > > looks daunting; I've messed around a fair amount in that code.) > > > > > > > > > > > > http://codereview.chromium.org/6973052/ > >
This is my last day before 1 1/2 weeks of vacation. I'm good with all changes
up the the final bit of test tweaking around avoiding collisions. I'm happy to
give my LGTM for everything besides those, and I'd be happy to go with Pawel's
LGTM for the test changes and Eric's for the new url handler. Or we can wait on
this poor CL until I get back :-} :-{.
On 2011/08/04 21:39:16, rdsmith wrote:
> On 2011/08/04 21:35:33, eroman wrote:
> > > Correct me if I'm wrong, but that basically requires creating a new url
> > request job
> > > handler parallel to URLRequestMockHttpJob, correct?
> >
> > Correct. The one which maps test directory can be left as-is, and a
> > new one introduced with the mirroring behavior.
> >
> > Or alternately, rather than adding a client-side hook, you could build
> > this logic into testserver (net/tools/testserver/testserver.py).
> > Although if you want less flaky tests, using the testserver probably
> > isn't the way to go :-)
>
> Yeah, there might be situations in which I'd reach for using an external
server,
> but this ain't one :-}.
>
> >
> > On Thu, Aug 4, 2011 at 1:17 PM, <mailto:rdsmith@chromium.org> wrote:
> > > On 2011/08/04 20:09:44, eroman wrote:
> > >>
> > >> The mock handler for the system temp dir doesn't seem necessary or even
> > >> desirable.
> > >
> > >> Correct me if I have misunderstood, but here is how I see things:
> > >
> > >> It looks like the code is creating a temporary file (with a unique name)
> > >> in
> > >
> > > the
> > >>
> > >> system temp directory, for the sole purpose of being able to use
> > >> URLRequestMockHTTPJob() to map http://mock.temp.http/%25253CPATH> to the
file
> > >> $TEMP/$PATH. This is done so that when the download is triggered, it will
> > >> have
> > >
> > > a
> > >>
> > >> unique filename.
> > >
> > >> That seems like overkill.
> > >
> > >> Rather, what you can do is register a URL request hook which directly
maps
> > >> ALL
> > >> URL paths under some host to a FIXED content (or echos the path if you
> > >
> > > prefer).
> > >>
> > >> This way you can still construct random paths which gives you random
> > >> filenames
> > >> on download, however without needing back it by a real file.
> > >
> > > Oh, that's a nice idea. Correct me if I'm wrong, but that basically
> > > requires
> > > creating a new url request job handler parallel to URLRequestMockHttpJob,
> > > correct?
> > >
> > > Feel free to stay on the CL as a reviewer or not as you wish--I feel
> > > competent
> > > to drive it from here.
> > >
> > > (Having said that, Haraken, I'm heading out on vacation this weekend for 1
> > > 1/2
> > > weeks. OTOH, when I get back I'm happy to do the change Eric
suggests
> if it
> > > looks daunting; I've messed around a fair amount in that code.)
> > >
> > >
> > >
> > >
> > >
> > > http://codereview.chromium.org/6973052/
> > >
Sorry, I could not update the patch today (since I had to do other work in WebKit). > I'd be happy to go with Pawel's LGTM for the test changes and Eric's for the new url handler. Thank you for your consideration. I would like to fix them (maybe on Monday) and ask a review of Pawel and Eric. However, if the change to other places were required, I am happy to wait for your return:-)
> Rather, what you can do is register a URL request hook which directly maps ALL > URL paths under some host to a FIXED content (or echos the path if you prefer). > This way you can still construct random paths which gives you random filenames > on download, however without needing back it by a real file. eroman: I guess that I implemented the above idea. (Sorry if I am misunderstanding your idea...) Would you please take a look at the change in url_request_mock_http_job.h and url_request_mock_http_job.cc, and the use cases in DownloadTest.DownloadFolder and SavePageBrowserTest.SavePageFolder3? randy, pawel: FYI http://codereview.chromium.org/6973052/diff/144001/content/browser/net/url_re... File content/browser/net/url_request_mock_http_job.cc (right): http://codereview.chromium.org/6973052/diff/144001/content/browser/net/url_re... content/browser/net/url_request_mock_http_job.cc:74: url.append("/"); Thanks, but now this routine is moved into browser_tests. Since there is no URL to be replaced in the browser_tests, I did not use GURL::ReplaceComponetns(). http://codereview.chromium.org/6973052/diff/144001/content/browser/net/url_re... File content/browser/net/url_request_mock_http_job.h (right): http://codereview.chromium.org/6973052/diff/144001/content/browser/net/url_re... content/browser/net/url_request_mock_http_job.h:27: static net::URLRequest::ProtocolFactory FactoryForMockUrlOfTempDir; On 2011/08/04 20:09:44, eroman wrote: > Our style disallows non-POD static initializers. Looks like there is already a > precedent here, but ideally we shouldn't have this. Done. http://codereview.chromium.org/6973052/diff/144001/content/browser/net/url_re... content/browser/net/url_request_mock_http_job.h:59: static FilePath test_dir_; Removed this variable.
What patchset should I be looking at? I am looking at url_request_mock_http_job.cc in patchset 28 and still see the use of temporary directory. My comment was about removing the need for temporary directory. Cheers.
> My comment was about removing the need for temporary directory. eroman: Sorry! I was misunderstanding your idea. Now I think that I fixed the code based on your idea. Would you please look at the change of DownloadTest.DownloadFolder and SavePageBrowserTest.SaveFolder3? randy, pawel: FYI http://codereview.chromium.org/6973052/diff/163001/content/browser/net/url_re... File content/browser/net/url_request_mock_http_job.h (right): http://codereview.chromium.org/6973052/diff/163001/content/browser/net/url_re... content/browser/net/url_request_mock_http_job.h:28: net::URLRequest* request, const std::string& scheme); This is just a refactoring.
Thanks for making that adjustment. Is there a file missing from this patch? It seems like there should be an implementation in url_request_mock_http_job.cc for me to review.
> Is there a file missing from this patch? It seems like there should be an > implementation in url_request_mock_http_job.cc for me to review. No. All the files are in the patch. In particular, no change in url_request_mock_http_job.cc. And the change in url_request_mock_http_job.h is just a refactoring and is not related to this CL.
The changes to the test files LGTM,
However I still don't understand how 'url_request_mock_http_job.cc' can be
missing from this changelist.
The header file ('url_request_mock_http_job.cc') is changing the declaration of
a variable into a method:
- static net::URLRequest::ProtocolFactory Factory;
+ // Generate a url request job on the DIR_TEST_DATA directory.
+ static net::URLRequestJob* Factory(
+ net::URLRequest* request, const std::string& scheme);
However I don't see that definition anywhere in this patch. Therefore I am
confused as to how this can compile. If this is in fact an unrelated refactoring
as you say, then please remove it from the CL to avoid committing it (if this
file is removed, you don't even need my OWNERS LGTM for net anymore ;)
Eroman:
Thank you very much for the review. I removed the change in
url_request_mock_http_job.h, since the refactoring is not related to
this CL. The reason why I refactored it is just that the signature of
Factory() declared in url_request_mock_http_job.h is different from
the signature of Factory() defined in url_request_mock_http_job.cc.
Pawel:
I received LGTM from eroman and randy. Is it OK to commit? (If you are
not interested in this CL any longer, please just say so:-) After all,
the change I made between the patch set 24 and the patch set 31 is as
follows :
DownloadTest.DownloadFolder and SavePageBrowserTest.SaveFolder3 create
and delete a file on the user's real "Downloads" folder, which is
globally shared among all tests on the testing environment. Therefore,
if we run browser tests in parallel, the file created by one browser
test may be deleted by another broswer test when the file name
conflicts. In order to avoid this problem, we use a special mock URL,
"http://mock.testfile.http/<random path>" for this download test.
Since we redirect "http://mock.testfile.http/<random path>" to
"chrome/test/data/{kTestFileName}" by using FactoryForTestFile(),
"chrome/test/data/{kTestFileName}" is used for the file to be
downloaded. Then, the downloaded file is saved as a name
"Downloads/<random path>", which is a unique file name in the user's
real "Downloads" folder.
Rubber-stamp LGTM with the following comments. http://codereview.chromium.org/6973052/diff/173001/chrome/browser/download/do... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/173001/chrome/browser/download/do... chrome/browser/download/download_browsertest.cc:854: NOTREACHED(); This is a browser_test, please use gtest macros. EXPECT_TRUE? http://codereview.chromium.org/6973052/diff/173001/chrome/browser/download/do... chrome/browser/download/download_browsertest.cc:1047: file_util::Delete(downloaded_file, false); Check return value, applies to all file_util functions. http://codereview.chromium.org/6973052/diff/173001/chrome/browser/download/do... chrome/browser/download/download_browsertest.cc:1051: std::cout << temporary_file.value() << std::endl; Don't use cout directly, but rather the LOG macros. Applies to entire CL. http://codereview.chromium.org/6973052/diff/173001/chrome/browser/download/do... File chrome/browser/download/download_util.cc (right): http://codereview.chromium.org/6973052/diff/173001/chrome/browser/download/do... chrome/browser/download/download_util.cc:167: &default_download_dir)) nit: Braces {} here. http://codereview.chromium.org/6973052/diff/173001/chrome/browser/download/sa... File chrome/browser/download/save_page_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/173001/chrome/browser/download/sa... chrome/browser/download/save_page_browsertest.cc:111: DCHECK(prefs->FindPreference(prefs::kDownloadDefaultDirectory)); ASSERT_TRUE. http://codereview.chromium.org/6973052/diff/173001/chrome/browser/download/sa... chrome/browser/download/save_page_browsertest.cc:180: NOTREACHED(); EXPECT_TRUE, applies to entire CL.
Drive-by: * Is the description on this CL up-to-date with what it actually does? There's so many patches and so much discussion that for all I know it's now totally different. For now I'm responding based on that. * Have you run this behavior by the UI leads? Personally, I think the parts about popping up a file chooser that starts in the Downloads/ folder are fine, but auto-changing the user's preference in the options dialog is not -- we should never change something the user explicitly did without at least letting them know. Having the browser auto-create the destination folder if it doesn't exist is something that I expect and have relied on in the past and the behavior proposed on the bug, at least for Windows, seems bizarre and frustrating.
http://codereview.chromium.org/6973052/diff/173001/chrome/browser/download/do... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/173001/chrome/browser/download/do... chrome/browser/download/download_browsertest.cc:854: NOTREACHED(); On 2011/08/12 17:00:59, Paweł Hajdan Jr. wrote: > This is a browser_test, please use gtest macros. EXPECT_TRUE? Done. http://codereview.chromium.org/6973052/diff/173001/chrome/browser/download/do... chrome/browser/download/download_browsertest.cc:1047: file_util::Delete(downloaded_file, false); This Delete() can fail when the file does not exist. However, it is OK, since what is important is just that the file does not exist after this statement, and we are not interested in the return value of Delete(). I described this as a comment. Also, I added return value checks in the following (line 1066 and line 1067) file_util::Delete(). http://codereview.chromium.org/6973052/diff/173001/chrome/browser/download/do... chrome/browser/download/download_browsertest.cc:1051: std::cout << temporary_file.value() << std::endl; Sorry, removed this cout. This cout is just for my debugging use. http://codereview.chromium.org/6973052/diff/173001/chrome/browser/download/do... File chrome/browser/download/download_util.cc (right): http://codereview.chromium.org/6973052/diff/173001/chrome/browser/download/do... chrome/browser/download/download_util.cc:167: &default_download_dir)) On 2011/08/12 17:00:59, Paweł Hajdan Jr. wrote: > nit: Braces {} here. Done. http://codereview.chromium.org/6973052/diff/173001/chrome/browser/download/sa... File chrome/browser/download/save_page_browsertest.cc (right): http://codereview.chromium.org/6973052/diff/173001/chrome/browser/download/sa... chrome/browser/download/save_page_browsertest.cc:111: DCHECK(prefs->FindPreference(prefs::kDownloadDefaultDirectory)); On 2011/08/12 17:00:59, Paweł Hajdan Jr. wrote: > ASSERT_TRUE. Done. http://codereview.chromium.org/6973052/diff/173001/chrome/browser/download/sa... chrome/browser/download/save_page_browsertest.cc:180: NOTREACHED(); On 2011/08/12 17:00:59, Paweł Hajdan Jr. wrote: > EXPECT_TRUE, applies to entire CL. Done.
> * Is the description on this CL up-to-date with what it actually does? There's > so many patches and so much discussion that for all I know it's now totally > different. For now I'm responding based on that. Peter: The description of the CL is still up-to-date. Most of the long discussion and many patches are about how we can write tests safely. > * Have you run this behavior by the UI leads? Personally, I think the parts > about popping up a file chooser that starts in the Downloads/ folder are fine, > but auto-changing the user's preference in the options dialog is not -- we > should never change something the user explicitly did without at least letting > them know. Having the browser auto-create the destination folder if it doesn't > exist is something that I expect and have relied on in the past and the behavior > proposed on the bug, at least for Windows, seems bizarre and frustrating. OK, I will ask UI leads for advice.
+chrome-ui-review chrome-ui-review: We are going to make the following change in the download UI. - When the download folder does not exist, change the download folder to a user's "Downloads" (or something depending on the platform) --- Set the download folder to Foo. You can download a PDF file to Foo without being asked anything (if you do not choose "Ask where to save each file before downloading"). Then delete Foo. Now, if you try to download the PDF file, then a file chooser dialog is displayed suggesting your "Downloads" folder. --- Set the download folder to Foo. Save an HTML to Bar by right-clicking and choosing "Save as ..." on the tab. If you try to save the HTML, the file chooser dialog is displayed suggesting Bar. Then delete Bar. Now, if you try to save the HTML, the file chooser dialog is displayed suggesting Foo. Then delete Foo. If you try to save the HTML, the file chooser dialog is displayed suggesting your "Downloads" folder. --- Set the download folder to Foo. Observe that Foo is displayed as the download location in the preferences page. Then delete Foo and reload the preferences page. Observe that your "Downloads" folder is displayed as the download location in the preferences page. If you are opening multiple preferences pages, then this change is automatically reflected to all the preferences pages. I received LGTMs from all reviewers, but Peter is casting suspicion on this UI change as follows: > * Have you run this behavior by the UI leads? Personally, I think the parts > about popping up a file chooser that starts in the Downloads/ folder are fine, > but auto-changing the user's preference in the options dialog is not -- we > should never change something the user explicitly did without at least letting > them know. Having the browser auto-create the destination folder if it doesn't > exist is something that I expect and have relied on in the past and the behavior > proposed on the bug, at least for Windows, seems bizarre and frustrating. Would you please give us any advice?
+Brian Brian: Would you please give us a comment about the UI change? > +chrome-ui-review > > chrome-ui-review: We are going to make the following change in the download UI. > > - When the download folder does not exist, change the download folder to a > user's "Downloads" (or something depending on the platform) > > --- Set the download folder to Foo. You can download a PDF file to Foo without > being asked anything (if you do not choose "Ask where to save each file before > downloading"). Then delete Foo. Now, if you try to download the PDF file, then a > file chooser dialog is displayed suggesting your "Downloads" folder. > --- Set the download folder to Foo. Save an HTML to Bar by right-clicking and > choosing "Save as ..." on the tab. If you try to save the HTML, the file chooser > dialog is displayed suggesting Bar. Then delete Bar. Now, if you try to save the > HTML, the file chooser dialog is displayed suggesting Foo. Then delete Foo. If > you try to save the HTML, the file chooser dialog is displayed suggesting your > "Downloads" folder. > --- Set the download folder to Foo. Observe that Foo is displayed as the > download location in the preferences page. Then delete Foo and reload the > preferences page. Observe that your "Downloads" folder is displayed as the > download location in the preferences page. If you are opening multiple > preferences pages, then this change is automatically reflected to all the > preferences pages. > > > I received LGTMs from all reviewers, but Peter is casting suspicion on this UI > change as follows: > > > * Have you run this behavior by the UI leads? Personally, I think the parts > > about popping up a file chooser that starts in the Downloads/ folder are fine, > > but auto-changing the user's preference in the options dialog is not -- we > > should never change something the user explicitly did without at least letting > > them know. Having the browser auto-create the destination folder if it > doesn't > > exist is something that I expect and have relied on in the past and the > behavior > > proposed on the bug, at least for Windows, seems bizarre and frustrating. > > Would you please give us any advice?
I chatted with Glen over IM. We concluded Chrome should create the user's desired folder if it doesn't exist, unless such creation is impossible (e.g. path not writable) in which case falling back to the system default folder would be fine. I think this is mostly what Chrome does today (except for the fallback-on-not-writable part?).
I'm a little slow today, and am a little confused about why we're changing the behavior here - the bug doesn't seem to contain any explanation. Recreating the folder seems ideal - you specified a path, and we should do the best we can to fulfill that specification (though reverting to default is OK if that's impossible). On 2011/08/17 15:47:36, haraken wrote: > +Brian > > Brian: Would you please give us a comment about the UI change? > > > +chrome-ui-review > > > > chrome-ui-review: We are going to make the following change in the download > UI. > > > > - When the download folder does not exist, change the download folder to a > > user's "Downloads" (or something depending on the platform) > > > > --- Set the download folder to Foo. You can download a PDF file to Foo without > > being asked anything (if you do not choose "Ask where to save each file before > > downloading"). Then delete Foo. Now, if you try to download the PDF file, then > a > > file chooser dialog is displayed suggesting your "Downloads" folder. > > --- Set the download folder to Foo. Save an HTML to Bar by right-clicking and > > choosing "Save as ..." on the tab. If you try to save the HTML, the file > chooser > > dialog is displayed suggesting Bar. Then delete Bar. Now, if you try to save > the > > HTML, the file chooser dialog is displayed suggesting Foo. Then delete Foo. If > > you try to save the HTML, the file chooser dialog is displayed suggesting your > > "Downloads" folder. > > --- Set the download folder to Foo. Observe that Foo is displayed as the > > download location in the preferences page. Then delete Foo and reload the > > preferences page. Observe that your "Downloads" folder is displayed as the > > download location in the preferences page. If you are opening multiple > > preferences pages, then this change is automatically reflected to all the > > preferences pages. > > > > > > I received LGTMs from all reviewers, but Peter is casting suspicion on this UI > > change as follows: > > > > > * Have you run this behavior by the UI leads? Personally, I think the parts > > > about popping up a file chooser that starts in the Downloads/ folder are > fine, > > > but auto-changing the user's preference in the options dialog is not -- we > > > should never change something the user explicitly did without at least > letting > > > them know. Having the browser auto-create the destination folder if it > > doesn't > > > exist is something that I expect and have relied on in the past and the > > behavior > > > proposed on the bug, at least for Windows, seems bizarre and frustrating. > > > > Would you please give us any advice?
Peter and Glen, thank you very much for the comments. Below are my replies. Hmmm... I still feel that the change of this patch is reasonable. I would also like to hear opinions of reviewers, Randy and Pawel. Peter: > Personally, I think the parts about popping up a file chooser > that starts in the Downloads/ folder are fine, I think so. > but auto-changing the user's preference in the options dialog is not -- we > should never change something the user explicitly did without at least > letting them know. At least, the folder displayed in the preference page should correspond to the folder to which a downloaded file will be saved. Thus, I think, provided that we are going to suggest the "Downloads" folder, then the folder in the preference page should be also the "Downloads" folder. In other words, the possible approach is either of the following two: Approach1: Display the folder that a user specified before in the preference page, even if the folder does not exist any longer. Then, when a file is downloaded, the folder is implicitly re-created and then the file is saved to the folder(, as the current Chrome does). Approach2: If the folder does not exist any longer, display the "Downloads" folder in the preference page. Then, when a file is downloaded, the "Downloads" folder is suggested to the user(, as my patch does). > Having the browser auto-create the destination folder if it > doesn't > exist is something that I expect and have relied on in the past and the > behavior > proposed on the bug, at least for Windows, seems bizarre and frustrating. Nit: Why do you think that Windows is special? Glen: > I'm a little slow today, and am a little confused about why we're changing > the behavior here - the bug doesn't seem to contain any explanation. > > Recreating the folder seems ideal - you specified a path, and we should do > the best we can to fulfill that specification (though reverting to default is OK > if that's impossible). Other browsers behave as follows, when the folder F that the user specified does not exist. Linux Firefox 5.0 and Mac Firefox 5.0.1: Implicitly create the folder F and save a downloaded file (Approach1). Mac Safari 5.0.5: The "Downloads" folder is suggested. If we reload the preference page after removing the folder F, the "Downloads" folder is displayed (Approach2). Linux Opera 11.50: The "Downloads" folder is suggested, but the folder in the preference page does not change and continues to display the folder F. In summary, in my opinion, the reason for this change is that Safari behaves so and that I "feel" the behavior is more reasonable.
On 2011/08/18 09:51:36, haraken wrote: > Approach1: Display the folder that a user specified before in the > preference page, even if the folder does not exist any longer. Then, > when a file is downloaded, the folder is implicitly re-created and > then the file is saved to the folder(, as the current Chrome does). Yes, this is what we're asking for. > > Having the browser auto-create the destination folder if it > > doesn't > > exist is something that I expect and have relied on in the past and the > > behavior > > proposed on the bug, at least for Windows, seems bizarre and frustrating. > > Nit: Why do you think that Windows is special? Windows is 95+% of our usage. So if we're going to do one behavior, and we're going to decide what that is based on platform conventions, then it should match typical Windows usage. If we're going to do platform-specific behavior, of course, that's different, or if we're going to make decisions based not on platform "conventions" but on use cases. > Other browsers behave as follows, when the folder F that the user > specified does not exist. Looking at other browsers' behavior is normally only a small piece of how we decide what to do. More important are particular use cases and general design principles. In this case both seem to argue for Chrome's current behavior: the principle of least surprise says we should do what the user asked us to do, and the use case of setting up Chrome or syncing settings to a new machine -- by far the most likely causes of this problem -- are better served by obeying what the user asked us to do. If there is a use case that you think is better served with your proposed behavior, please detail it. One takeaway lesson of this whole thread, regardless of outcome, is that you should not charge into implementing a behavioral change without checking whether we actually want that change first.
haraken-san: I'd be inclined to defer to Peter, and certainly to defer to UI lead input, on chrome behavior (as opposed to implementation). My apologies, as I really should have realized (much) earlier in this thread that we should confirm that the behavioral change being made were wanted--I tend to be pretty heads down in the implementation and it's not always clear "how big" a change needs to be to require UI lead interaction. OTOH, I should have at least asked the question earlier in this thread. Peter: I don't think it's unreasonable for someone new to the project, when confronted by a "Good first bug" submitted by another chromium committer, to assume the behavior is wanted. I'll take a mea culpa myself (as above) that I should have recognized the behavior change should be checked a bit more widely. But I don't think that haraken made any errors here; he just got caught with a nasty first bug (in ways beyond the question of whether the behavior was wanted--see rest of thread).
Sorry haraken, I should have modulated my tone a bit better -- I didn't mean to come across so snappy (and doubly so since I didn't realize you were new to the project). Please don't take my comments personally!
> haraken-san: I'd be inclined to defer to Peter, and certainly to defer to UI > lead input, on chrome behavior (as opposed to implementation). Randy, Peter and Glen: OK. Now, I understood the opinions of Peter and Glen, and so I would like to stop and close this CL (in a few days if no objections). I also discussed what the behavior should be with my team members, and concluded that there is room to debate, but at least, implicitly re-creating the preference folder would be better than implicitly changing the preference folder to the user's "Downloads" folder. (And now I know that this CL is not at all an urgent issue and we should not spend more time to this CL.) Randy and Pawel: I really cannot thank you enough for your dedicated and long long long assistance. I learned much from those discussions, e.g. the difficulty in implementation of using a temporary folder for the "Downloads" folder in tests, how to solve the problem safely by hacking the browser test mock, the potentiality that the invariant of download state transitions may break up, and more importantly we should ask UI leads earlier when we start to change the UI:-) Randy, you always lead me to the right direction in a clearly understandable manner, and most of all, in a manner that graciously encourages people (especially newcomers). I have enormous respect for you as a reviewer. 2011年8月20日1:19 <rdsmith@chromium.org>: > haraken-san: I'd be inclined to defer to Peter, and certainly to defer to UI > lead input, on chrome behavior (as opposed to implementation). My > apologies, as > I really should have realized (much) earlier in this thread that we should > confirm that the behavioral change being made were wanted--I tend to be > pretty > heads down in the implementation and it's not always clear "how big" a > change > needs to be to require UI lead interaction. OTOH, I should have at least > asked > the question earlier in this thread. > > Peter: I don't think it's unreasonable for someone new to the project, when > confronted by a "Good first bug" submitted by another chromium committer, to > assume the behavior is wanted. I'll take a mea culpa myself (as above) that > I > should have recognized the behavior change should be checked a bit more > widely. > But I don't think that haraken made any errors here; he just got caught with > a > nasty first bug (in ways beyond the question of whether the behavior was > wanted--see rest of thread). > > http://codereview.chromium.org/6973052/ > -- ===================== Kentaro Hara (原健太朗) http://haraken.info ===================== |
