Make DownloadHistory observe manager, items

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"
 #include "base/callback.h"
 #include "base/file_util.h"
 #include "base/rand_util.h"
 #include "base/stringprintf.h"
 #include "base/time.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/api/prefs/pref_member.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/download/download_completion_blocker.h"
 #include "chrome/browser/download/download_crx_util.h"
 #include "chrome/browser/download/download_extensions.h"
 #include "chrome/browser/download/download_file_picker.h"
 #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"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/extensions/extension_switch_utils.h"
 #include "chrome/common/extensions/user_script.h"
 #include "chrome/common/pref_names.h"
 #include "content/public/browser/download_item.h"
@@ skipping 103 matching lines @@
     : profile_(profile),
       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_));
-  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() {
   download_history_.reset();
   download_prefs_.reset();
 #if !defined(OS_ANDROID)
   extension_event_router_.reset();
 #endif
 }
 
@@ skipping 302 matching lines @@
 
 bool ChromeDownloadManagerDelegate::GenerateFileHash() {
 #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,
-                 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,
                                                bool* skip_dir_check) {
   Profile* profile = Profile::FromBrowserContext(browser_context);
   PrefService* prefs = profile->GetPrefs();
 
   // Check whether the preference has the preferred directory for saving file.
   // If not, initialize it with default directory.
   if (!prefs->FindPreference(prefs::kSaveFileDefaultDirectory)) {
@@ skipping 92 matching lines @@
   DownloadPathReservationTracker::GetReservedPath(
       download, target_path, default_download_path, should_uniquify_path,
       callback);
 }
 
 void ChromeDownloadManagerDelegate::CheckDownloadUrlDone(
     int32 download_id,
     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)
           << " 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,
-                 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(
     int32 download_id,
     DownloadProtectionService::DownloadCheckResult result) {
   DownloadItem* item = download_manager_->GetActiveDownloadItem(download_id);
   if (!item)
     return;
 
   VLOG(2) << __FUNCTION__ << "() download = " << item->DebugString(false)
@@ skipping 224 matching lines @@
     // TODO(asanka): This logic is a hack. DownloadFilePicker should give us a
     //               directory to persist. Or perhaps, if the Drive path
     //               substitution logic is moved here, then we would have a
     //               persistable path after the DownloadFilePicker is done.
     if (disposition == DownloadItem::TARGET_DISPOSITION_PROMPT &&
         !download->IsTemporary())
       last_download_path_ = target_path.DirName();
   }
   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);
-}