DownloadManager intereface refactoring to allow cleaner DownloadItem unit tests.

content/browser/download/download_item_impl.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/download/download_item_impl.h"
 
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/file_util.h"
 #include "base/format_macros.h"
 #include "base/i18n/case_conversion.h"
 #include "base/i18n/string_search.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/stringprintf.h"
 #include "base/utf_string_conversions.h"
 #include "content/browser/download/download_create_info.h"
 #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"
 #include "content/browser/download/interrupt_reasons.h"
 #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;
 
 // A DownloadItem normally goes through the following states:
 //      * Created (when download starts)
 //      * Made visible to consumers (e.g. Javascript) after the
 //        destination file has been determined.
 //      * Entered into the history database.
 //      * Made visible in the download shelf.
@@ skipping 75 matching lines @@
 };
 
 }  // namespace
 
 // Our download table ID starts at 1, so we use 0 to represent a download that
 // has started, but has not yet had its data persisted in the table. We use fake
 // database handles in incognito mode starting at -1 and progressively getting
 // 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),
       total_bytes_(info.total_bytes),
       received_bytes_(info.received_bytes),
       start_tick_(base::TimeTicks()),
       state_(static_cast<DownloadState>(info.state)),
       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),
       safety_state_(SAFE),
       auto_opened_(false),
       is_otr_(false),
       is_temporary_(false),
       all_data_saved_(false),
       opened_(info.opened),
       open_enabled_(true),
       delegate_delayed_complete_(false) {
   if (IsInProgress())
     state_ = CANCELLED;
   if (IsComplete())
     all_data_saved_ = true;
   Init(false /* not actively downloading */);
 }
 
 // Constructing for a regular download:
 DownloadItemImpl::DownloadItemImpl(
-    DownloadManager* download_manager,
+    Delegate* delegate,
     const DownloadCreateInfo& info,
     DownloadRequestHandleInterface* request_handle,
     bool is_otr)
     : state_info_(info.original_name, info.save_info.file_path,
                   info.has_user_gesture, info.transition_type,
                   info.prompt_user_for_save_location, info.path_uniquifier,
                   DownloadStateInfo::NOT_DANGEROUS),
       request_handle_(request_handle),
       download_id_(info.download_id),
       full_path_(info.path),
       url_chain_(info.url_chain),
       referrer_url_(info.referrer_url),
       suggested_filename_(UTF16ToUTF8(info.save_info.suggested_name)),
       content_disposition_(info.content_disposition),
       mime_type_(info.mime_type),
       original_mime_type_(info.original_mime_type),
       referrer_charset_(info.referrer_charset),
       total_bytes_(info.total_bytes),
       received_bytes_(0),
       last_reason_(DOWNLOAD_INTERRUPT_REASON_NONE),
       start_tick_(base::TimeTicks::Now()),
       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),
       safety_state_(SAFE),
       auto_opened_(false),
       is_otr_(is_otr),
       is_temporary_(!info.save_info.file_path.empty()),
       all_data_saved_(false),
       opened_(false),
       open_enabled_(true),
       delegate_delayed_complete_(false) {
   Init(true /* actively downloading */);
 }
 
 // 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,
                                    DownloadId download_id)
     : request_handle_(new NullDownloadRequestHandle()),
       download_id_(download_id),
       full_path_(path),
       url_chain_(1, url),
       referrer_url_(GURL()),
       total_bytes_(0),
       received_bytes_(0),
       last_reason_(DOWNLOAD_INTERRUPT_REASON_NONE),
       start_tick_(base::TimeTicks::Now()),
       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),
       safety_state_(SAFE),
       auto_opened_(false),
       is_otr_(is_otr),
       is_temporary_(false),
       all_data_saved_(false),
       opened_(false),
       open_enabled_(true),
       delegate_delayed_complete_(false) {
   Init(true /* actively downloading */);
 }
 
 DownloadItemImpl::~DownloadItemImpl() {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   TransitionTo(REMOVING);
-  download_manager_->AssertQueueStateConsistent(this);
+  delegate_->AssertStateConsistent(this);
 }
 
 void DownloadItemImpl::AddObserver(Observer* observer) {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   observers_.AddObserver(observer);
 }
 
 void DownloadItemImpl::RemoveObserver(Observer* observer) {
@@ skipping 12 matching lines @@
 
 bool DownloadItemImpl::CanShowInFolder() {
   return !IsCancelled() && !file_externally_removed_;
 }
 
 bool DownloadItemImpl::CanOpenDownload() {
   return !file_externally_removed_;
 }
 
 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.

 }
 
 void DownloadItemImpl::OpenDownload() {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (IsPartialDownload()) {
     open_when_complete_ = !open_when_complete_;
     return;
   }
 
   if (!IsComplete() || file_externally_removed_)
     return;
 
   // Ideally, we want to detect errors in opening and report them, but we
   // 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.
   if (!open_enabled_)
     return;
 
   content::GetContentClient()->browser()->OpenItem(GetFullPath());
 }
 
 void DownloadItemImpl::ShowDownloadInShell() {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   content::GetContentClient()->browser()->ShowItemInFolder(GetFullPath());
 }
 
 void DownloadItemImpl::DangerousDownloadValidated() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK_EQ(DANGEROUS, GetSafetyState());
 
   UMA_HISTOGRAM_ENUMERATION("Download.DangerousDownloadValidated",
                             GetDangerType(),
                             DownloadStateInfo::DANGEROUS_TYPE_MAX);
 
   safety_state_ = DANGEROUS_BUT_VALIDATED;
   UpdateObservers();
 
-  download_manager_->MaybeCompleteDownload(this);
+  delegate_->MaybeCompleteDownload(this);
 }
 
 void DownloadItemImpl::UpdateSize(int64 bytes_so_far) {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   received_bytes_ = bytes_so_far;
 
   // If we've received more data than we were expecting (bad server info?),
   // revert to 'unknown size mode'.
@@ skipping 29 matching lines @@
   if (!IsPartialDownload()) {
     // Small downloads might be complete before this method has
     // a chance to run.
     return;
   }
 
   download_stats::RecordDownloadCount(download_stats::CANCELLED_COUNT);
 
   TransitionTo(CANCELLED);
   if (user_cancel)
-    download_manager_->DownloadCancelledInternal(this);
+    delegate_->DownloadCancelled(this);
 }
 
 void DownloadItemImpl::MarkAsComplete() {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   DCHECK(all_data_saved_);
   end_time_ = base::Time::Now();
   TransitionTo(COMPLETE);
 }
@@ skipping 21 matching lines @@
 
 void DownloadItemImpl::Completed() {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   VLOG(20) << __FUNCTION__ << "() " << DebugString(false);
 
   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_) {
     // If it was already handled by the delegate, do nothing.
   } else if (GetOpenWhenComplete() ||
              ShouldOpenFileBasedOnExtension() ||
              IsTemporary()) {
     // If the download is temporary, like in drag-and-drop, do not open it but
     // we still need to set it auto-opened so that it can be removed from the
     // download shelf.
@@ skipping 65 matching lines @@
   BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
                           base::Bind(&DeleteDownloadedFile, full_path_));
   Remove();
   // We have now been deleted.
 }
 
 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.
 }
 
 bool DownloadItemImpl::TimeRemaining(base::TimeDelta* remaining) const {
   if (total_bytes_ <= 0)
     return false;  // We never received the content_length for this download.
 
   int64 speed = CurrentSpeed();
   if (speed == 0)
     return false;
@@ skipping 55 matching lines @@
 
   VLOG(20) << __FUNCTION__ << "()"
            << " needs rename = " << NeedsRename()
            << " " << DebugString(true);
   DCHECK_NE(DANGEROUS, GetSafetyState());
   DCHECK(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) {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   VLOG(20) << __FUNCTION__ << "()"
            << " full_path = \"" << full_path.value() << "\""
            << " needed rename = " << NeedsRename()
            << " " << DebugString(false);
   DCHECK(NeedsRename());
 
   Rename(full_path);
 
-  if (download_manager_->delegate()->ShouldOpenDownload(this)) {
+  if (delegate_->ShouldOpenDownload(this)) {
     Completed();
   } else {
     delegate_delayed_complete_ = true;
   }
 }
 
 bool DownloadItemImpl::MatchesQuery(const string16& query) const {
   if (query.empty())
     return true;
 
   DCHECK_EQ(query, base::i18n::ToLower(query));
 
   string16 url_raw(UTF8ToUTF16(GetURL().spec()));
   if (base::i18n::StringSearchIgnoringCaseAndAccents(query, url_raw))
     return true;
 
   // TODO(phajdan.jr): write a test case for the following code.
   // A good test case would be:
   //   "/\xe4\xbd\xa0\xe5\xa5\xbd\xe4\xbd\xa0\xe5\xa5\xbd",
   //   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;
 
   string16 path(GetFullPath().LossyDisplayName());
   return base::i18n::StringSearchIgnoringCaseAndAccents(query, path);
 }
 
 void DownloadItemImpl::SetFileCheckResults(const DownloadStateInfo& state) {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
@@ skipping 51 matching lines @@
 TabContents* DownloadItemImpl::GetTabContents() const {
   // TODO(rdsmith): Remove null check after removing GetTabContents() from
   // paths that might be used by DownloadItems created from history import.
   // Currently such items have null request_handle_s, where other items
   // (regular and SavePackage downloads) have actual objects off the pointer.
   if (request_handle_.get())
     return request_handle_->GetTabContents();
   return NULL;
 }
 
+content::BrowserContext* DownloadItemImpl::BrowserContext() const {
+  return delegate_->BrowserContext();
+}
+
 FilePath DownloadItemImpl::GetTargetFilePath() const {
   return full_path_.DirName().Append(state_info_.target_name);
 }
 
 FilePath DownloadItemImpl::GetFileNameToReportUser() const {
   if (state_info_.path_uniquifier > 0) {
     FilePath name(state_info_.target_name);
     DownloadFile::AppendNumberToPath(&name, state_info_.path_uniquifier);
     return name;
   }
   return state_info_.target_name;
 }
 
 FilePath DownloadItemImpl::GetUserVerifiedFilePath() const {
   return (safety_state_ == DownloadItem::SAFE) ?
       GetTargetFilePath() : full_path_;
 }
 
 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) {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   UpdateTarget();
   if (active) {
     download_stats::RecordDownloadCount(download_stats::START_COUNT);
   }
@@ skipping 111 matching lines @@
   total_bytes_ = total_bytes;
 }
 const std::string& DownloadItemImpl::GetHash() const { return hash_; }
 int64 DownloadItemImpl::GetReceivedBytes() const { return received_bytes_; }
 int32 DownloadItemImpl::GetId() const { return download_id_.local(); }
 DownloadId DownloadItemImpl::GetGlobalId() const { return download_id_; }
 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_;
 }
 void DownloadItemImpl::SetOpenWhenComplete(bool open) {
   open_when_complete_ = open;
 }
 bool DownloadItemImpl::GetFileExternallyRemoved() const {
   return file_externally_removed_;
 }
@@ skipping 15 matching lines @@
 void DownloadItemImpl::SetOpened(bool opened) { opened_ = opened; }
 bool DownloadItemImpl::GetOpened() const { return opened_; }
 InterruptReason DownloadItemImpl::GetLastReason() const {
   return last_reason_;
 }
 DownloadStateInfo DownloadItemImpl::GetStateInfo() const { return state_info_; }
 bool DownloadItemImpl::NeedsRename() const {
   return state_info_.target_name != full_path_.BaseName();
 }
 void DownloadItemImpl::MockDownloadOpenForTesting() { open_enabled_ = false; }