Make DownloadHistory observe manager, items

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 65f2666307b77ddab18cc5a470540273f5b8fb93..c7867c71d0fd9d91464aedf0f7bd158dc78ab61e 100644
--- a/chrome/browser/download/chrome_download_manager_delegate.cc
+++ b/chrome/browser/download/chrome_download_manager_delegate.cc
@@ -23,12 +23,16 @@
 #include "chrome/browser/download/download_history.h"
 #include "chrome/browser/download/download_path_reservation_tracker.h"
 #include "chrome/browser/download/download_prefs.h"
+#include "chrome/browser/download/download_service.h"
+#include "chrome/browser/download/download_service_factory.h"
 #include "chrome/browser/download/download_status_updater.h"
 #include "chrome/browser/download/download_util.h"
 #include "chrome/browser/download/save_package_file_picker.h"
 #include "chrome/browser/extensions/api/downloads/downloads_api.h"
 #include "chrome/browser/extensions/crx_installer.h"
 #include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/history/history.h"
+#include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/intents/web_intents_util.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
@@ -152,22 +156,29 @@ ChromeDownloadManagerDelegate::~ChromeDownloadManagerDelegate() {
 
 void ChromeDownloadManagerDelegate::SetDownloadManager(DownloadManager* dm) {
   download_manager_ = dm;
-  download_history_.reset(new DownloadHistory(profile_));
-  if (!profile_->IsOffTheRecord()) {
-    // DownloadManager should not be RefCountedThreadSafe.
-    // ChromeDownloadManagerDelegate outlives DownloadManager, and
-    // DownloadHistory uses a scoped canceller to cancel tasks when it is
-    // deleted. Almost all callbacks to DownloadManager should use weak pointers
-    // or bounce off a container object that uses ManagerGoingDown() to simulate
-    // a weak pointer.
-    download_history_->Load(
-        base::Bind(&DownloadManager::OnPersistentStoreQueryComplete,
-                   download_manager_));
-  }
 #if !defined(OS_ANDROID)
   extension_event_router_.reset(new ExtensionDownloadsEventRouter(
       profile_, download_manager_));
 #endif
+
+  if (!profile_->IsOffTheRecord()) {
+    HistoryService* hs = HistoryServiceFactory::GetForProfile(
+        profile_, Profile::EXPLICIT_ACCESS);
+    if (hs)
+      download_history_.reset(new DownloadHistory(download_manager_, hs));

[Randy Smith (Not in Mondays), 2012/09/24 18:03:25]

Why here rather than on DownloadService?  CDMD is intended as an embedder
appendage to the DM, and while it's admittedly grown a bit beyond that, I don't
think we need to add to it.  How ugly would the code be if
CheckVisitedReferrerBeforeDone indirected through the DownloadService?

[benjhayden, 2012/11/02 17:21:37]

Hm. How would you feel about making CDMD::CheckVisitedReferrerBefore access the
HistoryService directly instead of bouncing through DownloadHistory? CVRB
doesn't really belong with everything else that DH does.

[Randy Smith (Not in Mondays), 2012/11/02 23:31:24]

I'd be good with that.

[benjhayden, 2012/11/06 20:01:14]

Done.

+  }
+}
+
+void ChromeDownloadManagerDelegate::AddHistoryObserver(
+    DownloadHistory::Observer* observer) {
+  if (download_history_.get())
+    download_history_->AddObserver(observer);
+}
+
+void ChromeDownloadManagerDelegate::RemoveHistoryObserver(
+    DownloadHistory::Observer* observer) {
+  if (download_history_.get())
+    download_history_->RemoveObserver(observer);
 }
 
 void ChromeDownloadManagerDelegate::Shutdown() {
@@ -490,42 +501,6 @@ bool ChromeDownloadManagerDelegate::GenerateFileHash() {
 #endif
 }
 
-void ChromeDownloadManagerDelegate::AddItemToPersistentStore(
-    DownloadItem* item) {
-  if (profile_->IsOffTheRecord()) {
-    OnItemAddedToPersistentStore(
-        item->GetId(), download_history_->GetNextFakeDbHandle());
-    return;
-  }
-  download_history_->AddEntry(item,
-      base::Bind(&ChromeDownloadManagerDelegate::OnItemAddedToPersistentStore,
-                 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(BrowserContext* browser_context,
                                                FilePath* website_save_dir,
                                                FilePath* download_save_dir,
@@ -638,9 +613,8 @@ void ChromeDownloadManagerDelegate::CheckDownloadUrlDone(
     const content::DownloadTargetCallback& callback,
     DownloadProtectionService::DownloadCheckResult result) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  DownloadItem* download =
-      download_manager_->GetActiveDownloadItem(download_id);
-  if (!download)
+  DownloadItem* download = download_manager_->GetDownload(download_id);
+  if (!download || (download->GetState() != DownloadItem::IN_PROGRESS))
     return;
 
   VLOG(2) << __FUNCTION__ << "() download = " << download->DebugString(false)
@@ -649,10 +623,26 @@ void ChromeDownloadManagerDelegate::CheckDownloadUrlDone(
   if (result != DownloadProtectionService::SAFE)
     danger_type = content::DOWNLOAD_DANGER_TYPE_DANGEROUS_URL;
 
-  download_history_->CheckVisitedReferrerBefore(
-      download_id, download->GetReferrerUrl(),
-      base::Bind(&ChromeDownloadManagerDelegate::CheckVisitedReferrerBeforeDone,
-                 this, download_id, callback, danger_type));
+  DownloadHistory* history = download_history_.get();
+  if (!history && profile_->IsOffTheRecord()) {
+    // If there's no DownloadHistory because this profile IsOffTheRecord, then
+    // go to the original profile and ask its ChromeDownloadManagerDelegate to
+    // check the on-record visited db.
+    history = DownloadServiceFactory::GetForProfile(
+        profile_->GetOriginalProfile())->GetDownloadManagerDelegate()
+          ->download_history_.get();
+  }
+  if (history) {
+    history->CheckVisitedReferrerBefore(
+        download->GetReferrerUrl(), base::Bind(
+            &ChromeDownloadManagerDelegate::CheckVisitedReferrerBeforeDone,
+            this, download_id, callback, danger_type));
+    return;
+  }
+  // If there's no history but the profile is on-record, or if this profile is

[Randy Smith (Not in Mondays), 2012/09/24 18:03:25]

Are there any cases in which we're on-record and have no history?  What are
they?  I had thought that we had no history iff we were OTR; if that's true,
this seems like more complex logic than needed.

[benjhayden, 2012/11/02 17:21:37]

My understanding is that there is some rare case where we may be on-record
without a history. Maybe it's only in some tests, maybe there's a race window, I
forget exactly.
Either way, a couple branches and a fall-back case doesn't seem terribly
complicated to me. 
I added a DCHECK, so we'll see if it explodes on trybots.

[Randy Smith (Not in Mondays), 2012/11/02 23:31:24]

Cool, thanks. 

WRT the couple of branches & fall-back case: It's the death by a thousand cuts. 
Code that isn't used, rots, and takes up memory size, and makes it harder for
other people to understand the working code than they need to.  If we can get
rid of this & replace it with a DCHECK, I'd like to.  

[benjhayden, 2012/11/06 20:01:14]

The DCHECK exploded in unit_tests:
ChromeDownloadManagerDelegateTest.StartDownload_Basic.
Added a comment that unit_tests do not have a HistoryService.

+  // off-record and the original profile doesn't have a DownloadHistory, then
+  // give up and assume the referrer has not been visited before.
+  CheckVisitedReferrerBeforeDone(download_id, callback, danger_type, false);

[Randy Smith (Not in Mondays), 2012/09/24 18:03:25]

nit, suggestion: I have a slight preference for the "if" branch being the
uncommon case, and the least indented code being the common case, but I'm ok
with either way.

[benjhayden, 2012/11/02 17:21:37]

Done.

 }
 
 void ChromeDownloadManagerDelegate::CheckClientDownloadDone(
@@ -897,15 +887,3 @@ void ChromeDownloadManagerDelegate::OnTargetPathDetermined(
   }
   callback.Run(target_path, disposition, danger_type, intermediate_path);
 }
-
-void ChromeDownloadManagerDelegate::OnItemAddedToPersistentStore(
-    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);
-}