[Download] Treat RESUMING_INTERNAL as IN_PROGRESS in DownloadItem.

content/browser/download/download_item_impl_unittest.cc

Index: content/browser/download/download_item_impl_unittest.cc
diff --git a/content/browser/download/download_item_impl_unittest.cc b/content/browser/download/download_item_impl_unittest.cc
index 75fce50d7a8c3dca6153bbf7270e571afa147e3e..41aec9484fa3efc6cf360407cb6cbe30324b17c6 100644
--- a/content/browser/download/download_item_impl_unittest.cc
+++ b/content/browser/download/download_item_impl_unittest.cc
@@ -291,7 +291,8 @@ class DownloadItemTest : public testing::Test {
     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();
     }
@@ -420,7 +421,9 @@ TEST_F(DownloadItemTest, ContinueAfterInterrupted) {
   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
@@ -429,7 +432,9 @@ TEST_F(DownloadItemTest, ContinueAfterInterrupted) {
   // 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.
@@ -449,7 +454,7 @@ TEST_F(DownloadItemTest, RestartAfterInterrupted) {
   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.
@@ -476,7 +481,7 @@ TEST_F(DownloadItemTest, UnresumableInterrupt) {
   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) {
@@ -524,17 +529,16 @@ TEST_F(DownloadItemTest, LimitRestartsAfterInterrupted) {
                    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.
@@ -566,7 +570,6 @@ TEST_F(DownloadItemTest, ResumeUsingFinalURL) {
   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
@@ -575,7 +578,8 @@ TEST_F(DownloadItemTest, ResumeUsingFinalURL) {
   // 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) {