Kill DownloadItem::IsOtr()

chrome/browser/download/chrome_download_manager_delegate.cc

 // Copyright (c) 2012 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 "chrome/browser/download/chrome_download_manager_delegate.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 114 matching lines @@
       next_download_id_(0),
       download_prefs_(new DownloadPrefs(profile)) {
 }
 
 ChromeDownloadManagerDelegate::~ChromeDownloadManagerDelegate() {
 }
 
 void ChromeDownloadManagerDelegate::SetDownloadManager(DownloadManager* dm) {
   download_manager_ = dm;
   download_history_.reset(new DownloadHistory(profile_));
-  download_history_->Load(
-      base::Bind(&DownloadManager::OnPersistentStoreQueryComplete,
-                 base::Unretained(dm)));
+  if (profile_->GetOriginalProfile() == profile_) {

[Randy Smith (Not in Mondays), 2012/08/02 17:52:17]

Did you consider doing this at the DownloadHistory level?  What was the
arguments for conditionals in CDMD rather than a null DownloadHistory
implementation?

[benjhayden, 2012/08/13 15:08:09]

I think we agreed to keep the conditionals in CDMD because
DownloadHistory<Observer will reduce the number of calls from CDMD to DH to 2
(construction and CheckVisitedReferrerBefore). Let me know if I misremember.

[Randy Smith (Not in Mondays), 2012/08/13 18:52:58]

Yep, that's my memory too.

+    download_history_->Load(
+        base::Bind(&DownloadManager::OnPersistentStoreQueryComplete,
+                   download_manager_));

[Randy Smith (Not in Mondays), 2012/08/02 17:52:17]

I'm somewhat uncomfortable here.  I'll agree in advance with your (presumed :-})
argument that the lifetime guarantees for the history loading chain are not at
all clear, and thus that we're risking some future change resulting in a use
after free error.  But the lifetime guarantees are (I believe) there--I think
there's a cancellation between DownloadHistory and the DB thread to guarantee
that callbacks don't come back if download_history_ goes away (there at least
used to be).  And I think it's confusing to have multiple code that solves the
same problem.  So I'd rather cleanup the entire stack than just change this
point. 

An additional point is that I'd like to be moving towards fewer classes being
refcounted, and this relies on CDMD being refcounted.

Could you put in a TODO?  Either in place of this change ('lifetime guarantees
confusing; cleanup') or in addition to this change ('duplicates the effect of XX
in YY but is clearer in its effect; cleanup to have a single clear mechanism')?

(Applies throughout file for this change.)

[benjhayden, 2012/08/13 15:08:09]

This call is disappearing in my DownloadHistory CL.

A TODO for the other former Unretaineds seems unnecessary. We know that
RefCountedThreadSafe is bad. The compiler will let us know if we forget to
update a caller.

[Randy Smith (Not in Mondays), 2012/08/13 18:52:58]

The problem is someone (say a poor idealistic new grad--won't anyone think of
the children??) without background knowledge coming upon this code.  I imagine
they could waste a lot of time trying to figure out how to provide lifetime
guarantees to replace the loss of ref_counted, when they don't need to, because
the underlying code already provides those lifetime guarantees.  One line
comment where the Unretaineds used to be saying "lifetime guarantee also
provided by *Cancel* in DownloadHistory"?  (Or whatever it really should say--I
haven't looked at the code.)

In my ideal world we wouldn't use *Cancel* just because I don't think it's at
all obvious what it's doing; weak pointers strike me as a more understandable,
and more commonly known, lifetime protection mechanism.  That's why I originally
suggested a TODO.  But just some breadcrumb so that someone doesn't waste time
assuming the RCTS nature of CDMD is required at these locations when it isn't
would also be fine.

[benjhayden, 2012/08/13 20:26:24]

Done.

+  }
 #if !defined(OS_ANDROID)
   extension_event_router_.reset(new ExtensionDownloadsEventRouter(
       profile_, download_manager_));
 #endif
 }
 
 void ChromeDownloadManagerDelegate::Shutdown() {
   download_history_.reset();
   download_prefs_.reset();
 #if !defined(OS_ANDROID)
@@ skipping 269 matching lines @@
 #if defined(ENABLE_SAFE_BROWSING)
   return profile_->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled) &&
       g_browser_process->safe_browsing_service()->DownloadBinHashNeeded();
 #else
   return false;
 #endif
 }
 
 void ChromeDownloadManagerDelegate::AddItemToPersistentStore(
     DownloadItem* item) {
+  if (profile_->IsOffTheRecord()) {
+    OnItemAddedToPersistentStore(
+        item->GetId(), download_history_->GetNextFakeDbHandle());
+    return;
+  }
   download_history_->AddEntry(item,
       base::Bind(&ChromeDownloadManagerDelegate::OnItemAddedToPersistentStore,
-                 base::Unretained(this)));
+                 this));
 }
 
 void ChromeDownloadManagerDelegate::UpdateItemInPersistentStore(
     DownloadItem* item) {
   download_history_->UpdateEntry(item);
 }
 
 void ChromeDownloadManagerDelegate::UpdatePathForItemInPersistentStore(
     DownloadItem* item,
     const FilePath& new_path) {
   download_history_->UpdateDownloadPath(item, new_path);
 }
 
 void ChromeDownloadManagerDelegate::RemoveItemFromPersistentStore(
     DownloadItem* item) {
   download_history_->RemoveEntry(item);
 }
 
 void ChromeDownloadManagerDelegate::RemoveItemsFromPersistentStoreBetween(
     base::Time remove_begin,
     base::Time remove_end) {
+  if (profile_->IsOffTheRecord())
+    return;
   download_history_->RemoveEntriesBetween(remove_begin, remove_end);
 }
 
 void ChromeDownloadManagerDelegate::GetSaveDir(WebContents* web_contents,
                                                FilePath* website_save_dir,
                                                FilePath* download_save_dir,
                                                bool* skip_dir_check) {
   Profile* profile =
       Profile::FromBrowserContext(web_contents->GetBrowserContext());
   PrefService* prefs = profile->GetPrefs();
@@ skipping 110 matching lines @@
 
   VLOG(2) << __FUNCTION__ << "() download = " << download->DebugString(false)
           << " verdict = " << result;
   content::DownloadDangerType danger_type = download->GetDangerType();
   if (result != DownloadProtectionService::SAFE)
     danger_type = content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL;
 
   download_history_->CheckVisitedReferrerBefore(
       download_id, download->GetReferrerUrl(),
       base::Bind(&ChromeDownloadManagerDelegate::CheckVisitedReferrerBeforeDone,
-                 base::Unretained(this), download_id, callback, danger_type));
+                 this, download_id, callback, danger_type));
 }
 
 void ChromeDownloadManagerDelegate::CheckClientDownloadDone(
     int32 download_id,
     DownloadProtectionService::DownloadCheckResult result) {
   DownloadItem* item = download_manager_->GetActiveDownloadItem(download_id);
   if (!item)
     return;
 
   VLOG(2) << __FUNCTION__ << "() download = " << item->DebugString(false)
@@ skipping 228 matching lines @@
     int32 download_id, int64 db_handle) {
   // It's not immediately obvious, but HistoryBackend::CreateDownload() can
   // call this function with an invalid |db_handle|. For instance, this can
   // happen when the history database is offline. We cannot have multiple
   // DownloadItems with the same invalid db_handle, so we need to assign a
   // unique |db_handle| here.
   if (db_handle == DownloadItem::kUninitializedHandle)
     db_handle = download_history_->GetNextFakeDbHandle();
   download_manager_->OnItemAddedToPersistentStore(download_id, db_handle);
 }