Reduced the lifetime of DownloadCreateInfo.

chrome/browser/download/download_item.h

 // Copyright (c) 2011 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.
 //
 // Each download is represented by a DownloadItem, and all DownloadItems
 // are owned by the DownloadManager which maintains a global list of all
 // downloads. DownloadItems are created when a user initiates a download,
 // and exist for the duration of the browser life time.
 //
 // Download observers:
 //   DownloadItem::Observer:
 //     - allows observers to receive notifications about one download from start
 //       to completion
 // Use AddObserver() / RemoveObserver() on the appropriate download object to
 // receive state updates.
 
 #ifndef CHROME_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_H_
 #define CHROME_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_H_
 #pragma once
 
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/file_path.h"
 #include "base/observer_list.h"
 #include "base/time.h"
 #include "base/timer.h"
 #include "chrome/browser/download/download_process_handle.h"
+#include "chrome/browser/history/download_history_info.h"
 #include "googleurl/src/gurl.h"
 
+struct DownloadCreateInfo;
 class DownloadFileManager;
 class DownloadManager;
-struct DownloadCreateInfo;
 
 // One DownloadItem per download. This is the model class that stores all the
 // state for a download. Multiple views, such as a tab's download shelf and the
 // Destination tab's download view, may refer to a given DownloadItem.
 //
 // This is intended to be used only on the UI thread.
 class DownloadItem {
  public:
   enum DownloadState {
     // Download is actively progressing.
@@ skipping 35 matching lines @@
     // ALWAYS ADD NEW VALUES BEFORE THIS ONE.
     DANGEROUS_TYPE_MAX
   };
 
   // Reason for deleting the download.  Passed to Delete().
   enum DeleteReason {
     DELETE_DUE_TO_BROWSER_SHUTDOWN = 0,
     DELETE_DUE_TO_USER_DISCARD
   };
 
+  // Contains information relating to the process of determining what to do with
+  // the download.
+  struct DownloadStateInfo {

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

Why is it declared inside DownloadInfo? Is it possible to extract it to its own
file?

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

I've been having problems with creating new files . . .

I think this struct belongs in here, as it is not a member of any other struct
or class.

It is only used here and in download_manager.cc.

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

I'm not sure whether it's some local problem or some design issue... Could you
explain more?
Right, but it's passed around. Even if it's just between DownloadItem and
DownloadManager, I think it's better to extract it.

[ahendrickson, 2011/05/20 18:31:24]

Extracted.

+    DownloadStateInfo();
+    explicit DownloadStateInfo(const DownloadItem& item);
+    DownloadStateInfo(bool has_user_gesture,
+                      bool prompt_user_for_save_location);
+    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);
+
+    // Indicates if the download is dangerous.
+    bool IsDangerous() const;
+
+    // The original name for a dangerous download, specified by the request.
+    FilePath target_name;
+
+    FilePath suggested_path;
+
+    // A number that should be added to the suggested path to make it unique.
+    // 0 means no number should be appended.  It is eventually incorporated
+    // into the final file name.
+    int path_uniquifier;
+
+    bool has_user_gesture;
+
+    // True if we should display the 'save as...' UI and prompt the user
+    // for the download location.
+    // False if the UI should be suppressed and the download performed to the
+    // default location.
+    bool prompt_user_for_save_location;
+
+    // Whether this download file is potentially dangerous (ex: exe, dll, ...).
+    bool is_dangerous_file;
+
+    // If safebrowsing believes this URL leads to malware.
+    bool is_dangerous_url;
+
+    // Whether this download is for extension install or not.
+    bool is_extension_install;
+
+    // Whether this download's file name was specified initially.
+    FilePath force_file_name;
+  };
+
   // Interface that observers of a particular download must implement in order
   // to receive updates to the download's status.
   class Observer {
    public:
     virtual void OnDownloadUpdated(DownloadItem* download) = 0;
 
     // Called when a downloaded file has been opened.
     virtual void OnDownloadOpened(DownloadItem* download) = 0;
 
    protected:
     virtual ~Observer() {}
   };
 
   // Constructing from persistent store:
   DownloadItem(DownloadManager* download_manager,
-               const DownloadCreateInfo& info);
+               const DownloadHistoryInfo& info);
 
   // Constructing for a regular download:
   DownloadItem(DownloadManager* download_manager,
                const DownloadCreateInfo& info,
                bool is_otr);
 
   // Constructing for the "Save Page As..." feature:
   DownloadItem(DownloadManager* download_manager,
                const FilePath& path,
                const GURL& url,
@@ skipping 72 matching lines @@
   // Simple speed estimate in bytes/s
   int64 CurrentSpeed() const;
 
   // Rough percent complete, -1 means we don't know (since we didn't receive a
   // total size).
   int PercentComplete() const;
 
   // Whether or not this download has saved all of its data.
   bool all_data_saved() const { return all_data_saved_; }
 
-  // Update the fields that may have changed in DownloadCreateInfo as a
+  // Update the fields that may have changed in DownloadStateInfo as a
   // result of analyzing the file and figuring out its type, location, etc.
   // May only be called once.
-  void 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);
+  void SetFileCheckResults(const DownloadStateInfo& state);
+
+  // Updates the target file.
+  void UpdateTarget();
 
   // Update the download's path, the actual file is renamed on the download
   // thread.
   void Rename(const FilePath& full_path);
 
   // Allow the user to temporarily pause a download or resume a paused download.
   void TogglePause();
 
   // Called when the download is ready to complete.
   // This may perform final rename if necessary and will eventually call
@@ skipping 15 matching lines @@
   // Returns true if the download has been cancelled or was interrupted.
   bool IsCancelled() const;
 
   // Returns true if the download was interrupted.
   bool IsInterrupted() const;
 
   // Returns true if we have all the data and know the final file name.
   bool IsComplete() const;
 
   // Accessors
-  DownloadState state() const { return state_; }
-  FilePath full_path() const { return full_path_; }
-  void set_path_uniquifier(int uniquifier) { path_uniquifier_ = uniquifier; }
-  const GURL& url() const { return url_chain_.back(); }
-  const std::vector<GURL>& url_chain() const { return url_chain_; }
-  const GURL& original_url() const { return url_chain_.front(); }
-  const GURL& referrer_url() const { return referrer_url_; }
+  DownloadState state() const;
+  FilePath full_path() const { return history_info_.path; }
+  void set_path_uniquifier(int uniquifier) {
+    manager_state_.path_uniquifier = uniquifier;
+  }
+  const GURL& url() const;
+  const std::vector<GURL>& url_chain() const { return history_info_.url_chain; }
+  const GURL& original_url() const { return history_info_.url_chain.front(); }
+  const GURL& referrer_url() const { return history_info_.referrer_url; }
+  std::string content_disposition() const { return content_disposition_; }
   std::string mime_type() const { return mime_type_; }
   std::string original_mime_type() const { return original_mime_type_; }
-  int64 total_bytes() const { return total_bytes_; }
-  void set_total_bytes(int64 total_bytes) { total_bytes_ = total_bytes; }
-  int64 received_bytes() const { return received_bytes_; }
-  int32 id() const { return id_; }
-  base::Time start_time() const { return start_time_; }
-  void set_db_handle(int64 handle) { db_handle_ = handle; }
-  int64 db_handle() const { return db_handle_; }
+  std::string referrer_charset() const { return referrer_charset_; }
+  int64 total_bytes() const { return history_info_.total_bytes; }
+  void set_total_bytes(int64 total_bytes) {
+    history_info_.total_bytes = total_bytes;
+  }
+  int64 received_bytes() const { return history_info_.received_bytes; }
+  int32 id() const { return history_info_.download_id; }
+  base::Time start_time() const { return history_info_.start_time; }
+  void set_db_handle(int64 handle) { history_info_.db_handle = handle; }
+  int64 db_handle() const { return history_info_.db_handle; }
   bool is_paused() const { return is_paused_; }
   bool open_when_complete() const { return open_when_complete_; }
   void set_open_when_complete(bool open) { open_when_complete_ = open; }
   SafetyState safety_state() const { return safety_state_; }
   void set_safety_state(SafetyState safety_state) {
     safety_state_ = safety_state;
   }
   DangerType danger_type() { return danger_type_;}
   bool auto_opened() { return auto_opened_; }
-  FilePath target_name() const { return target_name_; }
-  bool save_as() const { return save_as_; }
+  FilePath target_name() const { return manager_state_.target_name; }
+  bool save_as() const { return manager_state_.prompt_user_for_save_location; }
   bool is_otr() const { return is_otr_; }
-  bool is_extension_install() const { return is_extension_install_; }
+  bool is_extension_install() const {
+    return manager_state_.is_extension_install;
+  }
   bool is_temporary() const { return is_temporary_; }
   void set_opened(bool opened) { opened_ = opened; }
   bool opened() const { return opened_; }
 
+  // History state mutators.
+  void set_path(const FilePath& path) { history_info_.path = path; }

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

I get nervous when we create new setters, specifically--it adds to the "bag of
bits" feeling of this structure.  Can you change this over to something like
"OnPathDetermined(const FilePath& path)" (i.e. something that indicates what is
happening in the lifecycle of the download)?  If there are any assertions about
that lifecycle that might be appropriate (download in progress, etc.) including
them would be good too.

Not a demand--it's not that bad.  But it's only used once, at one specific place
in the download process, and naming it that way would help call that out.

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

Done.

+
+  // Manager state accessors.
+  bool IsDangerous() const { return manager_state_.IsDangerous(); }

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

Why isn't this redundant with danger_type() == NOT_DANGEROUS?  I'm not sure it's
worth the extra accessor to save the callers a comparison.

More generally, now that we've got an abstraction barrier between inside and
outside of the DownloadItem, do we need to duplicate the dangerousness
information between the main structure and the manager_state_?

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

Done.

+
+  // Manager state mutators.
+  void set_dangerous_url(bool is_dangerous_url) {

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

I'm uncomfortable about this setter for the same reasons as above.  Maybe change
it to "MarkUrlDangerous" without an argument, and only call it if we've found a
dangerous URL? 

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

Done.

+    manager_state_.is_dangerous_url = is_dangerous_url;
+  }
+
   const DownloadProcessHandle& process_handle() const {
     return process_handle_;
   }
 
   // Returns the final target file path for the download.
   FilePath GetTargetFilePath() const;
 
   // Returns the file-name that should be reported to the user, which is
-  // target_name_ possibly with the uniquifier number.
+  // target_name possibly with the uniquifier number.
   FilePath GetFileNameToReportUser() const;
 
   // Returns the user-verified target file path for the download.
   // This returns the same path as GetTargetFilePath() for safe downloads
   // but does not for dangerous downloads until the name is verified.
   FilePath GetUserVerifiedFilePath() const;
 
   // Returns true if the current file name is not the final target name yet.
   bool NeedsRename() const {
-    return target_name_ != full_path_.BaseName();
+    return manager_state_.target_name != history_info_.path.BaseName();
   }
 
   std::string DebugString(bool verbose) const;
 
  private:
   void Init(bool start_timer);
 
   // Internal helper for maintaining consistent received and total sizes.
   void UpdateSize(int64 size);
 
   // Called when the entire download operation (including renaming etc)
   // is completed.
   void Completed();
 
   // Start/stop sending periodic updates to our observers
   void StartProgressTimer();
   void StopProgressTimer();
 
-  // Request ID assigned by the ResourceDispatcherHost.
-  int32 id_;
+  // Information that is saved to the history DB.
+  DownloadHistoryInfo history_info_;
 
-  // Full path to the downloaded or downloading file.
-  FilePath full_path_;
+  // State information used by the download manager.
+  DownloadStateInfo manager_state_;

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

nit: How about just state_info for consistency? I think it's also easier to
remember.

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

I called it |manager_state_| because it's information that the manager sets &
uses, on a per-item basis.  This distinguishes it from state internal to
DownloadItem.

A later refactor will probably get rid of this structure anyway.

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

I think that consistency is still more important. The comment has the necessary
details.
Great - how about a TODO or some comment that would clearly indicate we'd like
to get rid of it?

[ahendrickson, 2011/05/20 18:31:24]

Done.

 
-  // A number that should be appended to the path to make it unique, or 0 if the
-  // path should be used as is.
-  int path_uniquifier_;
+  // The handle to the process information.  Used for operations outside the
+  // download system.
+  DownloadProcessHandle process_handle_;
 
-  // The chain of redirects that leading up to and including the final URL.
-  std::vector<GURL> url_chain_;
+  // Information from the request.
+  // Content-disposition field from the header.
+  std::string content_disposition_;
 
-  // The URL of the page that initiated the download.
-  GURL referrer_url_;
-
-  // The mimetype of the download
+  // Mime-type from the header.  Subject to change.
   std::string mime_type_;
 
-  // The value of the content type header received when downloading
-  // this item.  |mime_type_| may be different because of type sniffing.
+  // The value of the content type header sent with the downloaded item.  It
+  // may be different from |mime_type_|, which may be set based on heuristics
+  // which may look at the file extension and first few bytes of the file.
   std::string original_mime_type_;
 
-  // Total bytes expected
-  int64 total_bytes_;
-
-  // Current received bytes
-  int64 received_bytes_;
+  // The charset of the referring page where the download request comes from.
+  // It's used to construct a suggested filename.
+  std::string referrer_charset_;
 
   // Last error.
   int last_os_error_;
 
   // Start time for calculating remaining time
   base::TimeTicks start_tick_;
 
-  // The current state of this download
-  DownloadState state_;
-
   // The views of this item in the download shelf and download tab
   ObserverList<Observer> observers_;
 
-  // Time the download was started
-  base::Time start_time_;
-
-  // Our persistent store handle
-  int64 db_handle_;
-
   // Timer for regularly updating our observers
   base::RepeatingTimer<DownloadItem> update_timer_;
 
   // Our owning object
   DownloadManager* download_manager_;
 
   // In progress downloads may be paused by the user, we note it here
   bool is_paused_;
 
   // A flag for indicating if the download should be opened at completion.
   bool open_when_complete_;
 
   // Whether the download is considered potentially safe or dangerous
   // (executable files are typically considered dangerous).
   SafetyState safety_state_;
 
   // Why |safety_state_| is not SAFE.
   DangerType danger_type_;
 
   // Whether the download was auto-opened. We set this rather than using
   // an observer as it's frequently possible for the download to be auto opened
   // before the observer is added.
   bool auto_opened_;
 
-  // Dangerous downloads or ongoing downloads are given temporary names until
-  // the user approves them or the downloads finish.
-  // This stores their final target name.
-  FilePath target_name_;
-
-  // The handle to the process information.  Used for operations outside the
-  // download system.
-  DownloadProcessHandle process_handle_;
-
-  // True if the item was downloaded as a result of 'save as...'
-  bool save_as_;
-
   // True if the download was initiated in an incognito window.
   bool is_otr_;
 
-  // True if the item was downloaded for an extension installation.
-  bool is_extension_install_;
-
   // True if the item was downloaded temporarily.
   bool is_temporary_;
 
   // True if we've saved all the data for the download.
   bool all_data_saved_;
 
   // Did the user open the item either directly or indirectly (such as by
   // setting always open files of this type)? The shelf also sets this field
   // when the user closes the shelf before the item has been opened but should
   // be treated as though the user opened it.
   bool opened_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadItem);
 };
 
 #endif  // CHROME_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_H_