Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9426029:
Test file errors in downloads. (Closed)
|
Created:
8 years, 10 months ago by ahendrickson Modified:
8 years, 9 months ago CC:
chromium-reviews, achuith+watch_chromium.org, joi+watch-content_chromium.org, rginda+watch_chromium.org, darin-cc_chromium.org, rdsmith+dwatch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionTest file errors in downloads.
Uses an error injection technique to simulate hard-to-reproduce file system errors.
BUG=None
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126732
Patch Set 1 #Patch Set 2 : Merged with parent #Patch Set 3 : Fixed issues with errors occurring at start of download. #Patch Set 4 : Merged with parent #Patch Set 5 : Merged with parent #
Total comments: 12
Patch Set 6 : Rearchitected to avoid reference counting #Patch Set 7 : Merged with parent #Patch Set 8 : Simplified injector classes further. #
Total comments: 40
Patch Set 9 : Merged with trunk #Patch Set 10 : Refactored per Chris' comments #
Total comments: 64
Patch Set 11 : Tests now use a callback to wait for DownloadItem to be created or error out. #Patch Set 12 : Now running multiple downloads per test; using URL as identifier. #Patch Set 13 : Split off 2 CLs, to simplify this one. #
Total comments: 47
Patch Set 14 : Merged with parent #
Total comments: 12
Patch Set 15 : Fixed CLANG issue. #
Total comments: 20
Patch Set 16 : Created DownloadId default constructor. Cleaned up per Chris' comments. #
Total comments: 2
Patch Set 17 : Merged with parent. #Patch Set 18 : Added test that injects an error on the 2nd write. #
Total comments: 10
Patch Set 19 : InjectErrors now posts to the file thread. #Patch Set 20 : Merged with parent. Removed unused variable. #
Total comments: 20
Patch Set 21 : Merged with parent. #Patch Set 22 : Refactored per Chris & Randy's comments. #
Total comments: 19
Patch Set 23 : Merged with parent. #Patch Set 24 : Changes per Chris' comments. #Patch Set 25 : Addressed GCC compile issue. #Patch Set 26 : Merged with parent. #Patch Set 27 : Renamed loop body functions. #Patch Set 28 : Merged with master. #Patch Set 29 : Merged with trunk. #Patch Set 30 : Fixed merge issue. #Patch Set 31 : Removed interface class for download file error injector. #Patch Set 32 : Added comment about dangling pointer. #Patch Set 33 : Merged with trunk. #Messages
Total messages: 47 (0 generated)
PTAL
BUG and TEST? I haven't done any actual reviewing yet, http://codereview.chromium.org/9426029/diff/12003/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/12003/content/test/test_file_erro... content/test/test_file_error_injector.h:12: enum FileOperationCode { Should this be in the content namespace?
It feels like this infrastructure could be simplified significantly - at least to make the new browsertests in this CL complete successfully. The back-and-forth calls between the TestFileErrorInjector, the DownloadFileWithErrorsFactory, and the DownloadFileWithErrors as well as refcounted ownership of the injector are pretty complicated. I encourage you to think of a simpler approach. http://codereview.chromium.org/9426029/diff/12003/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9426029/diff/12003/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:308: struct FileErrorInjectInfo { Document this and fields. http://codereview.chromium.org/9426029/diff/12003/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/12003/content/test/test_file_erro... content/test/test_file_error_injector.cc:207: // If the file already exists, simply add this entry to the error list. Do you need to handle this case? http://codereview.chromium.org/9426029/diff/12003/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/12003/content/test/test_file_erro... content/test/test_file_error_injector.h:19: // Must only be used on the FILE thread. Comment not entirely true - constructor can be called off of FILE thread. http://codereview.chromium.org/9426029/diff/12003/content/test/test_file_erro... content/test/test_file_error_injector.h:21: : public base::RefCountedThreadSafe<TestFileErrorInjector> { Why does this need to be refcounted? http://codereview.chromium.org/9426029/diff/12003/content/test/test_file_erro... content/test/test_file_error_injector.h:27: virtual bool InjectError(int file_index, Could this be simplified at all - especially since |file_index| is always 0?
On 2012/02/24 02:25:12, cbentzel wrote: > It feels like this infrastructure could be simplified significantly - at least > to make the new browsertests in this CL complete successfully. > > The back-and-forth calls between the TestFileErrorInjector, the > DownloadFileWithErrorsFactory, and the DownloadFileWithErrors as well as > refcounted ownership of the injector are pretty complicated. > > I encourage you to think of a simpler approach. I can discard the capability to inject errors after the download file has been created, since I'm not using it and it may not be necessary. This will simplify the code for adding an error, remove the need for DownloadFileWithErrors to register itself with TestFileErrorInjectorImpl, and so remove the need for the latter to be refcounted.
On Fri, Feb 24, 2012 at 11:27 AM, <ahendrickson@chromium.org> wrote: > On 2012/02/24 02:25:12, cbentzel wrote: > >> It feels like this infrastructure could be simplified significantly - at >> least >> to make the new browsertests in this CL complete successfully. >> > > The back-and-forth calls between the TestFileErrorInjector, the >> DownloadFileWithErrorsFactory, and the DownloadFileWithErrors as well as >> refcounted ownership of the injector are pretty complicated. >> > > I encourage you to think of a simpler approach. >> > > I can discard the capability to inject errors after the download file has > been > created, since I'm not using it and it may not be necessary. This will > simplify > the code for adding an error, remove the need for DownloadFileWithErrors to > register itself with TestFileErrorInjectorImpl, and so remove the need for > the > latter to be refcounted. > Exactly what I was thinking. > > > http://codereview.chromium.**org/9426029/<http://codereview.chromium.org/9426... >
I'll wait to do my review until you've done the restructuring you and Chris worked out.
http://codereview.chromium.org/9426029/diff/12003/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9426029/diff/12003/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:308: struct FileErrorInjectInfo { On 2012/02/24 02:25:12, cbentzel wrote: > Document this and fields. Done. http://codereview.chromium.org/9426029/diff/12003/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/12003/content/test/test_file_erro... content/test/test_file_error_injector.cc:207: // If the file already exists, simply add this entry to the error list. On 2012/02/24 02:25:12, cbentzel wrote: > Do you need to handle this case? Removed. http://codereview.chromium.org/9426029/diff/12003/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/12003/content/test/test_file_erro... content/test/test_file_error_injector.h:12: enum FileOperationCode { On 2012/02/24 01:54:37, cbentzel wrote: > Should this be in the content namespace? Done. http://codereview.chromium.org/9426029/diff/12003/content/test/test_file_erro... content/test/test_file_error_injector.h:19: // Must only be used on the FILE thread. On 2012/02/24 02:25:12, cbentzel wrote: > Comment not entirely true - constructor can be called off of FILE thread. Changed the comment. http://codereview.chromium.org/9426029/diff/12003/content/test/test_file_erro... content/test/test_file_error_injector.h:21: : public base::RefCountedThreadSafe<TestFileErrorInjector> { On 2012/02/24 02:25:12, cbentzel wrote: > Why does this need to be refcounted? Because the DownloadFileWithError instances can outlast the test body, which is where TestFileErrorInjected would normally be destroyed. However, if I remove the capability of injecting errors after the DownloadFile has been created, then this is no longer necessary. Changed. http://codereview.chromium.org/9426029/diff/12003/content/test/test_file_erro... content/test/test_file_error_injector.h:27: virtual bool InjectError(int file_index, On 2012/02/24 02:25:12, cbentzel wrote: > Could this be simplified at all - especially since |file_index| is always 0? I would like to be able to use this later, for auto-resume cases. Each resume creates a new DownloadFile, so I will need indices other than 0.
Unfortunately, I lost my comment (must have marked cancel). This is still far more complicated than I expect it needs to be - even if the tests pass. What you are ultimately trying to do is pass in the mock behavior into the DownloadFileWithErrors. A typical mock pattern would be to just pass it in at construction/initialization time - so you'd need to pass these into the factory, etc. This kind of does it, but it does it by reaching all the way back to the TestFileErrorInjector object - which also includes a slightly strange transfer of owner object into the DownloadFileWithErrorsFactory object it creates. I think you could retain the tests and the DownloadFileWithErrors, but simplify how the error behavior data gets passed into the DownloadFileWithErrors. Make sense?
Please take a look -- I simplified it some more. The DownloadFileWithErrors now only have one error each, and the factory keeps track of all the errors. As we discussed, I do need the callbacks from DownloadFileWithErrors in order to keep track of creation and destruction, for which I've added test code. The callbacks are now much simpler though. Andy On 2012/02/24 23:08:45, cbentzel wrote: > Unfortunately, I lost my comment (must have marked cancel). > > This is still far more complicated than I expect it needs to be - even if the > tests pass. > > What you are ultimately trying to do is pass in the mock behavior into the > DownloadFileWithErrors. A typical mock pattern would be to just pass it in at > construction/initialization time - so you'd need to pass these into the factory, > etc. > > This kind of does it, but it does it by reaching all the way back to the > TestFileErrorInjector object - which also includes a slightly strange transfer > of owner object into the DownloadFileWithErrorsFactory object it creates. > > I think you could retain the tests and the DownloadFileWithErrors, but simplify > how the error behavior data gets passed into the DownloadFileWithErrors. > > Make sense?
I still think this is a bit too complicated, but the complexity is at an acceptable level now. I'll do a more thorough review tonight. http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.cc:128: TestFileErrorInjectorImpl* injector_; scoped_ptr is much clearer declaration of ownership.
http://codereview.chromium.org/9426029/diff/28004/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9426029/diff/28004/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:753: // |injector| will be owned by the DownloadFileManager (indirectly). How hard would it be to change this so the injector is owned by the caller of Create, and the DownloadFileManager etc. could use a weak pointer? May be tough with threads. But worth considering. http://codereview.chromium.org/9426029/diff/28004/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:756: injector->InjectError(0, // First DownloadFile created. I'd remove this argument for now. http://codereview.chromium.org/9426029/diff/28004/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:758: info.operation_index, Do you need operation_index? It looks like it's always set to 0. http://codereview.chromium.org/9426029/diff/28004/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:2122: net::ERR_FILE_NO_SPACE Why would we get NO_SPACE on INITIALIZE? http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.cc:21: struct FileErrorInfo { This is kind of duplicated in the browsertest. Perhaps expose in the .h and have only one version? http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.cc:37: DCHECK(rdh != NULL); What cases could it be NULL? http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.cc:41: std::string DebugString(content::FileOperationCode code) { Is this needed? http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.cc:63: class DownloadFileWithErrors: public DownloadFileImpl { Could this be in an anonymous nested namespace in content? http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.cc:187: injector_->DownloadFileCreated(this); Are you worried at all about thread-safety issues on the injector_ here? What threads are these done on? http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.cc:215: int counter = operation_counter_[code]; Looks like the operation_instance is always 0 when used. Could remove this logic. Also, only really needed if code == error_info_[code] http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.cc:219: return original_net_error; // No error for this operation. Comments not really needed and a bit unclear actually. http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.cc:234: TestFileErrorInjectorImpl* injector) should be passed in via scoped_ptr if the Factory owns it. Still feels weird to own it, though. http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.cc:314: return files_.size(); What is the thread-safety here? http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.cc:322: bool TestFileErrorInjectorImpl::HasFile(int file_index) const { You aren't using this. Would also remove the need for a map of files_, just need a counter. http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.h:9: #include "base/memory/ref_counted.h" ref_counted no longer needed. http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.h:14: enum FileOperationCode { This seems pretty generic to be part of content namespace. Perhaps contain in TestFileErrorInjector. http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.h:21: // All errors must be injected before |DownloadFile|s are created. It might be clearer to have the API be like TestFileErrorInjector() AddError(); AddError(); Inject() Inject would actually create the DownloadFileErrorManager and inject into the DownloadFileManager. http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.h:23: // Will be owned by the |DownloadFileManager| via a |DownloadFileFactory| Yeah - this ownership is still really strange - especially because it's hidden.
http://codereview.chromium.org/9426029/diff/28004/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9426029/diff/28004/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:753: // |injector| will be owned by the DownloadFileManager (indirectly). On 2012/02/27 02:18:54, cbentzel wrote: > How hard would it be to change this so the injector is owned by the caller of > Create, and the DownloadFileManager etc. could use a weak pointer? > > May be tough with threads. But worth considering. Switched to using RefCountedThreadSafe for the injector. http://codereview.chromium.org/9426029/diff/28004/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:756: injector->InjectError(0, // First DownloadFile created. On 2012/02/27 02:18:54, cbentzel wrote: > I'd remove this argument for now. How strongly do you feel about that? http://codereview.chromium.org/9426029/diff/28004/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:758: info.operation_index, On 2012/02/27 02:18:54, cbentzel wrote: > Do you need operation_index? It looks like it's always set to 0. Right now the unit tests only try to download small files, which result in having only one write operation. I'd like to have tests in the future that use larger files, and have an error in, say, the 5th operation. http://codereview.chromium.org/9426029/diff/28004/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:2122: net::ERR_FILE_NO_SPACE On 2012/02/27 02:18:54, cbentzel wrote: > Why would we get NO_SPACE on INITIALIZE? Wouldn't we get that if there are no bytes available on a disk? http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.cc:21: struct FileErrorInfo { On 2012/02/27 02:18:54, cbentzel wrote: > This is kind of duplicated in the browsertest. Perhaps expose in the .h and have > only one version? Done. http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.cc:37: DCHECK(rdh != NULL); On 2012/02/27 02:18:54, cbentzel wrote: > What cases could it be NULL? There are tests (perhaps just unit tests) where RDH is NULL. I'm just double-checking that it is available. Should I remove the DCHECK? http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.cc:41: std::string DebugString(content::FileOperationCode code) { On 2012/02/27 02:18:54, cbentzel wrote: > Is this needed? It's used below. I think it's useful to trace error injections in tests. http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.cc:63: class DownloadFileWithErrors: public DownloadFileImpl { On 2012/02/27 02:18:54, cbentzel wrote: > Could this be in an anonymous nested namespace in content? Done. http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.cc:128: TestFileErrorInjectorImpl* injector_; On 2012/02/26 15:01:48, cbentzel wrote: > scoped_ptr is much clearer declaration of ownership. Now using RefCountedThreadSafe. http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.cc:187: injector_->DownloadFileCreated(this); On 2012/02/27 02:18:54, cbentzel wrote: > Are you worried at all about thread-safety issues on the injector_ here? What > threads are these done on? Switched to using RefCountedThreadSafe for the injector, and passing a callback to the DownloadFileWithErrors class that posts information to the injector on the UI thread. DownloadFiles are always created on the FILE thread. http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.cc:215: int counter = operation_counter_[code]; On 2012/02/27 02:18:54, cbentzel wrote: > Looks like the operation_instance is always 0 when used. Could remove this > logic. Also, only really needed if code == error_info_[code] Would prefer not to, as I have a use for this in mind, and it doesn't add much complexity. However, if you feel strongly about it, I will. http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.cc:219: return original_net_error; // No error for this operation. On 2012/02/27 02:18:54, cbentzel wrote: > Comments not really needed and a bit unclear actually. Removed. http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.cc:234: TestFileErrorInjectorImpl* injector) On 2012/02/27 02:18:54, cbentzel wrote: > should be passed in via scoped_ptr if the Factory owns it. Still feels weird to > own it, though. Now using RefCountedThreadSafe for the injector. http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.cc:314: return files_.size(); On 2012/02/27 02:18:54, cbentzel wrote: > What is the thread-safety here? All operations on files_ now happen on the UI thread. http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.cc:322: bool TestFileErrorInjectorImpl::HasFile(int file_index) const { On 2012/02/27 02:18:54, cbentzel wrote: > You aren't using this. Would also remove the need for a map of files_, just need > a counter. Now using it. http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.h:9: #include "base/memory/ref_counted.h" On 2012/02/27 02:18:54, cbentzel wrote: > ref_counted no longer needed. It is again. http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.h:14: enum FileOperationCode { On 2012/02/27 02:18:54, cbentzel wrote: > This seems pretty generic to be part of content namespace. Perhaps contain in > TestFileErrorInjector. Done. http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.h:21: // All errors must be injected before |DownloadFile|s are created. On 2012/02/27 02:18:54, cbentzel wrote: > It might be clearer to have the API be like > > TestFileErrorInjector() > AddError(); > AddError(); > Inject() > > Inject would actually create the DownloadFileErrorManager and inject into the > DownloadFileManager. Done. http://codereview.chromium.org/9426029/diff/28004/content/test/test_file_erro... content/test/test_file_error_injector.h:23: // Will be owned by the |DownloadFileManager| via a |DownloadFileFactory| On 2012/02/27 02:18:54, cbentzel wrote: > Yeah - this ownership is still really strange - especially because it's hidden. Switched to using RefCountedThreadSafe for the injector.
http://codereview.chromium.org/9426029/diff/28004/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9426029/diff/28004/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:756: injector->InjectError(0, // First DownloadFile created. On 2012/02/28 03:28:56, ahendrickson wrote: > On 2012/02/27 02:18:54, cbentzel wrote: > > I'd remove this argument for now. > > How strongly do you feel about that? Pretty strongly. I don't like adding arguments that are unneeded/unused: it adds complexity to the API, to callers, and to implementation. http://codereview.chromium.org/9426029/diff/28004/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:758: info.operation_index, On 2012/02/28 03:28:56, ahendrickson wrote: > On 2012/02/27 02:18:54, cbentzel wrote: > > Do you need operation_index? It looks like it's always set to 0. > > Right now the unit tests only try to download small files, which result in > having only one write operation. I'd like to have tests in the future that use > larger files, and have an error in, say, the 5th operation. Again, I'd hold off until you have concrete need for this.
Getting closer. http://codereview.chromium.org/9426029/diff/20002/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9426029/diff/20002/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:750: // |injector| will be owned by the DownloadFileManager (indirectly). This comment about ownership is no longer true - it's shared ownership. http://codereview.chromium.org/9426029/diff/20002/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:758: ui_test_utils::RunAllPendingInMessageLoop(); Are you guaranteed that all of the work on the file thread has been completed, and that all of the completed messages that get posted back to the UI thread are complete? I didn't do a full analysis, but I am worried about potential for flake here if the ordering isn't completely right. Also worried about reported leaks of the TestFileErrorInjector here. http://codereview.chromium.org/9426029/diff/20002/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:761: EXPECT_EQ(false, injector->HasFile(0)); Do you need this HasFile call? Doesn't the CurrentFileCount indicate it? http://codereview.chromium.org/9426029/diff/20002/content/browser/download/do... File content/browser/download/download_file_manager.h (right): http://codereview.chromium.org/9426029/diff/20002/content/browser/download/do... content/browser/download/download_file_manager.h:147: // This is only for unit tests. Comment doesn't totally seem required given the ForTesting response, but I'd have Randy+John provide judgement on that instead of me. http://codereview.chromium.org/9426029/diff/20002/content/browser/download/do... content/browser/download/download_file_manager.h:148: void SetFileFactoryForTesting(DownloadFileFactory* file_factory) { This should pass in a scoped_ptr argument so transfer of ownership is obvious. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.cc:32: typedef base::Callback<void(int index, bool creating)> DownloadFileCallback; Design nit: You typically want this on the DownloadFileWithErrors, not on the TestFileErrorInjectorImpl. The DownloadFileWithErrors class is the one that gets the callback, as well as operates on it. TestFileErrorInjectorImpl simply binds one of it's functions to be used with it. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.cc:44: virtual void DownloadFileCreated(int file_index); These don't need to be virtual. Could also be private. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.cc:188: int counter = operation_counter_[code]; I'd get rid of the operation_counter_ map here - not needed since we only care about operation_instance of 0. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.cc:257: injected_errors_[error_info.file_index] = error_info; Why are you worried about adding support for a map of files at this point when a) you aren't using it and b) haven't exercised it? Adding extra complexity that isn't needed is a bad idea. If you have some tests that will take advantage of this, let me know what they are. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.cc:281: // Transfers ownership to |DownloadFileManager|. Use scoped_ptr.Pass here instead. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.cc:342: #define TO_STRING(code) \ Although this macro is localized, it provides very little advantage over just writing directly. Also note that you don't need to clarify the content namespace here. Also - given that this is only used in this translation unit, you may just want to make it a non-static method and only display it here. I'm not even sure that the VLOG is needed, but given that this is test-only code won't push too hard against it. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.h:38: // Adds an error. Must be called before InjectErrors(). Nit: |InjectErrors()| http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.h:41: // Injects the errors. Can be called only once. Nit: Comment more about how this impacts the DownloadFileManager, the fact that it needs to already exist, etc. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.h:46: virtual size_t FileCreationCount() const = 0; Nit: Perhaps TotalFileCount to match CurrentFileCount? http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.h:49: // Debugging only. This isn't needed - or at least needed for public code since never used.
I think we should pull John in at some point to give the test injection interface a scan through, but probably after Chris and I are happy with it. http://codereview.chromium.org/9426029/diff/20002/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9426029/diff/20002/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:2111: IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorFileNoSpaceOpenNavigate) { There's a fair amount of overhead in starting up and shutting down a browser test instance. Any chance that these tests could be combined into one test that cycled through all the different errors? This would require allowing InjectErrors() to be called more than once (and defining clean semantics for that). http://codereview.chromium.org/9426029/diff/20002/content/browser/download/do... File content/browser/download/download_file_manager.h (right): http://codereview.chromium.org/9426029/diff/20002/content/browser/download/do... content/browser/download/download_file_manager.h:147: // This is only for unit tests. On 2012/02/28 14:51:41, cbentzel wrote: > Comment doesn't totally seem required given the ForTesting response, but I'd > have Randy+John provide judgement on that instead of me. Agreed with Chris. (Personal opinion is that we don't need John's opinion on the change to DFM since it isn't in content/public, but I have no objections to roping him in if either of you feel like it would be useful, and I would like him to at least look at the code in content/test.) http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.cc:43: // Callbacks from the download file, to record lifetimes. nit: Aren't these callbacks from the factory, not the file? http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.cc:54: // Our injected error list, mapped by file index. One per file. This is the first time I noticed that we could only have one injected error per file. Is this documented in the interface? http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.cc:228: injected_errors_[file_counter_++], What happens if this accesses an uninitialized entry in the array? http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.cc:238: injected_errors_[error_info.file_index] = error_info; Should this interface allow us to overwrite already existing entries in the array? http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.cc:276: download_file_factory->AddError(it->second); nit, suggestion: You could create download_file_factory on object construction, and accumulate the errors directly in that object to avoid having to keep TestFileErrorInjectorImpl::injected_errors_ around. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.h:17: : public base::RefCountedThreadSafe<TestFileErrorInjector> { I'd like some documentation as to why this class is RefCountedThreadSafe<>. That might be more appropriate in the implementation file than here, but it's declared here, so you should as least have a reference to the explanation. And if you can explain (in terms that are restricted to what's known by content customers) why we're RCTS here, that's better. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.h:27: int file_index; // 0-based index into created DownloadFile instances. I don't like references to DownloadFile in this file since this is part of the content/chrome interface and DownloadFile is not. Can you phrase it some other way? http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.h:27: int file_index; // 0-based index into created DownloadFile instances. I don't like 0 based indices, because they presume that there's some specified order (i.e. no racing) to download initiation. Maybe you could identify files by target name? Do we know the URL on the file thread? I'm looking for some other key than initiation order, which seems fragile to me. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.h:29: int operation_instance; // 0-based count of operation calls. These two options strike me as having some redundancy--i.e. the operation_instance fully specifies the operation to inject the error into, and the code is an extra restriction. Document what happens when these conflict? http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.h:36: static scoped_refptr<TestFileErrorInjector> Create(); Could you specify whether this has singleton semantics? If so, this should probably be termed "Get()". If not, what happens if you create multiple instances of this class? http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.h:41: // Injects the errors. Can be called only once. On 2012/02/28 14:51:41, cbentzel wrote: > Nit: Comment more about how this impacts the DownloadFileManager, the fact that > it needs to already exist, etc. In an ideal world, comments in this file would not refer to the DownloadFileManager, since this file is part of the content/chrome interface and the DFM is not. I.e. try to find a way to include the information Chris is requesting without refering to the DFM.
The last patch set added a lot of files for the callback interface; could that go in a separate CL that this one would then depend on?
On 2012/02/28 22:06:47, rdsmith wrote: > The last patch set added a lot of files for the callback interface; could that > go in a separate CL that this one would then depend on? Why did this need to go into DownloadManager's interface? I thought it was just going into the DownloadFileWithErrors+Factory.
I'll split off parts of this into other CLs soon. http://codereview.chromium.org/9426029/diff/20002/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9426029/diff/20002/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:750: // |injector| will be owned by the DownloadFileManager (indirectly). On 2012/02/28 14:51:41, cbentzel wrote: > This comment about ownership is no longer true - it's shared ownership. Done. http://codereview.chromium.org/9426029/diff/20002/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:758: ui_test_utils::RunAllPendingInMessageLoop(); On 2012/02/28 14:51:41, cbentzel wrote: > Are you guaranteed that all of the work on the file thread has been completed, > and that all of the completed messages that get posted back to the UI thread are > complete? > > I didn't do a full analysis, but I am worried about potential for flake here if > the ordering isn't completely right. Also worried about reported leaks of the > TestFileErrorInjector here. > Added code to run all pending messages on the FILE thread. Our notifications happen on the UI thread. http://codereview.chromium.org/9426029/diff/20002/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:761: EXPECT_EQ(false, injector->HasFile(0)); On 2012/02/28 14:51:41, cbentzel wrote: > Do you need this HasFile call? Doesn't the CurrentFileCount indicate it? Hmm, I guess it's not needed. Removed. http://codereview.chromium.org/9426029/diff/20002/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:2111: IN_PROC_BROWSER_TEST_F(DownloadTest, DownloadErrorFileNoSpaceOpenNavigate) { On 2012/02/28 22:06:13, rdsmith wrote: > There's a fair amount of overhead in starting up and shutting down a browser > test instance. Any chance that these tests could be combined into one test that > cycled through all the different errors? This would require allowing > InjectErrors() to be called more than once (and defining clean semantics for > that). Done. http://codereview.chromium.org/9426029/diff/20002/content/browser/download/do... File content/browser/download/download_file_manager.h (right): http://codereview.chromium.org/9426029/diff/20002/content/browser/download/do... content/browser/download/download_file_manager.h:147: // This is only for unit tests. On 2012/02/28 14:51:41, cbentzel wrote: > Comment doesn't totally seem required given the ForTesting response, but I'd > have Randy+John provide judgement on that instead of me. Removed comment. http://codereview.chromium.org/9426029/diff/20002/content/browser/download/do... content/browser/download/download_file_manager.h:147: // This is only for unit tests. On 2012/02/28 22:06:13, rdsmith wrote: > On 2012/02/28 14:51:41, cbentzel wrote: > > Comment doesn't totally seem required given the ForTesting response, but I'd > > have Randy+John provide judgement on that instead of me. > > Agreed with Chris. > > (Personal opinion is that we don't need John's opinion on the change to DFM > since it isn't in content/public, but I have no objections to roping him in if > either of you feel like it would be useful, and I would like him to at least > look at the code in content/test.) Done. http://codereview.chromium.org/9426029/diff/20002/content/browser/download/do... content/browser/download/download_file_manager.h:148: void SetFileFactoryForTesting(DownloadFileFactory* file_factory) { On 2012/02/28 14:51:41, cbentzel wrote: > This should pass in a scoped_ptr argument so transfer of ownership is obvious. Done. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.cc:32: typedef base::Callback<void(int index, bool creating)> DownloadFileCallback; On 2012/02/28 14:51:41, cbentzel wrote: > Design nit: You typically want this on the DownloadFileWithErrors, not on the > TestFileErrorInjectorImpl. The DownloadFileWithErrors class is the one that gets > the callback, as well as operates on it. TestFileErrorInjectorImpl simply binds > one of it's functions to be used with it. I wanted this class to be the first one declared. My choices are: - rearrange the classes and move the typedef, - make the typedef external to the classes, or - leave the typedef here. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.cc:43: // Callbacks from the download file, to record lifetimes. On 2012/02/28 22:06:13, rdsmith wrote: > nit: Aren't these callbacks from the factory, not the file? No, the factory passes the callback objects to the files, which then use them. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.cc:44: virtual void DownloadFileCreated(int file_index); On 2012/02/28 14:51:41, cbentzel wrote: > These don't need to be virtual. Could also be private. Done. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.cc:54: // Our injected error list, mapped by file index. One per file. On 2012/02/28 22:06:13, rdsmith wrote: > This is the first time I noticed that we could only have one injected error per > file. Is this documented in the interface? I added a comment to AddError(). http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.cc:228: injected_errors_[file_counter_++], On 2012/02/28 22:06:13, rdsmith wrote: > What happens if this accesses an uninitialized entry in the array? No error is injected. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.cc:238: injected_errors_[error_info.file_index] = error_info; On 2012/02/28 22:06:13, rdsmith wrote: > Should this interface allow us to overwrite already existing entries in the > array? No, although it just replaces the existing value. Fixed. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.cc:257: injected_errors_[error_info.file_index] = error_info; On 2012/02/28 14:51:41, cbentzel wrote: > Why are you worried about adding support for a map of files at this point when > a) you aren't using it and b) haven't exercised it? > > Adding extra complexity that isn't needed is a bad idea. If you have some tests > that will take advantage of this, let me know what they are. The new tests run multiple downloads, so we need this. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.cc:276: download_file_factory->AddError(it->second); On 2012/02/28 22:06:13, rdsmith wrote: > nit, suggestion: You could create download_file_factory on object construction, > and accumulate the errors directly in that object to avoid having to keep > TestFileErrorInjectorImpl::injected_errors_ around. I did that in an earlier pass, but Chris asked me to change it: On 2012/02/27 02:18:54, cbentzel wrote: > It might be clearer to have the API be like > > TestFileErrorInjector() > AddError(); > AddError(); > Inject() > > Inject would actually create the DownloadFileErrorManager and inject into the > DownloadFileManager. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.cc:281: // Transfers ownership to |DownloadFileManager|. On 2012/02/28 14:51:41, cbentzel wrote: > Use scoped_ptr.Pass here instead. Done. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.cc:342: #define TO_STRING(code) \ On 2012/02/28 14:51:41, cbentzel wrote: > Although this macro is localized, it provides very little advantage over just > writing directly. Also note that you don't need to clarify the content namespace > here. > > Also - given that this is only used in this translation unit, you may just want > to make it a non-static method and only display it here. I'm not even sure that > the VLOG is needed, but given that this is test-only code won't push too hard > against it. Removed the namespace reference. It is now used in the browser tests. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.h:17: : public base::RefCountedThreadSafe<TestFileErrorInjector> { On 2012/02/28 22:06:13, rdsmith wrote: > I'd like some documentation as to why this class is RefCountedThreadSafe<>. > That might be more appropriate in the implementation file than here, but it's > declared here, so you should as least have a reference to the explanation. And > if you can explain (in terms that are restricted to what's known by content > customers) why we're RCTS here, that's better. Done. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.h:27: int file_index; // 0-based index into created DownloadFile instances. On 2012/02/28 22:06:13, rdsmith wrote: > I don't like references to DownloadFile in this file since this is part of the > content/chrome interface and DownloadFile is not. Can you phrase it some other > way? Done. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.h:27: int file_index; // 0-based index into created DownloadFile instances. On 2012/02/28 22:06:13, rdsmith wrote: > I don't like 0 based indices, because they presume that there's some specified > order (i.e. no racing) to download initiation. Maybe you could identify files > by target name? Do we know the URL on the file thread? I'm looking for some > other key than initiation order, which seems fragile to me. There will be multiple calls with the same URL, and we don't have access to the uniquified file names to distinguish them by. The tests that use this class will only download one file at a time, so that the order of downloads is guaranteed. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.h:29: int operation_instance; // 0-based count of operation calls. On 2012/02/28 22:06:13, rdsmith wrote: > These two options strike me as having some redundancy--i.e. the > operation_instance fully specifies the operation to inject the error into, and > the code is an extra restriction. Document what happens when these conflict? No, they're not redundant. The code specifies the function to inject an error into (Initialize, write, etc.). The instance says how many calls to wait for before inserting an error. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.h:36: static scoped_refptr<TestFileErrorInjector> Create(); On 2012/02/28 22:06:13, rdsmith wrote: > Could you specify whether this has singleton semantics? If so, this should > probably be termed "Get()". If not, what happens if you create multiple > instances of this class? Hmm, I suppose it should have singleton semantics. It would definitely have unexpected behavior (from the test writer's perspective) if it were called more than once. Changed. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.h:38: // Adds an error. Must be called before InjectErrors(). On 2012/02/28 14:51:41, cbentzel wrote: > Nit: |InjectErrors()| Done. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.h:41: // Injects the errors. Can be called only once. On 2012/02/28 14:51:41, cbentzel wrote: > Nit: Comment more about how this impacts the DownloadFileManager, the fact that > it needs to already exist, etc. Done. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.h:41: // Injects the errors. Can be called only once. On 2012/02/28 22:06:13, rdsmith wrote: > On 2012/02/28 14:51:41, cbentzel wrote: > > Nit: Comment more about how this impacts the DownloadFileManager, the fact > that > > it needs to already exist, etc. > > In an ideal world, comments in this file would not refer to the > DownloadFileManager, since this file is part of the content/chrome interface and > the DFM is not. I.e. try to find a way to include the information Chris is > requesting without refering to the DFM. > Done. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.h:46: virtual size_t FileCreationCount() const = 0; On 2012/02/28 14:51:41, cbentzel wrote: > Nit: Perhaps TotalFileCount to match CurrentFileCount? Done. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.h:49: // Debugging only. On 2012/02/28 14:51:41, cbentzel wrote: > This isn't needed - or at least needed for public code since never used. It's used now.
Forgot to update one comment. Here it is. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.h:27: int file_index; // 0-based index into created DownloadFile instances. On 2012/03/01 09:17:32, ahendrickson wrote: > On 2012/02/28 22:06:13, rdsmith wrote: > > I don't like 0 based indices, because they presume that there's some specified > > order (i.e. no racing) to download initiation. Maybe you could identify files > > by target name? Do we know the URL on the file thread? I'm looking for some > > other key than initiation order, which seems fragile to me. > > There will be multiple calls with the same URL, and we don't have access to the > uniquified file names to distinguish them by. > > The tests that use this class will only download one file at a time, so that the > order of downloads is guaranteed. Now using the URL as the distinguishing value, allowing multiple downloads to be uniquely identified as long as we don't have two with the same URL downloading at once.
On 2012/02/29 19:13:24, cbentzel wrote: > On 2012/02/28 22:06:47, rdsmith wrote: > > The last patch set added a lot of files for the callback interface; could that > > go in a separate CL that this one would then depend on? > > Why did this need to go into DownloadManager's interface? I thought it was just > going into the DownloadFileWithErrors+Factory. Because the callback needs to be added to DownloadManager::DownloadUrl(), and so is exposed outside of content.
Split off 2 CLs.
Please see the BUG in TestFileErrorInjectorImpl. Also, I am concerned that this isn't waiting long enough for a navigation-triggered download but I could be wrong. http://codereview.chromium.org/9426029/diff/43020/content/browser/download/do... File content/browser/download/download_file_manager.h (right): http://codereview.chromium.org/9426029/diff/43020/content/browser/download/do... content/browser/download/download_file_manager.h:148: download_file_factory_.swap(file_factory); What are you trying to do with the swap here (as opposed to reset?) http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:35: typedef base::Callback<void(GURL url, content::DownloadId id)> It's a bit uncommon to have these on the TestFileErrorInjectorImpl instead of the Factory/File. As you see, the TestFileErrorInjectorImpl does not directly depend on these typedefs, but the File/Factory do. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:177: // Record creation. Comments are a bit inaccurate here - the DownloadFileWithErrors doesn't actually know about what's going on inside the callbacks. The code is also pretty self-explanatory. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:190: DownloadFileImpl::Initialize()); Question: Right now this always runs the underlying DownloadFileImpl:: methods even if we'll override with an injected error. Is that the behavior you want, or do you want to short-circuit that? http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:253: injected_errors_[info->url().spec()], This is a bug/flakyness issue if info->url().spec() is not in the map - which this seems to allow. FileErrorInfo is not a POD struct (it has std::string) so the default constructor generated by the compiler provides no guarantees about the values of primitive members such as code, operation_instance, and error. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:260: DCHECK_LE(0, error_info.operation_instance); This is redundant with the check in TestFileErrorInjectorImpl::AddError which is the public interface. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:285: injected_errors_[error_info.url] = error_info; You could also create the factory early in the injector and just populate the factory, rather than having a local map. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:300: if (!created_factory_) { Is it valid for InjectErrors to be called more than once? If it is, should you also clear out files_ and found_files_? http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:372: files_[url] = info; Why not just maintain a map from url to id, instead of this InjectedFileInfo struct? Seems like it would be sufficient. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:377: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); Should you DCHECK that |url| is in files_ here? http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:408: scoped_refptr<TestFileErrorInjector> single_injector_; What's going on here? This is always going to be NULL. Was this intended to be a static? http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.h:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. 2012 http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.h:47: // Creates an instance. Should this be called Create instead? http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.h:70: static std::string DebugString(FileOperationCode code); This doesn't need to be part of the public interface. http://codereview.chromium.org/9426029/diff/43021/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9426029/diff/43021/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:713: LOG(WARNING) << " " << __FUNCTION__ << "()" Remove this LOG(WARNING) http://codereview.chromium.org/9426029/diff/43021/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:754: // Navigate to URL normally, wait until done. Does this actually wait for the download to complete, or wait until it's been able to determine that the navigation is going to be a download? http://codereview.chromium.org/9426029/diff/43021/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:795: size_t i; This should be scoped in the for loop. http://codereview.chromium.org/9426029/diff/43021/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:806: LOG(WARNING) << " " << __FUNCTION__ << "()" Remove this LOG(WARNING) http://codereview.chromium.org/9426029/diff/43021/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:816: injector->ClearFoundFiles(); Now that you have the per-URL map and are using it, could you just set up the TestFileErrorInjector once and bypass all of the ClearFoundFiles()/ClearErrors()/AddError() stuff? http://codereview.chromium.org/9426029/diff/43021/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:1951: DownloadManager::OnStartedCallback()); Is there support for this in the CL? Or is this from the split?
http://codereview.chromium.org/9426029/diff/43020/content/browser/download/do... File content/browser/download/download_file_manager.h (right): http://codereview.chromium.org/9426029/diff/43020/content/browser/download/do... content/browser/download/download_file_manager.h:148: download_file_factory_.swap(file_factory); On 2012/03/01 15:44:20, cbentzel wrote: > What are you trying to do with the swap here (as opposed to reset?) This effectively does the same thing as download_file_factory_.reset(file_factory.release()); swap() method: file_factory will destroy the old pointer when it goes out of scope. reset() method: the old pointer is destroyed when by reset(), and file_factory becomes NULL. I can use the reset() method if you't prefer. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:35: typedef base::Callback<void(GURL url, content::DownloadId id)> On 2012/03/01 15:44:20, cbentzel wrote: > It's a bit uncommon to have these on the TestFileErrorInjectorImpl instead of > the Factory/File. As you see, the TestFileErrorInjectorImpl does not directly > depend on these typedefs, but the File/Factory do. I'd prefer to have TestFileErrorInjectorImpl declared before File/Factory, which means that the typedefs have to be declared before File/Factory. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:177: // Record creation. On 2012/03/01 15:44:20, cbentzel wrote: > Comments are a bit inaccurate here - the DownloadFileWithErrors doesn't actually > know about what's going on inside the callbacks. The code is also pretty > self-explanatory. Removed. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:190: DownloadFileImpl::Initialize()); On 2012/03/01 15:44:20, cbentzel wrote: > Question: Right now this always runs the underlying DownloadFileImpl:: methods > even if we'll override with an injected error. Is that the behavior you want, or > do you want to short-circuit that? It is the intended behavior. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:253: injected_errors_[info->url().spec()], On 2012/03/01 15:44:20, cbentzel wrote: > This is a bug/flakyness issue if info->url().spec() is not in the map - which > this seems to allow. > > FileErrorInfo is not a POD struct (it has std::string) so the default > constructor generated by the compiler provides no guarantees about the values of > primitive members such as code, operation_instance, and error. Fixed. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:260: DCHECK_LE(0, error_info.operation_instance); On 2012/03/01 15:44:20, cbentzel wrote: > This is redundant with the check in TestFileErrorInjectorImpl::AddError which is > the public interface. Done. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:285: injected_errors_[error_info.url] = error_info; On 2012/03/01 15:44:20, cbentzel wrote: > You could also create the factory early in the injector and just populate the > factory, rather than having a local map. In an earlier comment, you asked that the factory be created at injection time. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:300: if (!created_factory_) { On 2012/03/01 15:44:20, cbentzel wrote: > Is it valid for InjectErrors to be called more than once? If it is, should you > also clear out files_ and found_files_? It is. At the moment, the test checks that files_ is empty between downloads, and clears found_files_ itself. I moved the clearing of found_files_ here. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:372: files_[url] = info; On 2012/03/01 15:44:20, cbentzel wrote: > Why not just maintain a map from url to id, instead of this InjectedFileInfo > struct? Seems like it would be sufficient. DownloadId doesn't have a default constructor, so that won't work. Added a default constructor. Done. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:377: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); On 2012/03/01 15:44:20, cbentzel wrote: > Should you DCHECK that |url| is in files_ here? Done. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:408: scoped_refptr<TestFileErrorInjector> single_injector_; On 2012/03/01 15:44:20, cbentzel wrote: > What's going on here? This is always going to be NULL. Was this intended to be a > static? It was. Fixed. Sadly this means that the injector will hang around until the program ends. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.h:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2012/03/01 15:44:20, cbentzel wrote: > 2012 Done. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.h:47: // Creates an instance. On 2012/03/01 15:44:20, cbentzel wrote: > Should this be called Create instead? Randy pointed out that if this has singleton semantics (which I think it must), the function should be called Get(). http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.h:70: static std::string DebugString(FileOperationCode code); On 2012/03/01 15:44:20, cbentzel wrote: > This doesn't need to be part of the public interface. Then how do I use it in the browser test? http://codereview.chromium.org/9426029/diff/43021/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9426029/diff/43021/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:713: LOG(WARNING) << " " << __FUNCTION__ << "()" On 2012/03/01 15:44:20, cbentzel wrote: > Remove this LOG(WARNING) Done. http://codereview.chromium.org/9426029/diff/43021/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:754: // Navigate to URL normally, wait until done. On 2012/03/01 15:44:20, cbentzel wrote: > Does this actually wait for the download to complete, or wait until it's been > able to determine that the navigation is going to be a download? It waits until the navigation is complete. http://codereview.chromium.org/9426029/diff/43021/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:795: size_t i; On 2012/03/01 15:44:20, cbentzel wrote: > This should be scoped in the for loop. Done. http://codereview.chromium.org/9426029/diff/43021/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:806: LOG(WARNING) << " " << __FUNCTION__ << "()" On 2012/03/01 15:44:20, cbentzel wrote: > Remove this LOG(WARNING) Done. http://codereview.chromium.org/9426029/diff/43021/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:816: injector->ClearFoundFiles(); On 2012/03/01 15:44:20, cbentzel wrote: > Now that you have the per-URL map and are using it, could you just set up the > TestFileErrorInjector once and bypass all of the > ClearFoundFiles()/ClearErrors()/AddError() stuff? No, because we're reusing the same URL and injecting different errors each time. http://codereview.chromium.org/9426029/diff/43021/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:1951: DownloadManager::OnStartedCallback()); On 2012/03/01 15:44:20, cbentzel wrote: > Is there support for this in the CL? Or is this from the split? This is from the split.
I'm focussing on the error injection interface for the moment; I'll come back and evaluate the actual tests once we have that nailed down. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.cc:43: // Callbacks from the download file, to record lifetimes. On 2012/03/01 09:17:32, ahendrickson wrote: > On 2012/02/28 22:06:13, rdsmith wrote: > > nit: Aren't these callbacks from the factory, not the file? > > No, the factory passes the callback objects to the files, which then use them. Right, got it--sorry for the confusion. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.cc:228: injected_errors_[file_counter_++], On 2012/03/01 09:17:32, ahendrickson wrote: > On 2012/02/28 22:06:13, rdsmith wrote: > > What happens if this accesses an uninitialized entry in the array? > > No error is injected. I think Chris did the analysis that I was worried about here and trying to point towards: If we let the system default create an instance of the array, we don't know what's going to go into it. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.cc:276: download_file_factory->AddError(it->second); On 2012/03/01 09:17:32, ahendrickson wrote: > On 2012/02/28 22:06:13, rdsmith wrote: > > nit, suggestion: You could create download_file_factory on object > construction, > > and accumulate the errors directly in that object to avoid having to keep > > TestFileErrorInjectorImpl::injected_errors_ around. > > I did that in an earlier pass, but Chris asked me to change it: Given that I think Chris just suggested the same idea I did, you might want to circle back with him and make sure you understood his earlier concern. > > On 2012/02/27 02:18:54, cbentzel wrote: > > It might be clearer to have the API be like > > > > TestFileErrorInjector() > > AddError(); > > AddError(); > > Inject() > > > > Inject would actually create the DownloadFileErrorManager and inject into the > > DownloadFileManager. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.h:29: int operation_instance; // 0-based count of operation calls. On 2012/03/01 09:17:32, ahendrickson wrote: > On 2012/02/28 22:06:13, rdsmith wrote: > > These two options strike me as having some redundancy--i.e. the > > operation_instance fully specifies the operation to inject the error into, and > > the code is an extra restriction. Document what happens when these conflict? > > No, they're not redundant. The code specifies the function to inject an error > into (Initialize, write, etc.). The instance says how many calls to wait for > before inserting an error. Ah, got it. The key point I was missing was that the operation count that operation_instance was matched against was per operation type. You might want to modify the comment above. "... operation calls with code code" or something like it. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:35: typedef base::Callback<void(GURL url, content::DownloadId id)> On 2012/03/01 20:19:31, ahendrickson wrote: > On 2012/03/01 15:44:20, cbentzel wrote: > > It's a bit uncommon to have these on the TestFileErrorInjectorImpl instead of > > the Factory/File. As you see, the TestFileErrorInjectorImpl does not directly > > depend on these typedefs, but the File/Factory do. > > I'd prefer to have TestFileErrorInjectorImpl declared before File/Factory, which > means that the typedefs have to be declared before File/Factory. Why do you prefer that order? I agree with Chris that the interface is a little less clean having the typedef here. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:59: typedef std::map<GURL, InjectedFileInfo> FileMap; Why is this a map to InjectedFileInfo instead of just id? You never use the rest of the structure, as best I can tell, and it's redundant with the key anyway, which you always have access to (as best I can tell). http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:73: // Keep track of active DownloadFiles. How are you dealing with the threading issues involved here? (This class accessed on the UI thread by the callbacks actually happening on the file thread.) http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.h:47: // Creates an instance. On 2012/03/01 15:44:20, cbentzel wrote: > Should this be called Create instead? It was shifted in response to a comment of mine. It looked to me that it didn't make sense for TestFileErrorInjector not to have singleton semantics (if you have two, I think things get somewhat wonky), so I suggested singleton semantics, and the shift to Get was part of that. The comment should be updated, though. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.h:63: // Get information about the DownloadFiles created. Could you add some comments for these interfaces? What's the difference between CurrentFileCount() and TotalFileCount()? What does HadFile() mean? If you do two different downloads with the same URL serially, which does GetId() return? What does ClearFoundFiles() do/which of the other interfaces does it affect, and which doesn't it affect? http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... content/test/test_file_error_injector.cc:150: // Our injected error list, mapped by file index. One per file. This comment is no longer correct, right? http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... content/test/test_file_error_injector.cc:318: download_file_manager->GetFileFactoryForTesting()); An alternative implementation would be to make file_factory_ a member variable (of type DownloadFileWithErrorsFactory*) that was NULL before it was created. That would allow you to avoid the static cast (and would not increase the member variaables since created_factory_ would go away), but would have the minus of your having to reason about and document the lifetime issues. But I think the code fragility is about the same, since anything that would change get the DFM's idea of the file factory out of sync with the TFEII's would probably also violate the static cast assumption. Just a thought; up to you. http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... content/test/test_file_error_injector.h:27: // URLs are different. nit, suggestion: MIght be useful to explain why this restriction is in place. It's an interface level issue, so it wouldn't be out of place here. http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... content/test/test_file_error_injector.h:27: // URLs are different. Could you put in/have you put in a DCHECK() to confirm this? http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... content/test/test_file_error_injector.h:52: // Only one error per file will be used. So what happens if more than one AddError is called for a particular file? First one wins? Second one wins? Interface violation? (If the last, I'll want a DCHECK confirming it.) http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... content/test/test_file_error_injector.h:56: // Doesn't affect files already created. Based on the implementation, I think this comment is misleading, as the real boundary about what files are effected has to do with when InjectErrors() is called. I.e. if you called AddError(), InjectErrors(), ClearError(), and created a file, it would have the info from the add error. http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... content/test/test_file_error_injector.h:57: virtual void ClearErrors() = 0; It might be illuminating to include a comment with an example use of this class for a couple of files. http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... content/test/test_file_error_injector.h:61: virtual bool InjectErrors() = 0; It's not clear to me reading this header file how multiple calls to InjectErrors() is expected to be used. Are they allowed? If you do them, do they only inject the errors accumulated since the last call to InjectErrors()? Are previously injected errors cleared? (I think I've said this before, but my standard for classes is that the .h file have all the information in it needed for a consumer to use the class between the comments and the code.) http://codereview.chromium.org/9426029/diff/46004/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/46004/content/test/test_file_erro... content/test/test_file_error_injector.cc:411: scoped_refptr<TestFileErrorInjector> single_injector_; I think this needs to be static.
http://codereview.chromium.org/9426029/diff/43020/content/browser/download/do... File content/browser/download/download_file_manager.h (right): http://codereview.chromium.org/9426029/diff/43020/content/browser/download/do... content/browser/download/download_file_manager.h:148: download_file_factory_.swap(file_factory); On 2012/03/01 20:19:31, ahendrickson wrote: > On 2012/03/01 15:44:20, cbentzel wrote: > > What are you trying to do with the swap here (as opposed to reset?) > > This effectively does the same thing as > > download_file_factory_.reset(file_factory.release()); > > swap() method: file_factory will destroy the old pointer when it goes out of > scope. > > reset() method: the old pointer is destroyed when by reset(), and file_factory > becomes NULL. > > I can use the reset() method if you't prefer. > OK, normally when I see swap I expect there to be a transfer of ownership. In this case, the ownership would transfer but then file_factory will go out of scope in this block. This is fine. Maybe you could use Pass here? http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:408: scoped_refptr<TestFileErrorInjector> single_injector_; On 2012/03/01 20:19:31, ahendrickson wrote: > On 2012/03/01 15:44:20, cbentzel wrote: > > What's going on here? This is always going to be NULL. Was this intended to be > a > > static? > > It was. Fixed. > > Sadly this means that the injector will hang around until the program ends. I'll chat with Randy. I don't understand why this should be all singleton'y. It's only called once from the test driver. And then you have a static non-POD class around, which is totally frowned upon by the style guide. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.h:70: static std::string DebugString(FileOperationCode code); On 2012/03/01 20:19:31, ahendrickson wrote: > On 2012/03/01 15:44:20, cbentzel wrote: > > This doesn't need to be part of the public interface. > > Then how do I use it in the browser test? Ah OK. Then it does. Not sure if it should, but not going to fight.
http://codereview.chromium.org/9426029/diff/52001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9426029/diff/52001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:5: #include <sstream> Why not just use stringprintf to create this?
You were talking about doing some clean ups here with creating the factory early. If so, this seems to be going on a pretty good path. You'll want Randy or someone else on the downloads team to do the L G T M though. http://codereview.chromium.org/9426029/diff/52001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9426029/diff/52001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:703: s << " " << __FUNCTION__ << "()" I guess stringstream makes sense here. http://codereview.chromium.org/9426029/diff/52001/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/52001/content/test/test_file_erro... content/test/test_file_error_injector.cc:240: if (injected_errors_.find(url) == injected_errors_.end()) { Just as a question - do you ever expect this case to happen? Could you just DCHECK or return NULL? http://codereview.chromium.org/9426029/diff/52001/content/test/test_file_erro... content/test/test_file_error_injector.cc:242: content::TestFileErrorInjector::FileErrorInfo err_info = { You could also define a default constructor, that would be cleaner. http://codereview.chromium.org/9426029/diff/52001/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/52001/content/test/test_file_erro... content/test/test_file_error_injector.h:12: #include "content/public/browser/download_id.h" I think you can forward declare download_id since it's just being used for a return value.
http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.cc:188: int counter = operation_counter_[code]; On 2012/02/28 14:51:41, cbentzel wrote: > I'd get rid of the operation_counter_ map here - not needed since we only care > about operation_instance of 0. We're now using operation instances other than 0. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.cc:228: injected_errors_[file_counter_++], On 2012/03/01 20:28:24, rdsmith wrote: > On 2012/03/01 09:17:32, ahendrickson wrote: > > On 2012/02/28 22:06:13, rdsmith wrote: > > > What happens if this accesses an uninitialized entry in the array? > > > > No error is injected. > > I think Chris did the analysis that I was worried about here and trying to point > towards: If we let the system default create an instance of the array, we don't > know what's going to go into it. Fixed. http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.h:29: int operation_instance; // 0-based count of operation calls. On 2012/03/01 20:28:24, rdsmith wrote: > On 2012/03/01 09:17:32, ahendrickson wrote: > > On 2012/02/28 22:06:13, rdsmith wrote: > > > These two options strike me as having some redundancy--i.e. the > > > operation_instance fully specifies the operation to inject the error into, > and > > > the code is an extra restriction. Document what happens when these > conflict? > > > > No, they're not redundant. The code specifies the function to inject an error > > into (Initialize, write, etc.). The instance says how many calls to wait for > > before inserting an error. > > Ah, got it. The key point I was missing was that the operation count that > operation_instance was matched against was per operation type. You might want > to modify the comment above. "... operation calls with code code" or something > like it. Done. http://codereview.chromium.org/9426029/diff/43020/content/browser/download/do... File content/browser/download/download_file_manager.h (right): http://codereview.chromium.org/9426029/diff/43020/content/browser/download/do... content/browser/download/download_file_manager.h:148: download_file_factory_.swap(file_factory); On 2012/03/01 22:32:00, cbentzel wrote: > On 2012/03/01 20:19:31, ahendrickson wrote: > > On 2012/03/01 15:44:20, cbentzel wrote: > > > What are you trying to do with the swap here (as opposed to reset?) > > > > This effectively does the same thing as > > > > download_file_factory_.reset(file_factory.release()); > > > > swap() method: file_factory will destroy the old pointer when it goes out of > > scope. > > > > reset() method: the old pointer is destroyed when by reset(), and > file_factory > > becomes NULL. > > > > I can use the reset() method if you't prefer. > > > > OK, normally when I see swap I expect there to be a transfer of ownership. In > this case, the ownership would transfer but then file_factory will go out of > scope in this block. This is fine. Maybe you could use Pass here? Went the reset() way. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:59: typedef std::map<GURL, InjectedFileInfo> FileMap; On 2012/03/01 20:28:25, rdsmith wrote: > Why is this a map to InjectedFileInfo instead of just id? You never use the > rest of the structure, as best I can tell, and it's redundant with the key > anyway, which you always have access to (as best I can tell). Fixed in a later patch. The reason it didn't have a DownloadId as the value was that the latter didn't have a default constructor. I added one after consulting with Ben. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:73: // Keep track of active DownloadFiles. On 2012/03/01 20:28:25, rdsmith wrote: > How are you dealing with the threading issues involved here? (This class > accessed on the UI thread by the callbacks actually happening on the file > thread.) In my test code, I'm setting up all the injected errors prior to accessing any of them (within the download loop body), so there is no threading conflict. I have now set up a FILE-thread version of InjectErrors(), that takes a copy of the injected errors. The callbacks post to the UI thread for updating the records. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:285: injected_errors_[error_info.url] = error_info; On 2012/03/01 20:19:31, ahendrickson wrote: > On 2012/03/01 15:44:20, cbentzel wrote: > > You could also create the factory early in the injector and just populate the > > factory, rather than having a local map. > > In an earlier comment, you asked that the factory be created at injection time. Moved it to the constructor. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:408: scoped_refptr<TestFileErrorInjector> single_injector_; On 2012/03/01 22:32:00, cbentzel wrote: > On 2012/03/01 20:19:31, ahendrickson wrote: > > On 2012/03/01 15:44:20, cbentzel wrote: > > > What's going on here? This is always going to be NULL. Was this intended to > be > > a > > > static? > > > > It was. Fixed. > > > > Sadly this means that the injector will hang around until the program ends. > > I'll chat with Randy. I don't understand why this should be all singleton'y. > It's only called once from the test driver. And then you have a static non-POD > class around, which is totally frowned upon by the style guide. Now using a DCHECK to prevent multiple calls, and not keeping a static scoped_refptr<>. http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... content/test/test_file_error_injector.cc:150: // Our injected error list, mapped by file index. One per file. On 2012/03/01 20:28:25, rdsmith wrote: > This comment is no longer correct, right? Fixed. http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... content/test/test_file_error_injector.cc:318: download_file_manager->GetFileFactoryForTesting()); On 2012/03/01 20:28:25, rdsmith wrote: > An alternative implementation would be to make file_factory_ a member variable > (of type DownloadFileWithErrorsFactory*) that was NULL before it was created. > That would allow you to avoid the static cast (and would not increase the member > variaables since created_factory_ would go away), but would have the minus of > your having to reason about and document the lifetime issues. But I think the > code fragility is about the same, since anything that would change get the DFM's > idea of the file factory out of sync with the TFEII's would probably also > violate the static cast assumption. > > Just a thought; up to you. I'm now storing a copy of the file factory pointer in the injector, but it's only used to DCHECK the value returned from DownloadFileManager. http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... content/test/test_file_error_injector.h:27: // URLs are different. On 2012/03/01 20:28:25, rdsmith wrote: > nit, suggestion: MIght be useful to explain why this restriction is in place. > It's an interface level issue, so it wouldn't be out of place here. Done. http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... content/test/test_file_error_injector.h:27: // URLs are different. On 2012/03/01 20:28:25, rdsmith wrote: > Could you put in/have you put in a DCHECK() to confirm this? Done. http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... content/test/test_file_error_injector.h:52: // Only one error per file will be used. On 2012/03/01 20:28:25, rdsmith wrote: > So what happens if more than one AddError is called for a particular file? > First one wins? Second one wins? Interface violation? (If the last, I'll want > a DCHECK confirming it.) There's a DCHECK preventing it. http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... content/test/test_file_error_injector.h:56: // Doesn't affect files already created. On 2012/03/01 20:28:25, rdsmith wrote: > Based on the implementation, I think this comment is misleading, as the real > boundary about what files are effected has to do with when InjectErrors() is > called. I.e. if you called AddError(), InjectErrors(), ClearError(), and > created a file, it would have the info from the add error. Fixed comment. http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... content/test/test_file_error_injector.h:57: virtual void ClearErrors() = 0; On 2012/03/01 20:28:25, rdsmith wrote: > It might be illuminating to include a comment with an example use of this class > for a couple of files. Done. http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... content/test/test_file_error_injector.h:61: virtual bool InjectErrors() = 0; On 2012/03/01 20:28:25, rdsmith wrote: > It's not clear to me reading this header file how multiple calls to > InjectErrors() is expected to be used. Are they allowed? If you do them, do > they only inject the errors accumulated since the last call to InjectErrors()? > Are previously injected errors cleared? > > (I think I've said this before, but my standard for classes is that the .h file > have all the information in it needed for a consumer to use the class between > the comments and the code.) Multiple calls are allowed. They replace the factory's error list with whatever the injector has at the time. Added this to the comments. http://codereview.chromium.org/9426029/diff/46004/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/46004/content/test/test_file_erro... content/test/test_file_error_injector.cc:411: scoped_refptr<TestFileErrorInjector> single_injector_; On 2012/03/01 20:28:25, rdsmith wrote: > I think this needs to be static. Changed this function to simply create a new injector, but DCHECK if it's called more than once. http://codereview.chromium.org/9426029/diff/52001/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/52001/content/test/test_file_erro... content/test/test_file_error_injector.cc:240: if (injected_errors_.find(url) == injected_errors_.end()) { On 2012/03/02 20:45:19, cbentzel wrote: > Just as a question - do you ever expect this case to happen? Could you just > DCHECK or return NULL? Yes, I would like to handle the case where I have no injected errors for a URL. http://codereview.chromium.org/9426029/diff/52001/content/test/test_file_erro... content/test/test_file_error_injector.cc:242: content::TestFileErrorInjector::FileErrorInfo err_info = { On 2012/03/02 20:45:19, cbentzel wrote: > You could also define a default constructor, that would be cleaner. If I have a constructor, I can't use initializer lists, which would not be as nice in the browser test file. http://codereview.chromium.org/9426029/diff/52001/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/52001/content/test/test_file_erro... content/test/test_file_error_injector.h:12: #include "content/public/browser/download_id.h" On 2012/03/02 20:45:19, cbentzel wrote: > I think you can forward declare download_id since it's just being used for a > return value. Done.
Comments on the injector interface. I'll go on to do the rest of the review, but after my other reviews--this seems like enough to keep you busy :-}. (Note that despite having said that, I do think we're converging--this looks a lot better than the last round. There are still a lot of comments, but they're at a much lower level.) http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/20002/content/test/test_file_erro... content/test/test_file_error_injector.cc:342: #define TO_STRING(code) \ On 2012/02/28 14:51:41, cbentzel wrote: > Although this macro is localized, it provides very little advantage over just > writing directly. FWIW, I agree with Chris; I think writing it directly is better, though I won't block this version. > Also note that you don't need to clarify the content namespace > here. > > Also - given that this is only used in this translation unit, you may just want > to make it a non-static method and only display it here. I'm not even sure that > the VLOG is needed, but given that this is test-only code won't push too hard > against it. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:35: typedef base::Callback<void(GURL url, content::DownloadId id)> On 2012/03/01 20:28:25, rdsmith wrote: > On 2012/03/01 20:19:31, ahendrickson wrote: > > On 2012/03/01 15:44:20, cbentzel wrote: > > > It's a bit uncommon to have these on the TestFileErrorInjectorImpl instead > of > > > the Factory/File. As you see, the TestFileErrorInjectorImpl does not > directly > > > depend on these typedefs, but the File/Factory do. > > > > I'd prefer to have TestFileErrorInjectorImpl declared before File/Factory, > which > > means that the typedefs have to be declared before File/Factory. > > Why do you prefer that order? I agree with Chris that the interface is a little > less clean having the typedef here. Still looking for a response to this question. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:408: scoped_refptr<TestFileErrorInjector> single_injector_; On 2012/03/02 21:58:47, ahendrickson wrote: > On 2012/03/01 22:32:00, cbentzel wrote: > > On 2012/03/01 20:19:31, ahendrickson wrote: > > > On 2012/03/01 15:44:20, cbentzel wrote: > > > > What's going on here? This is always going to be NULL. Was this intended > to > > be > > > a > > > > static? > > > > > > It was. Fixed. > > > > > > Sadly this means that the injector will hang around until the program ends. > > > > I'll chat with Randy. I don't understand why this should be all singleton'y. > > It's only called once from the test driver. And then you have a static non-POD > > class around, which is totally frowned upon by the style guide. > > Now using a DCHECK to prevent multiple calls, and not keeping a static > scoped_refptr<>. > Perfectly happy with non-singleton semantics so long as the interface is at least fool-resistant (i.e. that reasonable things happen if multiple instances of the class are created). I'll scan for that in my review. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.h:63: // Get information about the DownloadFiles created. On 2012/03/01 20:28:25, rdsmith wrote: > Could you add some comments for these interfaces? What's the difference between > CurrentFileCount() and TotalFileCount()? What does HadFile() mean? If you do > two different downloads with the same URL serially, which does GetId() return? > What does ClearFoundFiles() do/which of the other interfaces does it affect, and > which doesn't it affect? Ping? http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... content/test/test_file_error_injector.cc:150: // Our injected error list, mapped by file index. One per file. On 2012/03/02 21:58:47, ahendrickson wrote: > On 2012/03/01 20:28:25, rdsmith wrote: > > This comment is no longer correct, right? > > Fixed. Doesn't look fixed? http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... content/test/test_file_error_injector.h:52: // Only one error per file will be used. On 2012/03/02 21:58:47, ahendrickson wrote: > On 2012/03/01 20:28:25, rdsmith wrote: > > So what happens if more than one AddError is called for a particular file? > > First one wins? Second one wins? Interface violation? (If the last, I'll > want > > a DCHECK confirming it.) > > There's a DCHECK preventing it. So that means that it's an interface violation to call AddError() more than once for the same file. The documentation should say that. (I.e. instead of "Only one error per file will be used" say something like "It is illegal to call this function more than once for a particular file.") http://codereview.chromium.org/9426029/diff/52001/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/52001/content/test/test_file_erro... content/test/test_file_error_injector.cc:242: content::TestFileErrorInjector::FileErrorInfo err_info = { On 2012/03/02 21:58:47, ahendrickson wrote: > On 2012/03/02 20:45:19, cbentzel wrote: > > You could also define a default constructor, that would be cleaner. > > If I have a constructor, I can't use initializer lists, which would not be as > nice in the browser test file. I don't think there's any conflict between having a default constructor and using initializer lists. If the member variables are public, I think you can use initializer lists. (And apologies if I've gotten this wrong and glitched on an obscure point of C++ syntax--it wouldn't be the first time.) http://codereview.chromium.org/9426029/diff/50005/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/50005/content/test/test_file_erro... content/test/test_file_error_injector.cc:59: void RecordDownloadFileConstruction(GURL url, content::DownloadId id); I'd be inclined to say that you should document the threading model here (i.e. that the first two may be called on any thread, and result in the second two being called on the UI thread). Since it's after the "private" it's part of the implementation, but that doesn't mean that a note to someone trying to understand the implementation would be a bad thing. http://codereview.chromium.org/9426029/diff/50005/content/test/test_file_erro... content/test/test_file_error_injector.cc:329: base::Passed(injected_errors_copy.Pass()))); Why not pass this by value? I'm not sure it's the right thing, but it would simplify the code here, and you do a copy above anyway (passing it by value might involve two copies). http://codereview.chromium.org/9426029/diff/50005/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/50005/content/test/test_file_erro... content/test/test_file_error_injector.h:40: // injector->AddError(); Shouldn't these refer to a & b? http://codereview.chromium.org/9426029/diff/50005/content/test/test_file_erro... content/test/test_file_error_injector.h:45: // ... wait for downloads to finish or get an injected error ... Thank you--I think this comment makes this class much more usable. http://codereview.chromium.org/9426029/diff/50005/content/test/test_file_erro... content/test/test_file_error_injector.h:64: TestFileErrorInjector() {} If you want all instances of this class to be created through Create(), this should be private. That also allows you to enforce that all instances of this class are accessed through a scoped_refptr<> which I think you want. http://codereview.chromium.org/9426029/diff/50005/content/test/test_file_erro... content/test/test_file_error_injector.h:66: // Creates an instance. May only be called once. I'd say something about lifetime here, since many people when they see "May only be called once" will think "Singleton". Maybe a suffix something like "but will not be kept alive past the last copy of the returned scoped_refptr"? http://codereview.chromium.org/9426029/diff/50005/content/test/test_file_erro... content/test/test_file_error_injector.h:70: // Must be called before |InjectErrors()| for a particular download file. This is a little misleading; you can have multiple calls to AddError() before or after InjectErrors(). I think what you're trying to say is "Will only take effect after the next call to InjectErorrs()"; if so, that's probably a better way to say it. http://codereview.chromium.org/9426029/diff/50005/content/test/test_file_erro... content/test/test_file_error_injector.h:71: // Only one error per file will be used. We have one comment thread indicating that it's an interface violation for this to be called more than once for a given file, but the implementation looks to me as if it just has the "last call wins" property. Whichever is true, it should be documented here (I don't care about the behavior--I just want it documented.)
The non-error injection stuff looks basically good (nits below). Could you include a link to the Testing Download Error Handling document in the CL somewhere (comment is fine) with a list of which cases in it this CL is supposed to test, so that I can evaluate the test cases? (E.g. the title of the CL is "Test file errors in downloads" but it doesn't test the file error ACCESS_DENIED). http://codereview.chromium.org/9426029/diff/68001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9426029/diff/68001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:696: void DownloadFilesCheckErrorsLoop(const DownloadInfo& download_info, "Loop" implies that there's going to be a loop in here, where what you mean is that it'll be a called within a loop. Different name? http://codereview.chromium.org/9426029/diff/68001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:730: // Go directly to download. Don't wait until it's done. I don't understand what "Don't wait until it's done" means? http://codereview.chromium.org/9426029/diff/68001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:767: // Wait till the download files are destroyed. Isn't this misleading? The files won't be destroyed in sucessful cases.
LGTM I'd like to see some of these issues I listed out addressed, but am OK with the current state of the CL. Please fix BUG= and TEST= entries in the bug description. Also, still wait for Randy's approval. http://codereview.chromium.org/9426029/diff/68001/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/68001/content/test/test_file_erro... content/test/test_file_error_injector.cc:133: void RecordDownloadFileConstruction(GURL url, content::DownloadId id); Nit: these should probably take const GURL& http://codereview.chromium.org/9426029/diff/68001/content/test/test_file_erro... content/test/test_file_error_injector.cc:182: Nit: extra newline http://codereview.chromium.org/9426029/diff/68001/content/test/test_file_erro... content/test/test_file_error_injector.cc:292: content::BrowserThread::PostTask( Why not pass this down at the same time that you do the InjectErrorsOnFileThread? That way you don't also have to do the static_cast for finding the download_manager. http://codereview.chromium.org/9426029/diff/68001/content/test/test_file_erro... content/test/test_file_error_injector.cc:325: injected_errors_.clear(); Maybe a CurrentlyOn check here. http://codereview.chromium.org/9426029/diff/68001/content/test/test_file_erro... content/test/test_file_error_injector.cc:441: DCHECK(!visited); // Only allowed to be called once. In this case, why do we care about it having singleton like properties? It should just be non-static and return a new refcounted version. http://codereview.chromium.org/9426029/diff/68001/content/test/test_file_erro... content/test/test_file_error_injector.cc:452: #define TO_STRING(code) \ I'd remove this macro - generally against the style guide and of very little benefit here. http://codereview.chromium.org/9426029/diff/68001/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/68001/content/test/test_file_erro... content/test/test_file_error_injector.h:21: // All errors for a download must be injected before it starts. Good comment.
More to come . . . http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.cc:35: typedef base::Callback<void(GURL url, content::DownloadId id)> Rearranged. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.h:63: // Get information about the DownloadFiles created. On 2012/03/01 20:28:25, rdsmith wrote: > Could you add some comments for these interfaces? What's the difference between > CurrentFileCount() and TotalFileCount()? What does HadFile() mean? If you do > two different downloads with the same URL serially, which does GetId() return? > What does ClearFoundFiles() do/which of the other interfaces does it affect, and > which doesn't it affect? Added comments. http://codereview.chromium.org/9426029/diff/43020/content/test/test_file_erro... content/test/test_file_error_injector.h:63: // Get information about the DownloadFiles created. On 2012/03/06 23:05:19, rdsmith wrote: > On 2012/03/01 20:28:25, rdsmith wrote: > > Could you add some comments for these interfaces? What's the difference > between > > CurrentFileCount() and TotalFileCount()? What does HadFile() mean? If you do > > two different downloads with the same URL serially, which does GetId() return? > > > What does ClearFoundFiles() do/which of the other interfaces does it affect, > and > > which doesn't it affect? > > Ping? Sorry, this was at the bottom of my list of CLs. http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... content/test/test_file_error_injector.cc:150: // Our injected error list, mapped by file index. One per file. On 2012/03/06 23:05:19, rdsmith wrote: > On 2012/03/02 21:58:47, ahendrickson wrote: > > On 2012/03/01 20:28:25, rdsmith wrote: > > > This comment is no longer correct, right? > > > > Fixed. > > Doesn't look fixed? Sorry, jumped the gun there. Fixed now. http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/43028/content/test/test_file_erro... content/test/test_file_error_injector.h:52: // Only one error per file will be used. On 2012/03/06 23:05:19, rdsmith wrote: > On 2012/03/02 21:58:47, ahendrickson wrote: > > On 2012/03/01 20:28:25, rdsmith wrote: > > > So what happens if more than one AddError is called for a particular file? > > > First one wins? Second one wins? Interface violation? (If the last, I'll > > want > > > a DCHECK confirming it.) > > > > There's a DCHECK preventing it. > > So that means that it's an interface violation to call AddError() more than once > for the same file. The documentation should say that. (I.e. instead of "Only > one error per file will be used" say something like "It is illegal to call this > function more than once for a particular file.") Done. http://codereview.chromium.org/9426029/diff/52001/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/52001/content/test/test_file_erro... content/test/test_file_error_injector.cc:242: content::TestFileErrorInjector::FileErrorInfo err_info = { On 2012/03/06 23:05:19, rdsmith wrote: > On 2012/03/02 21:58:47, ahendrickson wrote: > > On 2012/03/02 20:45:19, cbentzel wrote: > > > You could also define a default constructor, that would be cleaner. > > > > If I have a constructor, I can't use initializer lists, which would not be as > > nice in the browser test file. > > I don't think there's any conflict between having a default constructor and > using initializer lists. If the member variables are public, I think you can > use initializer lists. (And apologies if I've gotten this wrong and glitched on > an obscure point of C++ syntax--it wouldn't be the first time.) I actually added a default constructor & tried to build. It wouldn't compile. http://codereview.chromium.org/9426029/diff/50005/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/50005/content/test/test_file_erro... content/test/test_file_error_injector.cc:59: void RecordDownloadFileConstruction(GURL url, content::DownloadId id); On 2012/03/06 23:05:19, rdsmith wrote: > I'd be inclined to say that you should document the threading model here (i.e. > that the first two may be called on any thread, and result in the second two > being called on the UI thread). Since it's after the "private" it's part of the > implementation, but that doesn't mean that a note to someone trying to > understand the implementation would be a bad thing. Done. http://codereview.chromium.org/9426029/diff/50005/content/test/test_file_erro... content/test/test_file_error_injector.cc:329: base::Passed(injected_errors_copy.Pass()))); On 2012/03/06 23:05:19, rdsmith wrote: > Why not pass this by value? I'm not sure it's the right thing, but it would > simplify the code here, and you do a copy above anyway (passing it by value > might involve two copies). Done. http://codereview.chromium.org/9426029/diff/50005/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/50005/content/test/test_file_erro... content/test/test_file_error_injector.h:40: // injector->AddError(); On 2012/03/06 23:05:19, rdsmith wrote: > Shouldn't these refer to a & b? Done. http://codereview.chromium.org/9426029/diff/50005/content/test/test_file_erro... content/test/test_file_error_injector.h:64: TestFileErrorInjector() {} On 2012/03/06 23:05:19, rdsmith wrote: > If you want all instances of this class to be created through Create(), this > should be private. That also allows you to enforce that all instances of this > class are accessed through a scoped_refptr<> which I think you want. Made it protected, as another class inherits from this. http://codereview.chromium.org/9426029/diff/50005/content/test/test_file_erro... content/test/test_file_error_injector.h:66: // Creates an instance. May only be called once. On 2012/03/06 23:05:19, rdsmith wrote: > I'd say something about lifetime here, since many people when they see "May only > be called once" will think "Singleton". Maybe a suffix something like "but will > not be kept alive past the last copy of the returned scoped_refptr"? Done. http://codereview.chromium.org/9426029/diff/50005/content/test/test_file_erro... content/test/test_file_error_injector.h:70: // Must be called before |InjectErrors()| for a particular download file. On 2012/03/06 23:05:19, rdsmith wrote: > This is a little misleading; you can have multiple calls to AddError() before or > after InjectErrors(). I think what you're trying to say is "Will only take > effect after the next call to InjectErorrs()"; if so, that's probably a better > way to say it. Done. http://codereview.chromium.org/9426029/diff/50005/content/test/test_file_erro... content/test/test_file_error_injector.h:71: // Only one error per file will be used. On 2012/03/06 23:05:19, rdsmith wrote: > We have one comment thread indicating that it's an interface violation for this > to be called more than once for a given file, but the implementation looks to me > as if it just has the "last call wins" property. Whichever is true, it should > be documented here (I don't care about the behavior--I just want it documented.) The implementation currently DCHECKs if the same URL is used twice when you call AddError().
http://codereview.chromium.org/9426029/diff/68001/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/68001/content/test/test_file_erro... content/test/test_file_error_injector.cc:133: void RecordDownloadFileConstruction(GURL url, content::DownloadId id); On 2012/03/08 20:02:55, cbentzel wrote: > Nit: these should probably take const GURL& Done. http://codereview.chromium.org/9426029/diff/68001/content/test/test_file_erro... content/test/test_file_error_injector.cc:182: On 2012/03/08 20:02:55, cbentzel wrote: > Nit: extra newline Done. http://codereview.chromium.org/9426029/diff/68001/content/test/test_file_erro... content/test/test_file_error_injector.cc:325: injected_errors_.clear(); On 2012/03/08 20:02:55, cbentzel wrote: > Maybe a CurrentlyOn check here. Done. http://codereview.chromium.org/9426029/diff/68001/content/test/test_file_erro... content/test/test_file_error_injector.cc:441: DCHECK(!visited); // Only allowed to be called once. On 2012/03/08 20:02:55, cbentzel wrote: > In this case, why do we care about it having singleton like properties? It > should just be non-static and return a new refcounted version. Because there can only be one DownloadFileFactory class used at a time. If we allow more than one TFEI creation, the 2nd would have have a DFF in place and the first would not behave properly. http://codereview.chromium.org/9426029/diff/68001/content/test/test_file_erro... content/test/test_file_error_injector.cc:452: #define TO_STRING(code) \ On 2012/03/08 20:02:55, cbentzel wrote: > I'd remove this macro - generally against the style guide and of very little > benefit here. Done.
On 2012/03/07 22:19:29, rdsmith wrote: > The non-error injection stuff looks basically good (nits below). Could you > include a link to the Testing Download Error Handling document in the CL > somewhere (comment is fine) with a list of which cases in it this CL is supposed > to test, so that I can evaluate the test cases? (E.g. the title of the CL is > "Test file errors in downloads" but it doesn't test the file error > ACCESS_DENIED). I didn't see a response to this comment; is that coming, did I miss it? > > http://codereview.chromium.org/9426029/diff/68001/chrome/browser/download/dow... > File chrome/browser/download/download_browsertest.cc (right): > > http://codereview.chromium.org/9426029/diff/68001/chrome/browser/download/dow... > chrome/browser/download/download_browsertest.cc:696: void > DownloadFilesCheckErrorsLoop(const DownloadInfo& download_info, > "Loop" implies that there's going to be a loop in here, where what you mean is > that it'll be a called within a loop. Different name? > > http://codereview.chromium.org/9426029/diff/68001/chrome/browser/download/dow... > chrome/browser/download/download_browsertest.cc:730: // Go directly to download. > Don't wait until it's done. > I don't understand what "Don't wait until it's done" means? > > http://codereview.chromium.org/9426029/diff/68001/chrome/browser/download/dow... > chrome/browser/download/download_browsertest.cc:767: // Wait till the download > files are destroyed. > Isn't this misleading? The files won't be destroyed in sucessful cases.
High level comments: * I'm good with you roping John in for his opinion on the injector at this point. * I didn't see your responses to my comments on PS22 (and one of Chris'); were they supposed to be there? http://codereview.chromium.org/9426029/diff/50005/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/50005/content/test/test_file_erro... content/test/test_file_error_injector.h:64: TestFileErrorInjector() {} On 2012/03/08 22:00:08, ahendrickson wrote: > On 2012/03/06 23:05:19, rdsmith wrote: > > If you want all instances of this class to be created through Create(), this > > should be private. That also allows you to enforce that all instances of this > > class are accessed through a scoped_refptr<> which I think you want. > > Made it protected, as another class inherits from this. Good point; oops on my part. http://codereview.chromium.org/9426029/diff/50005/content/test/test_file_erro... content/test/test_file_error_injector.h:70: // Must be called before |InjectErrors()| for a particular download file. On 2012/03/08 22:00:08, ahendrickson wrote: > On 2012/03/06 23:05:19, rdsmith wrote: > > This is a little misleading; you can have multiple calls to AddError() before > or > > after InjectErrors(). I think what you're trying to say is "Will only take > > effect after the next call to InjectErorrs()"; if so, that's probably a better > > way to say it. > > Done. Not seeing it? http://codereview.chromium.org/9426029/diff/50005/content/test/test_file_erro... content/test/test_file_error_injector.h:71: // Only one error per file will be used. On 2012/03/08 22:00:08, ahendrickson wrote: > On 2012/03/06 23:05:19, rdsmith wrote: > > We have one comment thread indicating that it's an interface violation for > this > > to be called more than once for a given file, but the implementation looks to > me > > as if it just has the "last call wins" property. Whichever is true, it should > > be documented here (I don't care about the behavior--I just want it > documented.) > > The implementation currently DCHECKs if the same URL is used twice when you call > AddError(). Cool; thanks.
On 2012/03/12 01:24:26, rdsmith wrote: > On 2012/03/07 22:19:29, rdsmith wrote: > > The non-error injection stuff looks basically good (nits below). Could you > > include a link to the Testing Download Error Handling document in the CL > > somewhere (comment is fine) with a list of which cases in it this CL is > supposed > > to test, so that I can evaluate the test cases? (E.g. the title of the CL is > > "Test file errors in downloads" but it doesn't test the file error > > ACCESS_DENIED). > > I didn't see a response to this comment; is that coming, did I miss it? https://docs.google.com/a/google.com/document/d/1hXd0RNXl_zM8UXXq-VFMtVNJxhk-...
http://codereview.chromium.org/9426029/diff/50005/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/50005/content/test/test_file_erro... content/test/test_file_error_injector.h:70: // Must be called before |InjectErrors()| for a particular download file. On 2012/03/12 01:44:50, rdsmith wrote: > On 2012/03/08 22:00:08, ahendrickson wrote: > > On 2012/03/06 23:05:19, rdsmith wrote: > > > This is a little misleading; you can have multiple calls to AddError() > before > > or > > > after InjectErrors(). I think what you're trying to say is "Will only take > > > effect after the next call to InjectErorrs()"; if so, that's probably a > better > > > way to say it. > > > > Done. > > Not seeing it? I did change the comment. If it's still not clear, please let me know. http://codereview.chromium.org/9426029/diff/68001/chrome/browser/download/dow... File chrome/browser/download/download_browsertest.cc (right): http://codereview.chromium.org/9426029/diff/68001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:696: void DownloadFilesCheckErrorsLoop(const DownloadInfo& download_info, On 2012/03/07 22:19:30, rdsmith wrote: > "Loop" implies that there's going to be a loop in here, where what you mean is > that it'll be a called within a loop. Different name? Done. http://codereview.chromium.org/9426029/diff/68001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:730: // Go directly to download. Don't wait until it's done. On 2012/03/07 22:19:30, rdsmith wrote: > I don't understand what "Don't wait until it's done" means? Clarified comment. http://codereview.chromium.org/9426029/diff/68001/chrome/browser/download/dow... chrome/browser/download/download_browsertest.cc:767: // Wait till the download files are destroyed. On 2012/03/07 22:19:30, rdsmith wrote: > Isn't this misleading? The files won't be destroyed in sucessful cases. Clarified that I'm referring to DownloadFiles. http://codereview.chromium.org/9426029/diff/68001/content/test/test_file_erro... File content/test/test_file_error_injector.cc (right): http://codereview.chromium.org/9426029/diff/68001/content/test/test_file_erro... content/test/test_file_error_injector.cc:292: content::BrowserThread::PostTask( On 2012/03/08 20:02:55, cbentzel wrote: > Why not pass this down at the same time that you do the > InjectErrorsOnFileThread? That way you don't also have to do the static_cast for > finding the download_manager. Done.
LGTM, but do get John's review as well. http://codereview.chromium.org/9426029/diff/50005/content/test/test_file_erro... File content/test/test_file_error_injector.h (right): http://codereview.chromium.org/9426029/diff/50005/content/test/test_file_erro... content/test/test_file_error_injector.h:70: // Must be called before |InjectErrors()| for a particular download file. On 2012/03/12 11:51:02, ahendrickson wrote: > On 2012/03/12 01:44:50, rdsmith wrote: > > On 2012/03/08 22:00:08, ahendrickson wrote: > > > On 2012/03/06 23:05:19, rdsmith wrote: > > > > This is a little misleading; you can have multiple calls to AddError() > > before > > > or > > > > after InjectErrors(). I think what you're trying to say is "Will only > take > > > > effect after the next call to InjectErorrs()"; if so, that's probably a > > better > > > > way to say it. > > > > > > Done. > > > > Not seeing it? > > I did change the comment. If it's still not clear, please let me know. Yeah, sorry, I think the interleaving in the diff format confused me. This looks good.
jam: PTAL at the file error injector code interface code.
On 2012/03/12 18:13:21, ahendrickson wrote: > jam: PTAL at the file error injector code interface code. qq: why is there an interface for this class? we only add interfaces for files in content/public. for stuff in content/test we usually just put concrete classes (see the rest of the files there)
On 2012/03/13 16:19:21, John Abd-El-Malek wrote: > On 2012/03/12 18:13:21, ahendrickson wrote: > > jam: PTAL at the file error injector code interface code. > > qq: why is there an interface for this class? we only add interfaces for files > in content/public. for stuff in content/test we usually just put concrete > classes (see the rest of the files there) Because the concrete class needs to refer to things like DownloadFile, which are not exposed outside of content.
On 2012/03/13 16:19:21, John Abd-El-Malek wrote: > On 2012/03/12 18:13:21, ahendrickson wrote: > > jam: PTAL at the file error injector code interface code. > > qq: why is there an interface for this class? we only add interfaces for files > in content/public. for stuff in content/test we usually just put concrete > classes (see the rest of the files there) btw to give more explanation: the reason we have interfaces in the content api (i.e. content/public) is twofold: to hide embedders' (i.e. chrome) implementations from content, and also to hide content's implementation of core classes from embedders. in this case, there's only one implementation of that class. having the concrete class be visible to embedder's test code won't break any abstraction layers (as long as forward declaration is used to avoid leaking includes to other content source files). we avoid adding interfaces for these test files because it seems like unnecessary extra work without much benefit
On 2012/03/13 16:22:47, ahendrickson wrote: > On 2012/03/13 16:19:21, John Abd-El-Malek wrote: > > On 2012/03/12 18:13:21, ahendrickson wrote: > > > jam: PTAL at the file error injector code interface code. > > > > qq: why is there an interface for this class? we only add interfaces for files > > in content/public. for stuff in content/test we usually just put concrete > > classes (see the rest of the files there) > > Because the concrete class needs to refer to things like DownloadFile, which are > not exposed outside of content. that's just in the cc file, you can forward declare DownloadFileWithErrorsFactory.
I moved the implementation class declaration into the header, and removed the interface class. PTAL.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ahendrickson@chromium.org/9426029/101009
Change committed as 126732 |
