Kill DownloadItem::IsOtr()

chrome/browser/download/chrome_download_manager_delegate.cc

Index: chrome/browser/download/chrome_download_manager_delegate.cc
diff --git a/chrome/browser/download/chrome_download_manager_delegate.cc b/chrome/browser/download/chrome_download_manager_delegate.cc
index fc95cbf4f129ef68b2e2c50d187f02548b1c81d1..c873c2eb9e4749bc5bf84cf63a72a01aead9aaaa 100644
--- a/chrome/browser/download/chrome_download_manager_delegate.cc
+++ b/chrome/browser/download/chrome_download_manager_delegate.cc
@@ -132,9 +132,11 @@ 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_));
@@ -424,9 +426,14 @@ bool ChromeDownloadManagerDelegate::GenerateFileHash() {
 
 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(
@@ -448,6 +455,8 @@ void ChromeDownloadManagerDelegate::RemoveItemFromPersistentStore(
 void ChromeDownloadManagerDelegate::RemoveItemsFromPersistentStoreBetween(
     base::Time remove_begin,
     base::Time remove_end) {
+  if (profile_->IsOffTheRecord())
+    return;
   download_history_->RemoveEntriesBetween(remove_begin, remove_end);
 }
 
@@ -578,7 +587,7 @@ void ChromeDownloadManagerDelegate::CheckDownloadUrlDone(
   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(