Get rid of DownloadItemImpl::UpdateProgress()

Changed SavePackage and unit tests to use DestinationUpdated(). This
function does the same work as UpdateProgress(). We stick to
DestinationUpdated() since it's part of DownloadDestinationObserver
interface (so already used elsewhere).

This patch makes the interface implementation as public to allow
access from SavePackage and unit tests. These functions were already
public in the interface itself.

BUG=239475
TEST=content_unittests and making sure chrome is built

Review URL: https://chromiumcodereview.appspot.com/14646036

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@200301 0039d316-1c4b-4281-b951-d872f2087c98

AUTHORS

@@ -255,3 +255,4 @@ Aditya Bhargava <heuristicist@gmail.com>
 John Yani <vanuan@gmail.com>
 Kangil Han <kangil.han@samsung.com>
 Evan Wallace <evan.exe@gmail.com>
+Caio Marcelo de Oliveira Filho <caio.de.oliveira.filho@intel.com>

content/browser/download/download_item_impl.cc

@@ -883,46 +883,6 @@ void DownloadItemImpl::SetTotalBytes(int64 total_bytes) {
   total_bytes_ = total_bytes;
 }
 
-// Updates from the download thread may have been posted while this download
-// was being cancelled in the UI thread, so we'll accept them unless we're
-// complete.
-void DownloadItemImpl::UpdateProgress(int64 bytes_so_far,
-                                      int64 bytes_per_sec,
-                                      const std::string& hash_state) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  VLOG(20) << __FUNCTION__ << " so_far=" << bytes_so_far
-           << " per_sec=" << bytes_per_sec << " download=" << DebugString(true);
-
-  if (state_ != IN_PROGRESS_INTERNAL) {
-    // Ignore if we're no longer in-progress.  This can happen if we race a
-    // Cancel on the UI thread with an update on the FILE thread.
-    //
-    // TODO(rdsmith): Arguably we should let this go through, as this means
-    // the download really did get further than we know before it was
-    // cancelled.  But the gain isn't very large, and the code is more
-    // fragile if it has to support in progress updates in a non-in-progress
-    // state.  This issue should be readdressed when we revamp performance
-    // reporting.
-    return;
-  }
-  bytes_per_sec_ = bytes_per_sec;
-  hash_state_ = hash_state;
-  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 (bound_net_log_.IsLoggingAllEvents()) {
-    bound_net_log_.AddEvent(
-        net::NetLog::TYPE_DOWNLOAD_ITEM_UPDATED,
-        net::NetLog::Int64Callback("bytes_so_far", received_bytes_));
-  }
-
-  UpdateObservers();
-}
-
 void DownloadItemImpl::OnAllDataSaved(const std::string& final_hash) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
@@ -949,7 +909,8 @@ void DownloadItemImpl::DestinationUpdate(int64 bytes_so_far,
                                          int64 bytes_per_sec,
                                          const std::string& hash_state) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  VLOG(20) << __FUNCTION__ << " download=" << DebugString(true);
+  VLOG(20) << __FUNCTION__ << " so_far=" << bytes_so_far
+           << " per_sec=" << bytes_per_sec << " download=" << DebugString(true);
 
   if (!IsInProgress()) {
     // Ignore if we're no longer in-progress.  This can happen if we race a

content/browser/download/download_item_impl.h

@@ -196,19 +196,19 @@ class CONTENT_EXPORT DownloadItemImpl
   // Called by SavePackage to set the total number of bytes on the item.
   virtual void SetTotalBytes(int64 total_bytes);
 
-  // Indicate progress in saving data to its destination.
-  // |bytes_so_far| is the number of bytes received so far.
-  // |hash_state| is the current hash state.
-  virtual void UpdateProgress(int64 bytes_so_far,
-                              int64 bytes_per_sec,
-                              const std::string& hash_state);
-
   virtual void OnAllDataSaved(const std::string& final_hash);
 
   // Called by SavePackage to display progress when the DownloadItem
   // should be considered complete.
   virtual void MarkAsComplete();
 
+  // DownloadDestinationObserver
+  virtual void DestinationUpdate(int64 bytes_so_far,
+                                 int64 bytes_per_sec,
+                                 const std::string& hash_state) OVERRIDE;
+  virtual void DestinationError(DownloadInterruptReason reason) OVERRIDE;
+  virtual void DestinationCompleted(const std::string& final_hash) OVERRIDE;
+
  private:
   // Fine grained states of a download.
   enum DownloadInternalState {
@@ -240,13 +240,6 @@ class CONTENT_EXPORT DownloadItemImpl
     MAX_DOWNLOAD_INTERNAL_STATE,
   };
 
-  // DownloadDestinationObserver
-  virtual void DestinationUpdate(int64 bytes_so_far,
-                                 int64 bytes_per_sec,
-                                 const std::string& hash_state) OVERRIDE;
-  virtual void DestinationError(DownloadInterruptReason reason) OVERRIDE;
-  virtual void DestinationCompleted(const std::string& final_hash) OVERRIDE;
-
   // Normal progression of a download ------------------------------------------
 
   // These are listed in approximately chronological order.  There are also

content/browser/download/download_item_impl_unittest.cc

@@ -344,7 +344,7 @@ TEST_F(DownloadItemTest, NotificationAfterUpdate) {
   DownloadItemImpl* item = CreateDownloadItem();
   MockObserver observer(item);
 
-  item->UpdateProgress(kDownloadChunkSize, kDownloadSpeed, std::string());
+  item->DestinationUpdate(kDownloadChunkSize, kDownloadSpeed, std::string());
   ASSERT_TRUE(observer.CheckUpdated());
   EXPECT_EQ(kDownloadSpeed, item->CurrentSpeed());
 }

content/browser/download/save_package.cc

@@ -365,7 +365,7 @@ void SavePackage::OnMHTMLGenerated(const base::FilePath& path, int64 size) {
   // with SavePackage flow.
   if (download_->IsInProgress()) {
     download_->SetTotalBytes(size);
-    download_->UpdateProgress(size, 0, std::string());
+    download_->DestinationUpdate(size, 0, std::string());
     // Must call OnAllDataSaved here in order for
     // GDataDownloadObserver::ShouldUpload() to return true.
     // ShouldCompleteDownload() may depend on the gdata uploader to finish.
@@ -792,7 +792,7 @@ void SavePackage::Finish() {
     // with SavePackage flow.
     if (download_->IsInProgress()) {
       if (save_type_ != SAVE_PAGE_TYPE_AS_MHTML) {
-        download_->UpdateProgress(
+        download_->DestinationUpdate(
             all_save_items_count_, CurrentSpeed(), std::string());
         download_->OnAllDataSaved(DownloadItem::kEmptyFileHash);
       }
@@ -822,8 +822,10 @@ void SavePackage::SaveFinished(int32 save_id, int64 size, bool is_success) {
   // Hack to avoid touching download_ after user cancel.
   // TODO(rdsmith/benjhayden): Integrate canceling on DownloadItem
   // with SavePackage flow.
-  if (download_ && download_->IsInProgress())
-    download_->UpdateProgress(completed_count(), CurrentSpeed(), std::string());
+  if (download_ && download_->IsInProgress()) {
+    download_->DestinationUpdate(
+        completed_count(), CurrentSpeed(), std::string());
+  }
 
   if (save_item->save_source() == SaveFileCreateInfo::SAVE_FILE_FROM_DOM &&
       save_item->url() == page_url_ && !save_item->received_bytes()) {
@@ -867,8 +869,10 @@ void SavePackage::SaveFailed(const GURL& save_url) {
   // Hack to avoid touching download_ after user cancel.
   // TODO(rdsmith/benjhayden): Integrate canceling on DownloadItem
   // with SavePackage flow.
-  if (download_ && download_->IsInProgress())
-    download_->UpdateProgress(completed_count(), CurrentSpeed(), std::string());
+  if (download_ && download_->IsInProgress()) {
+    download_->DestinationUpdate(
+        completed_count(), CurrentSpeed(), std::string());
+  }
 
   if ((save_type_ == SAVE_PAGE_TYPE_AS_ONLY_HTML) ||
       (save_type_ == SAVE_PAGE_TYPE_AS_MHTML) ||