Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7646025:
Detect file system errors during downloads. (Closed)
|
Created:
9 years, 4 months ago by ahendrickson Modified:
9 years, 3 months ago Reviewers:
cbentzel, darin (slow to review), wtc, Randy Smith (Not in Mondays), commit-bot: I haz the power CC:
chromium-reviews, rginda+watch_chromium.org, achuith+watch_chromium.org, rdsmith+dwatch_chromium.org, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDetect file system errors during downloads.
Moving towards giving the user feedback when a file system error occurs during a download.
Split from CL 7134019.
BUG=85240
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98725
Patch Set 1 #
Total comments: 20
Patch Set 2 : Changes per Chris' comments. #Patch Set 3 : Merged with trunk. #
Total comments: 18
Patch Set 4 : Added unit test for BaseFile error detection. #Patch Set 5 : Refactored unit test to remove extraneous functions from FileStream. #
Total comments: 1
Patch Set 6 : Fixed comment. #
Total comments: 17
Patch Set 7 : Created unit test for Download Manager's handling of file errors. #Patch Set 8 : Added missing file #Patch Set 9 : Merged with trunk. #Patch Set 10 : Fixed merge error. #Patch Set 11 : Fixed Linux compile error. #
Total comments: 18
Patch Set 12 : Removed illegal include from content. #
Total comments: 6
Patch Set 13 : Cleaned up unit test per Randy's comments. #Patch Set 14 : Cleanup per Wan-Teh's comments. #Patch Set 15 : Merged with trunk. #
Total comments: 38
Patch Set 16 : Incorporated Randy and Chris' comments. #Patch Set 17 : Fixed Clang problem by moving MockFileStream method bodies to a cc file. #
Total comments: 2
Patch Set 18 : Added missing new file. #Patch Set 19 : Minor refactor of DownloadFileWithMockStream class. #
Total comments: 7
Patch Set 20 : Changed namespace for MockFileStream. #Messages
Total messages: 31 (0 generated)
I didn't see any tests for the file-error-on-append-bytes case. Are there any? http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... File chrome/browser/download/download_file_manager.cc (right): http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_file_manager.cc:177: bool write_succeeded = If this is going to end up with a net_error code return in a subsequent CL, I'm fine with using that here rather than a bool. http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_file_manager.cc:187: CancelDownload(id); BUG: I'm not sure the CancelDownload is right. a] It will lead to a garbage memory dereference below with download_file->bytes_so_far() because download_file will be deleted. b] The logic in DownloadManager::OnDownloadError is set up so it's only really executed the first time - the download is removed from the list so subsequent calls to OnDownloadError are no-ops. c] There's logic there to change some of the items, and then a PostTask to the file thread is called with CancelDownload. Keeping this in the main flow is probably correct. http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... File chrome/browser/download/download_file_manager.h (right): http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_file_manager.h:84: // |os_error| is 0 for normal completions, and non-0 for errors. |net_error| rather than |os_error| http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... File chrome/browser/download/download_file_unittest.cc (right): http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_file_unittest.cc:43: download_file_.reset(NULL); Was this a leak? Why is this needed now? http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... File chrome/browser/download/download_item.h (right): http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_item.h:190: void Interrupted(int64 size, int error); You changed this to net_error in the rest of the CL. Or is this going to become DownloadError? http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.cc:807: Nit: extra new line. http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.cc:816: Nit: extra new line http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.cc:818: Nit: extra new line http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.h:132: // |size| is the number of bytes that are currently downloaded. that were downloaded at the time of the interruption, maybe? is |error| a download error code, or a net_error code?
On 2011/08/16 15:50:36, cbentzel wrote: > I didn't see any tests for the file-error-on-append-bytes case. Are there any? > Not yet. http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... File chrome/browser/download/download_file_manager.cc (right): http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_file_manager.cc:177: bool write_succeeded = On 2011/08/16 15:50:36, cbentzel wrote: > If this is going to end up with a net_error code return in a subsequent CL, I'm > fine with using that here rather than a bool. AppendDataToFile() returns a bool currently. Another CL will change that. http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_file_manager.cc:187: CancelDownload(id); On 2011/08/16 15:50:36, cbentzel wrote: > BUG: I'm not sure the CancelDownload is right. > > a] It will lead to a garbage memory dereference below with > download_file->bytes_so_far() because download_file will be deleted. Fixed. Now get the number of bytes before canceling. > > b] The logic in DownloadManager::OnDownloadError is set up so it's only really > executed the first time - the download is removed from the list so subsequent > calls to OnDownloadError are no-ops. True. > > c] There's logic there to change some of the items, and then a PostTask to the > file thread is called with CancelDownload. Keeping this in the main flow is > probably correct. True, but we're making sure that the DownloadFile is deleted even if we don't make it back to the file thread due to a race condition. Calling CancelDownload() twice is OK. http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... File chrome/browser/download/download_file_manager.h (right): http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_file_manager.h:84: // |os_error| is 0 for normal completions, and non-0 for errors. On 2011/08/16 15:50:36, cbentzel wrote: > |net_error| rather than |os_error| Done. http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... File chrome/browser/download/download_file_unittest.cc (right): http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_file_unittest.cc:43: download_file_.reset(NULL); On 2011/08/16 15:50:36, cbentzel wrote: > Was this a leak? Why is this needed now? I did need that at one point, but it doesn't look like it's needed any more. Removed. http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... File chrome/browser/download/download_item.h (right): http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_item.h:190: void Interrupted(int64 size, int error); On 2011/08/16 15:50:36, cbentzel wrote: > You changed this to net_error in the rest of the CL. Or is this going to become > DownloadError? It will eventually be a DownloadError. http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.cc:807: On 2011/08/16 15:50:36, cbentzel wrote: > Nit: extra new line. Done. http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.cc:816: On 2011/08/16 15:50:36, cbentzel wrote: > Nit: extra new line Done. http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.cc:818: On 2011/08/16 15:50:36, cbentzel wrote: > Nit: extra new line Done. http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.h:132: // |size| is the number of bytes that are currently downloaded. On 2011/08/16 15:50:36, cbentzel wrote: > that were downloaded at the time of the interruption, maybe? Yes. Changed. > > is |error| a download error code, or a net_error code? ATM, it's a net_error code. Eventually it will be a DownloadError.
Please add a unit test. It would have caught the use-after-free error I discovered by inspecting code. It's also a good idea now because it will show you can isolate the "how to respond to a file error" from the "how to report a file error" logic. http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... File chrome/browser/download/download_file_manager.cc (right): http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_file_manager.cc:187: CancelDownload(id); On 2011/08/18 21:52:01, ahendrickson wrote: > On 2011/08/16 15:50:36, cbentzel wrote: > > BUG: I'm not sure the CancelDownload is right. > > > > a] It will lead to a garbage memory dereference below with > > download_file->bytes_so_far() because download_file will be deleted. > > Fixed. Now get the number of bytes before canceling. > > > > > b] The logic in DownloadManager::OnDownloadError is set up so it's only really > > executed the first time - the download is removed from the list so subsequent > > calls to OnDownloadError are no-ops. > > True. > > > > > c] There's logic there to change some of the items, and then a PostTask to the > > file thread is called with CancelDownload. Keeping this in the main flow is > > probably correct. > > True, but we're making sure that the DownloadFile is deleted even if we don't > make it back to the file thread due to a race condition. > Calling CancelDownload() twice is OK. What race condition? I agree that calling CancelDownload() twice is OK. However, it seems like this would be simpler if you just removed the call to CancelDownload(). This should work correctly without the call, and you end up with less code.
http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... File chrome/browser/download/download_file_manager.cc (right): http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_file_manager.cc:187: CancelDownload(id); On 2011/08/19 13:02:41, cbentzel wrote: > On 2011/08/18 21:52:01, ahendrickson wrote: > > On 2011/08/16 15:50:36, cbentzel wrote: > > > c] There's logic there to change some of the items, and then a PostTask to the > > > file thread is called with CancelDownload. Keeping this in the main flow is > > > probably correct. > > > > True, but we're making sure that the DownloadFile is deleted even if we don't > > make it back to the file thread due to a race condition. > > Calling CancelDownload() twice is OK. > > What race condition? > > I agree that calling CancelDownload() twice is OK. > > However, it seems like this would be simpler if you just removed the call to > CancelDownload(). This should work correctly without the call, and you end up > with less code. Hmm, I misspoke a bit, I think. We might not make it back to the file thread if we're shutting down, rather than due to a race. However, there is a race condition that we need to consider. The scenario we're worried about is this: We get a file error in DFM::UpdateDownload(), which then posts a message to DM::OnDownloadError(), and in turn that posts a message to DFM::CancelDownload(). Before the 2nd post occurs, we get *another* call to DFM::UpdateDownload(), which tries to write to the file some more. It may or may not succeed, but if it does it could corrupt the file. A later CL will try to resume uploading, but could end up with a corrupted file. One example of the write succeeding the second time but not the first is running out of space on the disk. Possibly the second time is trying to write less data to the file, and happens to fit on the disk. This would end up with a file that has a missing section, and possibly a corrupted hash. Putting the CancelDownload() here is safer.
sounds good, thanks for the explanation. I now agree that the CancelDownload here is safer. On Fri, Aug 19, 2011 at 2:11 PM, <ahendrickson@chromium.org> wrote: > > http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... > File chrome/browser/download/download_file_manager.cc (right): > > http://codereview.chromium.org/7646025/diff/1/chrome/browser/download/downloa... > chrome/browser/download/download_file_manager.cc:187: > CancelDownload(id); > On 2011/08/19 13:02:41, cbentzel wrote: >> >> On 2011/08/18 21:52:01, ahendrickson wrote: >> > On 2011/08/16 15:50:36, cbentzel wrote: >> > > c] There's logic there to change some of the items, and then a > > PostTask to the >> >> > > file thread is called with CancelDownload. Keeping this in the > > main flow is >> >> > > probably correct. >> > >> > True, but we're making sure that the DownloadFile is deleted even if > > we don't >> >> > make it back to the file thread due to a race condition. >> > Calling CancelDownload() twice is OK. > >> What race condition? > >> I agree that calling CancelDownload() twice is OK. > >> However, it seems like this would be simpler if you just removed the > > call to >> >> CancelDownload(). This should work correctly without the call, and you > > end up >> >> with less code. > > Hmm, I misspoke a bit, I think. Â We might not make it back to the file > thread if we're shutting down, rather than due to a race. > > However, there is a race condition that we need to consider. Â The > scenario we're worried about is this: > > We get a file error in DFM::UpdateDownload(), which then posts a message > to DM::OnDownloadError(), and in turn that posts a message to > DFM::CancelDownload(). > > Before the 2nd post occurs, we get *another* call to > DFM::UpdateDownload(), which tries to write to the file some more. Â It > may or may not succeed, but if it does it could corrupt the file. Â A > later CL will try to resume uploading, but could end up with a corrupted > file. > > One example of the write succeeding the second time but not the first is > running out of space on the disk. Â Possibly the second time is trying to > write less data to the file, and happens to fit on the disk. Â This would > end up with a file that has a missing section, and possibly a corrupted > hash. > > Putting the CancelDownload() here is safer. > > http://codereview.chromium.org/7646025/ >
At this point I'm happy with how this is implemented, but I am going to hold up giving approval until there's a test. I'll do a nit/lint pass as well.
On 2011/08/19 18:28:38, cbentzel wrote: > At this point I'm happy with how this is implemented, but I am going to hold up > giving approval until there's a test. I'll do a nit/lint pass as well. OK. Currently working on the test.
http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... File content/browser/download/download_file_manager.cc (right): http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... content/browser/download/download_file_manager.cc:180: std::string hash; hash does not appear to be used. http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... content/browser/download/download_file_manager.cc:184: // Calling this here in case we get more data, to avoid calling Update this comment to discuss the issue with writing to a corrupted file, it's a more compelling argument than this one. http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... content/browser/download/download_file_manager.cc:188: Perhaps you should do a download_file = NULL; after CancelDownload(). This makes it clear that it should no longer be dereferenced. It would also let you remove |had_error|. http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... File content/browser/download/download_file_manager.h (right): http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... content/browser/download/download_file_manager.h:150: // |rename_error| indicates what error caused the cancel. Mention that it's a net::error http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... File content/browser/download/download_manager.h (right): http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... content/browser/download/download_manager.h:121: // |size| is the number of bytes that are currently downloaded. Is |size| at this point the final size for OnResponseCompleted, rather than "that are currently downloaded"?
With the below notes, and agreeing with Chris about the unit test, LGTM. http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... File content/browser/download/download_file_manager.cc (right): http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... content/browser/download/download_file_manager.cc:184: // Calling this here in case we get more data, to avoid calling I'd suggest "this"->"CancelDownload" in the comment since you're a line away with another function call in between. http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... File content/browser/download/download_item.h (right): http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... content/browser/download/download_item.h:189: // |error| is the error code that the operation received. Shouldn't this comment specify the type/namespace of the error? http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... File content/browser/download/download_manager.cc (right): http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... content/browser/download/download_manager.cc:586: << " download = " << download->DebugString(true); It's up to you, but logging inside of "GetActiveDownload" (which is pure function, no state changes, and one that may be called from a lot of different places) seems like overkill. If I were putting in logging, I have a hard time imagining a situation in which this would give me useful information (== in which I wouldn't also want a log statement in the caller, and then ignore the results from this). http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... content/browser/download/download_manager.cc:795: // TODO(ahendrickson) -- This currently has no effect. Could you enhance this comment to answer the questions: * What's the actual TODO (i.e. what are you going to be doing in the future)? * What's the context/reason it has no effect?
http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... File content/browser/download/download_file_manager.cc (right): http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... content/browser/download/download_file_manager.cc:180: std::string hash; On 2011/08/19 18:44:21, cbentzel wrote: > hash does not appear to be used. Ah, leftover from a refactor. Removed. http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... content/browser/download/download_file_manager.cc:184: // Calling this here in case we get more data, to avoid calling On 2011/08/19 20:09:13, rdsmith wrote: > I'd suggest "this"->"CancelDownload" in the comment since you're a line away > with another function call in between. Moved the comment. http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... content/browser/download/download_file_manager.cc:184: // Calling this here in case we get more data, to avoid calling On 2011/08/19 18:44:21, cbentzel wrote: > Update this comment to discuss the issue with writing to a corrupted file, it's > a more compelling argument than this one. Done. http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... content/browser/download/download_file_manager.cc:188: On 2011/08/19 18:44:21, cbentzel wrote: > Perhaps you should do a > > download_file = NULL; > > after CancelDownload(). > > This makes it clear that it should no longer be dereferenced. It would also let > you remove |had_error|. Done. http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... File content/browser/download/download_file_manager.h (right): http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... content/browser/download/download_file_manager.h:150: // |rename_error| indicates what error caused the cancel. On 2011/08/19 18:44:21, cbentzel wrote: > Mention that it's a net::error Done. http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... File content/browser/download/download_item.h (right): http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... content/browser/download/download_item.h:189: // |error| is the error code that the operation received. On 2011/08/19 20:09:13, rdsmith wrote: > Shouldn't this comment specify the type/namespace of the error? Done. http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... File content/browser/download/download_manager.cc (right): http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... content/browser/download/download_manager.cc:586: << " download = " << download->DebugString(true); On 2011/08/19 20:09:13, rdsmith wrote: > It's up to you, but logging inside of "GetActiveDownload" (which is pure > function, no state changes, and one that may be called from a lot of different > places) seems like overkill. If I were putting in logging, I have a hard time > imagining a situation in which this would give me useful information (== in > which I wouldn't also want a log statement in the caller, and then ignore the > results from this). Done. http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... content/browser/download/download_manager.cc:795: // TODO(ahendrickson) -- This currently has no effect. On 2011/08/19 20:09:13, rdsmith wrote: > Could you enhance this comment to answer the questions: > * What's the actual TODO (i.e. what are you going to be doing in the future)? > * What's the context/reason it has no effect? Done. http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... File content/browser/download/download_manager.h (right): http://codereview.chromium.org/7646025/diff/11001/content/browser/download/do... content/browser/download/download_manager.h:121: // |size| is the number of bytes that are currently downloaded. On 2011/08/19 18:44:21, cbentzel wrote: > Is |size| at this point the final size for OnResponseCompleted, rather than > "that are currently downloaded"? Done.
Test LGTM. Who's your reviewer for the FileStream changes? http://codereview.chromium.org/7646025/diff/24001/content/browser/download/do... File content/browser/download/download_file_manager.cc (right): http://codereview.chromium.org/7646025/diff/24001/content/browser/download/do... content/browser/download/download_file_manager.cc:185: // processing data after an error. That could lead to files that "avoid calling processing data" seems awkward?
darin, wtc: PTAL net/base/file_stream.h. I made the FileStream class use virtual functions, in order to create a MockFileStream class.
LGTM on net/base/file_stream.h only. I didn't review the other files except for the MockFileStream class in base_file_unittest.cc. http://codereview.chromium.org/7646025/diff/28002/content/browser/download/ba... File content/browser/download/base_file_unittest.cc (right): http://codereview.chromium.org/7646025/diff/28002/content/browser/download/ba... content/browser/download/base_file_unittest.cc:25: void clear_forced_error() { forced_error_ = 0; } Nit: it may be better to use net::OK instead of 0 with the forced_error_ member of this class. http://codereview.chromium.org/7646025/diff/28002/content/browser/download/ba... content/browser/download/base_file_unittest.cc:50: } Nit: add a blank line after this line. http://codereview.chromium.org/7646025/diff/28002/content/browser/download/ba... content/browser/download/base_file_unittest.cc:57: } All the net::FileStream virtual methods that you override should have the "virtual" keyword. You should also use the OVERRIDE macro (defined in compiler_specific.h). See http://www.chromium.org/developers/coding-style#Method_Signatures http://codereview.chromium.org/7646025/diff/28002/net/base/file_stream.h File net/base/file_stream.h (right): http://codereview.chromium.org/7646025/diff/28002/net/base/file_stream.h#newc... net/base/file_stream.h:95: virtual int ReadUntilComplete(char *buf, int buf_len); ReadUntilComplete() may not need to be virtual. It is defined entirely in terms of Read(). I am not sure about this though.
I'm sorry I wasn't clearer. The unit test [or possibly browser test] I want to see is one where a DownloadItem ends up getting interrupted due to a bad write to the filesystem. I didn't see anything like that in this CL. http://codereview.chromium.org/7646025/diff/28002/content/browser/download/ba... File content/browser/download/base_file_unittest.cc (right): http://codereview.chromium.org/7646025/diff/28002/content/browser/download/ba... content/browser/download/base_file_unittest.cc:25: void clear_forced_error() { forced_error_ = 0; } On 2011/08/23 00:18:35, wtc wrote: > Nit: it may be better to use net::OK instead of 0 with > the forced_error_ member of this class. +1 http://codereview.chromium.org/7646025/diff/28002/content/browser/download/ba... content/browser/download/base_file_unittest.cc:62: int get_error(int function_error) { Perhaps GetError or even ReturnError() since this isn't a simple getter. http://codereview.chromium.org/7646025/diff/28002/content/browser/download/ba... content/browser/download/base_file_unittest.cc:133: // Create a new file stream if it is not provided. What does the "if it is not provided" mean? When is it ever provided? http://codereview.chromium.org/7646025/diff/28002/content/browser/download/ba... content/browser/download/base_file_unittest.cc:207: AppendDataToFile(kTestData1); All calls to AppendDataToFile should be wrapped in ASSERT/EXPECT_TRUE http://codereview.chromium.org/7646025/diff/28002/content/browser/download/ba... content/browser/download/base_file_unittest.cc:342: ForceError(net::ERR_FAILED); Should you use one of the more likely ERR's here? I suppose many of them will be introduced in another CL.
Added a unit test for DownloadManager's handling of file errors. http://codereview.chromium.org/7646025/diff/28002/content/browser/download/ba... File content/browser/download/base_file_unittest.cc (right): http://codereview.chromium.org/7646025/diff/28002/content/browser/download/ba... content/browser/download/base_file_unittest.cc:25: void clear_forced_error() { forced_error_ = 0; } On 2011/08/23 00:18:35, wtc wrote: > Nit: it may be better to use net::OK instead of 0 with > the forced_error_ member of this class. Done. http://codereview.chromium.org/7646025/diff/28002/content/browser/download/ba... content/browser/download/base_file_unittest.cc:50: } On 2011/08/23 00:18:35, wtc wrote: > Nit: add a blank line after this line. Done. http://codereview.chromium.org/7646025/diff/28002/content/browser/download/ba... content/browser/download/base_file_unittest.cc:57: } On 2011/08/23 00:18:35, wtc wrote: > All the net::FileStream virtual methods that you override > should have the "virtual" keyword. You should also use the > OVERRIDE macro (defined in compiler_specific.h). See > http://www.chromium.org/developers/coding-style#Method_Signatures Done. http://codereview.chromium.org/7646025/diff/28002/content/browser/download/ba... content/browser/download/base_file_unittest.cc:62: int get_error(int function_error) { On 2011/08/23 01:34:01, cbentzel wrote: > Perhaps GetError or even ReturnError() since this isn't a simple getter. Done. http://codereview.chromium.org/7646025/diff/28002/content/browser/download/ba... content/browser/download/base_file_unittest.cc:133: // Create a new file stream if it is not provided. On 2011/08/23 01:34:01, cbentzel wrote: > What does the "if it is not provided" mean? When is it ever provided? Removed. http://codereview.chromium.org/7646025/diff/28002/content/browser/download/ba... content/browser/download/base_file_unittest.cc:207: AppendDataToFile(kTestData1); On 2011/08/23 01:34:01, cbentzel wrote: > All calls to AppendDataToFile should be wrapped in ASSERT/EXPECT_TRUE Done. http://codereview.chromium.org/7646025/diff/28002/content/browser/download/ba... content/browser/download/base_file_unittest.cc:342: ForceError(net::ERR_FAILED); On 2011/08/23 01:34:01, cbentzel wrote: > Should you use one of the more likely ERR's here? I suppose many of them will be > introduced in another CL. Yes, they will. http://codereview.chromium.org/7646025/diff/28002/net/base/file_stream.h File net/base/file_stream.h (right): http://codereview.chromium.org/7646025/diff/28002/net/base/file_stream.h#newc... net/base/file_stream.h:95: virtual int ReadUntilComplete(char *buf, int buf_len); On 2011/08/23 00:18:35, wtc wrote: > ReadUntilComplete() may not need to be virtual. It is defined > entirely in terms of Read(). > > I am not sure about this though. It doesn't make any difference. However, this seems more consistent.
http://codereview.chromium.org/7646025/diff/38002/chrome/browser/download/dow... File chrome/browser/download/download_manager_unittest.cc (right): http://codereview.chromium.org/7646025/diff/38002/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:173: static_cast<testing::MockFileStream *>(file_stream_.get()); This kind of upcasting is nervous making and should be avoided unless you need to do it, and in this case I don't think you need to. You're creating the mock_stream; can't you just keep a reference to it and call set_forced_error on the stream instead of on the DownloadFile at the appropriate time? http://codereview.chromium.org/7646025/diff/38002/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:540: TEST_F(DownloadManagerTest, DownloadFileErrorTest) { There's enough "stuff" in this test that I think it'd be useful to put in a couple (3-5?) comments indicating what each section of the test is doing; it'll aid people in reading the function. http://codereview.chromium.org/7646025/diff/38002/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:540: TEST_F(DownloadManagerTest, DownloadFileErrorTest) { Comment before the function saying what functionality it's testing. http://codereview.chromium.org/7646025/diff/38002/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:540: TEST_F(DownloadManagerTest, DownloadFileErrorTest) { I don't think this belongs under DownloadManagerTest; maybe download_file_unittest.cc ? (ETA: It's fuzzier for the full set of functionality you're testing, but in an ideal world, functionality that was primarily around DownloadFile would be someplace else. But it's ok to keep this here given what you're actually testing.) http://codereview.chromium.org/7646025/diff/38002/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:542: using ::testing::CreateFunctor; This has the look of a pattern copied from elsewhere in the code, and if it's a common pattern used in the chrome code base, I'm good with it. But if it's not: I'm specifically concerned with "using ::testing::_", as "_" is a heck of a name to have have a specific meaning in a file. I think readers will have a hard time figuring out what that's about. http://codereview.chromium.org/7646025/diff/38002/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:549: scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo); Any reason to make it a scoped_ptr rather than just declare it inline in the stack? http://codereview.chromium.org/7646025/diff/38002/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:558: linked_ptr<net::FileStream> stream(mock_stream); Why a linked_ptr? They're rare enough that I think of them as only being used for special purposes; what's the purpose here? If there's a reason for this, please put a comment in explaining it; if not, maybe just use a scoped_ptr? http://codereview.chromium.org/7646025/diff/38002/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:563: info->save_info.file_stream = stream; I'd suggest moving the stream initialization to before the info initialization; that way each initialization is consistent and reading through you don't have to jump back and forth between them. http://codereview.chromium.org/7646025/diff/38002/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:614: download->Cancel(true); Everything from here on in doesn't test any error detection functionality, does it? If this functionality is tested elsewhere in the file, I'd eliminate it here, to keep this test simple.
I only reviewed the files under src/net. http://codereview.chromium.org/7646025/diff/40001/net/base/mock_file_stream.h File net/base/mock_file_stream.h (right): http://codereview.chromium.org/7646025/diff/40001/net/base/mock_file_stream.h... net/base/mock_file_stream.h:42: OVERRIDE { Nit: it may be better to list net::CompletionCallback* callback) on a separate line: virtual int Read(char* buf, int buf_len, net::CompletionCallback* callback) OVERRIDE { This is how you formatted the Write() method. http://codereview.chromium.org/7646025/diff/40001/net/base/mock_file_stream.h... net/base/mock_file_stream.h:64: const FilePath& get_path() const { return path_; } Nit: list get_path() along with the set_forced_error() and clear_forced_error() methods above, because they're all simple getters/setters. http://codereview.chromium.org/7646025/diff/40001/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/7646025/diff/40001/net/net.gyp#newcode937 net/net.gyp:937: 'base/mock_file_stream.h', I wonder if this file should be added to the 'net_test_support' target instead.
http://codereview.chromium.org/7646025/diff/38002/chrome/browser/download/dow... File chrome/browser/download/download_manager_unittest.cc (right): http://codereview.chromium.org/7646025/diff/38002/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:173: static_cast<testing::MockFileStream *>(file_stream_.get()); On 2011/08/26 18:51:40, rdsmith wrote: > This kind of upcasting is nervous making and should be avoided unless you need > to do it, and in this case I don't think you need to. You're creating the > mock_stream; can't you just keep a reference to it and call set_forced_error on > the stream instead of on the DownloadFile at the appropriate time? Leaving it in, per our conversation, but adding an extra MockFileStream parameter to the constructor. The info parameter is required to have a NULL file_stream pointer. http://codereview.chromium.org/7646025/diff/38002/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:540: TEST_F(DownloadManagerTest, DownloadFileErrorTest) { On 2011/08/26 18:51:40, rdsmith wrote: > There's enough "stuff" in this test that I think it'd be useful to put in a > couple (3-5?) comments indicating what each section of the test is doing; it'll > aid people in reading the function. Done. http://codereview.chromium.org/7646025/diff/38002/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:540: TEST_F(DownloadManagerTest, DownloadFileErrorTest) { On 2011/08/26 18:51:40, rdsmith wrote: > Comment before the function saying what functionality it's testing. Done. http://codereview.chromium.org/7646025/diff/38002/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:540: TEST_F(DownloadManagerTest, DownloadFileErrorTest) { On 2011/08/26 18:51:40, rdsmith wrote: > I don't think this belongs under DownloadManagerTest; maybe > download_file_unittest.cc ? (ETA: It's fuzzier for the full set of > functionality you're testing, but in an ideal world, functionality that was > primarily around DownloadFile would be someplace else. But it's ok to keep this > here given what you're actually testing.) OK. Keeping. http://codereview.chromium.org/7646025/diff/38002/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:542: using ::testing::CreateFunctor; On 2011/08/26 18:51:40, rdsmith wrote: > This has the look of a pattern copied from elsewhere in the code, and if it's a > common pattern used in the chrome code base, I'm good with it. But if it's not: > I'm specifically concerned with "using ::testing::_", as "_" is a heck of a name > to have have a specific meaning in a file. I think readers will have a hard > time figuring out what that's about. Yes, it was copied. The pattern is from using the gmock testing code, but I'm not using it here so I've deleted this section. http://codereview.chromium.org/7646025/diff/38002/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:549: scoped_ptr<DownloadCreateInfo> info(new DownloadCreateInfo); On 2011/08/26 18:51:40, rdsmith wrote: > Any reason to make it a scoped_ptr rather than just declare it inline in the > stack? Not really, other than as an example of how it is normally used. Changed. http://codereview.chromium.org/7646025/diff/38002/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:558: linked_ptr<net::FileStream> stream(mock_stream); On 2011/08/26 18:51:40, rdsmith wrote: > Why a linked_ptr? They're rare enough that I think of them as only being used > for special purposes; what's the purpose here? If there's a reason for this, > please put a comment in explaining it; if not, maybe just use a scoped_ptr? This was necessary in order to assign it to info->save_info.file_stream. However, with the change to the constructor of DownloadFileWithMockStream I don't need it any more. Removed. http://codereview.chromium.org/7646025/diff/38002/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:563: info->save_info.file_stream = stream; On 2011/08/26 18:51:40, rdsmith wrote: > I'd suggest moving the stream initialization to before the info initialization; > that way each initialization is consistent and reading through you don't have to > jump back and forth between them. Done. http://codereview.chromium.org/7646025/diff/38002/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:614: download->Cancel(true); On 2011/08/26 18:51:40, rdsmith wrote: > Everything from here on in doesn't test any error detection functionality, does > it? If this functionality is tested elsewhere in the file, I'd eliminate it > here, to keep this test simple. Done. http://codereview.chromium.org/7646025/diff/40001/net/base/mock_file_stream.h File net/base/mock_file_stream.h (right): http://codereview.chromium.org/7646025/diff/40001/net/base/mock_file_stream.h... net/base/mock_file_stream.h:42: OVERRIDE { On 2011/08/26 20:51:50, wtc wrote: > Nit: it may be better to list net::CompletionCallback* callback) > on a separate line: > virtual int Read(char* buf, > int buf_len, > net::CompletionCallback* callback) OVERRIDE { > > This is how you formatted the Write() method. Done. http://codereview.chromium.org/7646025/diff/40001/net/base/mock_file_stream.h... net/base/mock_file_stream.h:64: const FilePath& get_path() const { return path_; } On 2011/08/26 20:51:50, wtc wrote: > Nit: list get_path() along with the set_forced_error() > and clear_forced_error() methods above, because they're all > simple getters/setters. Done. http://codereview.chromium.org/7646025/diff/40001/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/7646025/diff/40001/net/net.gyp#newcode937 net/net.gyp:937: 'base/mock_file_stream.h', On 2011/08/26 20:51:50, wtc wrote: > I wonder if this file should be added to the > 'net_test_support' target instead. Done.
http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... File chrome/browser/download/download_manager_unittest.cc (right): http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:175: testing::MockFileStream* mock_stream = Please put in a comment as to why the upcast is ok (i.e. that the only places this can be set from are the constructor and the reset below, and in both cases you're setting from a MockFileStream). http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:573: ::testing::Mock::AllowLeak(download_file); Why does the leak occur? Shouldn't it be cleaned up when we clean up DownloadFileManager? (More generally, any way to write the test so that we cleanup as opposed to leaking?) http://codereview.chromium.org/7646025/diff/49001/content/browser/download/do... File content/browser/download/download_manager.cc (left): http://codereview.chromium.org/7646025/diff/49001/content/browser/download/do... content/browser/download/download_manager.cc:513: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); Why remove the DCHECK?
http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... File chrome/browser/download/download_manager_unittest.cc (right): http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:85: void UpdateData(int32 id, const void* data, size_t length) { Should |data| be const char* to be consistent with IOBuffer's representation? http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:88: // We need |AddRef()| because io_buffer is not a |scoped_refptr|, and we This comment isn't quite correct. If io_buffer were a scoped_refptr, than the Release() would be called at function termination. You need the Release which is called in DownloadFileManager::UpdateDownload. I took a look at the DownloadResourceHandler as a result of this, and it maintains the refcount in a funky way - it uses a scoped_refptr to bump it to 1, and then does a swap [should be a release to be clearer] to a raw pointer, followed by the release in DownloadFileManager::UpdateDownload. http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:94: base::AutoLock auto_lock(download_buffer_.lock); This is kinda gross, although it already existed. Seems like DownloadBuffer should move to a thread-safe class which hides when locks are acquired. http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:107: message_loop_.RunAllPending(); Why do you need to do this inside of UpdateData? http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:160: // as a friend of |DownloadFile| in order to access its private members. Could you just add a DownloadFile constructor which accepts a FileStream argument? The BaseFile one does. Then you wouldn't need to make this a friend class of BaseFile. http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:182: virtual void CreateFileStream() { OVERRIDE http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:182: virtual void CreateFileStream() { Is this expected to be called? http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:558: // don't call the function that deletes it, so we can use a stack variable. Looks like other tests use scoped_ptr. I'd either convert this to scoped_ptr or change the others to be stack variables to be consistent. By the way, what function normally deletes DownloadCreateInfo? This seems a bit fragile? http://codereview.chromium.org/7646025/diff/49001/content/browser/download/ba... File content/browser/download/base_file_unittest.cc (right): http://codereview.chromium.org/7646025/diff/49001/content/browser/download/ba... content/browser/download/base_file_unittest.cc:68: if (mock_file_stream_->Open( Nit: if (!mock_file_stream_->Open may be clearer http://codereview.chromium.org/7646025/diff/49001/net/base/mock_file_stream.h File net/base/mock_file_stream.h (right): http://codereview.chromium.org/7646025/diff/49001/net/base/mock_file_stream.h... net/base/mock_file_stream.h:19: class MockFileStream : public net::FileStream { You should run a try-job through one of the clang bots - it may not like the inlined destructor. http://codereview.chromium.org/7646025/diff/49001/net/base/mock_file_stream.h... net/base/mock_file_stream.h:22: MockFileStream(base::PlatformFile file, int flags) Nit: newline between these two. http://codereview.chromium.org/7646025/diff/49001/net/base/mock_file_stream.h... net/base/mock_file_stream.h:87: int forced_error_; Nit: indentation looks off relative to the other commands. http://codereview.chromium.org/7646025/diff/49001/net/base/mock_file_stream.h... net/base/mock_file_stream.h:87: int forced_error_; Should this be an int64? Or are the upcasts from int-to-int64 fully acceptable here?
Caveat: Still waiting on results of linux_clang bot on previous patch. http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... File chrome/browser/download/download_manager_unittest.cc (right): http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:85: void UpdateData(int32 id, const void* data, size_t length) { On 2011/08/29 15:05:27, cbentzel wrote: > Should |data| be const char* to be consistent with IOBuffer's representation? I don't have a strong opinion about this. Changed. http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:88: // We need |AddRef()| because io_buffer is not a |scoped_refptr|, and we On 2011/08/29 15:05:27, cbentzel wrote: > This comment isn't quite correct. If io_buffer were a scoped_refptr, than the > Release() would be called at function termination. You need the Release which is > called in DownloadFileManager::UpdateDownload. > > I took a look at the DownloadResourceHandler as a result of this, and it > maintains the refcount in a funky way - it uses a scoped_refptr to bump it to 1, > and then does a swap [should be a release to be clearer] to a raw pointer, > followed by the release in DownloadFileManager::UpdateDownload. Yeah, it's funky. Changed. http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:94: base::AutoLock auto_lock(download_buffer_.lock); On 2011/08/29 15:05:27, cbentzel wrote: > This is kinda gross, although it already existed. Seems like DownloadBuffer > should move to a thread-safe class which hides when locks are acquired. I think that's outside the scope of this CL. http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:107: message_loop_.RunAllPending(); On 2011/08/29 15:05:27, cbentzel wrote: > Why do you need to do this inside of UpdateData? For other operations. Most tests don't need to use UpdateData(), and the one that does also calls ContinueDownloadWithPath() and DownloadItem::Cancel(), both of which need to run the message loop. http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:160: // as a friend of |DownloadFile| in order to access its private members. On 2011/08/29 15:05:27, cbentzel wrote: > Could you just add a DownloadFile constructor which accepts a FileStream > argument? The BaseFile one does. Then you wouldn't need to make this a friend > class of BaseFile. It needs more information than just the file stream, for example, the DownloadManager. http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:175: testing::MockFileStream* mock_stream = On 2011/08/28 22:14:03, rdsmith wrote: > Please put in a comment as to why the upcast is ok (i.e. that the only places > this can be set from are the constructor and the reset below, and in both cases > you're setting from a MockFileStream). Done. http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:182: virtual void CreateFileStream() { On 2011/08/29 15:05:27, cbentzel wrote: > Is this expected to be called? Yes, from Open(). http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:182: virtual void CreateFileStream() { On 2011/08/29 15:05:27, cbentzel wrote: > OVERRIDE Done. http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:558: // don't call the function that deletes it, so we can use a stack variable. On 2011/08/29 15:05:27, cbentzel wrote: > Looks like other tests use scoped_ptr. I'd either convert this to scoped_ptr or > change the others to be stack variables to be consistent. > > By the way, what function normally deletes DownloadCreateInfo? This seems a bit > fragile? DownloadFileManager::CreateDownloadFile() normally deletes DownloadCreateInfo. I was using a scoped_ptr here, but Randy asked me to change it. http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:573: ::testing::Mock::AllowLeak(download_file); On 2011/08/28 22:14:03, rdsmith wrote: > Why does the leak occur? Shouldn't it be cleaned up when we clean up > DownloadFileManager? (More generally, any way to write the test so that we > cleanup as opposed to leaking?) Hmm, thinking about it, due to recent refactorings it should clean up the DownloadFile as a result of the error. Removed this line. http://codereview.chromium.org/7646025/diff/49001/content/browser/download/ba... File content/browser/download/base_file_unittest.cc (right): http://codereview.chromium.org/7646025/diff/49001/content/browser/download/ba... content/browser/download/base_file_unittest.cc:68: if (mock_file_stream_->Open( On 2011/08/29 15:05:27, cbentzel wrote: > Nit: if (!mock_file_stream_->Open > > may be clearer Hmm, I like this better because you can see that Open() is returning something that is not a bool. http://codereview.chromium.org/7646025/diff/49001/content/browser/download/do... File content/browser/download/download_manager.cc (left): http://codereview.chromium.org/7646025/diff/49001/content/browser/download/do... content/browser/download/download_manager.cc:513: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); On 2011/08/28 22:14:03, rdsmith wrote: > Why remove the DCHECK? I removed it because GetActiveDownload() has it. However, I don't feel strongly about it, so I'll put it back in. Done. http://codereview.chromium.org/7646025/diff/49001/net/base/mock_file_stream.h File net/base/mock_file_stream.h (right): http://codereview.chromium.org/7646025/diff/49001/net/base/mock_file_stream.h... net/base/mock_file_stream.h:19: class MockFileStream : public net::FileStream { On 2011/08/29 15:05:27, cbentzel wrote: > You should run a try-job through one of the clang bots - it may not like the > inlined destructor. Done. http://codereview.chromium.org/7646025/diff/49001/net/base/mock_file_stream.h... net/base/mock_file_stream.h:22: MockFileStream(base::PlatformFile file, int flags) On 2011/08/29 15:05:27, cbentzel wrote: > Nit: newline between these two. Done. http://codereview.chromium.org/7646025/diff/49001/net/base/mock_file_stream.h... net/base/mock_file_stream.h:87: int forced_error_; On 2011/08/29 15:05:27, cbentzel wrote: > Should this be an int64? Or are the upcasts from int-to-int64 fully acceptable > here? They are acceptable, and this prevents having to downcast from int64 to int in other places. http://codereview.chromium.org/7646025/diff/49001/net/base/mock_file_stream.h... net/base/mock_file_stream.h:87: int forced_error_; On 2011/08/29 15:05:27, cbentzel wrote: > Nit: indentation looks off relative to the other commands. Done.
A few question responses on discussions between Chris and you; continuing on to full review now. http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... File chrome/browser/download/download_manager_unittest.cc (right): http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:94: base::AutoLock auto_lock(download_buffer_.lock); On 2011/08/29 16:30:53, ahendrickson wrote: > On 2011/08/29 15:05:27, cbentzel wrote: > > This is kinda gross, although it already existed. Seems like DownloadBuffer > > should move to a thread-safe class which hides when locks are acquired. > > I think that's outside the scope of this CL. Agreed with both points (suggested refactor, and that it's outside the scope of this CL). http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:558: // don't call the function that deletes it, so we can use a stack variable. On 2011/08/29 16:30:53, ahendrickson wrote: > On 2011/08/29 15:05:27, cbentzel wrote: > > Looks like other tests use scoped_ptr. I'd either convert this to scoped_ptr > or > > change the others to be stack variables to be consistent. > > > > By the way, what function normally deletes DownloadCreateInfo? This seems a > bit > > fragile? > > DownloadFileManager::CreateDownloadFile() normally deletes DownloadCreateInfo. > > I was using a scoped_ptr here, but Randy asked me to change it. I'm cool with shifting back to scoped_ptr; I agree with Chris that consistency is more important.
LGTM, with the below nit, and the caveat that I couldn't figure out what file you mentioned to me (offline conversation) that you hadn't added. Clear that up with me before you commit? http://codereview.chromium.org/7646025/diff/59001/chrome/browser/download/dow... File chrome/browser/download/download_manager_unittest.cc (right): http://codereview.chromium.org/7646025/diff/59001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:186: void DownloadFileWithMockStream::CreateFileStream() { nit: I'd like to see the class written consistently; willing to split out all the other methods too?
LGTM http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... File chrome/browser/download/download_manager_unittest.cc (right): http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:182: virtual void CreateFileStream() { On 2011/08/29 16:30:53, ahendrickson wrote: > On 2011/08/29 15:05:27, cbentzel wrote: > > Is this expected to be called? > > Yes, from Open(). Well, it could be called from Open(), but that only happens if file_stream_ isn't set. In this case, shouldn't it always be set? http://codereview.chromium.org/7646025/diff/49001/content/browser/download/ba... File content/browser/download/base_file_unittest.cc (right): http://codereview.chromium.org/7646025/diff/49001/content/browser/download/ba... content/browser/download/base_file_unittest.cc:68: if (mock_file_stream_->Open( On 2011/08/29 16:30:53, ahendrickson wrote: > On 2011/08/29 15:05:27, cbentzel wrote: > > Nit: if (!mock_file_stream_->Open > > > > may be clearer > > Hmm, I like this better because you can see that Open() is returning something > that is not a bool. OK
wtc: PTAL http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... File chrome/browser/download/download_manager_unittest.cc (right): http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:182: virtual void CreateFileStream() { On 2011/08/29 17:27:55, cbentzel wrote: > On 2011/08/29 16:30:53, ahendrickson wrote: > > On 2011/08/29 15:05:27, cbentzel wrote: > > > Is this expected to be called? > > > > Yes, from Open(). > > Well, it could be called from Open(), but that only happens if file_stream_ > isn't set. In this case, shouldn't it always be set? No, the rename operation does a Close() (which resets file_stream_ to NULL), and then an Open(), which calls this. http://codereview.chromium.org/7646025/diff/49001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:558: // don't call the function that deletes it, so we can use a stack variable. On 2011/08/29 16:57:57, rdsmith wrote: > On 2011/08/29 16:30:53, ahendrickson wrote: > > On 2011/08/29 15:05:27, cbentzel wrote: > > > Looks like other tests use scoped_ptr. I'd either convert this to scoped_ptr > > or > > > change the others to be stack variables to be consistent. > > > > > > By the way, what function normally deletes DownloadCreateInfo? This seems a > > bit > > > fragile? > > > > DownloadFileManager::CreateDownloadFile() normally deletes DownloadCreateInfo. > > > > I was using a scoped_ptr here, but Randy asked me to change it. > > I'm cool with shifting back to scoped_ptr; I agree with Chris that consistency > is more important. Done. http://codereview.chromium.org/7646025/diff/59001/chrome/browser/download/dow... File chrome/browser/download/download_manager_unittest.cc (right): http://codereview.chromium.org/7646025/diff/59001/chrome/browser/download/dow... chrome/browser/download/download_manager_unittest.cc:186: void DownloadFileWithMockStream::CreateFileStream() { On 2011/08/29 17:02:07, rdsmith wrote: > nit: I'd like to see the class written consistently; willing to split out all > the other methods too? Done.
LGTM on the files under src/net in Patch Set 19. Please address my comments below before checking this in. http://codereview.chromium.org/7646025/diff/61006/net/base/mock_file_stream.cc File net/base/mock_file_stream.cc (right): http://codereview.chromium.org/7646025/diff/61006/net/base/mock_file_stream.c... net/base/mock_file_stream.cc:9: #include "net/base/net_errors.h" Remove these three headers. They are already included by "net/base/mock_file_stream.h. http://codereview.chromium.org/7646025/diff/61006/net/base/mock_file_stream.h File net/base/mock_file_stream.h (right): http://codereview.chromium.org/7646025/diff/61006/net/base/mock_file_stream.h... net/base/mock_file_stream.h:17: namespace testing { IMPORTANT: I believe the top-level namespace "testing" is used by gtest. So we should either just use our namespace "net", or make this nested inside our namespace "net". http://codereview.chromium.org/7646025/diff/61006/net/base/mock_file_stream.h... net/base/mock_file_stream.h:38: virtual int Flush() OVERRIDE; IMPORTANT: You didn't override the following methods: virtual void Close(); virtual bool IsOpen() const; Is that intentional?
http://codereview.chromium.org/7646025/diff/61006/net/base/mock_file_stream.cc File net/base/mock_file_stream.cc (right): http://codereview.chromium.org/7646025/diff/61006/net/base/mock_file_stream.c... net/base/mock_file_stream.cc:9: #include "net/base/net_errors.h" On 2011/08/29 18:17:05, wtc wrote: > Remove these three headers. They are already included > by "net/base/mock_file_stream.h. Done. http://codereview.chromium.org/7646025/diff/61006/net/base/mock_file_stream.h File net/base/mock_file_stream.h (right): http://codereview.chromium.org/7646025/diff/61006/net/base/mock_file_stream.h... net/base/mock_file_stream.h:17: namespace testing { On 2011/08/29 18:17:05, wtc wrote: > IMPORTANT: I believe the top-level namespace "testing" is > used by gtest. So we should either just use our namespace > "net", or make this nested inside our namespace "net". Now using the net::testing namespace. http://codereview.chromium.org/7646025/diff/61006/net/base/mock_file_stream.h... net/base/mock_file_stream.h:38: virtual int Flush() OVERRIDE; On 2011/08/29 18:17:05, wtc wrote: > IMPORTANT: You didn't override the following methods: > virtual void Close(); > virtual bool IsOpen() const; > > Is that intentional? Yes. Close() does not return an error code, so it doesn't need an override. IsOpen() only reports the current state of the file handle, so it doesn't either.
LGTM on src/net in Patch Set 20. My preference would be to put MockFileStream in the "net" namespace, because that's where the existing Mock* classes in src/net are. You may want to poll a few people on the network team for their opinion. You can deal with this issue after committing this CL. I don't want this CL to drag on too long.
http://codereview.chromium.org/7646025/diff/61006/net/base/mock_file_stream.h File net/base/mock_file_stream.h (right): http://codereview.chromium.org/7646025/diff/61006/net/base/mock_file_stream.h... net/base/mock_file_stream.h:38: virtual int Flush() OVERRIDE; On 2011/08/29 19:42:38, ahendrickson wrote: > > IsOpen() only reports the current state of the file handle, so it doesn't > [need an override ]either. It seems that if MockFileStream::Open() fails, then MockFileStream::IsOpen() should return false to be consistent. Is this guaranteed? Anyway, this is just a nit. I just wanted to ask you to consider this.
Change committed as 98725 |
