Remove DownloadManager::GetDownloadItem in favor of GetDownload()

Remove DownloadManager::GetDownloadItem() and GetActiveDownloadItem() in favor of GetDownload()
Followup to GetAllDownloads() does now: http://codereview.chromium.org/10913015/
Precursor to DownloadHistory-is-an-Observer: http://codereview.chromium.org/10665049/

Reviewers:
estade: chrome/browser/ui/webui/downloads_dom_handler.cc

Done:
bauerb: chrome/browser/plugins/plugin_installer.cc
aa: chrome/browser/extensions/webstore_installer.cc
achuith: chrome/browser/chromeos/gdata
jcivelli: content/public/test
jochen: content/shell
rdsmith: *

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158595

[benjhayden, 2012-09-10 21:57:32]

PTAL

[Randy Smith (Not in Mondays), 2012-09-11 19:02:21]

First round of poking :-}.

http://codereview.chromium.org/10912183/diff/10018/chrome/browser/download/ch...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10912183/diff/10018/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:627: DownloadItem*
download = download_manager_->GetDownload(download_id);
So this seems like a behavior change that I don't offhand see why we should do? 
If the item was cancelled during the download url check, we'd drop it on the
floor at this point; now we don't.  Do we want that?

Same comment for other GetActiveDownloadItem->GetDownload transitions in this
file.

http://codereview.chromium.org/10912183/diff/10018/chrome/browser/download/do...
File chrome/browser/download/download_file_picker_chromeos.cc (left):

http://codereview.chromium.org/10912183/diff/10018/chrome/browser/download/do...
chrome/browser/download/download_file_picker_chromeos.cc:58:
download_manager_->GetActiveDownloadItem(download_id_);
Huh.  Does this path handle null downloads?  If so, same comment as in cdmd.cc. 
If not, I'm surprised we haven't crashed previously--we need better cancel
tests.

http://codereview.chromium.org/10912183/diff/10018/chrome/browser/extensions/...
File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right):

http://codereview.chromium.org/10912183/diff/10018/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:1413: if
(!item) item = GetCurrentManager()->GetDownload(result_id);
I'm guessing that you query-replaced and didn't really read this code?  Or maybe
you're trolling your reviewers?  :-}  But whether insanity or statistics, I
don't think you want to try the same operation twice in hopes of getting
different results :-}.

(Comment applies throughout file.)

http://codereview.chromium.org/10912183/diff/10018/chrome/browser/ui/webui/do...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

http://codereview.chromium.org/10912183/diff/10018/chrome/browser/ui/webui/do...
chrome/browser/ui/webui/downloads_dom_handler.cc:503: content::DownloadItem*
item = download_manager_->GetDownload(download_id);
I think we want a cancellation/interrupt guard somewhere along this pathway.  I
don't care if it's here or in DangerousDownloadValidated().

http://codereview.chromium.org/10912183/diff/10018/content/public/test/downlo...
File content/public/test/download_test_observer.cc (left):

http://codereview.chromium.org/10912183/diff/10018/content/public/test/downlo...
content/public/test/download_test_observer.cc:32: DownloadItem* download =
download_manager->GetDownloadItem(download_id);
Same comment as in DownloadsDOMHandler: I think we want a cancel/interrupt guard
on the DDV path.

http://codereview.chromium.org/10912183/diff/10018/content/shell/shell_downlo...
File content/shell/shell_download_manager_delegate.cc (right):

http://codereview.chromium.org/10912183/diff/10018/content/shell/shell_downlo...
content/shell/shell_download_manager_delegate.cc:126: DownloadItem* item =
download_manager_->GetDownload(download_id);
Same comment as in CDMD: this seems like a noticeable behavior change--can you
make a pitch as to why we'd want it?

[benjhayden, 2012-09-12 18:17:25]

PTAL

http://codereview.chromium.org/10912183/diff/10018/chrome/browser/download/ch...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

http://codereview.chromium.org/10912183/diff/10018/chrome/browser/download/ch...
chrome/browser/download/chrome_download_manager_delegate.cc:627: DownloadItem*
download = download_manager_->GetDownload(download_id);
On 2012/09/11 19:02:21, rdsmith wrote:
> So this seems like a behavior change that I don't offhand see why we should
do? 
> If the item was cancelled during the download url check, we'd drop it on the
> floor at this point; now we don't.  Do we want that?
> 
> Same comment for other GetActiveDownloadItem->GetDownload transitions in this
> file.

Done.

http://codereview.chromium.org/10912183/diff/10018/chrome/browser/download/do...
File chrome/browser/download/download_file_picker_chromeos.cc (left):

http://codereview.chromium.org/10912183/diff/10018/chrome/browser/download/do...
chrome/browser/download/download_file_picker_chromeos.cc:58:
download_manager_->GetActiveDownloadItem(download_id_);
On 2012/09/11 19:02:21, rdsmith wrote:
> Huh.  Does this path handle null downloads?  If so, same comment as in
cdmd.cc. 
> If not, I'm surprised we haven't crashed previously--we need better cancel
> tests.

SubstituteDriveDownloadPath calls SetDownloadParams(), which returns early if
!download, but then SDDP also does some other stuff. I'd rather not make it not
do that other stuff in this CL.

http://codereview.chromium.org/10912183/diff/10018/chrome/browser/extensions/...
File chrome/browser/extensions/api/downloads/downloads_api_unittest.cc (right):

http://codereview.chromium.org/10912183/diff/10018/chrome/browser/extensions/...
chrome/browser/extensions/api/downloads/downloads_api_unittest.cc:1413: if
(!item) item = GetCurrentManager()->GetDownload(result_id);
On 2012/09/11 19:02:21, rdsmith wrote:
> I'm guessing that you query-replaced and didn't really read this code?  Or
maybe
> you're trolling your reviewers?  :-}  But whether insanity or statistics, I
> don't think you want to try the same operation twice in hopes of getting
> different results :-}.
> 
> (Comment applies throughout file.)

Done.

http://codereview.chromium.org/10912183/diff/10018/chrome/browser/ui/webui/do...
File chrome/browser/ui/webui/downloads_dom_handler.cc (right):

http://codereview.chromium.org/10912183/diff/10018/chrome/browser/ui/webui/do...
chrome/browser/ui/webui/downloads_dom_handler.cc:503: content::DownloadItem*
item = download_manager_->GetDownload(download_id);
On 2012/09/11 19:02:21, rdsmith wrote:
> I think we want a cancellation/interrupt guard somewhere along this pathway. 
I
> don't care if it's here or in DangerousDownloadValidated().

Done.

http://codereview.chromium.org/10912183/diff/10018/content/public/test/downlo...
File content/public/test/download_test_observer.cc (left):

http://codereview.chromium.org/10912183/diff/10018/content/public/test/downlo...
content/public/test/download_test_observer.cc:32: DownloadItem* download =
download_manager->GetDownloadItem(download_id);
On 2012/09/11 19:02:21, rdsmith wrote:
> Same comment as in DownloadsDOMHandler: I think we want a cancel/interrupt
guard
> on the DDV path.

Done.

http://codereview.chromium.org/10912183/diff/10018/content/shell/shell_downlo...
File content/shell/shell_download_manager_delegate.cc (right):

http://codereview.chromium.org/10912183/diff/10018/content/shell/shell_downlo...
content/shell/shell_download_manager_delegate.cc:126: DownloadItem* item =
download_manager_->GetDownload(download_id);
On 2012/09/11 19:02:21, rdsmith wrote:
> Same comment as in CDMD: this seems like a noticeable behavior change--can you
> make a pitch as to why we'd want it?

Done.

[Randy Smith (Not in Mondays), 2012-09-13 18:52:43]

Two high level comments:

* Given the frequency with which you get a download and then check to see if
it's IN_PROGRESS, I'm inclined to think that that code should be wrapped rather
than duplicated in all the different consumers.  I.e. I think you should keep
GetActiveDownloadItem(), and just implement it by probing the state of the
download rather than by it having its own queue.

* You'll need consumer-specific reviewers for the various consumers you're
changing (GDrive, plugin installer, etc.)

http://codereview.chromium.org/10912183/diff/10018/chrome/browser/download/do...
File chrome/browser/download/download_file_picker_chromeos.cc (left):

http://codereview.chromium.org/10912183/diff/10018/chrome/browser/download/do...
chrome/browser/download/download_file_picker_chromeos.cc:58:
download_manager_->GetActiveDownloadItem(download_id_);
On 2012/09/12 18:17:25, benjhayden_chromium wrote:
> On 2012/09/11 19:02:21, rdsmith wrote:
> > Huh.  Does this path handle null downloads?  If so, same comment as in
> cdmd.cc. 
> > If not, I'm surprised we haven't crashed previously--we need better cancel
> > tests.
> 
> SubstituteDriveDownloadPath calls SetDownloadParams(), which returns early if
> !download, but then SDDP also does some other stuff. I'd rather not make it
not
> do that other stuff in this CL.

I understand about wanting to avoid mission creep, but if the code is resilient
to null downloads in some fashion, that may mean that you're changing behavior
by making cancelled downloads actually show up now.  I think you need to grok
the code well enough to figure out if that's ok (or test against IN_PROGRESS
here) for this CL.

http://codereview.chromium.org/10912183/diff/9005/chrome/browser/plugin_insta...
File chrome/browser/plugin_installer.cc (right):

http://codereview.chromium.org/10912183/diff/9005/chrome/browser/plugin_insta...
chrome/browser/plugin_installer.cc:228: DownloadItem* download_item =
dlm->GetDownload(download_id.local());
I'd be inclined to put a DCHECK after this that we're in IN_PROGRESS.

http://codereview.chromium.org/10912183/diff/9005/chrome/browser/plugin_insta...
chrome/browser/plugin_installer.cc:231: return;
Why the addition of the error return?  GetDownload() will return NULL in fewer
cases than GetActiveDownloadItem.

http://codereview.chromium.org/10912183/diff/9005/content/public/test/downloa...
File content/public/test/download_test_observer.cc (right):

http://codereview.chromium.org/10912183/diff/9005/content/public/test/downloa...
content/public/test/download_test_observer.cc:41:
ASSERT_TRUE(download->IsPartialDownload());
(Feel free to ignore; not a natural part of this CL) I find it weird that we'd
have this assertion here and not above; it doesn't seem like the two would be
different.   Looking at the code, I think that we could actually land here if we
aren't IN_PROGRESS, so duplicating the above logic might make sense.

[benjhayden, 2012-09-13 20:19:35]

We talked offline about keeping GetActiveDownload().
Even though 2 options (GetDownload(), GetActiveDownload()) are infinitely better
than 5 in several ways, we decided to go ahead and kill GetActiveDownload() now,
wait a bit to see how consumers are dealing with the bluntness of GetDownload(),
then consider re-adding GetActiveDownload (GetDownloadIfInProgress?) as new pure
syntactic sugar instead of something that used to be broken.

Adding reviewers momentarily.

http://codereview.chromium.org/10912183/diff/10018/chrome/browser/download/do...
File chrome/browser/download/download_file_picker_chromeos.cc (left):

http://codereview.chromium.org/10912183/diff/10018/chrome/browser/download/do...
chrome/browser/download/download_file_picker_chromeos.cc:58:
download_manager_->GetActiveDownloadItem(download_id_);
On 2012/09/13 18:52:43, rdsmith wrote:
> On 2012/09/12 18:17:25, benjhayden_chromium wrote:
> > On 2012/09/11 19:02:21, rdsmith wrote:
> > > Huh.  Does this path handle null downloads?  If so, same comment as in
> > cdmd.cc. 
> > > If not, I'm surprised we haven't crashed previously--we need better cancel
> > > tests.
> > 
> > SubstituteDriveDownloadPath calls SetDownloadParams(), which returns early
if
> > !download, but then SDDP also does some other stuff. I'd rather not make it
> not
> > do that other stuff in this CL.
> 
> I understand about wanting to avoid mission creep, but if the code is
resilient
> to null downloads in some fashion, that may mean that you're changing behavior
> by making cancelled downloads actually show up now.  I think you need to grok
> the code well enough to figure out if that's ok (or test against IN_PROGRESS
> here) for this CL.

Ah! Good idea.
I added the IN_PROGRESS test in DriveDownloadObserver::SetDownloadParams in
chrome/browser/chromeos/gdata/drive_download_observer.cc
I'll make sure to ask achuith about it unless tests turn up a specific reason to
not test for IN_PROGRESS.

http://codereview.chromium.org/10912183/diff/9005/chrome/browser/plugin_insta...
File chrome/browser/plugin_installer.cc (right):

http://codereview.chromium.org/10912183/diff/9005/chrome/browser/plugin_insta...
chrome/browser/plugin_installer.cc:228: DownloadItem* download_item =
dlm->GetDownload(download_id.local());
On 2012/09/13 18:52:44, rdsmith wrote:
> I'd be inclined to put a DCHECK after this that we're in IN_PROGRESS.

TODone, see response to below comment.

http://codereview.chromium.org/10912183/diff/9005/chrome/browser/plugin_insta...
chrome/browser/plugin_installer.cc:231: return;
On 2012/09/13 18:52:44, rdsmith wrote:
> Why the addition of the error return?  GetDownload() will return NULL in fewer
> cases than GetActiveDownloadItem.

We saw in WebstoreInstaller where the DownloadStarted callback may be called
with net::OK but an invalid id. This is just defensive programming while we hunt
that bug down. I'll add a TODO to turn it into a DCHECK when possible.

http://codereview.chromium.org/10912183/diff/9005/content/public/test/downloa...
File content/public/test/download_test_observer.cc (right):

http://codereview.chromium.org/10912183/diff/9005/content/public/test/downloa...
content/public/test/download_test_observer.cc:41:
ASSERT_TRUE(download->IsPartialDownload());
On 2012/09/13 18:52:44, rdsmith wrote:
> (Feel free to ignore; not a natural part of this CL) I find it weird that we'd
> have this assertion here and not above; it doesn't seem like the two would be
> different.   Looking at the code, I think that we could actually land here if
we
> aren't IN_PROGRESS, so duplicating the above logic might make sense.

Done.

[Randy Smith (Not in Mondays), 2012-09-14 16:54:57]

LGTM modulo try bots and other reviewers ok.

[Jay Civelli, 2012-09-14 20:28:45]

LGTM for content/public/test

[jochen (gone - plz use gerrit), 2012-09-17 19:38:21]

content/shell lgtm

[achuithb, 2012-09-17 23:11:47]

chromeos/gdata lgtm

[commit-bot: I haz the power, 2012-09-21 17:30:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10912183/40001

[commit-bot: I haz the power, 2012-09-21 17:30:52]

Presubmit check for 10912183-40001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/webui
    chrome/browser/plugins
    chrome/browser/extensions

Presubmit checks took 1.9s to calculate.

[Aaron Boodman, 2012-09-21 17:56:56]

c/b/e lgtm

[benjhayden, 2012-09-21 20:58:32]

James, Bernhard, PTAL for OWNERShip?

[Bernhard Bauer, 2012-09-21 23:00:06]

lgtm

[benjhayden, 2012-09-24 20:15:09]

Evan, PTAL?

[Evan Stade, 2012-09-25 06:16:30]

webui lgtm

[commit-bot: I haz the power, 2012-09-25 14:55:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10912183/43001

[commit-bot: I haz the power, 2012-09-25 17:23:03]

Change committed as 158595