From cceb39bf4a939b70a563527a1df73e8d515605ad Mon Sep 17 00:00:00 2001 From: "caio.de.oliveira.filho@intel.com" Date: Wed, 15 May 2013 17:00:04 +0000 Subject: [PATCH] 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 | 1 + .../browser/download/download_item_impl.cc | 43 +------------------ content/browser/download/download_item_impl.h | 21 +++------ .../download/download_item_impl_unittest.cc | 2 +- content/browser/download/save_package.cc | 16 ++++--- 5 files changed, 21 insertions(+), 62 deletions(-) diff --git a/AUTHORS b/AUTHORS index ea35c5a05fc596..ee7d132aae7139 100644 --- a/AUTHORS +++ b/AUTHORS @@ -255,3 +255,4 @@ Aditya Bhargava John Yani Kangil Han Evan Wallace +Caio Marcelo de Oliveira Filho diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc index 1413393f00e013..f1e45f25590a23 100644 --- a/content/browser/download/download_item_impl.cc +++ b/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 diff --git a/content/browser/download/download_item_impl.h b/content/browser/download/download_item_impl.h index 083a1591ab4696..448742326965e6 100644 --- a/content/browser/download/download_item_impl.h +++ b/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 diff --git a/content/browser/download/download_item_impl_unittest.cc b/content/browser/download/download_item_impl_unittest.cc index 7a29f28e71ccae..fab8e996c746e8 100644 --- a/content/browser/download/download_item_impl_unittest.cc +++ b/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()); } diff --git a/content/browser/download/save_package.cc b/content/browser/download/save_package.cc index 7553a54f77217d..9bbe1662da7ad3 100644 --- a/content/browser/download/save_package.cc +++ b/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) ||