Make DownloadHistory observe manager, items

content/browser/download/download_item_impl.cc

Index: content/browser/download/download_item_impl.cc
diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc
index 72159e425e25f81b1872f58c51a5d71f36eea475..ccc1193e52ca0b382a07f9e9b3ebad962c5c5e9c 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -45,7 +45,6 @@
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/content_browser_client.h"
-#include "content/public/browser/download_persistent_store_info.h"
 #include "net/base/net_util.h"
 
 using content::BrowserThread;
@@ -53,7 +52,6 @@ using content::DownloadFile;
 using content::DownloadId;
 using content::DownloadItem;
 using content::DownloadManager;
-using content::DownloadPersistentStoreInfo;
 using content::WebContents;
 
 namespace {
@@ -108,55 +106,49 @@ class NullDownloadRequestHandle : public DownloadRequestHandleInterface {
 
 namespace content {
 
-// Our download table ID starts at 1, so we use 0 to represent a download that
-// has started, but has not yet had its data persisted in the table. We use fake
-// database handles in incognito mode starting at -1 and progressively getting
-// more negative.
-// static
-const int DownloadItem::kUninitializedHandle = 0;
-
 const char DownloadItem::kEmptyFileHash[] = "";
 
 }
 
-// Our download table ID starts at 1, so we use 0 to represent a download that
-// has started, but has not yet had its data persisted in the table. We use fake
-// database handles in incognito mode starting at -1 and progressively getting
-// more negative.
-
 // Constructor for reading from the history service.
 DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate,
                                    DownloadId download_id,
-                                   const DownloadPersistentStoreInfo& info,
+                                   const FilePath& path,
+                                   const GURL& url,
+                                   const GURL& referrer_url,
+                                   const base::Time& start_time,
+                                   const base::Time& end_time,
+                                   int64 received_bytes,
+                                   int64 total_bytes,
+                                   DownloadItem::DownloadState state,
+                                   bool opened,
                                    const net::BoundNetLog& bound_net_log)
     : download_id_(download_id),
-      current_path_(info.path),
-      target_path_(info.path),
+      current_path_(path),
+      target_path_(path),
       target_disposition_(TARGET_DISPOSITION_OVERWRITE),
-      url_chain_(1, info.url),
-      referrer_url_(info.referrer_url),
+      url_chain_(1, url),
+      referrer_url_(referrer_url),
       transition_type_(content::PAGE_TRANSITION_LINK),
       has_user_gesture_(false),
-      total_bytes_(info.total_bytes),
-      received_bytes_(info.received_bytes),
+      total_bytes_(total_bytes),
+      received_bytes_(received_bytes),
       bytes_per_sec_(0),
       last_reason_(content::DOWNLOAD_INTERRUPT_REASON_NONE),
       start_tick_(base::TimeTicks()),
-      state_(ExternalToInternalState(info.state)),
+      state_(ExternalToInternalState(state)),
       danger_type_(content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS),
-      start_time_(info.start_time),
-      end_time_(info.end_time),
-      db_handle_(info.db_handle),
+      start_time_(start_time),
+      end_time_(end_time),
       delegate_(delegate),
       is_paused_(false),
       open_when_complete_(false),
       file_externally_removed_(false),
       safety_state_(SAFE),
       auto_opened_(false),
-      is_persisted_(true),
       is_temporary_(false),
       all_data_saved_(false),
-      opened_(info.opened),
+      opened_(opened),
       open_enabled_(true),
       delegate_delayed_complete_(false),
       bound_net_log_(bound_net_log),
@@ -200,14 +192,12 @@ DownloadItemImpl::DownloadItemImpl(
       state_(IN_PROGRESS_INTERNAL),
       danger_type_(content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS),
       start_time_(info.start_time),
-      db_handle_(DownloadItem::kUninitializedHandle),
       delegate_(delegate),
       is_paused_(false),
       open_when_complete_(false),
       file_externally_removed_(false),
       safety_state_(SAFE),
       auto_opened_(false),
-      is_persisted_(false),
       is_temporary_(!info.save_info.file_path.empty()),
       all_data_saved_(false),
       opened_(false),
@@ -255,14 +245,12 @@ DownloadItemImpl::DownloadItemImpl(DownloadItemImplDelegate* delegate,
       state_(IN_PROGRESS_INTERNAL),
       danger_type_(content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS),
       start_time_(base::Time::Now()),
-      db_handle_(DownloadItem::kUninitializedHandle),
       delegate_(delegate),
       is_paused_(false),
       open_when_complete_(false),
       file_externally_removed_(false),
       safety_state_(SAFE),
       auto_opened_(false),
-      is_persisted_(false),
       is_temporary_(false),
       all_data_saved_(false),
       opened_(false),
@@ -441,10 +429,6 @@ DownloadId DownloadItemImpl::GetGlobalId() const {
   return download_id_;
 }
 
-int64 DownloadItemImpl::GetDbHandle() const {
-  return db_handle_;
-}
-
 DownloadItem::DownloadState DownloadItemImpl::GetState() const {
   return InternalToExternalState(state_);
 }
@@ -461,10 +445,6 @@ bool DownloadItemImpl::IsTemporary() const {
   return is_temporary_;
 }
 
-bool DownloadItemImpl::IsPersisted() const {
-  return is_persisted_;
-}
-
 // TODO(ahendrickson) -- Move |INTERRUPTED| from |IsCancelled()| to
 // |IsPartialDownload()|, when resuming interrupted downloads is implemented.
 bool DownloadItemImpl::IsPartialDownload() const {
@@ -681,20 +661,6 @@ bool DownloadItemImpl::GetOpened() const {
   return opened_;
 }
 
-DownloadPersistentStoreInfo DownloadItemImpl::GetPersistentStoreInfo() const {
-  // TODO(asanka): Persist GetTargetFilePath() as well.
-  return DownloadPersistentStoreInfo(GetFullPath(),
-                                     GetURL(),
-                                     GetReferrerUrl(),
-                                     GetStartTime(),
-                                     GetEndTime(),
-                                     GetReceivedBytes(),
-                                     GetTotalBytes(),
-                                     GetState(),
-                                     GetDbHandle(),
-                                     GetOpened());
-}
-
 content::BrowserContext* DownloadItemImpl::GetBrowserContext() const {
   return delegate_->GetBrowserContext();
 }
@@ -756,7 +722,6 @@ std::string DownloadItemImpl::DebugString(bool verbose) const {
 
   if (verbose) {
     description += base::StringPrintf(
-        " db_handle = %" PRId64
         " total = %" PRId64
         " received = %" PRId64
         " reason = %s"
@@ -767,7 +732,6 @@ std::string DownloadItemImpl::DebugString(bool verbose) const {
         " url_chain = \n\t\"%s\"\n\t"
         " full_path = \"%" PRFilePath "\""
         " target_path = \"%" PRFilePath "\"",
-        GetDbHandle(),
         GetTotalBytes(),
         GetReceivedBytes(),
         InterruptReasonDebugString(last_reason_).c_str(),
@@ -892,19 +856,6 @@ void DownloadItemImpl::MarkAsComplete() {
   TransitionTo(COMPLETE_INTERNAL);
 }
 
-void DownloadItemImpl::SetIsPersisted() {
-  is_persisted_ = true;
-  UpdateObservers();
-}
-
-void DownloadItemImpl::SetDbHandle(int64 handle) {
-  db_handle_ = handle;
-
-  bound_net_log_.AddEvent(
-      net::NetLog::TYPE_DOWNLOAD_ITEM_IN_HISTORY,
-      net::NetLog::Int64Callback("db_handle", db_handle_));
-}
-
 // **** Download progression cascade
 
 void DownloadItemImpl::Init(bool active,
@@ -931,18 +882,15 @@ void DownloadItemImpl::Init(bool active,
       file_name = GetURL().ExtractFileName();
   }
 
-  bound_net_log_.BeginEvent(
-      net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE,
-      base::Bind(&download_net_logs::ItemActivatedCallback,
-                 this, download_type, &file_name));
-
-  // If this is not an active download, end the ACTIVE event now.
-  if (!active) {
+  base::Callback<base::Value*(net::NetLog::LogLevel)> active_data = base::Bind(
+      &download_net_logs::ItemActivatedCallback, this,
+      download_type, &file_name);
+  if (active) {
+    bound_net_log_.BeginEvent(
+        net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE, active_data);
+  } else {
     bound_net_log_.AddEvent(

[asanka, 2012/09/28 19:02:30]

EndEvent()

[benjhayden, 2012/10/01 14:10:03]

Can you elaborate why?
This is Init(), which is only called when downloads are created, so there was no
corresponding BeginEvent. I'd expect it to be incorrect to call EndEvent with no
corresponding BeginEvent.

[asanka, 2012/10/01 15:59:36]

Ignore my comment. I misread.

-        net::NetLog::TYPE_DOWNLOAD_ITEM_IN_HISTORY,
-        net::NetLog::Int64Callback("db_handle", db_handle_));
-
-    bound_net_log_.EndEvent(net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE);
+        net::NetLog::TYPE_DOWNLOAD_ITEM_ACTIVE, active_data);
   }
 
   VLOG(20) << __FUNCTION__ << "() " << DebugString(true);
@@ -1079,7 +1027,7 @@ void DownloadItemImpl::OnDownloadRenamedToFinalName(
   DCHECK(!full_path.empty());
   target_path_ = full_path;
   SetFullPath(full_path);
-  delegate_->DownloadRenamedToFinalName(this);
+  UpdateObservers();

[asanka, 2012/09/28 19:02:30]

Given that with this change we always call UpdateObservers() after
SetFullPath(), we might as well fold the call into SetFullPath().

[benjhayden, 2012/10/01 14:10:03]

Done.

 
   // Complete the download and release the DownloadFile.
   BrowserThread::PostTask(