Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6096003:
Put history insertion for downloads processing inline. (Closed)
|
Created:
9 years, 12 months ago by Randy Smith (Not in Mondays) Modified:
9 years, 7 months ago CC:
chromium-reviews, rdsmith+dwatch_chromium.org, Paweł Hajdan Jr. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPut history insertion for downloads processing inline.
This change shifts standard download processing to wait on history
insertion (note that actual data download does not wait). This makes the
download process less racy and is expected to make the download tests less
flaky.
Note that it also brings the rename to the download intermediate file rename
inline into standard download processing; i.e. now we always rename to the
intermediate file name and then to the final file name. This again reduces
raciness and possibly tset flakiness.
BUG=63237
TEST=All current download tests.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72301
Patch Set 1 #
Total comments: 16
Patch Set 2 : Incorporated latest round of comments. #
Total comments: 8
Patch Set 3 : Incorporated comments, fixed bugs. #
Total comments: 9
Patch Set 4 : Incorporated comments, fixed windows int64->int compile error. #
Total comments: 2
Patch Set 5 : Split state and data reception changes, got rid of pending_finished_downloads_. #
Total comments: 8
Patch Set 6 : Incorporated Pawel's comments. #
Messages
Total messages: 20 (0 generated)
Hey, folks, could you take a look at this change? I'm hopefully it'll be a pretty major step towards deflaking the tests (== it's the only currently known root cause for all the test flakiness, but I'm sure I'll find more when this is gotten out of the way). Quick detailed list of changes: * Rewrite join betweeen data download and UI setup pathway to be somewhat clearer (this is in MaybeCompleteDownload and its callers). * Move the UI side of that join to OnCreateDownloadEntryComplete so that we have fewer race cases to handle; the entry will always have been made in the history now when we finish the download. * Unilaterally change file name to intermediate name, so we always go through the same code path no matter who wins the data received<->UI setup race. * Add a comment on normal download_item.cc life cycle. * Move calls to UpdateAppIcon() to near the changes that make them necessary and comment them with reference to those changes.
On 2010/12/27 23:08:11, rdsmith wrote: > Hey, folks, could you take a look at this change? I'm hopefully it'll be a > pretty major step towards deflaking the tests (== it's the only currently known > root cause for all the test flakiness, but I'm sure I'll find more when this is > gotten out of the way). > > Quick detailed list of changes: > * Rewrite join betweeen data download and UI setup pathway to be somewhat > clearer (this is in MaybeCompleteDownload and its callers). > * Move the UI side of that join to OnCreateDownloadEntryComplete so that we have > fewer race cases to handle; the entry will always have been made in the history > now when we finish the download. > * Unilaterally change file name to intermediate name, so we always go through > the same code path no matter who wins the data received<->UI setup race. > * Add a comment on normal download_item.cc life cycle. > * Move calls to UpdateAppIcon() to near the changes that make them necessary and > comment them with reference to those changes. Basically, LGTM, but it has problems with DownloadRenameTest.
On 2010/12/29 15:37:29, ahendrickson wrote: > Basically, LGTM, but it has problems with DownloadRenameTest. Yep, I had just noticed that, and in 20-20 hindsight, it makes sense that it would. I've added all the DownloadManagerTest unittests to my standard suite that I run before upload (oops), and I won't commit until I resolve the DownloadRenameTest issue. Thanks for the quick review!
http://codereview.chromium.org/6096003/diff/1/chrome/browser/download/downloa... File chrome/browser/download/download_item.cc (right): http://codereview.chromium.org/6096003/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_item.cc:30: // The lifecycle of a DownloadItem under normal conditions: I'm not sure if it's a good idea to put all that info in a comment. It's very likely to get outdated and increase confusion. http://codereview.chromium.org/6096003/diff/1/chrome/browser/download/downloa... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6096003/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.cc:465: UpdateAppIcon(); // Reflect entry into in_progress_. nit: Just two spaces between code and comment. Please fix in entire CL. http://codereview.chromium.org/6096003/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.cc:511: DCHECK_EQ(pending_finished_downloads_.count(download_id), 0U); nit: 0U should be first parameter (convention). http://codereview.chromium.org/6096003/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.cc:522: if (!(pending_finished_downloads_.count(download_id) != 0 && Can we extract this logic to a separate method? http://codereview.chromium.org/6096003/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.cc:531: DCHECK(in_progress_.count(download_id) == 1); nit: DCHECK_EQ? http://codereview.chromium.org/6096003/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.cc:532: DCHECK(pending_finished_downloads_.count(download_id) == 1); nit: DCHECK_EQ? http://codereview.chromium.org/6096003/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.cc:535: DCHECK(history_downloads_.count(download->db_handle()) == 1); nit: DCHECK_EQ? http://codereview.chromium.org/6096003/diff/1/chrome/browser/download/downloa... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/6096003/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.h:132: void MaybeCompleteDownload(int32 download_id, int64 size); Can we find a more descriptive name, i.e. one that describes what this method does, not when it is called?
Incorporated Pawel's comments; waiting for LGTMs from Pawel and Brett. Also still need to get a successful pyauto test run before committing (which may take some debugging) as well as making sure I haven't messed up the Save page/link pathway. So I won't be committing instantly regardless. http://codereview.chromium.org/6096003/diff/1/chrome/browser/download/downloa... File chrome/browser/download/download_item.cc (right): http://codereview.chromium.org/6096003/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_item.cc:30: // The lifecycle of a DownloadItem under normal conditions: On 2011/01/03 09:43:03, Paweł Hajdan Jr. wrote: > I'm not sure if it's a good idea to put all that info in a comment. It's very > likely to get outdated and increase confusion. Fair enough. Long term I really feel like there needs to be more documentation on the lifecycle of a download item, but this is probably too much. Let me know what you think of my much shorter version. http://codereview.chromium.org/6096003/diff/1/chrome/browser/download/downloa... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6096003/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.cc:465: UpdateAppIcon(); // Reflect entry into in_progress_. On 2011/01/03 09:43:03, Paweł Hajdan Jr. wrote: > nit: Just two spaces between code and comment. Please fix in entire CL. Done. http://codereview.chromium.org/6096003/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.cc:511: DCHECK_EQ(pending_finished_downloads_.count(download_id), 0U); On 2011/01/03 09:43:03, Paweł Hajdan Jr. wrote: > nit: 0U should be first parameter (convention). Done. http://codereview.chromium.org/6096003/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.cc:522: if (!(pending_finished_downloads_.count(download_id) != 0 && On 2011/01/03 09:43:03, Paweł Hajdan Jr. wrote: > Can we extract this logic to a separate method? Done. http://codereview.chromium.org/6096003/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.cc:531: DCHECK(in_progress_.count(download_id) == 1); On 2011/01/03 09:43:03, Paweł Hajdan Jr. wrote: > nit: DCHECK_EQ? Done. http://codereview.chromium.org/6096003/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.cc:532: DCHECK(pending_finished_downloads_.count(download_id) == 1); On 2011/01/03 09:43:03, Paweł Hajdan Jr. wrote: > nit: DCHECK_EQ? Done. http://codereview.chromium.org/6096003/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.cc:535: DCHECK(history_downloads_.count(download->db_handle()) == 1); On 2011/01/03 09:43:03, Paweł Hajdan Jr. wrote: > nit: DCHECK_EQ? Done. http://codereview.chromium.org/6096003/diff/1/chrome/browser/download/downloa... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/6096003/diff/1/chrome/browser/download/downloa... chrome/browser/download/download_manager.h:132: void MaybeCompleteDownload(int32 download_id, int64 size); On 2011/01/03 09:43:03, Paweł Hajdan Jr. wrote: > Can we find a more descriptive name, i.e. one that describes what this method > does, not when it is called? Hmmm. My call would be that MaybeCompleteDownload actually does describe what the call does. Based on a certain set of conditions it may or may not complete the download. But I think your concern applies well to the comment, and I've tried to update the comment to reflect that concern. Let me know what you think.
Only minor comments. http://codereview.chromium.org/6096003/diff/9001/chrome/browser/download/down... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6096003/diff/9001/chrome/browser/download/down... chrome/browser/download/download_manager.cc:522: // linear, so we can simply check for history insertion. Can you DCHECK that this assumption is valid? This code is probably going to be heavily refactored, so things may change a bit unpredictably. http://codereview.chromium.org/6096003/diff/9001/chrome/browser/download/down... chrome/browser/download/download_manager.cc:523: return nit: This is a bit complicated, so maybe we can convert to a series of early returns like: if (pending_finished_downloads_.count(...) == 0) return false; if (in_progress_.count(...) == 0) return false; ... return true; http://codereview.chromium.org/6096003/diff/9001/chrome/browser/download/down... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/6096003/diff/9001/chrome/browser/download/down... chrome/browser/download/download_manager.h:131: bool DownloadReadyForCompletion(int32 download_id); nit: Can we rename it to IsDownloadReadyForCompletion, to avoid confusion with an outside notification that the download is ready for completion? Or maybe simpler IsDownloadComplete? nit: We have now two very similar concepts of completion in the download system: "completed" and "finished". This is probably not something that can be fixed in this CL, but it would be nice to remove this confusion or at least explain it. http://codereview.chromium.org/6096003/diff/9001/chrome/browser/download/down... File chrome/browser/download/download_manager_unittest.cc (right): http://codereview.chromium.org/6096003/diff/9001/chrome/browser/download/down... chrome/browser/download/download_manager_unittest.cc:155: // Safe download, download finishes BEFORE file determined. nit: file determined -> file name determined?
Ok, here's (what I hope is :-}) the final upload for this patch. The tests are passing much more reliably, as befits a fairly large step forward towards deracing the core system. I had two very sporadic (i.e. one each out of ~20 runs) failures in the pyauto tests that I filed as issues 69460 and 69489 and will deal with later (one I have a possible explanation for that isn't related to this CL, the other I doubt is related to this CL, but have no hypothesis for, but downloads are buggy enough that I'd rather not hold this CL up on fixing it). I will get a clean (or analyzed and believed good) try bot run before committing. In this patch set I've shifted the signature of MaybeCompleteDownload(), as the last patch set was accessing pending_finished_downloads_[download_id] before it was set, and as a side effect, setting it. Thanks to Andy for the large influx of new tests, several of which pointed straight at that particular bug. I also removed the TODO() in OnCreateDownloadEntryComplete, as the point of this change is moving actual download completion after that point in the code, which means we don't need to worry about racing with the COMPLETE status in the DownloadItem. Let me know what you think when you have a chance. (Brett, haven't heard anything from you on this CL yet?) http://codereview.chromium.org/6096003/diff/9001/chrome/browser/download/down... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6096003/diff/9001/chrome/browser/download/down... chrome/browser/download/download_manager.cc:522: // linear, so we can simply check for history insertion. On 2011/01/05 07:46:30, Paweł Hajdan Jr. wrote: > Can you DCHECK that this assumption is valid? This code is probably going to be > heavily refactored, so things may change a bit unpredictably. It's going to be heavily refactored to make it more and more linear :-}. Still, I hear you. The problem is that we don't have reliable markers for anything but history insertion. UI Display is a consumer of ours that registers for notifications but that we don't really have a pointer to. And the file name determination and intermediate file rename is a mess that needs its own refactor--I could come up with some hacky way to probe that we've done that in the current code, but I think it would be far more likely to break in subsequent refactors than to show anything useful. Eventually what I want to do is have each of the steps I mention above correspond to an (internally visible only) state in the DownloadItem, have a single routine that does state transitions, and do DCHECKs in that routine for valid state transition. IMO, that's the right way to handle this request. But if you'll allow me, I'd like to postpone the request until that point--it's not that I disagree with your goal, but I don't see a way to satisfy it without making this CL much more complicated than it is (and make the base code somewhat more complicated that I'd prefer). http://codereview.chromium.org/6096003/diff/9001/chrome/browser/download/down... chrome/browser/download/download_manager.cc:523: return On 2011/01/05 07:46:30, Paweł Hajdan Jr. wrote: > nit: This is a bit complicated, so maybe we can convert to a series of early > returns like: > > if (pending_finished_downloads_.count(...) == 0) > return false; > > if (in_progress_.count(...) == 0) > return false; > > ... > > return true; Done. http://codereview.chromium.org/6096003/diff/9001/chrome/browser/download/down... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/6096003/diff/9001/chrome/browser/download/down... chrome/browser/download/download_manager.h:131: bool DownloadReadyForCompletion(int32 download_id); On 2011/01/05 07:46:30, Paweł Hajdan Jr. wrote: > nit: Can we rename it to IsDownloadReadyForCompletion, to avoid confusion with > an outside notification that the download is ready for completion? Or maybe > simpler IsDownloadComplete? Changed to IsDownloadReadyForCompletion() (IsDownloadComplete() isn't accurate; it isn't complete until we finish what we're doing). > nit: We have now two very similar concepts of completion in the download system: > "completed" and "finished". This is probably not something that can be fixed in > this CL, but it would be nice to remove this confusion or at least explain it. I think my other CL is doing some of this; specifically, we're moving DownloadItem::COMPLETE to mean "really done" and the intermediate state is DATA_RECEIVED. We need to get ride of pending_finished_downloads_ (which I want to do anyway) and it may take a little while to bring all the comments up to date with that nomenclature. I'll take an AI to take a swing at that. http://codereview.chromium.org/6096003/diff/9001/chrome/browser/download/down... File chrome/browser/download/download_manager_unittest.cc (right): http://codereview.chromium.org/6096003/diff/9001/chrome/browser/download/down... chrome/browser/download/download_manager_unittest.cc:155: // Safe download, download finishes BEFORE file determined. On 2011/01/05 07:46:30, Paweł Hajdan Jr. wrote: > nit: file determined -> file name determined? Done (all four cases).
http://codereview.chromium.org/6096003/diff/17001/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6096003/diff/17001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:495: UpdateAppIcon(); // Reflect size updates. Why not set a flag if we update a download, then call |UpdateAppIcon()| only if the flag is set, after the loop? The current way would call it multiple times if there is more than one download in progress. http://codereview.chromium.org/6096003/diff/17001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:505: DCHECK_EQ(0U, pending_finished_downloads_.count(download_id)); Would |ContainsKey()| work here, and in succeeding instances? |count()| will only return 0 or 1 for a set or a map (as opposed to a multi-map), right? I find it more readable. http://codereview.chromium.org/6096003/diff/17001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:558: download->OnAllDataSaved(size); It seems odd not to call |download->OnAllDataSaved()| as soon as all the data has been saved (that is, wait on the final name). Perhaps the function name needs to be changed.
http://codereview.chromium.org/6096003/diff/17001/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6096003/diff/17001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:558: download->OnAllDataSaved(size); On 2011/01/13 03:15:17, ahendrickson wrote: > It seems odd not to call |download->OnAllDataSaved()| as soon as all the data > has been saved (that is, wait on the final name). Yes, definitely. I thought that OnAllDataSaved should trigger running the body of MaybeCompleteDownload, not the other way around.
Round n+1 :-}. Just about all tests pass, though I'm occasionally seeing 69460 (which I'll fix in a different CL). I am still struggling with the try server to get a try run that compiles, primarily because this CL has been spread over many intervening changes to the underlying files. But I don't expect there to be any problems once I get past the patch point on the try server. http://codereview.chromium.org/6096003/diff/17001/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6096003/diff/17001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:495: UpdateAppIcon(); // Reflect size updates. On 2011/01/13 03:15:17, ahendrickson wrote: > Why not set a flag if we update a download, then call |UpdateAppIcon()| only if > the flag is set, after the loop? > > The current way would call it multiple times if there is more than one download > in progress. I'm very confused. What loop? We call find, get a single download, and test if the state is IN_PROGRESS. What am I missing? http://codereview.chromium.org/6096003/diff/17001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:505: DCHECK_EQ(0U, pending_finished_downloads_.count(download_id)); On 2011/01/13 03:15:17, ahendrickson wrote: > Would |ContainsKey()| work here, and in succeeding instances? > > |count()| will only return 0 or 1 for a set or a map (as opposed to a > multi-map), right? > > I find it more readable. So if you ask me for this change again I'll do it. But I don't like ContainsKey(), as it's not part of the standard library and count() is; it's yet another routine that readers of the code have to remember what it does. When I can accomplish the same functionality in the same number of lines using a smaller subset of routines, I prefer to do that. http://codereview.chromium.org/6096003/diff/17001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:558: download->OnAllDataSaved(size); Well, DownloadManager::OnAllDataSaved triggers DownloadManager::MaybeCompleteDownload, so that much is right in the world :-}. Some of the point for me of the overall refactor is to have anything happening in the background be recognized at very specific points in the linear chain, and this is where I want to recognize that all the data has been saved in the download item (and make the transition to the COMPLETE) state. I've changed the name to OnReadyToFinish, and changed the documentation around the COMPLETE state in the DownloadItem to indicate that that state indicates that we've gotten all the data, chosen the final filename, entered ourselves in the history, and been displayed ot the user. But I'm not sure if that'll really address your concerns; any suggestions for a better name? On 2011/01/13 08:43:10, Paweł Hajdan Jr. wrote: > On 2011/01/13 03:15:17, ahendrickson wrote: > > It seems odd not to call |download->OnAllDataSaved()| as soon as all the data > > has been saved (that is, wait on the final name). > > Yes, definitely. I thought that OnAllDataSaved should trigger running the body > of MaybeCompleteDownload, not the other way around.
http://codereview.chromium.org/6096003/diff/17001/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6096003/diff/17001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:558: download->OnAllDataSaved(size); On 2011/01/16 20:43:56, rdsmith wrote: > I've changed the name to OnReadyToFinish, and changed the documentation around > the COMPLETE state in the DownloadItem to indicate that that state indicates > that we've gotten all the data, chosen the final filename, entered ourselves in > the history, and been displayed ot the user. But I'm not sure if that'll really > address your concerns; any suggestions for a better name? I'm sorry, I actually think that OnReadyToFinish is a step backwards (the name is more vague than OnAllDataSaved). The main concern is that it seems calling OnAllDataSaved here wasn't needed before. Is it now really required, or can we do the related refactoring in a separate change? http://codereview.chromium.org/6096003/diff/24001/chrome/browser/download/dow... File chrome/browser/download/download_item.h (right): http://codereview.chromium.org/6096003/diff/24001/chrome/browser/download/dow... chrome/browser/download/download_item.h:48: // a dangerous download, and renaming the file to the final name and nit: "and..." - is the later part of this comment missing?
http://codereview.chromium.org/6096003/diff/17001/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/6096003/diff/17001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:558: download->OnAllDataSaved(size); On 2011/01/17 18:13:23, Paweł Hajdan Jr. wrote: > On 2011/01/16 20:43:56, rdsmith wrote: > > I've changed the name to OnReadyToFinish, and changed the documentation around > > the COMPLETE state in the DownloadItem to indicate that that state indicates > > that we've gotten all the data, chosen the final filename, entered ourselves > in > > the history, and been displayed ot the user. But I'm not sure if that'll > really > > address your concerns; any suggestions for a better name? > > I'm sorry, I actually think that OnReadyToFinish is a step backwards (the name > is more vague than OnAllDataSaved). The main concern is that it seems calling > OnAllDataSaved here wasn't needed before. Is it now really required, or can we > do the related refactoring in a separate change? Sorry, now you've gotten me confused. download->OnAllDataSaved() was there before, in the DownloadManager::OnAllDataSaved() function. I've moved a lot (close to all) of the processing out of DownloadManager::OnAllDataSaved() and into MaybeCompleteDownload() as part of linearizing the processing of the downloads. I thought about moving download->OnAllDataSaved() back into DownloadManager::OnAllDataSaved() in response to your comment, but download->OnAllDataSaved() changes the DownloadItem state, and if there's one thing that I really want to delay into MaybeCompleteDownload(), it's that. Did that explanation clarify any? If not, let's try to sync up by phone on Tuesday--there's only so much confusion email is good for resolving :-} :-|.
On 2011/01/17 19:56:17, rdsmith wrote: > I thought about moving download->OnAllDataSaved() back into > DownloadManager::OnAllDataSaved() in response to your comment, but > download->OnAllDataSaved() changes the DownloadItem state, and if there's one > thing that I really want to delay into MaybeCompleteDownload(), it's that. I see. I don't really like the OnReadyToFinish name. The state transition seems to be the most important factor, so how about this: - keep DownloadItem::OnAllDataSaved, but move state_ update out of it - call DownloadItem::OnAllDataSaved from DownloadManager::OnAllDataSaved like before - switch DownloadItem's state to COMPLETE in DownloadManager::MaybeCompleteDownload
On 2011/01/18 17:28:15, Paweł Hajdan Jr. wrote: > On 2011/01/17 19:56:17, rdsmith wrote: > > I thought about moving download->OnAllDataSaved() back into > > DownloadManager::OnAllDataSaved() in response to your comment, but > > download->OnAllDataSaved() changes the DownloadItem state, and if there's one > > thing that I really want to delay into MaybeCompleteDownload(), it's that. > > I see. I don't really like the OnReadyToFinish name. The state transition seems > to be the most important factor, so how about this: > > - keep DownloadItem::OnAllDataSaved, but move state_ update out of it > - call DownloadItem::OnAllDataSaved from DownloadManager::OnAllDataSaved like > before > - switch DownloadItem's state to COMPLETE in > DownloadManager::MaybeCompleteDownload This is the right thing to do; thank you for the suggestion. It's a bit tricky to come up with a routine name that makes the distinction between all the data having been received and the state transition that that allows, but I think this is still the correct choice. I needed to take advantage of Andy's active_downloads_ CL to do this, and as long as I was doing that, I took full advantage of that CL to turn pending_finished_downloads_ into a boolean inside the DownloadItem (which, IMO, is where it should be). All this will be in my next patch set (as yet unpublished).
Next round. I hope I've address Pawel's concerns. I also extended some of the changes required for Pawel's concerns to take advantage of Andy's active_downloads_ fix to remove pending_finished_downloads_. Let me know what you think. http://codereview.chromium.org/6096003/diff/24001/chrome/browser/download/dow... File chrome/browser/download/download_item.h (right): http://codereview.chromium.org/6096003/diff/24001/chrome/browser/download/dow... chrome/browser/download/download_item.h:48: // a dangerous download, and renaming the file to the final name and On 2011/01/17 18:13:24, Paweł Hajdan Jr. wrote: > nit: "and..." - is the later part of this comment missing? Done.
http://codereview.chromium.org/6096003/diff/33002/chrome/browser/download/dow... File chrome/browser/download/download_item.cc (right): http://codereview.chromium.org/6096003/diff/33002/chrome/browser/download/dow... chrome/browser/download/download_item.cc:321: void DownloadItem::OnDataReceptionAccepted() { This is another ambiguous name that I'd like to avoid. How about MarkAsComplete? http://codereview.chromium.org/6096003/diff/33002/chrome/browser/download/dow... chrome/browser/download/download_item.cc:328: all_data_received_ = true; nit: Now we have two names for the same thing: "saved" and "received". I'd prefer to stay with Saved. http://codereview.chromium.org/6096003/diff/33002/chrome/browser/download/dow... chrome/browser/download/download_item.cc:352: // Notify our observers that we are complete (the call to OnReadyToFinish() nit: This method name no longer exists. http://codereview.chromium.org/6096003/diff/33002/chrome/browser/download/dow... chrome/browser/download/download_item.cc:406: bool DownloadItem::AllDataReceived() const { nit: Can we add prefix like "Is" or "Has" to unambiguously indicate it's a getter? By the way, I think it should just use hacker_style instead of CamelCase. Then no prefix should be needed.
Next round. http://codereview.chromium.org/6096003/diff/33002/chrome/browser/download/dow... File chrome/browser/download/download_item.cc (right): http://codereview.chromium.org/6096003/diff/33002/chrome/browser/download/dow... chrome/browser/download/download_item.cc:321: void DownloadItem::OnDataReceptionAccepted() { On 2011/01/19 17:16:15, Paweł Hajdan Jr. wrote: > This is another ambiguous name that I'd like to avoid. How about MarkAsComplete? Done. http://codereview.chromium.org/6096003/diff/33002/chrome/browser/download/dow... chrome/browser/download/download_item.cc:328: all_data_received_ = true; On 2011/01/19 17:16:15, Paweł Hajdan Jr. wrote: > nit: Now we have two names for the same thing: "saved" and "received". I'd > prefer to stay with Saved. Done. http://codereview.chromium.org/6096003/diff/33002/chrome/browser/download/dow... chrome/browser/download/download_item.cc:352: // Notify our observers that we are complete (the call to OnReadyToFinish() On 2011/01/19 17:16:15, Paweł Hajdan Jr. wrote: > nit: This method name no longer exists. Fixed, though now that you point it out, I don't completely like that--I'd like to have an invariant of modifying the state and updating observers. In a different CL. http://codereview.chromium.org/6096003/diff/33002/chrome/browser/download/dow... chrome/browser/download/download_item.cc:406: bool DownloadItem::AllDataReceived() const { On 2011/01/19 17:16:15, Paweł Hajdan Jr. wrote: > nit: Can we add prefix like "Is" or "Has" to unambiguously indicate it's a > getter? > > By the way, I think it should just use hacker_style instead of CamelCase. Then > no prefix should be needed. The reason I hadn't done this is that I'm not providing a setter (and have no intention of doing so), so I don't think of it is "just exporting an accessor." But when I read the style guide more carefully, it's pretty solidly on your side. Done.
LGTM, thank you for patience.
LGTM.
I just looked at a high level, LGTM. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
