Use a DownloadRequestHandle pointer in construction to allow mocking for tests.

content/browser/download/download_item.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.h"
 
 #include "base/basictypes.h"
 #include "base/file_util.h"
 #include "base/format_macros.h"
 #include "base/i18n/case_conversion.h"
@@ skipping 136 matching lines @@
   if (IsInProgress())
     state_ = CANCELLED;
   if (IsComplete())
     all_data_saved_ = true;
   Init(false /* not actively downloading */);
 }
 
 // Constructing for a regular download:
 DownloadItem::DownloadItem(DownloadManager* download_manager,
                            const DownloadCreateInfo& info,
-                           const DownloadRequestHandle& request_handle,
+                           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,
                   false, false),
       request_handle_(request_handle),
       download_id_(info.download_id),
       full_path_(info.path),
       url_chain_(info.url_chain),
       referrer_url_(info.referrer_url),
@@ skipping 401 matching lines @@
   DCHECK(!full_path.empty());
   full_path_ = full_path;
 }
 
 void DownloadItem::TogglePause() {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   DCHECK(IsInProgress());
   if (is_paused_)
-    request_handle_.ResumeRequest();
+    request_handle_->ResumeRequest();
   else
-    request_handle_.PauseRequest();
+    request_handle_->PauseRequest();
   is_paused_ = !is_paused_;
   UpdateObservers();
 }
 
 void DownloadItem::OnDownloadCompleting(DownloadFileManager* file_manager) {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   VLOG(20) << __FUNCTION__ << "()"
            << " needs rename = " << NeedsRename()
@@ skipping 43 matching lines @@
   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 = request_handle_.GetTabContents();
+  TabContents* tab = request_handle_->GetTabContents();
   if (tab)
     languages = content::GetContentClient()->browser()->GetAcceptLangs(tab);
   string16 url_formatted(net::FormatUrl(GetURL(), languages));
   if (base::i18n::StringSearchIgnoringCaseAndAccents(query, url_formatted))
     return true;
 
   string16 path(full_path().LossyDisplayName());
   return base::i18n::StringSearchIgnoringCaseAndAccents(query, path);
 }
 
@@ skipping 39 matching lines @@
                                      referrer_url(),
                                      start_time(),
                                      end_time(),
                                      received_bytes(),
                                      total_bytes(),
                                      state(),
                                      db_handle(),
                                      opened());
 }
 
+TabContents* DownloadItem::GetTabContents() const {
+  if (request_handle_.get())

[cbentzel, 2011/10/27 17:20:39]

Why is this needed? DownloadItem::MatchesQuery doesn't use it, for example, and
it looks like other places don't check for NULL either.

[Randy Smith (Not in Mondays), 2011/10/27 18:21:22]

See DownloadManager::RestartDownload.  I ran into this because in some tests, a
null DownloadRequestHandle is now represented by a null pointer rather than a
default constructed download handle.  But for cleanliness of mocking, I'd rather
avoid cases where something external to a class gets a reference to an object
contained within the class, so I prefer the wrapping pattern anyway.

If you'd like me to merge in the switch over to mocks for DownloadRequestHandle
in this CL, I will, but I'd like to keep this wrapper.

[cbentzel, 2011/10/27 18:45:34]

I have no problem with the wrapper - just was more confused about why we were
checking for NULL here but not in other cases.

+    return request_handle_->GetTabContents();
+  return NULL;
+}
+
 FilePath DownloadItem::GetTargetFilePath() const {
   return full_path_.DirName().Append(state_info_.target_name);
 }
 
 FilePath DownloadItem::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 DownloadItem::GetUserVerifiedFilePath() const {
   return (safety_state_ == DownloadItem::SAFE) ?
       GetTargetFilePath() : full_path_;
 }
 
 void DownloadItem::OffThreadCancel(DownloadFileManager* file_manager) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  request_handle_.CancelRequest();
+  request_handle_->CancelRequest();
 
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       NewRunnableMethod(
           file_manager, &DownloadFileManager::CancelDownload, global_id()));
 }
 
 void DownloadItem::Init(bool active) {
   // TODO(rdsmith): Change to DCHECK after http://crbug.com/85408 resolved.
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ skipping 76 matching lines @@
         state_info_.target_name.value().c_str(),
         full_path().value().c_str());
   } else {
     description += base::StringPrintf(" url = \"%s\"", url_list.c_str());
   }
 
   description += " }";
 
   return description;
 }