[Download] Treat RESUMING_INTERNAL as IN_PROGRESS in DownloadItem.

content/browser/download/download_item_impl_unittest.cc

 // Copyright (c) 2012 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.
 
 #include "base/callback.h"
 #include "base/feature_list.h"
 #include "base/message_loop/message_loop.h"
 #include "base/stl_util.h"
 #include "base/threading/thread.h"
 #include "content/browser/byte_stream.h"
@@ skipping 273 matching lines @@
   }
 
   // Cleanup a download item (specifically get rid of the DownloadFile on it).
   // The item must be in the expected state.
   void CleanupItem(DownloadItemImpl* item,
                    MockDownloadFile* download_file,
                    DownloadItem::DownloadState expected_state) {
     EXPECT_EQ(expected_state, item->GetState());
 
     if (expected_state == DownloadItem::IN_PROGRESS) {
-      EXPECT_CALL(*download_file, Cancel());
+      if (download_file)
+        EXPECT_CALL(*download_file, Cancel());
       item->Cancel(true);
       loop_.RunUntilIdle();
     }
   }
 
   // Destroy a previously created download item.
   void DestroyDownloadItem(DownloadItem* item) {
     allocated_downloads_.erase(item);
     delete item;
   }
@@ skipping 108 matching lines @@
   // Interrupt the download, using a continuable interrupt.
   EXPECT_CALL(*download_file, FullPath())
       .WillOnce(Return(base::FilePath()));
   EXPECT_CALL(*download_file, Detach());
   EXPECT_CALL(*mock_delegate(), GetBrowserContext())
       .WillRepeatedly(Return(&test_browser_context));
   EXPECT_CALL(*mock_delegate(), MockResumeInterruptedDownload(_, _)).Times(1);
   item->DestinationObserverAsWeakPtr()->DestinationError(
       DOWNLOAD_INTERRUPT_REASON_FILE_TRANSIENT_ERROR);
   ASSERT_TRUE(observer.CheckAndResetDownloadUpdated());
-  ASSERT_EQ(1, observer.interrupt_count());
+  // Since the download is resumed automatically, the interrupt count doesn't
+  // increase.
+  ASSERT_EQ(0, observer.interrupt_count());
 
   // Test expectations verify that ResumeInterruptedDownload() is called (by way
   // of MockResumeInterruptedDownload) after the download is interrupted. But
   // the mock doesn't follow through with the resumption.
   // ResumeInterruptedDownload() being called is sufficient for verifying that
   // the automatic resumption was triggered.
   RunAllPendingInMessageLoops();
 
-  CleanupItem(item, download_file, DownloadItem::INTERRUPTED);
+  // The download item is currently in RESUMING_INTERNAL state, which maps to
+  // IN_PROGRESS.
+  CleanupItem(item, nullptr, DownloadItem::IN_PROGRESS);
 }
 
 // Same as above, but with a non-continuable interrupt.
 TEST_F(DownloadItemTest, RestartAfterInterrupted) {
   DownloadItemImpl* item = CreateDownloadItem();
   TestDownloadItemObserver observer(item);
   MockDownloadFile* download_file =
       DoIntermediateRename(item, DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS);
 
   // Interrupt the download, using a restartable interrupt.
   EXPECT_CALL(*download_file, Cancel());
   item->DestinationObserverAsWeakPtr()->DestinationError(
       DOWNLOAD_INTERRUPT_REASON_FILE_FAILED);
   ASSERT_TRUE(observer.CheckAndResetDownloadUpdated());
   // Should not try to auto-resume.
   ASSERT_EQ(1, observer.interrupt_count());
   ASSERT_EQ(0, observer.resume_count());
   RunAllPendingInMessageLoops();
 
-  CleanupItem(item, download_file, DownloadItem::INTERRUPTED);
+  CleanupItem(item, nullptr, DownloadItem::INTERRUPTED);
 }
 
 // Check we do correct cleanup for RESUME_MODE_INVALID interrupts.
 TEST_F(DownloadItemTest, UnresumableInterrupt) {
   DownloadItemImpl* item = CreateDownloadItem();
   TestDownloadItemObserver observer(item);
   MockDownloadFile* download_file =
       DoIntermediateRename(item, DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS);
 
   // Fail final rename with unresumable reason.
   EXPECT_CALL(*mock_delegate(), ShouldCompleteDownload(item, _))
       .WillOnce(Return(true));
   EXPECT_CALL(*download_file, RenameAndAnnotate(base::FilePath(kDummyPath), _))
       .WillOnce(ScheduleRenameCallback(DOWNLOAD_INTERRUPT_REASON_FILE_BLOCKED,
                                        base::FilePath(kDummyPath)));
   EXPECT_CALL(*download_file, Cancel());
 
   // Complete download to trigger final rename.
   item->DestinationObserverAsWeakPtr()->DestinationCompleted(std::string());
   RunAllPendingInMessageLoops();
 
   ASSERT_TRUE(observer.CheckAndResetDownloadUpdated());
   // Should not try to auto-resume.
   ASSERT_EQ(1, observer.interrupt_count());
   ASSERT_EQ(0, observer.resume_count());
 
-  CleanupItem(item, download_file, DownloadItem::INTERRUPTED);
+  CleanupItem(item, nullptr, DownloadItem::INTERRUPTED);
 }
 
 TEST_F(DownloadItemTest, LimitRestartsAfterInterrupted) {
   TestBrowserContext test_browser_context;
   DownloadItemImpl* item = CreateDownloadItem();
   base::WeakPtr<DownloadDestinationObserver> as_observer(
       item->DestinationObserverAsWeakPtr());
   TestDownloadItemObserver observer(item);
   MockDownloadFile* mock_download_file(NULL);
   scoped_ptr<DownloadFile> download_file;
@@ skipping 27 matching lines @@
       base::FilePath target_path(kDummyPath);
       base::FilePath intermediate_path(
           target_path.InsertBeforeExtensionASCII("x"));
       EXPECT_CALL(*mock_download_file, RenameAndUniquify(intermediate_path, _))
           .WillOnce(ScheduleRenameCallback(DOWNLOAD_INTERRUPT_REASON_NONE,
                                            intermediate_path));
       callback.Run(target_path, DownloadItem::TARGET_DISPOSITION_OVERWRITE,
                    DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, intermediate_path);
       RunAllPendingInMessageLoops();
     }
-    ASSERT_EQ(i, observer.resume_count());
 
     // Use a continuable interrupt.
     item->DestinationObserverAsWeakPtr()->DestinationError(
         DOWNLOAD_INTERRUPT_REASON_FILE_TRANSIENT_ERROR);
 
-    ASSERT_EQ(i + 1, observer.interrupt_count());
     ::testing::Mock::VerifyAndClearExpectations(mock_download_file);
   }
 
-  CleanupItem(item, mock_download_file, DownloadItem::INTERRUPTED);
+  EXPECT_EQ(1, observer.interrupt_count());
+  CleanupItem(item, nullptr, DownloadItem::INTERRUPTED);
 }
 
 // Test that resumption uses the final URL in a URL chain when resuming.
 TEST_F(DownloadItemTest, ResumeUsingFinalURL) {
   TestBrowserContext test_browser_context;
   scoped_ptr<DownloadCreateInfo> create_info(new DownloadCreateInfo);
   create_info->save_info = scoped_ptr<DownloadSaveInfo>(new DownloadSaveInfo());
   create_info->save_info->prompt_for_save_location = false;
   create_info->etag = "SomethingToSatisfyResumption";
   create_info->url_chain.push_back(GURL("http://example.com/a"));
@@ skipping 11 matching lines @@
   EXPECT_CALL(*mock_delegate(), GetBrowserContext())
       .WillRepeatedly(Return(&test_browser_context));
   EXPECT_CALL(*mock_delegate(), MockResumeInterruptedDownload(
                                     Property(&DownloadUrlParameters::url,
                                              GURL("http://example.com/c")),
                                     _))
       .Times(1);
   item->DestinationObserverAsWeakPtr()->DestinationError(
       DOWNLOAD_INTERRUPT_REASON_FILE_TRANSIENT_ERROR);
   ASSERT_TRUE(observer.CheckAndResetDownloadUpdated());
-  ASSERT_EQ(1, observer.interrupt_count());

[svaldez, 2015/12/21 21:18:43]

Shouldn't we keep this line with ASSERT_EQ(0?

[asanka, 2015/12/21 21:46:10]

I chose to remove it because the assertion isn't pertinent to what's being
tested and only serves to make this a characterization test. The only thing that
we really want to test is that Delegate::ResumeInterruptedDownload() was called
with the correct URL.

Arguably, we should also remove the CheckAndResetDownloadUpdated() assertion as
well. So I removed that as well. As explained in the comment below, the
verification of test expectations at tear down is sufficient for the test to
fail if the correct ResumeInterruptedDownload() call was not seen.

The CheckAndResetDownloadUpdated() is necessary for the ContinueAfterInterrupted
test which specifically verifies the behavior of automatic resumption.

[svaldez, 2015/12/21 21:54:01]

Acknowledged.

 
   // Test expectations verify that ResumeInterruptedDownload() is called (by way
   // of MockResumeInterruptedDownload) after the download is interrupted. But
   // the mock doesn't follow through with the resumption.
   // ResumeInterruptedDownload() being called is sufficient for verifying that
   // the resumption was triggered.
   RunAllPendingInMessageLoops();
 
-  CleanupItem(item, download_file, DownloadItem::INTERRUPTED);
+  // The download is currently in RESUMING_INTERNAL, which maps to IN_PROGRESS.
+  CleanupItem(item, nullptr, DownloadItem::IN_PROGRESS);

[svaldez, 2015/12/21 21:18:44]

Should this be nullptr? We haven't Canceled DownloadItem in this test.

[asanka, 2015/12/21 21:46:10]

The DownloadResourceHandler/UrlDownloader and the DownloadFile are both
associated with a single network request. When a DownloadItem no longer has an
active network request (because it was cancelled, completed or interrupted),
those objects go away. In this case, the download was interrupted, and hence the
DownloadFile has also been destroyed already. A new one would be created once a
server response is received for the resumption request. But we end the test
before that point. Hence there's no DownloadFile here.

[svaldez, 2015/12/21 21:54:01]

Acknowledged.

 }
 
 TEST_F(DownloadItemTest, NotificationAfterRemove) {
   DownloadItemImpl* item = CreateDownloadItem();
   MockDownloadFile* download_file = AddDownloadFileToDownloadItem(item, NULL);
   EXPECT_CALL(*download_file, Cancel());
   EXPECT_CALL(*mock_delegate(), DownloadRemoved(_));
   TestDownloadItemObserver observer(item);
 
   item->Remove();
@@ skipping 720 matching lines @@
                  base::Unretained(&returned_path)));
   RunAllPendingInMessageLoops();
   EXPECT_TRUE(returned_path.empty());
 }
 
 TEST(MockDownloadItem, Compiles) {
   MockDownloadItem mock_item;
 }
 
 }  // namespace content