Added member data to classes to support download resumption.

content/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 CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_H_
 #define CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_H_
 #pragma once
 
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/file_path.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/observer_list.h"
+#include "base/pickle.h"
 #include "base/time.h"
 #include "content/browser/download/download_id.h"
 #include "content/browser/download/download_request_handle.h"
 #include "content/browser/download/download_state_info.h"
 #include "content/browser/download/interrupt_reasons.h"
 #include "content/common/content_export.h"
 #include "googleurl/src/gurl.h"
 #include "net/base/net_errors.h"
 
 class DownloadFileManager;
@@ skipping 102 matching lines @@
   // Open the file associated with this download (wait for the download to
   // complete if it is in progress).
   void OpenDownload();
 
   // Show the download via the OS shell.
   void ShowDownloadInShell();
 
   // Called when the user has validated the download of a dangerous file.
   void DangerousDownloadValidated();
 
-  // Received a new chunk of data
-  void Update(int64 bytes_so_far);
+  // Received a new chunk of data.
+  // |bytes_so_far| is the number of bytes received so far.
+  // |hash_state| is the current hash state.
+  void Update(int64 bytes_so_far, const Pickle& hash_state);

[Randy Smith (Not in Mondays), 2011/11/21 02:10:17]

So this makes me concerned, as pickling and unpickling can be something of a
heavyweight operation, and I'd rather not be doing it on a regular, updating
basis.  I'm not sure what the right answer is here, but by code smell I'm
tempted to reverse myself on my earlier acceptance and say that we should avoid
updating the size/hash pair in the DB except on interrupt and completion (and
I'd leave the hash blank on completion--I don't think it'd be used on restart,
and as previously noted, the complete hash is a completely different beast than
the intermediate hash).  All of which will take some plumbing :-{.  

I could be argued out of this--it's probably not *too* expensive to pickle on
every timer interrupt on the FILE thread.  But it really doesn't feel like the
right thing to be doing.

[ahendrickson, 2011/11/21 20:34:47]

FWIW, the size of the pickle payload is around 300 bytes (for the larger hash
scheme), and the overhead is on the order of 32 more bytes.

Pickle copying is mainly a call to memcpy(), and WriteData() is a memcpy() plus
some accounting.

That doesn't seem to me an expensive operation; what do you think?

[Randy Smith (Not in Mondays), 2011/11/22 17:37:49]

That sounds ok.  Could you figure out about how often (in time or bytes, either
way) the operation will occur?  I'd like to have a handle on the likely
performance implications before signing off.

[ahendrickson, 2011/11/22 23:46:44]

Operations are in bytes

SHA256ContextStr has 3 fields:

Type      Name                 Size
--------- -------------------- ----
byte[]    u:                    256
int32[]   h:                     32
int32     sizeHi:                 4
int32     sizeLo:                 4
                               -----
Total:                          296

We add to the pickle:

Type      Name                 Size
--------- -------------------- ----
int32     version:                4
char[]    "NSS":                  4
                               -----
Total:                            8

Pickle overhead (depending on 32/64-bit arch):

Type      Name                 Size
--------- -------------------- ----
Header*   header_                 4/8
size_t    header_size_            4/8
size_t    capacity_               4/8
size_t    variable_buffer_offset_ 4/8
                               -------
Total:                           16/32

Pickle size:                    296
                                  8
                                 16/32
                               -------
Total:                          320/352

SecureHashSHA256NSS::Serialize()
    fills pickle:                                                   320/352
  BaseFile::GetSha256HashState()
      creates string:                                               320/352
  BaseFile::GetSha256HashState()
      copies string to return:                                      320/352
    DownloadFileManager::UpdateInProgressDownloads()
        copies returned string:                                     320/352
    DownloadFileManager::UpdateInProgressDownloads()
        POSTs copy of string:                                       320/352
      DownloadManagerImpl::UpdateDownload()
        DownloadItemImpl::Update()
          DownloadItemImpl::UpdateSizeAndHashState()
            copies string:                                          320/352
        ChromeDownloadManagerDelegate::UpdateItemInPersistentStore()
              copies string:                                        320/352
          DownloadHistory::UpdateEntry()
            ...
            HistoryBackend::UpdateDownload()
              DownloadDatabase::UpdateDownload()
                  saves data in DB                                  320/352
                                                                   ---------
Total:                                                             2560/2816

This happens twice a second.

[Randy Smith (Not in Mondays), 2011/11/23 19:25:13]

I'm not too worried about the space costs; more about the overhead.  OTOH, it's
sounding like the copying of the bytes is the major cost.  And that's a fair
amount of copying.  

I think what I'll say is: Do some manual testing (before/after) using jar's
performance monitor--how much extra CPU time does this cost us for some
representative downloads?  If it's negligible, great.  If not, raise the flag
again--we probably don't want to do something extravagant to avoid this, but we
might (e.g.) reduce the frequency with which we send updates.

[ahendrickson, 2011/12/20 00:04:13]

I downloaded a 50MB file, which took about 10-15 seconds.

http://ftp.belnet.be/packages/damnsmalllinux/current/dsl-4.4.10-syslinux.iso

From the profiler:

DRH::OnReadCompleted() was called 1,694 times, for a total run time of 2.29 sec
(avg 0.001 sec).

DFM::UpdateInProgressDownloads() was called 56 times, for a total run time of
0.090 sec (avg 0.002 sec).

Other points of interest, not related to the above:

SBS::StartDownloadCheck() was called twice, with a total run time 0f 0.5 sec
(avg 0.26 sec).

CDMD::CheckIfSuggestedPathExists() was called once, with a run time of 0.27 sec.

DFM::StartDownload() was called once, with a run time of 1.67 sec.

[Randy Smith (Not in Mondays), 2011/12/20 17:05:40]

Cool.  What's the comparison?  (I.e. what that would look like without your
changes?)

[ahendrickson, 2011/12/20 22:06:54]

Without the hash state being passed around:


DRH::OnReadCompleted() was called 1,688 times, for a total run time of 2.20 sec
(avg 0.001 sec).

DFM::UpdateInProgressDownloads() was called 40 times, for a total run time of
0.20 sec (avg 0.005 sec).

Other points of interest, not related to the above:

SBS::StartDownloadCheck() was called twice, with a total run time 0f 1.5 sec
(avg 0.78 sec).

CDMD::CheckIfSuggestedPathExists() was called once, with a run time of 0.011
sec.

DFM::StartDownload() was called once, with a run time of 1.11 sec.

--

I'd say the hash copying is lost in the noise.

[Randy Smith (Not in Mondays), 2011/12/21 19:14:49]

Agreed.  Thanks for the due diligence.

 
   // Cancel the download operation. We need to distinguish between cancels at
   // exit (DownloadManager destructor) from user interface initiated cancels
   // because at exit, the history system may not exist, and any updates to it
   // require AddRef'ing the DownloadManager in the destructor which results in
   // a DCHECK failure. Set |user_cancel| to false when canceling from at
   // exit to prevent this crash. This may result in a difference between the
   // downloaded file's size on disk, and what the history system's last record
   // of it is. At worst, we'll end up re-downloading a small portion of the file
   // when resuming a download (assuming the server supports byte ranges).
   void Cancel(bool user_cancel);
 
   // Called by external code (SavePackage) using the DownloadItem interface
   // to display progress when the DownloadItem should be considered complete.
   void MarkAsComplete();
 
   // Called by the delegate after it delayed opening the download in
   // DownloadManagerDelegate::ShouldOpenDownload.
   void DelayedDownloadOpened();
 
   // Called when all data has been saved.
   void OnAllDataSaved(int64 size, const std::string& final_hash);
 
   // Called when the downloaded file is removed.
   void OnDownloadedFileRemoved();
 
   // Download operation had an error.
   // |size| is the amount of data received at interruption.
+  // |hash_state| is the current hash state at interruption.
   // |reason| is the download interrupt reason code that the operation received.
-  void Interrupted(int64 size, InterruptReason reason);
+  void Interrupted(int64 size,
+                   const Pickle& hash_state,
+                   InterruptReason reason);
 
   // Deletes the file from disk and removes the download from the views and
   // history.  |user| should be true if this is the result of the user clicking
   // the discard button, and false if it is being deleted for other reasons like
   // browser shutdown.
   void Delete(DeleteReason reason);
 
   // Removes the download from the views and history.
   void Remove();
 
@@ skipping 99 matching lines @@
   const FilePath& target_name() const { return state_info_.target_name; }
   bool prompt_user_for_save_location() const {
     return state_info_.prompt_user_for_save_location;
   }
   bool is_otr() const { return is_otr_; }
   const FilePath& suggested_path() const { return state_info_.suggested_path; }
   bool is_temporary() const { return is_temporary_; }
   void set_opened(bool opened) { opened_ = opened; }
   bool opened() const { return opened_; }
 
+  const std::string& last_modified_time() const { return last_modified_time_; }
+  const std::string& etag() const { return etag_; }
+
   InterruptReason last_reason() const { return last_reason_; }
 
   DownloadPersistentStoreInfo GetPersistentStoreInfo() const;
   DownloadStateInfo state_info() const { return state_info_; }
 
   TabContents* GetTabContents() const;
 
   // Returns the final target file path for the download.
   FilePath GetTargetFilePath() const;
 
@@ skipping 21 matching lines @@
 
   std::string DebugString(bool verbose) const;
 
   void MockDownloadOpenForTesting() { open_enabled_ = false; }
 
  private:
   // Construction common to all constructors. |active| should be true for new
   // downloads and false for downloads from the history.
   void Init(bool active);
 
+  // Internal helper for maintaining consistent received and total sizes, and
+  // hash state.
+  void UpdateSizeAndHashState(int64 bytes_so_far, const Pickle& hash_state);
+
   // Internal helper for maintaining consistent received and total sizes.
-  void UpdateSize(int64 size);
+  // Should only be called from |UpdateSizeAndHashState| and |OnAllDataSaved|.
+  void UpdateSize(int64 bytes_so_far);
 
   // Called when the entire download operation (including renaming etc)
   // is completed.
   void Completed();
 
   // Call to transition state; all state transitions should go through this.
   void TransitionTo(DownloadState new_state);
 
   // Called when safety_state_ should be recomputed from the DangerType of the
   // state info.
@@ skipping 41 matching lines @@
 
   // 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_;
 
   // The charset of the referring page where the download request comes from.
   // It's used to construct a suggested filename.
   std::string referrer_charset_;
 
-  // Total bytes expected
+  // Total bytes expected.
   int64 total_bytes_;
 
-  // Current received bytes
+  // Current received bytes.
   int64 received_bytes_;
 
   // Sha256 hash of the content.  This might be empty either because
-  // the download isn't done yet or because the hash isn't needed
+  // the download hasn't started yet or because the hash isn't needed
   // (ChromeDownloadManagerDelegate::GenerateFileHash() returned false).
   std::string hash_;
 
+  Pickle hash_state_;
+
+  // Server's time stamp for the file.
+  std::string last_modified_time_;
+
+  // Server's ETAG for the file.
+  std::string etag_;
+
   // Last reason.
   InterruptReason last_reason_;
 
-  // Start time for calculating remaining time
+  // Start time for calculating remaining time.
   base::TimeTicks start_tick_;
 
-  // The current state of this download
+  // The current state of this download.
   DownloadState state_;
 
-  // The views of this item in the download shelf and download tab
+  // The views of this item in the download shelf and download tab.
   ObserverList<Observer> observers_;
 
-  // Time the download was started
+  // Time the download was started.
   base::Time start_time_;
 
-  // Time the download completed
+  // Time the download completed.
   base::Time end_time_;
 
-  // Our persistent store handle
+  // Our persistent store handle.
   int64 db_handle_;
 
-  // Our owning object
+  // Our owning object.
   DownloadManager* download_manager_;
 
-  // In progress downloads may be paused by the user, we note it here
+  // 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_;
 
   // A flag for indicating if the downloaded file is externally removed.
   bool file_externally_removed_;
 
   // Indicates if the download is considered potentially safe or dangerous
   // (executable files are typically considered dangerous).
@@ skipping 23 matching lines @@
   // only.
   bool open_enabled_;
 
   // Did the delegate delay calling Complete on this download?
   bool delegate_delayed_complete_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadItem);
 };
 
 #endif  // CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_ITEM_H_