Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7277073:
Support for adding save page download items into downloads history. (Closed)
|
Created:
9 years, 5 months ago by achuithb Modified:
9 years, 4 months ago CC:
chromium-reviews, rdsmith+dwatch_chromium.org, Paweł Hajdan Jr., ahendrickson Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionSupport for adding save page download items into downloads history.
BUG=4823
TEST=right-click an html page, save it, restart the browser. The item should continue to appear in the downloads history (ctrl-j).
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95466
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 1
Patch Set 7 : '' #Patch Set 8 : '' #Patch Set 9 : '' #
Total comments: 8
Patch Set 10 : '' #
Total comments: 16
Patch Set 11 : '' #Patch Set 12 : '' #Patch Set 13 : '' #Patch Set 14 : '' #Patch Set 15 : '' #Patch Set 16 : '' #Patch Set 17 : '' #Patch Set 18 : '' #Patch Set 19 : '' #Patch Set 20 : '' #Patch Set 21 : '' #Patch Set 22 : '' #
Total comments: 19
Patch Set 23 : '' #Patch Set 24 : '' #Patch Set 25 : '' #Patch Set 26 : '' #Patch Set 27 : '' #Patch Set 28 : '' #Patch Set 29 : '' #Patch Set 30 : '' #
Total comments: 2
Patch Set 31 : '' #
Total comments: 5
Patch Set 32 : '' #Patch Set 33 : '' #
Total comments: 16
Patch Set 34 : '' #
Total comments: 4
Patch Set 35 : '' #Patch Set 36 : '' #Patch Set 37 : '' #Patch Set 38 : '' #
Total comments: 3
Patch Set 39 : '' #
Total comments: 7
Patch Set 40 : '' #Patch Set 41 : '' #
Total comments: 10
Patch Set 42 : '' #Patch Set 43 : '' #Patch Set 44 : '' #Patch Set 45 : '' #
Total comments: 2
Patch Set 46 : '' #
Total comments: 3
Patch Set 47 : '' #Messages
Total messages: 50 (0 generated)
Randy, could you please review at your convenience? I've also added Pawel in case he has time to take a look, but I'm not planning on waiting for him. Thanks, and have a great Jul 4th weekend!
For reference, here is the previous CL by Magnus: http://codereview.chromium.org/7212017/
Actually, it's this one: http://codereview.chromium.org/6312027
So my high level comment is that we need to have more testing, and a bit more specification (which could be the same thing--I don't need a design document). What happens when you click on a Save Page item in the downloads.html page? Does that behavior change if it's been restored from the history? Can you cancel a save page from the download shelf? From the download page? What happens if you do? As much of this as we can manage should be tested from the save_page_browsertest.cc. (It's arguable as to whether Cancel is your problem or not--it doesn't work now from the download shelf. But now that we're extending the semantics to the download page, I'd like to fix it if we can, and if we can't, I want to make sure that trying doesn't do something really ugly.) http://codereview.chromium.org/7277073/diff/3010/chrome/browser/download/save... File chrome/browser/download/save_page_browsertest.cc (right): http://codereview.chromium.org/7277073/diff/3010/chrome/browser/download/save... chrome/browser/download/save_page_browsertest.cc:111: history_urls_.end()); I'm inclined to think that we should have a lot more than this--specifically, that we should define what we think all the history items look like, and check to see that that's accurate.
Inline. On 2011/07/06 20:04:04, rdsmith wrote: > So my high level comment is that we need to have more testing, and a bit more > specification (which could be the same thing--I don't need a design document). > What happens when you click on a Save Page item in the downloads.html page? It takes you to a file:// url with the page references to contents that are also file:// urls. It mostly works, though urls created by javascript don't necessarily work correctly. > Does that behavior change if it's been restored from the history? Nope, still works like you'd expect. Can you > cancel a save page from the download shelf? From the download page? What > happens if you do? The link to Pause/Cancel is available on the shelf and page, but no actual pause/cancel occurs. The download continues to completion. There is a transient state where the UI informs you that the download is paused/canceled like you requested but the reality is that the download is actually merrily continuing on. Eventually the download reaches the finished state. This is bug http://crbug.com/58183, not addressed by this CL. I feel like I should fix that bug in a separate CL. As much of this as we can manage should be tested from the > save_page_browsertest.cc. > (It's arguable as to whether Cancel is your problem or not--it doesn't work now > from the download shelf. But now that we're extending the semantics to the > download page, I'd like to fix it if we can, and if we can't, I want to make > sure that trying doesn't do something really ugly.) We can probably disable the Cancel option for save_package as you suggested. I'd like to do it in a different CL. > http://codereview.chromium.org/7277073/diff/3010/chrome/browser/download/save... > File chrome/browser/download/save_page_browsertest.cc (right): > > http://codereview.chromium.org/7277073/diff/3010/chrome/browser/download/save... > chrome/browser/download/save_page_browsertest.cc:111: history_urls_.end()); > I'm inclined to think that we should have a lot more than this--specifically, > that we should define what we think all the history items look like, and check > to see that that's accurate. The only other items in the history info are * filepath (we can test this) * start_time (not really test-worthy) * received_bytes/total_bytes - this is worth testing, but I suspect it's not correct for save_package. * state of the download.
Thanks for the clarification! A couple of responses: On 2011/07/06 22:54:11, achuith.bhandarkar wrote: > Inline. > > On 2011/07/06 20:04:04, rdsmith wrote: > > So my high level comment is that we need to have more testing, and a bit more > > specification (which could be the same thing--I don't need a design document). > > > What happens when you click on a Save Page item in the downloads.html page? > > It takes you to a file:// url with the page references to contents that are also > file:// urls. It mostly works, though urls created by javascript don't > necessarily work correctly. Huh. So just to confirm I'm understanding this behavior correctly: Clicking on the "file" link on the downloads.html page opens a file:// url? (That's fine behavior, I'm just a little surprised. Though I suppose if it goes through the "open file" pathway, it'll bounce off the OS and hand the URL back to Chrome, and we'll open it that way). > > Does that behavior change if it's been restored from the history? > > Nope, still works like you'd expect. > > Can you > > cancel a save page from the download shelf? From the download page? What > > happens if you do? > > The link to Pause/Cancel is available on the shelf and page, but no actual > pause/cancel occurs. The download continues to completion. There is a transient > state where the UI informs you that the download is paused/canceled like you > requested but the reality is that the download is actually merrily continuing > on. Eventually the download reaches the finished state. This is bug > http://crbug.com/58183, not addressed by this CL. I feel like I should fix that > bug in a separate CL. That's fine. > As much of this as we can manage should be tested from the > > save_page_browsertest.cc. I would like to have some testing of the "open" pathway for save page. It can be as simple as confirming that the string that gets passed to "open" has the correct URL. You might be able to use download_browsertest.cc AutoOpen as a model. (Just let me know if we do already test this--I didn't notice any extra tests in my first pass through.) > > (It's arguable as to whether Cancel is your problem or not--it doesn't work > now > > from the download shelf. But now that we're extending the semantics to the > > download page, I'd like to fix it if we can, and if we can't, I want to make > > sure that trying doesn't do something really ugly.) > > We can probably disable the Cancel option for save_package as you suggested. I'd > like to do it in a different CL. That's fine. You could even convince me that it isn't yours to do, though I'd be grateful if you did it (and it's probably not very hard). http://codereview.chromium.org/7277073/diff/3010/chrome/browser/download/save... > > chrome/browser/download/save_page_browsertest.cc:111: history_urls_.end()); > > I'm inclined to think that we should have a lot more than this--specifically, > > that we should define what we think all the history items look like, and check > > to see that that's accurate. > > The only other items in the history info are > * filepath (we can test this) *nod* Related to the above. > * start_time (not really test-worthy) Agreed. > * received_bytes/total_bytes - this is worth testing, but I suspect it's not > correct for save_package. It's different in a somewhat bogus way for save_package, but it still has a spec (# of bytes == # of files, if I remember right) that can be tested. > * state of the download. This should be tested; I don't have a problem imagining this getting skewed by the parallel pathways.
On 2011/07/07 14:55:47, rdsmith wrote: > Thanks for the clarification! A couple of responses: > > Huh. So just to confirm I'm understanding this behavior correctly: Clicking on > the "file" link on the downloads.html page opens a file:// url? (That's fine > behavior, I'm just a little surprised. Though I suppose if it goes through the > "open file" pathway, it'll bounce off the OS and hand the URL back to Chrome, > and we'll open it that way). I don't know about the 'open file' pathway, but I believe your description is accurate. > I would like to have some testing of the "open" pathway for save page. It can > be as simple as confirming that the string that gets passed to "open" has the > correct URL. You might be able to use download_browsertest.cc AutoOpen as a > model. (Just let me know if we do already test this--I didn't notice any extra > tests in my first pass through.) I lost you here. The filename link is exactly equal to the filepath. I've recently added file:// to it, but a filepath check should be sufficient, right? > That's fine. You could even convince me that it isn't yours to do, though I'd > be grateful if you did it (and it's probably not very hard). I feel like I promised you a fix for Cancel and in-progress behavior of Save package, so I'll start work on another CL for that once this one lands. > > The only other items in the history info are > > * filepath (we can test this) > > *nod* Related to the above. This test should be identical to testing the file link as noted above. > > * start_time (not really test-worthy) > > Agreed. > > > * received_bytes/total_bytes - this is worth testing, but I suspect it's not > > correct for save_package. > > It's different in a somewhat bogus way for save_package, but it still has a spec > (# of bytes == # of files, if I remember right) that can be tested. I didn't know this. This explains why the progress info is so off. Good to know. It turns out that received_bytes is number of files, and total_bytes is always 0. I've added a test for these. > > * state of the download. > > This should be tested; I don't have a problem imagining this getting skewed by > the parallel pathways. Done. I've uploaded another patch, PTAL. Only save_page_browsertest has changed.
On 2011/07/07 19:52:17, achuith.bhandarkar wrote: > On 2011/07/07 14:55:47, rdsmith wrote: > > I would like to have some testing of the "open" pathway for save page. It can > > be as simple as confirming that the string that gets passed to "open" has the > > correct URL. You might be able to use download_browsertest.cc AutoOpen as a > > model. (Just let me know if we do already test this--I didn't notice any > extra > > tests in my first pass through.) > > I lost you here. The filename link is exactly equal to the filepath. I've > recently added file:// to it, but a filepath check should be sufficient, right? I think that I've learned (or remembered) more about how this pathway works since this comment; I believe that left-click on the file link is handled by an explicit onclick handler, and the actual link value won't affect whether the file is opened. I do suspect we're ricocheting off the base OS for this case, but I don't think that matters. At this point I think I'm happy with the testing, but I did a more thorough review of the implementation and have a couple of comments (sending separately).
CCing Andy FHI (I think of him as other than me having had the most experience/understanding of download queues and control flow, and he's eventually going to be taking on SavePackage/Downloads integration, so I want to keep him updated and give him a chance to comment if he wants to). http://codereview.chromium.org/7277073/diff/19001/chrome/browser/download/sav... File chrome/browser/download/save_package.cc (right): http://codereview.chromium.org/7277073/diff/19001/chrome/browser/download/sav... chrome/browser/download/save_package.cc:337: NewCallback(this, &SavePackage::OnDownloadEntryAdded)); I'm concerned that there's a race here if SavePackage::Stop() or SavePackage::Finish() get called before the history system callback. In that case, there won't be a db_handle() on the download, and the history system won't be updated. That might be the correct (or least incorrect) thing to do, but the code should at least have comments documenting what the behavior will be in those cases and why it's acceptable. http://codereview.chromium.org/7277073/diff/19001/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1490: void SavePackage::ManagerGoingDown() { Where is the SavePackage registered as an observer of the download manager? (DownloadManager::AddObserver) And, arguably more importantly given the below, where is the SavePackage *de*registered as an observer of DownloadManager? If the SavePackages complets and is destroyed before the download manager is shutdown (common case, I think) that means any download manager notifications will be indirecting through invalid pointers. http://codereview.chromium.org/7277073/diff/19001/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1491: download_manager_ = NULL; I believe that it isn't possible for the download manager to go away while a save page in in progress. Specifically, for the download manager to go away, the profile owning it has to go away, and I think that requires all tabs associated with the profile to have been killed (which would have resulted in SavePackage::Stop()). Given that the consequences are indirecting through a pointer to garbage memory, I don't mind the belt-and-suspenders of the observering the download manager and setting the internal pointer to null if it does go down, but if you agree with my logic could you put DCHECKs that the download_manager_ is still alive before each use?
http://codereview.chromium.org/7277073/diff/19001/chrome/browser/download/sav... File chrome/browser/download/save_page_browsertest.cc (right): http://codereview.chromium.org/7277073/diff/19001/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:117: info.received_bytes == num_files_ && I think this deserves a comment . . .
PTAL. Only changes are to save_package.*, and one comment as per ahendrickson added to save_page_browsertest.cc. Thanks! http://codereview.chromium.org/7277073/diff/19001/chrome/browser/download/sav... File chrome/browser/download/save_package.cc (right): http://codereview.chromium.org/7277073/diff/19001/chrome/browser/download/sav... chrome/browser/download/save_package.cc:337: NewCallback(this, &SavePackage::OnDownloadEntryAdded)); On 2011/07/08 16:56:25, rdsmith wrote: > I'm concerned that there's a race here if SavePackage::Stop() or > SavePackage::Finish() get called before the history system callback. In that > case, there won't be a db_handle() on the download, and the history system won't > be updated. That might be the correct (or least incorrect) thing to do, but the > code should at least have comments documenting what the behavior will be in > those cases and why it's acceptable. Done. http://codereview.chromium.org/7277073/diff/19001/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1490: void SavePackage::ManagerGoingDown() { I left it out by mistake. I've added the calls to AddObserver/RemoveObserver, and re-done this code a bit. Done. http://codereview.chromium.org/7277073/diff/19001/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1491: download_manager_ = NULL; Makes sense. DCHECKs added. Done. http://codereview.chromium.org/7277073/diff/19001/chrome/browser/download/sav... File chrome/browser/download/save_page_browsertest.cc (right): http://codereview.chromium.org/7277073/diff/19001/chrome/browser/download/sav... chrome/browser/download/save_page_browsertest.cc:117: info.received_bytes == num_files_ && On 2011/07/08 18:47:37, hendrickson_a wrote: > I think this deserves a comment . . . Done.
This review is probably starting to give you the flavor of why I was scared to make the change in the first place :-}. http://codereview.chromium.org/7277073/diff/13005/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7277073/diff/13005/chrome/browser/download/dow... chrome/browser/download/download_manager.h:269: DownloadHistory* download_history() { return download_history_.get(); } I'm inclined to think that this should be const (the method, not the return value). (I'm not ecstatic with making it visible at all, but I couldn't come up with another suggestion as to how to accomplish the same thing. But if you see some clean way to fold the Add/Update entry for download items into download manger, I'd prefer that.) http://codereview.chromium.org/7277073/diff/13005/chrome/browser/download/sav... File chrome/browser/download/save_package.cc (right): http://codereview.chromium.org/7277073/diff/13005/chrome/browser/download/sav... chrome/browser/download/save_package.cc:339: // Add this entry to the history service, which also notifies the UI. I think it's worthwhile making clear that the UI is notified in the callback, not directly by the history service. http://codereview.chromium.org/7277073/diff/13005/chrome/browser/download/sav... chrome/browser/download/save_package.cc:341: NewCallback(this, &SavePackage::OnDownloadEntryAdded)); I'm not an expert on this, but I think that NewCallback doesn't take a reference on the object passed, which means that this call has no protection against the callback arriving at a freed object. (The way this works in DownloadManager is the download_history_'s lifetime is tied to DownloadManager, so the callback chain gets blocked at download_history_ if DownloadManager's gone away.) Could you confirm that you're safe against the callback being delayed past object destruction? http://codereview.chromium.org/7277073/diff/13005/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1499: // In that case, this item would not get added to the history db. Nope, it's been added to the DB, it just won't be added to the now-non-existent download manager. No harm, no foul. http://codereview.chromium.org/7277073/diff/13005/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1500: DCHECK(download_manager_); This is the one case in which I think download_manager_ can indeed be null in normal operation (because callbacks may be arbitrarily delayed). So no DCHECK() here. http://codereview.chromium.org/7277073/diff/13005/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1508: download_manager_->download_history()->UpdateEntry(download_); So there's a pretty subtle but important difference between the flow in the DownloadManager vs. the flow in SavePackage with this CL. In the DownloadManager, we delay just about anything happening until the history callback comes back--the download item can be cancelled (new CL going in to do this properly which you just made ChromeOS review comments on; you may want to examine the history interactions in download_manager.cc in http://codereview.chromium.org/7294013/), but we don't let it progress in any meaningful way (bytes downloaded and dumped into a file, but that's it) until the history db gets back to us. So the main downloads code allows a race between history callback and cancel (either one can complete first) but forces completion to be strictly after history callback. This CL, if I'm reading it right, allows races between history callback and both cancellation and completion. And I don't see any special code to handle either race. In the cancellation race case I think we end up with a started download in the history that's never marked as completing/canceling, which isn't horrible--there are corner cases that do that for the main code. In the completion case we'll end up with a completed download that isn't completed in the history, and will later show up as cancelled--I don't think that's ok. I'm afraid I'd like to request that you mimic the main download code, and hold off completion until you get the history callback--de-racing that code was, to my mind, the key thing that we did to deflake the tests, and I don't want to create new, racy code here. And putting in explicit comments about what occurs and why it's ok when the history/cancel race happens in the wrong order (cancel occurs before history callback) would also be very good. http://codereview.chromium.org/7277073/diff/13005/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1508: download_manager_->download_history()->UpdateEntry(download_); If you could also take a look at how hard it would be to put in tests which explore each of these races (history versus cancel, history versus completion) I'd be grateful. There may not be good ways--the way I eventually want to do it for the main download system is by mocking the history system and using a test mock object that delays the callback until explicitly triggered. But there may be ways to make it likely that one or another event happens first based on the current state of the system. http://codereview.chromium.org/7277073/diff/13005/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1510: download_manager_ = NULL; I'd like a CHECK() in the destructor that the download_manager_ is null at that point; this CL has tied observing/non-observing state to download_manager_ not-null/null, and we want to be really really sure that we don't stay observing the download manager after destruction. I'm requesting a CHECK() rather that a DCHECK, because if we are observing the download manager after destruction, that's a very dangerous security issue (function pointer indirection through freed memory).
Some of the changes from patchset 10: 1. I've moved the history AddEntry code into download_manager since save_package may be deleted before the callback arrives. This change also makes it not necessary to expose the history_service() except for the browser test. 2. The save page download items are stored in a save_page_downloads_ map. The history service callback returns with an id, so we need to look up the DownloadItem by id before we can proceed. Ids are generated by the download_manager_ in this case, and need only be unique for the save_page_downloads_ map. PTAL http://codereview.chromium.org/7277073/diff/13005/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7277073/diff/13005/chrome/browser/download/dow... chrome/browser/download/download_manager.h:269: DownloadHistory* download_history() { return download_history_.get(); } On 2011/07/10 19:37:40, rdsmith wrote: > I'm inclined to think that this should be const (the method, not the return > value). > > (I'm not ecstatic with making it visible at all, but I couldn't come up with > another suggestion as to how to accomplish the same thing. But if you see some > clean way to fold the Add/Update entry for download items into download manger, > I'd prefer that.) I made it const (along with some other accessors), and it's only visible for the browser test now. http://codereview.chromium.org/7277073/diff/13005/chrome/browser/download/sav... File chrome/browser/download/save_package.cc (right): http://codereview.chromium.org/7277073/diff/13005/chrome/browser/download/sav... chrome/browser/download/save_package.cc:339: // Add this entry to the history service, which also notifies the UI. On 2011/07/10 19:37:40, rdsmith wrote: > I think it's worthwhile making clear that the UI is notified in the callback, > not directly by the history service. Done. http://codereview.chromium.org/7277073/diff/13005/chrome/browser/download/sav... chrome/browser/download/save_package.cc:341: NewCallback(this, &SavePackage::OnDownloadEntryAdded)); I decided to move this code into download_manager. The other option was to AddRef() here and Release() in the callback to ensure that the save_package doesn't go away. But I think having this code in the download_manager is more natural, and also makes it unnecessary to expose the history_service (except for testing). http://codereview.chromium.org/7277073/diff/13005/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1499: // In that case, this item would not get added to the history db. This code is now in DownloadManager. http://codereview.chromium.org/7277073/diff/13005/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1500: DCHECK(download_manager_); This code is now in DownloadManager. http://codereview.chromium.org/7277073/diff/13005/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1508: download_manager_->download_history()->UpdateEntry(download_); I'm now handling the race between completion and history callback. Cancellation doesn't work at all for save_package, but I believe the code is correct for that case as well. http://codereview.chromium.org/7277073/diff/13005/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1508: download_manager_->download_history()->UpdateEntry(download_); I think adding a test for the races should be doable. I'm going to take a look. I'm concerned that the test itself may end up being tricky, racy and/or flaky so I'd like to do it separate from this CL. http://codereview.chromium.org/7277073/diff/13005/chrome/browser/download/sav... chrome/browser/download/save_package.cc:1510: download_manager_ = NULL; On 2011/07/10 19:37:40, rdsmith wrote: > I'd like a CHECK() in the destructor that the download_manager_ is null at that > point; this CL has tied observing/non-observing state to download_manager_ > not-null/null, and we want to be really really sure that we don't stay observing > the download manager after destruction. I'm requesting a CHECK() rather that a > DCHECK, because if we are observing the download manager after destruction, > that's a very dangerous security issue (function pointer indirection through > freed memory). Done.
[Adding Ben to CC FHI and in case he wants to comment re: the new id added to DownloadManager. ] Getting there. I'm uncomfortable about the one race left in the code, but there's probably not a way around it without noticeably slowing down save page. And I believe it's basically correct. http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:126: save_page_downloads_.clear(); If I'm reading the protocol right, save_page_downloads_ should always be empty at this point (for the download maanger to be shutdown, its profile must be shutting down, which requires all tabs associated with the profile to be killed, which will have Stop()'d all SavePackage instances.) http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:1382: DCHECK(it != save_page_downloads_.end()); I believe this DCHECK is inaccurate--specifically, if there's a race between the TabContents closing (and hence SavePackage::Stop() being called) and the history callback, the history callback could show up with nothing in save_page_downloads_. If you agree, could you copy the pattern in http://codereview.chromium.org/7294013/ to turn around and yank the new entry back out of the history system? If you want to leave it in the history system and mark it as cancelled, that's ok too; I think it's even ok to leave the code as it is, as long as you document that you're relying on it showing up as cancelled when it's read back in from the history system. http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:1393: // The download may have completed before we received the DB handle. Not requesting a change, just registering a disquiet here. We put some effort into removing (by serialization) all races not absolutely required in the download system, and this is putting a new one in. I think the code is correct in that nothing particularly ugly will happen. There is a shutdown race (save page completes->Shutdown->history callback completes) that'll result in the save looking like it's been cancelled in the history when it's actually finished, but I don't think that's too big a deal. And this basically parallels the one race we've left in the download code (versus downloading the actual data). So I think I'm ok with having this in here until we unify the packages. Just a bit ootsy about it. http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... chrome/browser/download/download_manager.h:205: DownloadHistory* download_history() const { return download_history_.get(); } If this is now only used by the browsertest, can you make it private and put in a FRIEND_TEST or friend class? http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... chrome/browser/download/download_manager.h:410: DownloadMap save_page_downloads_; There's no point to having both this and save_page_as_downloads_; could you remove the latter and substitue this where it was used?
Another round of patches. Please take a look, rdsmith. http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:126: save_page_downloads_.clear(); Didn't know that. I've deleted the line. Do you think I ought to add a DCHECK here? This does brings up two points. Firstly, if we're guaranteed that the DownloadManager does not initiate the Shutdown sequence until all SavePackage instances are gone, there's really no point in having SavePackage listen for ManagerGoingDown(). Don't you agree? SavePackage should not be a DownloadManager::Observer if it never receives the ManagerGoingDown callback. The other point is that in AssertContainersConsistent, I should just assert that save_page_downloads_ is empty and not include it in the set union consistency checks. Do you agree? http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:1382: DCHECK(it != save_page_downloads_.end()); The item remains in save_page_downloads_ until both the history callback and the save_package Finalize have been called. The DCHECK should hold, unless I'm mistaken. The item should not show up as canceled - it should be properly finished, no matter how the race goes. http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:1393: // The download may have completed before we received the DB handle. I've been thinking about this, and no elegant solution presents itself. To get to serialization, I have to AddRef the SavePackage to make sure it doesn't go away, then in the history callback, I'd have to lookup a map of download ids to SavePackage pointers to call back into SavePackage to allow it to proceed. I need another data structure of pointers and some tricky lifetime management that I'd rather avoid in this CL. These problems seem stem from the fact that SavePackage is not more tightly integrated into the downloads system. I understand and acknowledge your disquiet. If you can suggest a simple way to fix this race, I'd be happy to implement it. http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... chrome/browser/download/download_manager.h:205: DownloadHistory* download_history() const { return download_history_.get(); } On 2011/07/15 17:58:39, rdsmith wrote: > If this is now only used by the browsertest, can you make it private and put in > a FRIEND_TEST or friend class? Done. http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... chrome/browser/download/download_manager.h:410: DownloadMap save_page_downloads_; On 2011/07/15 17:58:39, rdsmith wrote: > There's no point to having both this and save_page_as_downloads_; could you > remove the latter and substitue this where it was used? Done.
Next round. One thing the below comments bring up: Have you traced out pathways/trying testing the "Remove From List" option on the downloads page? Could you put in a test that tries to remove an in progress save page operation? I know that cancel won't work, but we should confirm that neither cancel nor remove crashes or does other nasty stuff, and though I haven't traced the code, the discussion below makes me worried that the remove operation will lead to executing methods on deleted objects. http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:126: save_page_downloads_.clear(); On 2011/07/16 20:15:32, achuith.bhandarkar wrote: > Didn't know that. I've deleted the line. Do you think I ought to add a DCHECK > here? Yes, if you would. > This does brings up two points. Firstly, if we're guaranteed that the > DownloadManager does not initiate the Shutdown sequence until all SavePackage > instances are gone, there's really no point in having SavePackage listen for > ManagerGoingDown(). Don't you agree? SavePackage should not be a > DownloadManager::Observer if it never receives the ManagerGoingDown callback. Hmmm. Fair enough. I'd like some sort of check for that, though; if we're wrong, we'll end up indirecting through a freed object. I'm not sure how best to do this--maybe leave in the manager doing down notification and called NOTREACHED() there with a note? > The other point is that in AssertContainersConsistent, I should just assert that > save_page_downloads_ is empty and not include it in the set union consistency > checks. Do you agree? Nope--AssertContainersConsistent is intended to be callable at any time about any download item. So it might be called when there is a save page in progress. http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:1382: DCHECK(it != save_page_downloads_.end()); On 2011/07/16 20:15:32, achuith.bhandarkar wrote: > The item remains in save_page_downloads_ until both the history callback and the > save_package Finalize have been called. The DCHECK should hold, unless I'm > mistaken. The item should not show up as canceled - it should be properly > finished, no matter how the race goes. Whoops, I'm sorry; I hadn't tracked that part of the protocol on the last review. I'd really rather not do it that way--the invariant for the rest of the download manager is that, on cancel, the download is tossed out of the pool (the active_downloads_, in_progress_ queues) and that after shutdown, there are no download items associated with the download manager anymore (or at least, that'll be the invariant when I get my CL committed, hopefully within the next day or two). We need to do it that way for downloads because we may have removed the download as well as cancelled it, which will result in the DownloadItem being deleted, and we don't want to hold onto any pointers to a deleted object. Would you be willing to shift this protocol to match the above? I.e. on a cancel, the download item is removed from save_page_downloads_, and the history callback does the (a) right thing when it doesn't find the entry in save_page_downloads_? Note that my other comment about save_page_downloads_ being null on Shutdown() relies on this pattern (== my misunderstanding of your original code); it's perfectly possible for the history service callback to come back after shutdown. http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:1393: // The download may have completed before we received the DB handle. On 2011/07/16 20:15:32, achuith.bhandarkar wrote: > I've been thinking about this, and no elegant solution presents itself. To get > to serialization, I have to AddRef the SavePackage to make sure it doesn't go > away, then in the history callback, I'd have to lookup a map of download ids to > SavePackage pointers to call back into SavePackage to allow it to proceed. I > need another data structure of pointers and some tricky lifetime management that > I'd rather avoid in this CL. These problems seem stem from the fact that > SavePackage is not more tightly integrated into the downloads system. > > I understand and acknowledge your disquiet. If you can suggest a simple way to > fix this race, I'd be happy to implement it. Nope--else I would have. I came to the same conclusion as you--better integration would solve this problem, and short of that there's not a simple solution.
http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:1406: int32 DownloadManager::GetNextSavePageId() { Apologies if this was addressed elsewhere, but how are the semantics of this counter different from DownloadFileManager::GetNextId()? I'm making some changes to DFM::GNI() and hope to someday merge all of DownloadManager's maps into one map, which means that save_page ids should be in the same space as normal download ids. Perhaps my new next id counter will be compatible with this one? Not asking for any changes, I'm happy to figure out how to merge the counters later. I'm just trying to understand how big that job will be.
http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:1406: int32 DownloadManager::GetNextSavePageId() { Not addressed elsewhere. There's a GetNextId in both DownloadFileManager and SaveFileManager. However both are created and used on the file thread, which is not appropriate for me. I need ids on the UI thread... I expect you'll still need ids on different threads, so you will need at least 2 id counters. To store them in the same map, maybe you can just offset one of the starting ids by some large number, or add a non-numeral prefix?
http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:1406: int32 DownloadManager::GetNextSavePageId() { On 2011/07/18 18:33:18, achuith.bhandarkar wrote: > Not addressed elsewhere. > > There's a GetNextId in both DownloadFileManager and SaveFileManager. However > both are created and used on the file thread, which is not appropriate for me. I > need ids on the UI thread... > > I expect you'll still need ids on different threads, so you will need at least 2 > id counters. To store them in the same map, maybe you can just offset one of the > starting ids by some large number, or add a non-numeral prefix? If the multi-thread accessibility is the only issue, then it won't be any trouble at all. If you're curious, see the Description here for the current plan http://codereview.chromium.org/7237034/
http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:1406: int32 DownloadManager::GetNextSavePageId() { From reading the CL description, I feel like the integration of this counter into your scheme should be straightforward. The only reason I introduced a new one is because none existed on the UI thread and I was too lazy to make one of the existing ones thread-safe.
I hadn't traced the RemoveDownload pathways, but you are right. We do end up accessing download_ after DownloadManager has deleted it. This appears to be an existing bug in SavePackage. To fix this, I now use ModelChanged to monitor the deletion of DownloadItem, and I test download_ before I use it everywhere. I added a test for Remove, but it doesn't test the race we're interested in. We would need to Remove the item after the history callback but before the save finishes. I tried to do this but quickly discovered that it's really hard. PTAL. http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:126: save_page_downloads_.clear(); On 2011/07/17 18:25:47, rdsmith wrote: > On 2011/07/16 20:15:32, achuith.bhandarkar wrote: > > Didn't know that. I've deleted the line. Do you think I ought to add a DCHECK > > here? > > Yes, if you would. > > > This does brings up two points. Firstly, if we're guaranteed that the > > DownloadManager does not initiate the Shutdown sequence until all SavePackage > > instances are gone, there's really no point in having SavePackage listen for > > ManagerGoingDown(). Don't you agree? SavePackage should not be a > > DownloadManager::Observer if it never receives the ManagerGoingDown callback. > > Hmmm. Fair enough. I'd like some sort of check for that, though; if we're > wrong, we'll end up indirecting through a freed object. I'm not sure how best > to do this--maybe leave in the manager doing down notification and called > NOTREACHED() there with a note? > > > The other point is that in AssertContainersConsistent, I should just assert > that > > save_page_downloads_ is empty and not include it in the set union consistency > > checks. Do you agree? > > Nope--AssertContainersConsistent is intended to be callable at any time about > any download item. So it might be called when there is a save page in progress. Done. http://codereview.chromium.org/7277073/diff/22020/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:1382: DCHECK(it != save_page_downloads_.end()); Ok, I've done what you suggest. I was following the existing pattern from OnCreateDownloadEntryComplete. I see that in http://codereview.chromium.org/7294013/, you are not allowing this race anymore, and that you are no longer completing the download in OnCreateDownloadEntryComplete. The fact that the download remains in progress forever when the history callback comes late is rather unfortunate. Are you sure this is the way you want it?
Wow, this is the downside to the incremental deduction approach to race elimination. On the bright side, we're doing it in a CL rather than in the tree :-J. Copying the pattern in the download system would work if we could reproduce it completely, but we aren't; specifically, the download system holds off download completion until the history callback occurs, and we've so far elected not to take that on. Unfortunately, I think that's incompatible with allowing cancel, remove, and completion to completely remove the item from queues (cancel from the active queue, remove from all queues) specifically because until we get the history callback we can't put it in history_downloads_, which means if the download completes before the history callback occurs and we remove it from the save_page queue, the history callback doesn't know if it's completed or cancelled. Can we do this?: * Cancel/remove continues to get everything out of all the queues. * The save page completes separately from the download item. Specifically, we don't mark the download item as completed until the history callback comes back. That means that if the history callback comes back and the download item isn't there, the history callback can assume cancel/remove and pull it back out of the history system. But we can complete the download if that hasn't been done. This does allow the user to do a "remove" in the downloads.html page, and if they manage that before the history callback comes back, it won't be in the history, but that's not very high on my concern list (at least based on my current thoughts as to consequences; if you think it's a problem, please raise the flag). WRT the "remove" case being an existing vulnerability within the SavePackage system: Yes, definitely at the code level. I think practically it wasn't an issue because until the download item was visible in the downloads.html page the user didn't have a code pathway leading to the Remove() function. But I'm not 100% certain on that. I'm sorry about the complexity of all this. Thank you for bearing with me while we work through the race issues. http://codereview.chromium.org/7277073/diff/57009/content/browser/download/sa... File content/browser/download/save_package.cc (right): http://codereview.chromium.org/7277073/diff/57009/content/browser/download/sa... content/browser/download/save_package.cc:1369: && download_manager_->GetDownloadItem(download_->id()) == NULL) This is an ok way to do this, but the standard pattern is to observer the DownloadItem, and when you get a notification when the item is in the REMOVING state, to stop observation at that point.
To be clear, this is all in reaction to your point about the download item not completing if the history callback shows up after completion?" to which my answer is, not surprisingly, "no, that's not good." On 2011/07/20 19:48:43, rdsmith wrote: > Wow, this is the downside to the incremental deduction approach to race > elimination. On the bright side, we're doing it in a CL rather than in the tree > :-J. > > Copying the pattern in the download system would work if we could reproduce it > completely, but we aren't; specifically, the download system holds off download > completion until the history callback occurs, and we've so far elected not to > take that on. Unfortunately, I think that's incompatible with allowing cancel, > remove, and completion to completely remove the item from queues (cancel from > the active queue, remove from all queues) specifically because until we get the > history callback we can't put it in history_downloads_, which means if the > download completes before the history callback occurs and we remove it from the > save_page queue, the history callback doesn't know if it's completed or > cancelled. > > Can we do this?: > * Cancel/remove continues to get everything out of all the queues. > * The save page completes separately from the download item. Specifically, we > don't mark the download item as completed until the history callback comes back. > That means that if the history callback comes back and the download item isn't > there, the history callback can assume cancel/remove and pull it back out of the > history system. But we can complete the download if that hasn't been done. > This does allow the user to do a "remove" in the downloads.html page, and if > they manage that before the history callback comes back, it won't be in the > history, but that's not very high on my concern list (at least based on my > current thoughts as to consequences; if you think it's a problem, please raise > the flag). > > WRT the "remove" case being an existing vulnerability within the SavePackage > system: Yes, definitely at the code level. I think practically it wasn't an > issue because until the download item was visible in the downloads.html page the > user didn't have a code pathway leading to the Remove() function. But I'm not > 100% certain on that. > > I'm sorry about the complexity of all this. Thank you for bearing with me while > we work through the race issues. > > http://codereview.chromium.org/7277073/diff/57009/content/browser/download/sa... > File content/browser/download/save_package.cc (right): > > http://codereview.chromium.org/7277073/diff/57009/content/browser/download/sa... > content/browser/download/save_package.cc:1369: && > download_manager_->GetDownloadItem(download_->id()) == NULL) > This is an ok way to do this, but the standard pattern is to observer the > DownloadItem, and when you get a notification when the item is in the REMOVING > state, to stop observation at that point.
Sorry Randy, I didn't completely follow what you wanted me to do, but I think this should be right. I've put in the code back that terminates the download incase the history callback comes late. There isn't a way for Remove to occur before the history callback, since the history callback must occur before the item shows up in downloads.html. in_progress and active_downloads_ are not involved in save_package, so there's no book-keeping necessary there. Cancel and completion are equivalent for save_package, so no special handling is necessary. save_page_downloads_ holds on to the item until the later of download completion or history callback. The item reaches the correct terminal state regardless of the order of history-callback/download-complete/remove-from-list. PTAL. http://codereview.chromium.org/7277073/diff/57009/content/browser/download/sa... File content/browser/download/save_package.cc (right): http://codereview.chromium.org/7277073/diff/57009/content/browser/download/sa... content/browser/download/save_package.cc:1369: && download_manager_->GetDownloadItem(download_->id()) == NULL) That is much better. Thanks. Done.
Given that we can't make this look just like the downloads system flow until we unify the two, I'll stop trying to make it close to the downloads system flow, as close doesn't really get you anything in avoiding races. Given that, it struck me as necessary to try and do a full exploration of the race space (below--avoiding this was why I was trying so hard to make this look like the current downloads structure), and based on that analysis I think I'm basically happy with the current CL. But I would like you to add a comment (either in download_manager.h near the interface with the save package functionality, or in save_package.cc near the top of the file) explaining the interface with download manager and the various possible races and how they're handled. I've taken an AI to do this same thing for the main downloads flow--I've should have done this earlier, but it is simpler (because of the serialization and the fact that Remove & Shutdown do cancel first, so if cancel really gets everything out of all the queues and thus is the last thing other than remove that can happen, the space is simpler). The AIs that came out of the full race evaluation are also comments in the code. Race analysis: + The actions that can race against each other are: * History callback * Completion (save page as finishes) * Cancel (SavePackage::Stop()) * Shutdown * Remove + Cancel and completion can be considered equivalent for race processing, as they have the same effect on the download manager, and SavePackage won't produce any more events after either of them. I'll refer to the OR of these two events as "Finishing". + Anything that occurs after both History callback and Finishing is uninteresting, as there won't be anything in the queues anymore. + Only the History Callback or Remove can occur after Shutdown (the SavePackage system won't produce any more events after these two). + Remove does not cancel (probably not a mistake you'd make, but it does on downloads, so I'm noting it for myself). + Shutdown can only occur after Finishing(Cancel) (destroying a profile happens strictly after destroying all TabContents associated with the profile). + Remove can only occur after the History Callback. This leads to the following possible orders of events: -- Finishing, History Callback Variations on the basic one. Ok. -- Finishing(Cancel), Shutdown, History Callback The cancel leaves the DI in the queue, shutdown removes and deletes it, and the history callback doesn't find it, leaving it in the DB as IN_PROGRESS, which will be turned into Cancelled on next load. Ok. -- History Callback, Finishing The basic order. Ok. -- History Callback, Remove, Finishing Remove doesn't remove the DI from the save_page_downloads_ list, which results in a double deletion on shutdown. Easily fixed. This does result in a lack of update of the database to indicate that the download has completed (if the finishing action is completed), but that's not solvable until we cancel on Remove (which would update the DB). I would still feel more comfortable if the Cancel operation removed the download from the save_page_downloads_ list (which is what I was suggesting in my last round of comments) but I did the analysis for that case, and it was a bit more complex (five events instead of four) and didn't have any obvious benefits other than being more like the download system flow and allowing Remove() to occur before the history callback (which is a programmatic ability I'd like and will use in the downloads system, but which we don't need here). But with some discomfort, I'll put that off until we unify the two systems. http://codereview.chromium.org/7277073/diff/66001/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/7277073/diff/66001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:915: DCHECK_EQ(1, downloads_count); We need to remove the download from save_page_downloads_ here as well. http://codereview.chromium.org/7277073/diff/66001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:1376: // SavePackage into DownloadManager. This comment is no longer correct and should be updated.
Randy, I put your race analysis in the comments. It is quite verbose, but it also covers all the scenarios so it's probably appropriate. What do you think? I've also factored out the DownloadItem removal code into its own function. This function also removes the item from save_page_downloads_. PTAL. Thanks! http://codereview.chromium.org/7277073/diff/66001/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/7277073/diff/66001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:915: DCHECK_EQ(1, downloads_count); I took the liberty of pulling out all the code that removes a DownloadItem from the internal maps/sets into its own function. Let me know what you think. http://codereview.chromium.org/7277073/diff/66001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:1376: // SavePackage into DownloadManager. I updated the comment. This can now only happen if shutdown happened before the history callback.
http://codereview.chromium.org/7277073/diff/59011/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/7277073/diff/59011/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:919: const int num_deleted = static_cast<int>(pending_deletes.size()); nit: "const int" inside a method is generally an overkill. http://codereview.chromium.org/7277073/diff/59011/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:1358: DCHECK(download); nit: Better use the idiom below: if (!download) { NOTREACHED(); return; } http://codereview.chromium.org/7277073/diff/59011/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7277073/diff/59011/chrome/browser/download/dow... chrome/browser/download/download_manager.h:201: Profile* profile() const { return profile_; } Why? It is risky (in terms of hard to misuse API) to make a const method return a non-const pointer. Same for below. http://codereview.chromium.org/7277073/diff/59011/chrome/browser/download/dow... chrome/browser/download/download_manager.h:365: typedef std::vector<DownloadItem*> DownloadVec; nit: Please avoid abbreviations like Vec. Use Vector, the full name. Also, don't we already have some typedef for this? Just checking. Or maybe we could just use DownloadSet for simplicity? http://codereview.chromium.org/7277073/diff/59011/content/browser/download/sa... File content/browser/download/save_package.cc (right): http://codereview.chromium.org/7277073/diff/59011/content/browser/download/sa... content/browser/download/save_package.cc:56: Race analysis: I'm not sure about adding this as a comment in the code, it is likely to get out of sync with reality.
http://codereview.chromium.org/7277073/diff/66001/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/7277073/diff/66001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:915: DCHECK_EQ(1, downloads_count); On 2011/07/27 01:04:57, achuith.bhandarkar wrote: > I took the liberty of pulling out all the code that removes a DownloadItem from > the internal maps/sets into its own function. Let me know what you think. I think this was a good idea that probably proactively fixed some bugs--thank you. http://codereview.chromium.org/7277073/diff/59011/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7277073/diff/59011/chrome/browser/download/dow... chrome/browser/download/download_manager.h:201: Profile* profile() const { return profile_; } On 2011/07/27 16:53:51, Paweł Hajdan Jr. wrote: > Why? It is risky (in terms of hard to misuse API) to make a const method return > a non-const pointer. Same for below. This was my fault; an earlier patchset was exposing download_history() and I was struggling for some way to limit it's usage so I suggested making it const. I'm good with pulling the consts back off these methods. http://codereview.chromium.org/7277073/diff/59011/chrome/browser/download/dow... chrome/browser/download/download_manager.h:308: DownloadHistory* download_history() const { return download_history_.get(); } I don't think this is used anymore; can we remove it? http://codereview.chromium.org/7277073/diff/59011/chrome/browser/download/dow... chrome/browser/download/download_manager.h:365: typedef std::vector<DownloadItem*> DownloadVec; On 2011/07/27 16:53:51, Paweł Hajdan Jr. wrote: > nit: Please avoid abbreviations like Vec. Use Vector, the full name. Also, don't > we already have some typedef for this? Just checking. I don't think we have a typedef for this--before this PS, there were several methods in DM that took an explicit std::vector<DownloadItem*>. I wouldn't mind having a public typedef for this, but I agree with Pawel; it shouldn't be abbreviated. > Or maybe we could just use DownloadSet for simplicity? I'd rather not do that; DownloadSet is a private typedef having to do with the internal implementation of the DownloadManager, and I don't want to expose it. I also think that vectors are more comfortable to programmers than sets, and vectors are already in the DM interface (see, e.g., GetAllDownloads). http://codereview.chromium.org/7277073/diff/59011/content/browser/download/sa... File content/browser/download/save_package.cc (right): http://codereview.chromium.org/7277073/diff/59011/content/browser/download/sa... content/browser/download/save_package.cc:56: Race analysis: On 2011/07/27 16:53:51, Paweł Hajdan Jr. wrote: > I'm not sure about adding this as a comment in the code, it is likely to get out > of sync with reality. Yeah. Another take on this is that if we need this kind of analysis in order to reassure yourself that there aren't races in our code, we're writing fragile code. I'm just not seeing a better way to handle this issue short of integrating with the download system :-{. Achuith: I think you can summarize my analysis below in substantially fewer words. Something like: * The history callback will call OnSavePageDownloadEntryAdded and either completion or canceling of the save page will call SavePageDownloadFinished()--those two functions are written to correctly manage the download item whichever order they occur in. * If something removes the download item from the download manager (Remove, Shutdown) the result will be that the SavePage system will not be able to properly update the download item (which no longer exists) or the download history, but the action will complete properly anyway. This may lead to the history entry being wrong on a reload of chrome (specifically in the case of Initiation -> History Callback -> Removal -> Completion), but there's no way to solve that without canceling on Remove (which would then update the DB). ------------ I'm also realizing looking at this writeup, that it's really out of context here. It's really about the events sent to the DownloadManager by the SavePackage system and the history callback. As such, the comment should be near those functions in DownloadManager. Could you also move it there? Pawel, what do you think of that set of modifications? http://codereview.chromium.org/7277073/diff/77001/chrome/browser/automation/a... File chrome/browser/automation/automation_provider_observers.h (right): http://codereview.chromium.org/7277073/diff/77001/chrome/browser/automation/a... chrome/browser/automation/automation_provider_observers.h:1208: SavePackageNotificationObserver(DownloadManager* download_manager, Why are you making this change? I don't see the motivation. http://codereview.chromium.org/7277073/diff/77001/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/7277073/diff/77001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:1377: content::NOTIFICATION_SAVE_PACKAGE_SUCCESSFULLY_FINISHED, I'm not sure what's your change and what's the stuff you've merged in, so I'll ask the high level question: Are you sure the notification behavior used to be to notify ("successfully finished") on a save page cancel?
First of all - Pawel, welcome back! Hope your internship is going well. I've addressed most of Pawel's nits. I've abbreviated the comments about the races. I've had to adjust the timing of NOTIFICATION_SAVE_PACKAGE_SUCCESSFULLY_FINISHED. SavePackage may actually no longer exist when this notification is sent out, so the DownloadManager is now the source, and the details are the DownloadItem that just finished. PTAL. http://codereview.chromium.org/7277073/diff/59011/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/7277073/diff/59011/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:919: const int num_deleted = static_cast<int>(pending_deletes.size()); Do you feel strongly about this? I like const auto vars as it makes the code easier to read (you can focus on the variables that change). http://codereview.chromium.org/7277073/diff/59011/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:1358: DCHECK(download); On 2011/07/27 16:53:51, Paweł Hajdan Jr. wrote: > nit: Better use the idiom below: > > if (!download) { > NOTREACHED(); > return; > } Done. http://codereview.chromium.org/7277073/diff/59011/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7277073/diff/59011/chrome/browser/download/dow... chrome/browser/download/download_manager.h:201: Profile* profile() const { return profile_; } If you are saying that the const does nothing of value, I agree with that - it is somewhat misleading since the pointer is not const. Anyway, I've removed the consts. http://codereview.chromium.org/7277073/diff/59011/chrome/browser/download/dow... chrome/browser/download/download_manager.h:308: DownloadHistory* download_history() const { return download_history_.get(); } It is used in SavePageBrowserTest::QueryDownloadHistory(). I can make this method non-const (doesn't really matter). I can also directly access download_history_ since SavePageBrowserTest is a friend, but I think the accessor is nicer. http://codereview.chromium.org/7277073/diff/59011/chrome/browser/download/dow... chrome/browser/download/download_manager.h:365: typedef std::vector<DownloadItem*> DownloadVec; I created a public typedef for DownloadVector and used it in the other methods that return a vector of DownloadItem*. This change has nothing to do with this CL, so I can revert it if you don't like it. For RemoveDownloadItem, I went with using DownloadSet, since the order doesn't really matter. http://codereview.chromium.org/7277073/diff/59011/content/browser/download/sa... File content/browser/download/save_package.cc (right): http://codereview.chromium.org/7277073/diff/59011/content/browser/download/sa... content/browser/download/save_package.cc:56: Race analysis: I've moved the comments to the end of download_manager.cc where the SavePackage methods lie, and abbreviated them. PTAL. I agree that these comments about the races are necessary since these are treacherous waters and the code looks deceptively simple. http://codereview.chromium.org/7277073/diff/77001/chrome/browser/automation/a... File chrome/browser/automation/automation_provider_observers.h (right): http://codereview.chromium.org/7277073/diff/77001/chrome/browser/automation/a... chrome/browser/automation/automation_provider_observers.h:1208: SavePackageNotificationObserver(DownloadManager* download_manager, Explained elsewhere http://codereview.chromium.org/7277073/diff/77001/chrome/browser/download/dow... File chrome/browser/download/download_manager.cc (right): http://codereview.chromium.org/7277073/diff/77001/chrome/browser/download/dow... chrome/browser/download/download_manager.cc:1377: content::NOTIFICATION_SAVE_PACKAGE_SUCCESSFULLY_FINISHED, NOTIFICATION_SAVE_PACKAGE_SUCCESSFULLY_FINISHED is currently only used in testing (in testing_automation_provider and save_page_browsertest). The tests wait for this success notification before proceeding with their checks. In the case where the history callback occurs after the download finishes, this notification is sent too early and various assertions/checks fail (both CheckDownloadUI and CheckDownloadHistory fail). This change makes it so this notification is sent after the history item is updated. I made another change to ensure that this notification is only sent on status COMPLETE and not on CANCELLED, to address your point. PTAL.
LGTM--I'd like the requested mod below, but I'm not set on it. Pawel, WDYT? http://codereview.chromium.org/7277073/diff/59011/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7277073/diff/59011/chrome/browser/download/dow... chrome/browser/download/download_manager.h:308: DownloadHistory* download_history() const { return download_history_.get(); } On 2011/07/28 00:45:58, achuith.bhandarkar wrote: > It is used in SavePageBrowserTest::QueryDownloadHistory(). I can make this > method non-const (doesn't really matter). I can also directly access > download_history_ since SavePageBrowserTest is a friend, but I think the > accessor is nicer. Nope, that makes sense. I don't care a lot about private accessors, and this balance is right anyway from my POV--it's not used by code inside of DownloadManager, so it shouldn't use the member variables, but it's from a test, so a friend test + private restricts access. Thanks for the explanation. http://codereview.chromium.org/7277073/diff/83003/chrome/browser/download/dow... chrome/browser/download/download_manager.h:369: // DownloadManager. This includes downloads started by the user in Actually, thinking a bit more about this function, I think I'd like an interface change. This isn't a strong feeling; if you disagree, let me know. But my thoughts go like this: RemoveDownloadItems is currently called from two places, one of which removes a single download item, and one of which removes several download items. So the requirement for the function to take multiple download items just comes from one place. If we hoist the loop traversal from RemoveDownloadItems into that single caller, we've not increased the amount of code, and we've simplified the interface (because RemoveDownlaodItems would then be singular, taking a DownloadItem). WDYT?
http://codereview.chromium.org/7277073/diff/83003/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7277073/diff/83003/chrome/browser/download/dow... chrome/browser/download/download_manager.h:369: int RemoveDownloadItems(const DownloadSet& items); I can't get this to work. In addition to removing the items from the map, RemoveDownloadItems should call NotifyModelChanged and delete the item as side-effects, since these actions logically accompany the removal. I can implement a RemoveDownloadItem method that removes a single item, calls NotifyModelChanged and deletes the item. RemoveDownloadsBetween then calls RemoveDownloadItem in a loop. The problem is that NotifyModelChanged gets called a number of times. Is this safe and efficient? I worry that in the case of RemoveAllDownloads with a large history, this could be an unnecessary performance hit with n invocations of NotifyModelChanged where 1 would suffice. The other option is to have RemoveDownload strictly remove the item from the maps, and not call NotifyModelChanged or delete, and have those be done in RemoveDownloadsBetween and RemoveDownload. I'm afraid I don't like this option since it calls for code duplication :/ I've gone back to using a DownloadVector - I think it's better to be able to create a temporary container with the 1 object instead of devoting 2 additional lines of code to do this, as in the case of the set. Let me know what you think.
http://codereview.chromium.org/7277073/diff/83003/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7277073/diff/83003/chrome/browser/download/dow... chrome/browser/download/download_manager.h:369: int RemoveDownloadItems(const DownloadSet& items); On 2011/07/28 22:23:27, achuith.bhandarkar wrote: > I can't get this to work. > > In addition to removing the items from the map, RemoveDownloadItems should call > NotifyModelChanged and delete the item as side-effects, since these actions > logically accompany the removal. > > I can implement a RemoveDownloadItem method that removes a single item, calls > NotifyModelChanged and deletes the item. RemoveDownloadsBetween then calls > RemoveDownloadItem in a loop. The problem is that NotifyModelChanged gets called > a number of times. Is this safe and efficient? I worry that in the case of > RemoveAllDownloads with a large history, this could be an unnecessary > performance hit with n invocations of NotifyModelChanged where 1 would suffice. > > The other option is to have RemoveDownload strictly remove the item from the > maps, and not call NotifyModelChanged or delete, and have those be done in > RemoveDownloadsBetween and RemoveDownload. I'm afraid I don't like this option > since it calls for code duplication :/ > > I've gone back to using a DownloadVector - I think it's better to be able to > create a temporary container with the 1 object instead of devoting 2 additional > lines of code to do this, as in the case of the set. > > Let me know what you think. Good points. The way that you've got it LGTM.
Pawel, do you want me to wait for your review?
Thanks Randy! > Good points. The way that you've got it LGTM.
Note: if there are any review comments by a reviewer you _must_ wait for another review unless nothed otherwise. Sorry for the delay, feel free to ping me on IRC, IM, or at my desk. Also, please ping me if I failed to respond to some questions/comments. http://codereview.chromium.org/7277073/diff/79019/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7277073/diff/79019/chrome/browser/download/dow... chrome/browser/download/download_manager.h:96: typedef std::vector<DownloadItem*> DownloadVector; nit: Add empty line below. http://codereview.chromium.org/7277073/diff/79019/chrome/browser/download/dow... chrome/browser/download/download_manager.h:305: DownloadHistory* download_history() const { return download_history_.get(); } My earlier comment about non-const return from const method still applies. http://codereview.chromium.org/7277073/diff/79019/chrome/browser/download/dow... chrome/browser/download/download_manager.h:362: typedef std::set<DownloadItem*> DownloadSet; nit: Follow the style-guide ordering of class declarations. http://codereview.chromium.org/7277073/diff/79019/chrome/browser/download/dow... chrome/browser/download/download_manager.h:434: // Save Page Ids. This is so vague. How about "The ID to be used for next saved page"?
Pawel, PTAL http://codereview.chromium.org/7277073/diff/79019/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7277073/diff/79019/chrome/browser/download/dow... chrome/browser/download/download_manager.h:305: DownloadHistory* download_history() const { return download_history_.get(); } On 2011/07/29 18:03:30, Paweł Hajdan Jr. wrote: > My earlier comment about non-const return from const method still applies. Done. http://codereview.chromium.org/7277073/diff/79019/chrome/browser/download/dow... chrome/browser/download/download_manager.h:362: typedef std::set<DownloadItem*> DownloadSet; On 2011/07/29 18:03:30, Paweł Hajdan Jr. wrote: > nit: Follow the style-guide ordering of class declarations. Done. http://codereview.chromium.org/7277073/diff/79019/chrome/browser/download/dow... chrome/browser/download/download_manager.h:434: // Save Page Ids. On 2011/07/29 18:03:30, Paweł Hajdan Jr. wrote: > This is so vague. How about "The ID to be used for next saved page"? Done.
http://codereview.chromium.org/7277073/diff/94003/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7277073/diff/94003/chrome/browser/download/dow... chrome/browser/download/download_manager.h:307: DownloadHistory* download_history() { return download_history_.get(); } nit: Why private? If it's needed just go ahead and make it public. It's totally fine. http://codereview.chromium.org/7277073/diff/94003/chrome/browser/download/dow... chrome/browser/download/download_manager.h:433: // Save Page Ids. nit: Hey, please actually see my earlier comment about this and fix it. Or did you just forgot to upload the fix?
Need a comment from Randy before I upload next patchset. http://codereview.chromium.org/7277073/diff/94003/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7277073/diff/94003/chrome/browser/download/dow... chrome/browser/download/download_manager.h:307: DownloadHistory* download_history() { return download_history_.get(); } I had it as public but Randy preferred to have this be private. Randy, what was your rationale for wanting this to be private? http://codereview.chromium.org/7277073/diff/94003/chrome/browser/download/dow... chrome/browser/download/download_manager.h:433: // Save Page Ids. Sorry, I changed the comments of the public GetNextSavePageIds() method but not of the private variable. I'll upload it with the next patchset
Pawel, PTAL. http://codereview.chromium.org/7277073/diff/94003/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7277073/diff/94003/chrome/browser/download/dow... chrome/browser/download/download_manager.h:307: DownloadHistory* download_history() { return download_history_.get(); } I forgot Randy is on vacation. I'm going to make this public as I originally had it. Making it private is a small change that we can do later if Randy wants it that way.
GetSaveInfo() can apparently be called before Init(), so I moved the initialization of download_manager_ to InternalInit() in patch 44.
Will be trying to do a new review later today--just wanting to get the below comment in in case you're moving towards committing faster than that. http://codereview.chromium.org/7277073/diff/94003/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7277073/diff/94003/chrome/browser/download/dow... chrome/browser/download/download_manager.h:307: DownloadHistory* download_history() { return download_history_.get(); } On 2011/08/01 21:19:54, achuith.bhandarkar wrote: > I had it as public but Randy preferred to have this be private. Randy, what was > your rationale for wanting this to be private? My memory is that it was only needed by tests. I care a lot about the interface to a class, and I consider private functions part of the implementation rather than the interface, so if at all possible, I like to avoid putting things in the public interface to classes. Specifically, if something's only needed for tests, I don't want to pollute the interface (that later code may target) with it. Remind me what uses this? (I'm tossing this comment in in hopes of resolving this before you commit.)
http://codereview.chromium.org/7277073/diff/94003/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7277073/diff/94003/chrome/browser/download/dow... chrome/browser/download/download_manager.h:307: DownloadHistory* download_history() { return download_history_.get(); } On 2011/08/03 16:03:09, rdsmith wrote: > On 2011/08/01 21:19:54, achuith.bhandarkar wrote: > > I had it as public but Randy preferred to have this be private. Randy, what > was > > your rationale for wanting this to be private? > > My memory is that it was only needed by tests. I care a lot about the interface > to a class, and I consider private functions part of the implementation rather > than the interface, so if at all possible, I like to avoid putting things in the > public interface to classes. Specifically, if something's only needed for > tests, I don't want to pollute the interface (that later code may target) with > it. > > Remind me what uses this? (I'm tossing this comment in in hopes of resolving > this before you commit.) I think it's better to use #ifdef UNIT_TEST then rather than private. Friending gives you access to many more internals. http://codereview.chromium.org/7277073/diff/108001/chrome/browser/download/do... chrome/browser/download/download_manager.h:311: Bleh, still private.
http://codereview.chromium.org/7277073/diff/94003/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7277073/diff/94003/chrome/browser/download/dow... chrome/browser/download/download_manager.h:307: DownloadHistory* download_history() { return download_history_.get(); } Pawel: It's not a unit test. Is UNIT_TEST also applicable to browser tests? Randy: It's only needed by SavePageBrowserTest. I've made this private again. http://codereview.chromium.org/7277073/diff/108001/chrome/browser/download/do... chrome/browser/download/download_manager.h:311: It was public briefly in patchset 44. I've made it private again since that's Randy's preference. It's gone public->private->public->private so far.
http://codereview.chromium.org/7277073/diff/94003/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7277073/diff/94003/chrome/browser/download/dow... chrome/browser/download/download_manager.h:307: DownloadHistory* download_history() { return download_history_.get(); } On 2011/08/03 19:52:43, achuith.bhandarkar wrote: > Pawel: It's not a unit test. Is UNIT_TEST also applicable to browser tests? If it isn't it's a bug.
http://codereview.chromium.org/7277073/diff/94003/chrome/browser/download/dow... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7277073/diff/94003/chrome/browser/download/dow... chrome/browser/download/download_manager.h:307: DownloadHistory* download_history() { return download_history_.get(); } On 2011/08/03 20:49:40, Paweł Hajdan Jr. wrote: > On 2011/08/03 19:52:43, achuith.bhandarkar wrote: > > Pawel: It's not a unit test. Is UNIT_TEST also applicable to browser tests? > > If it isn't it's a bug. Done.
Change still LGTM, but I believe this isn't what Pawel wanted; see below. http://codereview.chromium.org/7277073/diff/111002/chrome/browser/download/do... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7277073/diff/111002/chrome/browser/download/do... chrome/browser/download/download_manager.h:279: #if defined(UNIT_TEST) It's Pawel's suggestion, not mine, but I don't think this is any different than friending the tests directly. I believe what he was suggesting was leaving the declarations of (e.g.) download_history() public but wrapping it in this #if. I'm happy either way (i.e. I yield on the me/Pawel debate) but I don't think this is what Pawel had in mind.
http://codereview.chromium.org/7277073/diff/111002/chrome/browser/download/do... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7277073/diff/111002/chrome/browser/download/do... chrome/browser/download/download_manager.h:279: #if defined(UNIT_TEST) On 2011/08/04 16:01:32, rdsmith wrote: > It's Pawel's suggestion, not mine, but I don't think this is any different than > friending the tests directly. I believe what he was suggesting was leaving the > declarations of (e.g.) download_history() public but wrapping it in this #if. > > I'm happy either way (i.e. I yield on the me/Pawel debate) but I don't think > this is what Pawel had in mind. Well, putting friend declarations for test classes in #ifdef UNIT_TEST makes absolutely *no* sense. Randy is right, my point is to put the download_history() accessor inside the #ifdef _and_ make it public.
http://codereview.chromium.org/7277073/diff/111002/chrome/browser/download/do... File chrome/browser/download/download_manager.h (right): http://codereview.chromium.org/7277073/diff/111002/chrome/browser/download/do... chrome/browser/download/download_manager.h:279: #if defined(UNIT_TEST) On 2011/08/04 18:11:47, Paweł Hajdan Jr. wrote: > On 2011/08/04 16:01:32, rdsmith wrote: > > It's Pawel's suggestion, not mine, but I don't think this is any different > than > > friending the tests directly. I believe what he was suggesting was leaving > the > > declarations of (e.g.) download_history() public but wrapping it in this #if. > > > > > I'm happy either way (i.e. I yield on the me/Pawel debate) but I don't think > > this is what Pawel had in mind. > > Well, putting friend declarations for test classes in #ifdef UNIT_TEST makes > absolutely *no* sense. Randy is right, my point is to put the download_history() > accessor inside the #ifdef _and_ make it public. Done.
LGTM
On 2011/08/04 18:37:57, Paweł Hajdan Jr. wrote: > LGTM Thanks! |
