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 528520cca56f5f5738c2a41d3ae1c7ba98965197..bae9b0890e6ed88e72cb2e1f56cc9db931a06924 100644
--- a/chrome/browser/download/chrome_download_manager_delegate.cc
+++ b/chrome/browser/download/chrome_download_manager_delegate.cc
@@ -29,6 +29,7 @@
 #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_service_factory.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
@@ -151,22 +152,17 @@ 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));
+  }
 }
 
 void ChromeDownloadManagerDelegate::Shutdown() {
@@ -475,42 +471,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(WebContents* web_contents,
                                                FilePath* website_save_dir,
                                                FilePath* download_save_dir,
@@ -635,10 +595,14 @@ 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));
+  if (download_history_.get()) {

[Randy Smith (Not in Mondays), 2012/09/11 18:36:53]

I believe this changes behavior; do you agree?  (Key question.)

If it does change behavior, I'm inclined to think it's not a behavior change we
want.  As I understand it, incognito mode is allowed to "see" everything about
non-incognito mode, it simply shouldn't leave any footprints.  So if we've
visited this website previously in non-incognito mode, we should be able to find
that out.

If it doesn't change behavior, than I'm sure someone smarter than me had a
reason why we shouldn't see previous visits in non-incognito mode :-}.

[benjhayden, 2012/09/13 15:18:16]

This does not change behavior.
Before this CL, DownloadHistory::CheckVisitedReferrerBefore() immediately passes
false to the callback (CheckVisitedReferrerBeforeDone()) if HistoryService is
unavailable.
I don't understand.

[Randy Smith (Not in Mondays), 2012/09/13 19:53:19]

As discussed offline: The original code does appear to redirect to the original
(non-incognito) profile for this query if we're in incognito mode, and we should
too.

+    download_history_->CheckVisitedReferrerBefore(
+        download_id, download->GetReferrerUrl(), base::Bind(
+            &ChromeDownloadManagerDelegate::CheckVisitedReferrerBeforeDone,
+            this, download_id, callback, danger_type));
+  } else {
+    CheckVisitedReferrerBeforeDone(download_id, callback, danger_type, false);
+  }
 }
 
 void ChromeDownloadManagerDelegate::CheckClientDownloadDone(
@@ -883,15 +847,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);
-}