Caching some persistent data in DownloadItem

Currently DownloadDB records some of the persistent for DownloadItem.
And DownloadItem will constantly requesting these data from the
DownloadDBCache during its life cycle. Which is unnecessary as these
data are already passed to DownloadItem during the creation time.
This CL allows DownloadItem to cache those persistent data after
creation. So there is no longer any need to request them again, and
we can remove them from the DownloadDBCache in future CLs to save
memory.

BUG=893651

Change-Id: Ic360bd81e283272105ef9d43d91e2b8dd00d9145
Reviewed-on: https://chromium-review.googlesource.com/c/1338328Reviewed-by: Xing Liu <xingliu@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Commit-Queue: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608949}

components/download/internal/common/download_db_cache.cc

@@ -56,45 +56,6 @@ ShouldUpdateDownloadDBResult ShouldUpdateDownloadDB(
   return ShouldUpdateDownloadDBResult::UPDATE;
 }
 
-bool GetFetchErrorBody(base::Optional<DownloadDBEntry> entry) {
-  if (!entry)
-    return false;
-
-  if (!entry->download_info)
-    return false;
-
-  base::Optional<InProgressInfo>& in_progress_info =
-      entry->download_info->in_progress_info;
-  if (!in_progress_info)
-    return false;
-
-  return in_progress_info->fetch_error_body;
-}
-
-DownloadUrlParameters::RequestHeadersType GetRequestHeadersType(
-    base::Optional<DownloadDBEntry> entry) {
-  DownloadUrlParameters::RequestHeadersType ret;
-  if (!entry)
-    return ret;
-
-  if (!entry->download_info)
-    return ret;
-
-  base::Optional<InProgressInfo>& in_progress_info =
-      entry->download_info->in_progress_info;
-  if (!in_progress_info)
-    return ret;
-
-  return in_progress_info->request_headers;
-}
-
-UkmInfo GetUkmInfo(base::Optional<DownloadDBEntry> entry) {
-  if (entry && entry->download_info && entry->download_info->ukm_info)
-    return entry->download_info->ukm_info.value();
-
-  return UkmInfo(DownloadSource::UNKNOWN, GetUniqueDownloadId());
-}
-
 void CleanUpInProgressEntry(DownloadDBEntry* entry) {
   if (!entry->download_info)
     return;
@@ -213,13 +174,8 @@ void DownloadDBCache::OnDownloadUpdated(DownloadItem* download) {
       return;
     }
   }
-  base::Optional<DownloadDBEntry> current = RetrieveEntry(download->GetGuid());
-  bool fetch_error_body = GetFetchErrorBody(current);
-  DownloadUrlParameters::RequestHeadersType request_header_type =
-      GetRequestHeadersType(current);
-  UkmInfo ukm_info = GetUkmInfo(current);
   DownloadDBEntry entry = CreateDownloadDBEntryFromItem(
-      *download, ukm_info, fetch_error_body, request_header_type);
+      *(static_cast<DownloadItemImpl*>(download)));
   AddOrReplaceEntry(entry);
 }
 

components/download/internal/common/download_item_impl.cc

@@ -1304,17 +1304,24 @@ void DownloadItemImpl::Init(bool active,
   if (active) {
     TRACE_EVENT_ASYNC_BEGIN1("download", "DownloadItemActive", download_id_,
                              "download_item", std::move(active_data));
+    ukm_download_id_ = GetUniqueDownloadId();
   } else {
     TRACE_EVENT_INSTANT1("download", "DownloadItemActive",
                          TRACE_EVENT_SCOPE_THREAD, "download_item",
                          std::move(active_data));
 
     // Read data from in-progress cache.
+    // TODO(qinmin): Remove this once we initialize the data in DownloadItemImpl
+    // ctor.
     auto in_progress_entry = delegate_->GetInProgressEntry(this);
     if (in_progress_entry) {
       download_source_ = in_progress_entry->download_source;
       fetch_error_body_ = in_progress_entry->fetch_error_body;
       request_headers_ = in_progress_entry->request_headers;
+      ukm_download_id_ = in_progress_entry->ukm_download_id;
+      bytes_wasted_ = in_progress_entry->bytes_wasted;
+    } else {
+      ukm_download_id_ = GetUniqueDownloadId();
     }
   }
 
@@ -1393,17 +1400,14 @@ void DownloadItemImpl::Start(
     }
     RecordDownloadMimeType(mime_type_);
     DownloadContent file_type = DownloadContentFromMimeType(mime_type_, false);
-    auto in_progress_entry = delegate_->GetInProgressEntry(this);
-    if (in_progress_entry) {
-      bool is_same_host_download =
-          base::StringPiece(new_create_info.url().host())
-              .ends_with(new_create_info.site_url.host());
-      DownloadConnectionSecurity state = CheckDownloadConnectionSecurity(
-          new_create_info.url(), new_create_info.url_chain);
-      DownloadUkmHelper::RecordDownloadStarted(
-          in_progress_entry->ukm_download_id, new_create_info.ukm_source_id,
-          file_type, download_source_, state, is_same_host_download);
-    }
+    bool is_same_host_download =
+        base::StringPiece(new_create_info.url().host())
+            .ends_with(new_create_info.site_url.host());
+    DownloadConnectionSecurity state = CheckDownloadConnectionSecurity(
+        new_create_info.url(), new_create_info.url_chain);
+    DownloadUkmHelper::RecordDownloadStarted(
+        ukm_download_id_, new_create_info.ukm_source_id, file_type,
+        download_source_, state, is_same_host_download);
 
     if (!delegate_->IsOffTheRecord()) {
       RecordDownloadCountWithSource(NEW_DOWNLOAD_COUNT_NORMAL_PROFILE,
@@ -1445,13 +1449,8 @@ void DownloadItemImpl::OnDownloadFileInitialized(DownloadInterruptReason result,
             << "() result:" << DownloadInterruptReasonToString(result);
 
   if (bytes_wasted > 0) {
-    bytes_wasted_ = bytes_wasted;
-    auto in_progress_entry = delegate_->GetInProgressEntry(this);
-    if (in_progress_entry.has_value()) {
-      DownloadEntry entry = in_progress_entry.value();
-      bytes_wasted_ = entry.bytes_wasted + bytes_wasted;
-      delegate_->ReportBytesWasted(this);
-    }
+    bytes_wasted_ += bytes_wasted;
+    delegate_->ReportBytesWasted(this);
   }
 
   // Handle download interrupt reason.
@@ -1786,12 +1785,8 @@ void DownloadItemImpl::Completed() {
   // If all data is saved, the number of received bytes is resulting file size.
   int resulting_file_size = GetReceivedBytes();
 
-  auto in_progress_entry = delegate_->GetInProgressEntry(this);
-  if (in_progress_entry) {
-    DownloadUkmHelper::RecordDownloadCompleted(
-        in_progress_entry->ukm_download_id, resulting_file_size,
-        time_since_start, in_progress_entry->bytes_wasted);
-  }
+  DownloadUkmHelper::RecordDownloadCompleted(
+      ukm_download_id_, resulting_file_size, time_since_start, bytes_wasted_);
 
   // After all of the records are done, then update the observers.
   UpdateObservers();
@@ -1942,17 +1937,14 @@ void DownloadItemImpl::InterruptWithPartialState(
 
   base::TimeDelta time_since_start = base::Time::Now() - GetStartTime();
   int resulting_file_size = GetReceivedBytes();
-  auto in_progress_entry = delegate_->GetInProgressEntry(this);
   base::Optional<int> change_in_file_size;
-  if (in_progress_entry) {
-    if (total_bytes_ >= 0) {
-      change_in_file_size = total_bytes_ - resulting_file_size;
-    }
-
-    DownloadUkmHelper::RecordDownloadInterrupted(
-        in_progress_entry->ukm_download_id, change_in_file_size, reason,
-        resulting_file_size, time_since_start, in_progress_entry->bytes_wasted);
+  if (total_bytes_ >= 0) {
+    change_in_file_size = total_bytes_ - resulting_file_size;
   }
+
+  DownloadUkmHelper::RecordDownloadInterrupted(
+      ukm_download_id_, change_in_file_size, reason, resulting_file_size,
+      time_since_start, bytes_wasted_);
   if (reason == DOWNLOAD_INTERRUPT_REASON_SERVER_CONTENT_LENGTH_MISMATCH) {
     received_bytes_at_length_mismatch_ = GetReceivedBytes();
   }
@@ -2295,10 +2287,6 @@ void DownloadItemImpl::ResumeInterruptedDownload(
   // involve any compression,
   download_params->add_request_header("Accept-Encoding", "identity");
 
-  auto entry = delegate_->GetInProgressEntry(this);
-  if (entry)
-    download_params->set_request_origin(entry.value().request_origin);
-
   // Note that resumed downloads disallow redirects. Hence the referrer URL
   // (which is the contents of the Referer header for the last download request)
   // will only be sent to the URL returned by GetURL().
@@ -2313,11 +2301,9 @@ void DownloadItemImpl::ResumeInterruptedDownload(
                                 download_source_);
 
   base::TimeDelta time_since_start = base::Time::Now() - GetStartTime();
-  auto in_progress_entry = delegate_->GetInProgressEntry(this);
-  if (in_progress_entry) {
-    DownloadUkmHelper::RecordDownloadResumed(in_progress_entry->ukm_download_id,
-                                             GetResumeMode(), time_since_start);
-  }
+
+  DownloadUkmHelper::RecordDownloadResumed(ukm_download_id_, GetResumeMode(),
+                                           time_since_start);
 
   delegate_->ResumeInterruptedDownload(std::move(download_params),
                                        request_info_.site_url);

components/download/internal/common/download_utils.cc

@@ -10,7 +10,6 @@
 #include "base/strings/stringprintf.h"
 #include "components/download/public/common/download_create_info.h"
 #include "components/download/public/common/download_interrupt_reasons_utils.h"
-#include "components/download/public/common/download_item.h"
 #include "components/download/public/common/download_save_info.h"
 #include "components/download/public/common/download_stats.h"
 #include "components/download/public/common/download_task_runner.h"
@@ -350,22 +349,7 @@ std::unique_ptr<net::HttpRequestHeaders> GetAdditionalRequestHeaders(
   return headers;
 }
 
-DownloadEntry CreateDownloadEntryFromItem(
-    const DownloadItem& item,
-    const std::string& request_origin,
-    DownloadSource download_source,
-    bool fetch_error_body,
-    const DownloadUrlParameters::RequestHeadersType& request_headers) {
-  return DownloadEntry(item.GetGuid(), request_origin, download_source,
-                       fetch_error_body, request_headers,
-                       GetUniqueDownloadId());
-}
-
-DownloadDBEntry CreateDownloadDBEntryFromItem(
-    const DownloadItem& item,
-    const UkmInfo& ukm_info,
-    bool fetch_error_body,
-    const DownloadUrlParameters::RequestHeadersType& request_headers) {
+DownloadDBEntry CreateDownloadDBEntryFromItem(const DownloadItemImpl& item) {
   DownloadDBEntry entry;
   DownloadInfo download_info;
   download_info.guid = item.GetGuid();
@@ -376,8 +360,8 @@ DownloadDBEntry CreateDownloadDBEntryFromItem(
   in_progress_info.site_url = item.GetSiteUrl();
   in_progress_info.tab_url = item.GetTabUrl();
   in_progress_info.tab_referrer_url = item.GetTabReferrerUrl();
-  in_progress_info.fetch_error_body = fetch_error_body;
-  in_progress_info.request_headers = request_headers;
+  in_progress_info.fetch_error_body = item.fetch_error_body();
+  in_progress_info.request_headers = item.request_headers();
   in_progress_info.etag = item.GetETag();
   in_progress_info.last_modified = item.GetLastModifiedTime();
   in_progress_info.mime_type = item.GetMimeType();
@@ -399,7 +383,8 @@ DownloadDBEntry CreateDownloadDBEntryFromItem(
 
   download_info.in_progress_info = in_progress_info;
 
-  download_info.ukm_info = ukm_info;
+  download_info.ukm_info =
+      UkmInfo(item.download_source(), item.ukm_download_id());
   entry.download_info = download_info;
   return entry;
 }

components/download/internal/common/in_progress_download_manager.cc

@@ -355,9 +355,8 @@ void InProgressDownloadManager::StartDownloadWithItem(
     base::Optional<DownloadDBEntry> entry_opt =
         download_db_cache_->RetrieveEntry(download->GetGuid());
     if (!entry_opt.has_value()) {
-      download_db_cache_->AddOrReplaceEntry(CreateDownloadDBEntryFromItem(
-          *download, UkmInfo(info->download_source, GetUniqueDownloadId()),
-          info->fetch_error_body, info->request_headers));
+      download_db_cache_->AddOrReplaceEntry(
+          CreateDownloadDBEntryFromItem(*download));
     }
     download->RemoveObserver(download_db_cache_.get());
     download->AddObserver(download_db_cache_.get());

components/download/public/common/download_item_impl.h

@@ -329,8 +329,6 @@ class COMPONENTS_DOWNLOAD_EXPORT DownloadItemImpl
   // should be considered complete.
   virtual void MarkAsComplete();
 
-  DownloadSource download_source() const { return download_source_; }
-
   // DownloadDestinationObserver
   void DestinationUpdate(
       int64_t bytes_so_far,
@@ -346,6 +344,16 @@ class COMPONENTS_DOWNLOAD_EXPORT DownloadItemImpl
 
   void SetDelegate(DownloadItemImplDelegate* delegate);
 
+  const DownloadUrlParameters::RequestHeadersType& request_headers() const {
+    return request_headers_;
+  }
+
+  bool fetch_error_body() const { return fetch_error_body_; }
+
+  DownloadSource download_source() const { return download_source_; }
+
+  uint64_t ukm_download_id() const { return ukm_download_id_; }
+
  private:
   // Fine grained states of a download.
   //
@@ -774,6 +782,9 @@ class COMPONENTS_DOWNLOAD_EXPORT DownloadItemImpl
   DownloadCreationType download_type_ =
       DownloadCreationType::TYPE_ACTIVE_DOWNLOAD;
 
+  // UKM ID for reporting, default to 0 if uninitialized.
+  uint64_t ukm_download_id_ = 0;
+
   THREAD_CHECKER(thread_checker_);
 
   base::WeakPtrFactory<DownloadItemImpl> weak_ptr_factory_;

components/download/public/common/download_utils.h

@@ -10,6 +10,7 @@
 #include "components/download/public/common/download_export.h"
 #include "components/download/public/common/download_interrupt_reasons.h"
 #include "components/download/public/common/download_item.h"
+#include "components/download/public/common/download_item_impl.h"
 #include "components/download/public/common/download_source.h"
 #include "components/download/public/common/resume_mode.h"
 #include "net/base/net_errors.h"
@@ -67,20 +68,9 @@ COMPONENTS_DOWNLOAD_EXPORT int GetLoadFlags(DownloadUrlParameters* params,
 COMPONENTS_DOWNLOAD_EXPORT std::unique_ptr<net::HttpRequestHeaders>
 GetAdditionalRequestHeaders(DownloadUrlParameters* params);
 
-// Helper functions for DownloadItem -> DownloadEntry for InProgressCache.
-COMPONENTS_DOWNLOAD_EXPORT DownloadEntry CreateDownloadEntryFromItem(
-    const DownloadItem& item,
-    const std::string& request_origin,
-    DownloadSource download_source,
-    bool fetch_error_body,
-    const DownloadUrlParameters::RequestHeadersType& request_headers);
-
 // Helper functions for DownloadItem -> DownloadDBEntry for DownloadDB.
-COMPONENTS_DOWNLOAD_EXPORT DownloadDBEntry CreateDownloadDBEntryFromItem(
-    const DownloadItem& item,
-    const UkmInfo& ukm_info,
-    bool fetch_error_body,
-    const DownloadUrlParameters::RequestHeadersType& request_headers);
+COMPONENTS_DOWNLOAD_EXPORT DownloadDBEntry
+CreateDownloadDBEntryFromItem(const DownloadItemImpl& item);
 
 // Helper function to convert DownloadDBEntry to DownloadEntry.
 // TODO(qinmin): remove this function after DownloadEntry is deprecated.