DownloadManager intereface refactoring to allow cleaner DownloadItem unit tests.

content/browser/download/download_item_impl.cc

Index: content/browser/download/download_item_impl.cc
diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc
index 0a76e98db11d58d0c47ac5b96ad8f7b8342da56c..6610c0f9703bbad5e95639fc8e11dcc7a82f2657 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -20,7 +20,6 @@
 #include "content/browser/download/download_file.h"
 #include "content/browser/download/download_file_manager.h"
 #include "content/browser/download/download_id.h"
-#include "content/browser/download/download_manager.h"
 #include "content/browser/download/download_persistent_store_info.h"
 #include "content/browser/download/download_request_handle.h"
 #include "content/browser/download/download_stats.h"
@@ -28,7 +27,6 @@
 #include "content/browser/tab_contents/tab_contents.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/content_browser_client.h"
-#include "content/public/browser/download_manager_delegate.h"
 #include "net/base/net_util.h"
 
 using content::BrowserThread;
@@ -124,9 +122,10 @@ class NullDownloadRequestHandle : public DownloadRequestHandleInterface {
 // more negative.
 
 // Constructor for reading from the history service.
-DownloadItemImpl::DownloadItemImpl(DownloadManager* download_manager,
+DownloadItemImpl::DownloadItemImpl(Delegate* delegate,
+                                   DownloadId download_id,
                                    const DownloadPersistentStoreInfo& info)
-    : download_id_(download_manager->GetNextId()),
+    : download_id_(download_id),
       full_path_(info.path),
       url_chain_(1, info.url),
       referrer_url_(info.referrer_url),
@@ -137,7 +136,7 @@ DownloadItemImpl::DownloadItemImpl(DownloadManager* download_manager,
       start_time_(info.start_time),
       end_time_(info.end_time),
       db_handle_(info.db_handle),
-      download_manager_(download_manager),
+      delegate_(delegate),
       is_paused_(false),
       open_when_complete_(false),
       file_externally_removed_(false),
@@ -158,7 +157,7 @@ DownloadItemImpl::DownloadItemImpl(DownloadManager* download_manager,
 
 // Constructing for a regular download:
 DownloadItemImpl::DownloadItemImpl(
-    DownloadManager* download_manager,
+    Delegate* delegate,
     const DownloadCreateInfo& info,
     DownloadRequestHandleInterface* request_handle,
     bool is_otr)
@@ -183,7 +182,7 @@ DownloadItemImpl::DownloadItemImpl(
       state_(IN_PROGRESS),
       start_time_(info.start_time),
       db_handle_(DownloadItem::kUninitializedHandle),
-      download_manager_(download_manager),
+      delegate_(delegate),
       is_paused_(false),
       open_when_complete_(false),
       file_externally_removed_(false),
@@ -199,7 +198,7 @@ DownloadItemImpl::DownloadItemImpl(
 }
 
 // Constructing for the "Save Page As..." feature:
-DownloadItemImpl::DownloadItemImpl(DownloadManager* download_manager,
+DownloadItemImpl::DownloadItemImpl(Delegate* delegate,
                                    const FilePath& path,
                                    const GURL& url,
                                    bool is_otr,
@@ -216,7 +215,7 @@ DownloadItemImpl::DownloadItemImpl(DownloadManager* download_manager,
       state_(IN_PROGRESS),
       start_time_(base::Time::Now()),
       db_handle_(DownloadItem::kUninitializedHandle),
-      download_manager_(download_manager),
+      delegate_(delegate),
       is_paused_(false),
       open_when_complete_(false),
       file_externally_removed_(false),
@@ -236,7 +235,7 @@ DownloadItemImpl::~DownloadItemImpl() {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   TransitionTo(REMOVING);
-  download_manager_->AssertQueueStateConsistent(this);
+  delegate_->AssertStateConsistent(this);
 }
 
 void DownloadItemImpl::AddObserver(Observer* observer) {
@@ -269,7 +268,7 @@ bool DownloadItemImpl::CanOpenDownload() {
 }
 
 bool DownloadItemImpl::ShouldOpenFileBasedOnExtension() {
-  return download_manager_->delegate()->ShouldOpenFileBasedOnExtension(
+  return delegate_->ShouldOpenFileBasedOnExtension(
       GetUserVerifiedFilePath());

[cbentzel, 2011/11/29 21:30:44]

Nit: this could probably fit on previous line.

[Randy Smith (Not in Mondays), 2011/11/30 22:44:05]

Done.

 }
 
@@ -289,11 +288,11 @@ void DownloadItemImpl::OpenDownload() {
   // don't generally have the proper interface for that to the external
   // program that opens the file.  So instead we spawn a check to update
   // the UI if the file has been deleted in parallel with the open.
-  download_manager_->CheckForFileRemoval(this);
+  delegate_->CheckForFileRemoval(this);
   download_stats::RecordOpen(GetEndTime(), !GetOpened());
   opened_ = true;
   FOR_EACH_OBSERVER(Observer, observers_, OnDownloadOpened(this));
-  download_manager_->MarkDownloadOpened(this);
+  delegate_->DownloadOpened(this);
 
   // For testing: If download opening is disabled on this item,
   // make the rest of the routine a no-op.
@@ -321,7 +320,7 @@ void DownloadItemImpl::DangerousDownloadValidated() {
   safety_state_ = DANGEROUS_BUT_VALIDATED;
   UpdateObservers();
 
-  download_manager_->MaybeCompleteDownload(this);
+  delegate_->MaybeCompleteDownload(this);
 }
 
 void DownloadItemImpl::UpdateSize(int64 bytes_so_far) {
@@ -371,7 +370,7 @@ void DownloadItemImpl::Cancel(bool user_cancel) {
 
   TransitionTo(CANCELLED);
   if (user_cancel)
-    download_manager_->DownloadCancelledInternal(this);
+    delegate_->DownloadCancelled(this);
 }
 
 void DownloadItemImpl::MarkAsComplete() {
@@ -413,7 +412,7 @@ void DownloadItemImpl::Completed() {
   DCHECK(all_data_saved_);
   end_time_ = base::Time::Now();
   TransitionTo(COMPLETE);
-  download_manager_->DownloadCompleted(GetId());
+  delegate_->DownloadCompleted(this);
   download_stats::RecordDownloadCompleted(start_tick_, received_bytes_);
 
   if (auto_opened_) {
@@ -499,12 +498,12 @@ void DownloadItemImpl::Remove() {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  download_manager_->AssertQueueStateConsistent(this);
+  delegate_->AssertStateConsistent(this);

[cbentzel, 2011/11/29 21:30:44]

It seems strange to have the DownloadItem ask the delegate to see if invariants
are upheld. Is there any way to move this checking into the delegate (aka
DownloadManagerImpl) and remove it from the Delegate interface?

[Randy Smith (Not in Mondays), 2011/11/30 22:44:05]

I tried, but it turned out that we're still getting crashes that are being
detected in the DownloadItem and might not be detected if we executed the
download item logic and did the assertion in DownloadManagerImpl.  In response
to your comment, I did a bit of a more thorough examination and found that:

* 15.0.874.121 has 5083 crashes in AssertStateConsistent, all of which are
coming from download manager.
* The max count of crashes post-15 is in 17.0.942.0, which has 36 crashes in
AssertStateConsistent, 19 of which are from downloads manager, and 17 from
download_item.cc.
* The pathway from download_item.cc is the Remove() pathway.  

So I think this CL needs to keep AssertStateConsistent, and that I should do
another swing at figuring out that crash in the near future :-J.

, but I didn't feel comfortable doing this while we were still having crashes in
the field that come from assertions inside of the DownloadItemImpl.  I can
certainly pull it out; it just involves removing all AssertStateConsistent calls
that are in the DownloadItem, and we're still occasionally tripping on some of
those :-J.

   Cancel(true);
-  download_manager_->AssertQueueStateConsistent(this);
+  delegate_->AssertStateConsistent(this);
 
   TransitionTo(REMOVING);
-  download_manager_->RemoveDownload(db_handle_);
+  delegate_->DownloadRemoved(this);
   // We have now been deleted.
 }
 
@@ -580,16 +579,17 @@ void DownloadItemImpl::OnDownloadCompleting(DownloadFileManager* file_manager) {
   if (NeedsRename()) {
     BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
         base::Bind(&DownloadFileManager::RenameCompletingDownloadFile,
-                   file_manager, GetGlobalId(),
+                   file_manager, download_id_,
                    GetTargetFilePath(), GetSafetyState() == SAFE));
     return;
   }
 
   Completed();
 
-  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-                          base::Bind(&DownloadFileManager::CompleteDownload,
-                                     file_manager, GetGlobalId()));
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&DownloadFileManager::CompleteDownload,
+                 file_manager, download_id_));
 }
 
 void DownloadItemImpl::OnDownloadRenamedToFinalName(const FilePath& full_path) {
@@ -604,7 +604,7 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName(const FilePath& full_path) {
 
   Rename(full_path);
 
-  if (download_manager_->delegate()->ShouldOpenDownload(this)) {
+  if (delegate_->ShouldOpenDownload(this)) {
     Completed();
   } else {
     delegate_delayed_complete_ = true;
@@ -627,11 +627,8 @@ bool DownloadItemImpl::MatchesQuery(const string16& query) const {
   //   L"/\x4f60\x597d\x4f60\x597d",
   //   "/%E4%BD%A0%E5%A5%BD%E4%BD%A0%E5%A5%BD"
   std::string languages;
-  TabContents* tab = GetTabContents();
-  if (tab) {
-    languages = content::GetContentClient()->browser()->GetAcceptLangs(
-        tab->browser_context());
-  }
+  languages = content::GetContentClient()->browser()->GetAcceptLangs(
+      delegate_->BrowserContext());

[cbentzel, 2011/11/29 21:30:44]

Should DownloadItem::BrowserContext be used here instead?

[Randy Smith (Not in Mondays), 2011/11/30 22:44:05]

In an ideal world, DownloadItem wouldn't have a BrowserContext :-J.  But that's
a lot of plumbing (I looked).  I'll change it, but I hope I can yank it out
later. 

   string16 url_formatted(net::FormatUrl(GetURL(), languages));
   if (base::i18n::StringSearchIgnoringCaseAndAccents(query, url_formatted))
     return true;
@@ -703,6 +700,10 @@ TabContents* DownloadItemImpl::GetTabContents() const {
   return NULL;
 }
 
+content::BrowserContext* DownloadItemImpl::BrowserContext() const {
+  return delegate_->BrowserContext();
+}
+
 FilePath DownloadItemImpl::GetTargetFilePath() const {
   return full_path_.DirName().Append(state_info_.target_name);
 }
@@ -725,9 +726,10 @@ void DownloadItemImpl::OffThreadCancel(DownloadFileManager* file_manager) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   request_handle_->CancelRequest();
 
-  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-                          base::Bind(&DownloadFileManager::CancelDownload,
-                                     file_manager, GetGlobalId()));
+  BrowserThread::PostTask(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&DownloadFileManager::CancelDownload,
+                 file_manager, download_id_));
 }
 
 void DownloadItemImpl::Init(bool active) {
@@ -859,9 +861,6 @@ base::Time DownloadItemImpl::GetStartTime() const { return start_time_; }
 base::Time DownloadItemImpl::GetEndTime() const { return end_time_; }
 void DownloadItemImpl::SetDbHandle(int64 handle) { db_handle_ = handle; }
 int64 DownloadItemImpl::GetDbHandle() const { return db_handle_; }
-DownloadManager* DownloadItemImpl::GetDownloadManager() {
-  return download_manager_;
-}
 bool DownloadItemImpl::IsPaused() const { return is_paused_; }
 bool DownloadItemImpl::GetOpenWhenComplete() const {
   return open_when_complete_;