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: *

Review URL: https://chromiumcodereview.appspot.com/10912183

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@158595 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 19 insertions, 32 deletions

diff --git a/chrome/browser/extensions/api/downloads/downloads_api.cc b/chrome/browser/extensions/api/downloads/downloads_api.cc
index f2394a1..2e21cca 100644
--- a/chrome/browser/extensions/api/downloads/downloads_api.cc
+++ b/chrome/browser/extensions/api/downloads/downloads_api.cc
@@ -860,7 +860,7 @@ bool DownloadsGetFileIconFunction::RunImpl() {
   DownloadItem* download_item = manager->GetDownload(params->download_id);
   if (!download_item && incognito_manager)
     download_item = incognito_manager->GetDownload(params->download_id);
-  if (!download_item) {
+  if (!download_item || download_item->GetTargetFilePath().empty()) {
     // The DownloadItem is is added to history when the path is determined. If
     // the download is not in history, then we don't have a path / final
     // filename and no icon.
@@ -870,12 +870,11 @@ bool DownloadsGetFileIconFunction::RunImpl() {
   // In-progress downloads return the intermediate filename for GetFullPath()
   // which doesn't have the final extension. Therefore we won't be able to
   // derive a good file icon for it. So we use GetTargetFilePath() instead.
-  FilePath path = download_item->GetTargetFilePath();
-  DCHECK(!path.empty());
   DCHECK(icon_extractor_.get());
   DCHECK(icon_size == 16 || icon_size == 32);
   EXTENSION_FUNCTION_VALIDATE(icon_extractor_->ExtractIconURLForPath(
-      path, IconLoaderSizeFromPixelSize(icon_size),
+      download_item->GetTargetFilePath(),
+      IconLoaderSizeFromPixelSize(icon_size),
       base::Bind(&DownloadsGetFileIconFunction::OnIconURLExtracted, this)));
   return true;
 }
diff --git a/chrome/browser/extensions/api/downloads/downloads_api_unittest.cc b/chrome/browser/extensions/api/downloads/downloads_api_unittest.cc
index aef64d6..b5e961c 100644
--- a/chrome/browser/extensions/api/downloads/downloads_api_unittest.cc
+++ b/chrome/browser/extensions/api/downloads/downloads_api_unittest.cc
@@ -1403,8 +1403,7 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
   ASSERT_TRUE(result.get());
   int result_id = -1;
   ASSERT_TRUE(result->GetAsInteger(&result_id));
-  DownloadItem* item = GetCurrentManager()->GetActiveDownloadItem(result_id);
-  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  DownloadItem* item = GetCurrentManager()->GetDownload(result_id);
   ASSERT_TRUE(item);
   ScopedCancellingItem canceller(item);
   ASSERT_EQ(download_url, item->GetOriginalUrl().spec());
@@ -1444,8 +1443,7 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
   ASSERT_TRUE(result.get());
   int result_id = -1;
   ASSERT_TRUE(result->GetAsInteger(&result_id));
-  DownloadItem* item = GetCurrentManager()->GetActiveDownloadItem(result_id);
-  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  DownloadItem* item = GetCurrentManager()->GetDownload(result_id);
   ASSERT_TRUE(item);
   ScopedCancellingItem canceller(item);
   ASSERT_EQ(download_url, item->GetOriginalUrl().spec());
@@ -1598,8 +1596,7 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
   ASSERT_TRUE(result.get());
   int result_id = -1;
   ASSERT_TRUE(result->GetAsInteger(&result_id));
-  DownloadItem* item = GetCurrentManager()->GetActiveDownloadItem(result_id);
-  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  DownloadItem* item = GetCurrentManager()->GetDownload(result_id);
   ASSERT_TRUE(item);
   ScopedCancellingItem canceller(item);
   ASSERT_EQ(download_url, item->GetOriginalUrl().spec());
@@ -1639,8 +1636,7 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
   ASSERT_TRUE(result.get());
   int result_id = -1;
   ASSERT_TRUE(result->GetAsInteger(&result_id));
-  DownloadItem* item = GetCurrentManager()->GetActiveDownloadItem(result_id);
-  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  DownloadItem* item = GetCurrentManager()->GetDownload(result_id);
   ASSERT_TRUE(item);
   ScopedCancellingItem canceller(item);
   ASSERT_EQ(download_url, item->GetOriginalUrl().spec());
@@ -1683,8 +1679,7 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
   ASSERT_TRUE(result.get());
   int result_id = -1;
   ASSERT_TRUE(result->GetAsInteger(&result_id));
-  DownloadItem* item = GetCurrentManager()->GetActiveDownloadItem(result_id);
-  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  DownloadItem* item = GetCurrentManager()->GetDownload(result_id);
   ASSERT_TRUE(item);
   ScopedCancellingItem canceller(item);
   ASSERT_EQ(download_url, item->GetOriginalUrl().spec());
@@ -1728,8 +1723,7 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
   ASSERT_TRUE(result.get());
   int result_id = -1;
   ASSERT_TRUE(result->GetAsInteger(&result_id));
-  DownloadItem* item = GetCurrentManager()->GetActiveDownloadItem(result_id);
-  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  DownloadItem* item = GetCurrentManager()->GetDownload(result_id);
   ASSERT_TRUE(item);
   ScopedCancellingItem canceller(item);
   ASSERT_EQ(download_url, item->GetOriginalUrl().spec());
@@ -1763,8 +1757,7 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
   ASSERT_TRUE(result.get());
   int result_id = -1;
   ASSERT_TRUE(result->GetAsInteger(&result_id));
-  DownloadItem* item = GetCurrentManager()->GetActiveDownloadItem(result_id);
-  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  DownloadItem* item = GetCurrentManager()->GetDownload(result_id);
   ASSERT_TRUE(item);
   ScopedCancellingItem canceller(item);
   ASSERT_EQ(download_url, item->GetOriginalUrl().spec());
@@ -1811,8 +1804,7 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
   ASSERT_TRUE(result.get());
   int result_id = -1;
   ASSERT_TRUE(result->GetAsInteger(&result_id));
-  DownloadItem* item = GetCurrentManager()->GetActiveDownloadItem(result_id);
-  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  DownloadItem* item = GetCurrentManager()->GetDownload(result_id);
   ASSERT_TRUE(item);
   ScopedCancellingItem canceller(item);
   ASSERT_EQ(download_url, item->GetOriginalUrl().spec());
@@ -1849,8 +1841,7 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
   ASSERT_TRUE(result.get());
   int result_id = -1;
   ASSERT_TRUE(result->GetAsInteger(&result_id));
-  DownloadItem* item = GetCurrentManager()->GetActiveDownloadItem(result_id);
-  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  DownloadItem* item = GetCurrentManager()->GetDownload(result_id);
   ASSERT_TRUE(item);
   ScopedCancellingItem canceller(item);
   ASSERT_EQ(download_url, item->GetOriginalUrl().spec());
@@ -1888,8 +1879,7 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
   ASSERT_TRUE(result.get());
   int result_id = -1;
   ASSERT_TRUE(result->GetAsInteger(&result_id));
-  DownloadItem* item = GetCurrentManager()->GetActiveDownloadItem(result_id);
-  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  DownloadItem* item = GetCurrentManager()->GetDownload(result_id);
   ASSERT_TRUE(item);
   ScopedCancellingItem canceller(item);
   ASSERT_EQ(download_url, item->GetOriginalUrl().spec());
@@ -1935,8 +1925,7 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
   ASSERT_TRUE(result.get());
   int result_id = -1;
   ASSERT_TRUE(result->GetAsInteger(&result_id));
-  DownloadItem* item = GetCurrentManager()->GetActiveDownloadItem(result_id);
-  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  DownloadItem* item = GetCurrentManager()->GetDownload(result_id);
   ASSERT_TRUE(item);
   ScopedCancellingItem canceller(item);
   ASSERT_EQ(download_url, item->GetOriginalUrl().spec());
@@ -1974,8 +1963,7 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
   ASSERT_TRUE(result.get());
   int result_id = -1;
   ASSERT_TRUE(result->GetAsInteger(&result_id));
-  DownloadItem* item = GetCurrentManager()->GetActiveDownloadItem(result_id);
-  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  DownloadItem* item = GetCurrentManager()->GetDownload(result_id);
   ASSERT_TRUE(item);
   ScopedCancellingItem canceller(item);
   ASSERT_EQ(download_url, item->GetOriginalUrl().spec());
@@ -2009,8 +1997,7 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
   ASSERT_TRUE(result.get());
   int result_id = -1;
   ASSERT_TRUE(result->GetAsInteger(&result_id));
-  DownloadItem* item = GetCurrentManager()->GetActiveDownloadItem(result_id);
-  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  DownloadItem* item = GetCurrentManager()->GetDownload(result_id);
   ASSERT_TRUE(item);
   ScopedCancellingItem canceller(item);
   ASSERT_EQ(download_url, item->GetOriginalUrl().spec());
@@ -2058,8 +2045,7 @@ IN_PROC_BROWSER_TEST_F(DownloadExtensionTest,
   int result_id = -1;
   ASSERT_TRUE(result->GetAsInteger(&result_id));
 
-  DownloadItem* item = GetCurrentManager()->GetActiveDownloadItem(result_id);
-  if (!item) item = GetCurrentManager()->GetDownloadItem(result_id);
+  DownloadItem* item = GetCurrentManager()->GetDownload(result_id);
   ASSERT_TRUE(item);
   ScopedCancellingItem canceller(item);
   ASSERT_EQ(download_url, item->GetOriginalUrl().spec());
diff --git a/chrome/browser/extensions/webstore_installer.cc b/chrome/browser/extensions/webstore_installer.cc
index 88e68dd..ffae640 100644
--- a/chrome/browser/extensions/webstore_installer.cc
+++ b/chrome/browser/extensions/webstore_installer.cc
@@ -282,6 +282,8 @@ void WebstoreInstaller::OnDownloadStarted(DownloadId id, net::Error error) {
   if (!download_manager)
     return;
   download_item_ = download_manager->GetDownload(id.local());
+  // TODO(benjhayden): DCHECK(item && item->IsInProgress()) after investigating
+  // the relationship between net::OK and invalid id.
   if (download_item_) {
     download_item_->AddObserver(this);
     if (approval_.get())