Reduced the lifetime of DownloadCreateInfo.

chrome/browser/download/download_item.cc

Index: chrome/browser/download/download_item.cc
diff --git a/chrome/browser/download/download_item.cc b/chrome/browser/download/download_item.cc
index 59126a86d43769586c96fbf06678a0fd2ce9b69c..2157a9e4cb3f5ede4f8542d10813ded634f93c14 100644
--- a/chrome/browser/download/download_item.cc
+++ b/chrome/browser/download/download_item.cc
@@ -14,13 +14,13 @@
 #include "base/timer.h"
 #include "base/utf_string_conversions.h"
 #include "net/base/net_util.h"
+#include "chrome/browser/download/download_create_info.h"
 #include "chrome/browser/download/download_extensions.h"
 #include "chrome/browser/download/download_file_manager.h"
 #include "chrome/browser/download/download_history.h"
 #include "chrome/browser/download/download_manager.h"
 #include "chrome/browser/download/download_prefs.h"
 #include "chrome/browser/download/download_util.h"
-#include "chrome/browser/history/download_create_info.h"
 #include "chrome/browser/platform_util.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
@@ -112,37 +112,81 @@ DownloadItem::DangerType GetDangerType(bool dangerous_file,
 
 }  // namespace
 
+DownloadItem::DownloadStateInfo::DownloadStateInfo()
+  : path_uniquifier(0),
+    has_user_gesture(false),
+    prompt_user_for_save_location(false),
+    is_dangerous_file(false),
+    is_dangerous_url(false),
+    is_extension_install(false) {
+}
+
+DownloadItem::DownloadStateInfo::DownloadStateInfo(const DownloadItem& item)
+  : target_name(item.manager_state_.target_name),
+    suggested_path(item.manager_state_.suggested_path),
+    path_uniquifier(item.manager_state_.path_uniquifier),
+    has_user_gesture(item.manager_state_.has_user_gesture),
+    prompt_user_for_save_location(
+        item.manager_state_.prompt_user_for_save_location),
+    is_dangerous_file(item.manager_state_.is_dangerous_file),
+    is_dangerous_url(item.manager_state_.is_dangerous_url),
+    is_extension_install(item.manager_state_.is_extension_install),
+    force_file_name(item.manager_state_.force_file_name) {
+}
+
+DownloadItem::DownloadStateInfo::DownloadStateInfo(
+    bool has_user_gesture,
+    bool prompt_user_for_save_location)
+  : path_uniquifier(0),
+    has_user_gesture(has_user_gesture),
+    prompt_user_for_save_location(prompt_user_for_save_location),
+    is_dangerous_file(false),
+    is_dangerous_url(false),
+    is_extension_install(false) {
+}
+
+DownloadItem::DownloadStateInfo::DownloadStateInfo(
+    const FilePath& target,
+    const FilePath& forced_name,
+    bool has_user_gesture,
+    bool prompt_user_for_save_location,
+    int uniquifier,
+    bool dangerous_file,
+    bool dangerous_url,
+    bool extension_install)
+  : target_name(target),
+    path_uniquifier(uniquifier),
+    has_user_gesture(has_user_gesture),
+    prompt_user_for_save_location(prompt_user_for_save_location),
+    is_dangerous_file(dangerous_file),
+    is_dangerous_url(dangerous_url),
+    is_extension_install(extension_install),
+    force_file_name(forced_name) {
+}
+
+bool DownloadItem::DownloadStateInfo::IsDangerous() const {
+  return is_dangerous_url || is_dangerous_file;
+}
+
 // Constructor for reading from the history service.
 DownloadItem::DownloadItem(DownloadManager* download_manager,
-                           const DownloadCreateInfo& info)
-    : id_(-1),
-      full_path_(info.path),
-      path_uniquifier_(0),
-      url_chain_(info.url_chain),
-      referrer_url_(info.referrer_url),
-      mime_type_(info.mime_type),
-      original_mime_type_(info.original_mime_type),
-      total_bytes_(info.total_bytes),
-      received_bytes_(info.received_bytes),
+                           const DownloadHistoryInfo& info)
+    : history_info_(info.path, info.url_chain, info.referrer_url,
+                    info.start_time, info.received_bytes, info.total_bytes,
+                    info.state, info.db_handle, info.download_id),
       start_tick_(base::TimeTicks()),
-      state_(static_cast<DownloadState>(info.state)),
-      start_time_(info.start_time),
-      db_handle_(info.db_handle),
       download_manager_(download_manager),
       is_paused_(false),
       open_when_complete_(false),
       safety_state_(SAFE),
       danger_type_(NOT_DANGEROUS),
       auto_opened_(false),
-      target_name_(info.original_name),
-      save_as_(false),
       is_otr_(false),
-      is_extension_install_(info.is_extension_install),
       is_temporary_(false),
       all_data_saved_(false),
       opened_(false) {
   if (IsInProgress())
-    state_ = CANCELLED;
+    history_info_.state = CANCELLED;
   if (IsComplete())
     all_data_saved_ = true;
   Init(false /* don't start progress timer */);
@@ -152,20 +196,21 @@ DownloadItem::DownloadItem(DownloadManager* download_manager,
 DownloadItem::DownloadItem(DownloadManager* download_manager,
                            const DownloadCreateInfo& info,
                            bool is_otr)
-    : id_(info.download_id),
-      full_path_(info.path),
-      path_uniquifier_(info.path_uniquifier),
-      url_chain_(info.url_chain),
-      referrer_url_(info.referrer_url),
+    : history_info_(info.path, info.url_chain, info.referrer_url,
+                    info.start_time, info.received_bytes, info.total_bytes,
+                    IN_PROGRESS, DownloadHistory::kUninitializedHandle,
+                    info.download_id),
+      manager_state_(info.original_name, info.save_info.file_path,
+             info.has_user_gesture, info.prompt_user_for_save_location,
+             info.path_uniquifier, info.is_dangerous_file,
+             info.is_dangerous_url, info.is_extension_install),
+      process_handle_(info.process_handle),
+      content_disposition_(info.content_disposition),
       mime_type_(info.mime_type),
       original_mime_type_(info.original_mime_type),
-      total_bytes_(info.total_bytes),
-      received_bytes_(0),
+      referrer_charset_(info.referrer_charset),
       last_os_error_(0),
       start_tick_(base::TimeTicks::Now()),
-      state_(IN_PROGRESS),
-      start_time_(info.start_time),
-      db_handle_(DownloadHistory::kUninitializedHandle),
       download_manager_(download_manager),
       is_paused_(false),
       open_when_complete_(false),
@@ -174,11 +219,7 @@ DownloadItem::DownloadItem(DownloadManager* download_manager,
       danger_type_(GetDangerType(info.is_dangerous_file,
                                  info.is_dangerous_url)),
       auto_opened_(false),
-      target_name_(info.original_name),
-      process_handle_(info.process_handle),
-      save_as_(info.prompt_user_for_save_location),
       is_otr_(is_otr),
-      is_extension_install_(info.is_extension_install),
       is_temporary_(!info.save_info.file_path.empty()),
       all_data_saved_(false),
       opened_(false) {
@@ -190,29 +231,16 @@ DownloadItem::DownloadItem(DownloadManager* download_manager,
                            const FilePath& path,
                            const GURL& url,
                            bool is_otr)
-    : id_(1),
-      full_path_(path),
-      path_uniquifier_(0),
-      url_chain_(1, url),
-      referrer_url_(GURL()),
-      mime_type_(std::string()),
-      original_mime_type_(std::string()),
-      total_bytes_(0),
-      received_bytes_(0),
+    : history_info_(path, url, base::Time::Now(), 0, 0, IN_PROGRESS),
       last_os_error_(0),
       start_tick_(base::TimeTicks::Now()),
-      state_(IN_PROGRESS),
-      start_time_(base::Time::Now()),
-      db_handle_(DownloadHistory::kUninitializedHandle),
       download_manager_(download_manager),
       is_paused_(false),
       open_when_complete_(false),
       safety_state_(SAFE),
       danger_type_(NOT_DANGEROUS),
       auto_opened_(false),
-      save_as_(false),
       is_otr_(is_otr),
-      is_extension_install_(false),
       is_temporary_(false),
       all_data_saved_(false),
       opened_(false) {
@@ -220,7 +248,7 @@ DownloadItem::DownloadItem(DownloadManager* download_manager,
 }
 
 DownloadItem::~DownloadItem() {
-  state_ = REMOVING;
+  history_info_.state = REMOVING;
   UpdateObservers();
 }
 
@@ -237,7 +265,7 @@ void DownloadItem::UpdateObservers() {
 }
 
 bool DownloadItem::CanOpenDownload() {
-  return !Extension::IsExtension(target_name_);
+  return !Extension::IsExtension(manager_state_.target_name);
 }
 
 bool DownloadItem::ShouldOpenFileBasedOnExtension() {
@@ -296,12 +324,12 @@ void DownloadItem::DangerousDownloadValidated() {
 }
 
 void DownloadItem::UpdateSize(int64 bytes_so_far) {
-  received_bytes_ = bytes_so_far;
+  history_info_.received_bytes = bytes_so_far;
 
   // If we've received more data than we were expecting (bad server info?),
   // revert to 'unknown size mode'.
-  if (received_bytes_ > total_bytes_)
-    total_bytes_ = 0;
+  if (history_info_.received_bytes > history_info_.total_bytes)
+    history_info_.total_bytes = 0;
 }
 
 void DownloadItem::StartProgressTimer() {
@@ -336,16 +364,16 @@ void DownloadItem::Cancel(bool update_history) {
 
   download_util::RecordDownloadCount(download_util::CANCELLED_COUNT);
 
-  state_ = CANCELLED;
+  history_info_.state = CANCELLED;
   UpdateObservers();
   StopProgressTimer();
   if (update_history)
-    download_manager_->DownloadCancelled(id_);
+    download_manager_->DownloadCancelled(history_info_.download_id);
 }
 
 void DownloadItem::MarkAsComplete() {
   DCHECK(all_data_saved_);
-  state_ = COMPLETE;
+  history_info_.state = COMPLETE;
   UpdateObservers();
 }
 
@@ -380,7 +408,7 @@ void DownloadItem::Completed() {
   }
 
   DCHECK(all_data_saved_);
-  state_ = COMPLETE;
+  history_info_.state = COMPLETE;
   UpdateObservers();
   download_manager_->DownloadCompleted(id());
 }
@@ -388,7 +416,7 @@ void DownloadItem::Completed() {
 void DownloadItem::Interrupted(int64 size, int os_error) {
   if (!IsInProgress())
     return;
-  state_ = INTERRUPTED;
+  history_info_.state = INTERRUPTED;
   last_os_error_ = os_error;
   UpdateSize(size);
   StopProgressTimer();
@@ -410,28 +438,28 @@ void DownloadItem::Delete(DeleteReason reason) {
   }
 
   BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
-      NewRunnableFunction(&DeleteDownloadedFile, full_path_));
+      NewRunnableFunction(&DeleteDownloadedFile, history_info_.path));
   Remove();
   // We have now been deleted.
 }
 
 void DownloadItem::Remove() {
   Cancel(true);
-  state_ = REMOVING;
-  download_manager_->RemoveDownload(db_handle_);
+  history_info_.state = REMOVING;
+  download_manager_->RemoveDownload(history_info_.db_handle);
   // We have now been deleted.
 }
 
 bool DownloadItem::TimeRemaining(base::TimeDelta* remaining) const {
-  if (total_bytes_ <= 0)
+  if (history_info_.total_bytes <= 0)
     return false;  // We never received the content_length for this download.
 
   int64 speed = CurrentSpeed();
   if (speed == 0)
     return false;
 
-  *remaining =
-      base::TimeDelta::FromSeconds((total_bytes_ - received_bytes_) / speed);
+  *remaining = base::TimeDelta::FromSeconds(
+      (history_info_.total_bytes - history_info_.received_bytes) / speed);
   return true;
 }
 
@@ -440,12 +468,14 @@ int64 DownloadItem::CurrentSpeed() const {
     return 0;
   base::TimeDelta diff = base::TimeTicks::Now() - start_tick_;
   int64 diff_ms = diff.InMilliseconds();
-  return diff_ms == 0 ? 0 : received_bytes_ * 1000 / diff_ms;
+  return diff_ms == 0 ? 0 : history_info_.received_bytes * 1000 / diff_ms;
 }
 
 int DownloadItem::PercentComplete() const {
-  return (total_bytes_ > 0) ?
-      static_cast<int>(received_bytes_ * 100.0 / total_bytes_) : -1;
+  return (history_info_.total_bytes > 0) ?
+      static_cast<int>(history_info_.received_bytes * 100.0 /
+          history_info_.total_bytes) :
+      -1;
 }
 
 void DownloadItem::Rename(const FilePath& full_path) {
@@ -453,12 +483,12 @@ void DownloadItem::Rename(const FilePath& full_path) {
            << " full_path = \"" << full_path.value() << "\""
            << " " << DebugString(true);
   DCHECK(!full_path.empty());
-  full_path_ = full_path;
+  history_info_.path = full_path;
 }
 
 void DownloadItem::TogglePause() {
   DCHECK(IsInProgress());
-  download_manager_->PauseDownload(id_, !is_paused_);
+  download_manager_->PauseDownload(history_info_.download_id, !is_paused_);
   is_paused_ = !is_paused_;
   UpdateObservers();
 }
@@ -487,7 +517,7 @@ void DownloadItem::OnDownloadCompleting(DownloadFileManager* file_manager) {
 
 void DownloadItem::OnDownloadRenamedToFinalName(const FilePath& full_path) {
   VLOG(20) << __FUNCTION__ << "()"
-           << " full_path = " << full_path.value()
+           << " full_path = \"" << full_path.value() << "\""
            << " needed rename = " << NeedsRename()
            << " " << DebugString(false);
   DCHECK(NeedsRename());
@@ -526,60 +556,42 @@ bool DownloadItem::MatchesQuery(const string16& query) const {
   return (path.find(query) != string16::npos);
 }
 
-void DownloadItem::SetFileCheckResults(const FilePath& path,
-                                       bool is_dangerous_file,
-                                       bool is_dangerous_url,
-                                       int path_uniquifier,
-                                       bool prompt,
-                                       bool is_extension_install,
-                                       const FilePath& original_name) {
-  VLOG(20) << " " << __FUNCTION__ << "()"
-           << " path = \"" << path.value() << "\""
-           << " is_dangerous_file = " << is_dangerous_file
-           << " is_dangerous_url = " << is_dangerous_url
-           << " path_uniquifier = " << path_uniquifier
-           << " prompt = " << prompt
-           << " is_extension_install = " << is_extension_install
-           << " path = \"" << path.value() << "\""
-           << " original_name = \"" << original_name.value() << "\""
-           << " " << DebugString(true);
-  // Make sure the initial file name is set only once.
-  DCHECK(full_path_.empty());
-  DCHECK(!path.empty());
+void DownloadItem::SetFileCheckResults(const DownloadStateInfo& state) {
+  VLOG(20) << " " << __FUNCTION__ << "()" << " this = " << DebugString(true);
+  manager_state_ = state;
+  VLOG(20) << " " << __FUNCTION__ << "()" << " this = " << DebugString(true);
 
-  full_path_ = path;
-  safety_state_ = GetSafetyState(is_dangerous_file, is_dangerous_url);
-  danger_type_ = GetDangerType(is_dangerous_file, is_dangerous_url);
-  path_uniquifier_ = path_uniquifier;
-  save_as_ = prompt;
-  is_extension_install_ = is_extension_install;
-  target_name_ = original_name;
+  safety_state_ = GetSafetyState(manager_state_.is_dangerous_file,
+                                 manager_state_.is_dangerous_url);
+  danger_type_ = GetDangerType(manager_state_.is_dangerous_file,
+                               manager_state_.is_dangerous_url);
+}
 
-  if (target_name_.value().empty())
-    target_name_ = full_path_.BaseName();
+void DownloadItem::UpdateTarget() {
+  if (manager_state_.target_name.value().empty())
+    manager_state_.target_name = history_info_.path.BaseName();
 }
 
 FilePath DownloadItem::GetTargetFilePath() const {
-  return full_path_.DirName().Append(target_name_);
+  return history_info_.path.DirName().Append(manager_state_.target_name);
 }
 
 FilePath DownloadItem::GetFileNameToReportUser() const {
-  if (path_uniquifier_ > 0) {
-    FilePath name(target_name_);
-    download_util::AppendNumberToPath(&name, path_uniquifier_);
+  if (manager_state_.path_uniquifier > 0) {
+    FilePath name(manager_state_.target_name);
+    download_util::AppendNumberToPath(&name, manager_state_.path_uniquifier);
     return name;
   }
-  return target_name_;
+  return manager_state_.target_name;
 }
 
 FilePath DownloadItem::GetUserVerifiedFilePath() const {
   return (safety_state_ == DownloadItem::SAFE) ?
-      GetTargetFilePath() : full_path_;
+      GetTargetFilePath() : history_info_.path;
 }
 
 void DownloadItem::Init(bool start_timer) {
-  if (target_name_.value().empty())
-    target_name_ = full_path_.BaseName();
+  UpdateTarget();
   if (start_timer)
     StartProgressTimer();
   VLOG(20) << __FUNCTION__ << "() " << DebugString(true);
@@ -588,38 +600,61 @@ void DownloadItem::Init(bool start_timer) {
 // TODO(ahendrickson) -- Move |INTERRUPTED| from |IsCancelled()| to
 // |IsPartialDownload()|, when resuming interrupted downloads is implemented.
 bool DownloadItem::IsPartialDownload() const {
-  return (state_ == IN_PROGRESS);
+  return (history_info_.state == IN_PROGRESS);
 }
 
 bool DownloadItem::IsInProgress() const {
-  return (state_ == IN_PROGRESS);
+  return (history_info_.state == IN_PROGRESS);
 }
 
 bool DownloadItem::IsCancelled() const {
-  return (state_ == CANCELLED) || (state_ == INTERRUPTED);
+  return (history_info_.state == CANCELLED) ||
+         (history_info_.state == INTERRUPTED);
 }
 
 bool DownloadItem::IsInterrupted() const {
-  return (state_ == INTERRUPTED);
+  return (history_info_.state == INTERRUPTED);
 }
 
 bool DownloadItem::IsComplete() const {
-  return (state_ == COMPLETE);
+  return (history_info_.state == COMPLETE);
+}
+
+// This function converts history_info_.state (which is an int32) to a
+// DownloadItem::DownloadState enum, and returns it.
+// A DCHECK() will be fire if it has an illegal value.
+DownloadItem::DownloadState DownloadItem::state() const {

[Paweł Hajdan Jr., 2011/05/19 16:18:25]

nit: This isn't a simple inlined accessor, could you rename it to GetState()?

[ahendrickson, 2011/05/19 20:16:49]

Would you be willing to let me do this in a separate CL?  It will touch a lot of
files that aren't currently affected, due to all the observers.

[Paweł Hajdan Jr., 2011/05/20 09:04:42]

Yes, that makes sense. Please add a TODO in this CL to remember about this.

+  DCHECK_LE(IN_PROGRESS, history_info_.state);
+  DCHECK_GE(INTERRUPTED, history_info_.state);

[Randy Smith (Not in Mondays), 2011/05/19 17:05:27]

I'd rather have these DCHECKs (and the comparisons below) be against 0 and
"MAX_DOWNLOAD_STATE" as I think those values are more likely to stay accurate (0
won't ever be modified, and MAX_DOWNLOAD_STATE will be at the end of the enum
and very clearly needing to stay at the end of it as the enum is modified.)

[ahendrickson, 2011/05/19 20:16:49]

Done.

+
+  if ((history_info_.state < IN_PROGRESS) ||
+      (history_info_.state > INTERRUPTED)) {
+      return IN_PROGRESS;

[Paweł Hajdan Jr., 2011/05/19 16:18:25]

I think it'd be simpler to write NOTREACHED() << history_info_.state inside the
"if" instead of the two DCHECKs above.

Also, the indentation of the return seems to be off.

[ahendrickson, 2011/05/19 20:16:49]

Done.

+  }
+  return static_cast<DownloadState>(history_info_.state);
+}
+
+const GURL& DownloadItem::url() const {

[Paweł Hajdan Jr., 2011/05/19 16:18:25]

nit: This isn't a simple inlined accessor, could you rename it to GetURL()?

[ahendrickson, 2011/05/19 20:16:49]

Done.

+  return history_info_.url_chain.empty() ?
+             GURL::EmptyGURL() : history_info_.url_chain.back();
 }
 
 std::string DownloadItem::DebugString(bool verbose) const {
-  std::string description = base::StringPrintf(
-      "{ id_ = %d state = %s", id_, DebugDownloadStateString(state()));
+  std::string description =
+      base::StringPrintf("{ id = %d"
+                         " state = %s",
+                         history_info_.download_id,
+                         DebugDownloadStateString(state()));
 
   // Construct a string of the URL chain.
   std::string url_list("<none>");
-  if (!url_chain_.empty()) {
-    std::vector<GURL>::const_iterator iter = url_chain_.begin();
-    std::vector<GURL>::const_iterator last = url_chain_.end();
+  if (!history_info_.url_chain.empty()) {
+    std::vector<GURL>::const_iterator iter = history_info_.url_chain.begin();
+    std::vector<GURL>::const_iterator last = history_info_.url_chain.end();
     url_list = (*iter).spec();
     ++iter;
     for ( ; verbose && (iter != last); ++iter) {
-      url_list += " -> ";
+      url_list += " ->\n\t";
       const GURL& next_url = *iter;
       url_list += next_url.spec();
     }
@@ -629,12 +664,12 @@ std::string DownloadItem::DebugString(bool verbose) const {
     description += base::StringPrintf(
         " db_handle = %" PRId64
         " total_bytes = %" PRId64
-        " is_paused = " "%c"
-        " is_extension_install = " "%c"
-        " is_otr = " "%c"
-        " safety_state = " "%s"
-        " url_chain = " "\"%s\""
-        " target_name_ = \"%" PRFilePath "\""
+        " is_paused = %c"
+        " is_extension_install = %c"
+        " is_otr = %c"
+        " safety_state = %s"
+        " url_chain = \n\t\"%s\"\n\t"
+        " target_name = \"%" PRFilePath "\""
         " full_path = \"%" PRFilePath "\"",
         db_handle(),
         total_bytes(),
@@ -643,10 +678,13 @@ std::string DownloadItem::DebugString(bool verbose) const {
         is_otr() ? 'T' : 'F',
         DebugSafetyStateString(safety_state()),
         url_list.c_str(),
-        target_name_.value().c_str(),
+        manager_state_.target_name.value().c_str(),
         full_path().value().c_str());
   } else {
     description += base::StringPrintf(" url = \"%s\"", url_list.c_str());
   }
+
+  description += " }";
+
   return description;
 }