Stop using SpawnedTestServer in DownloadContentTest.*

content/browser/download/download_browsertest.cc

Index: content/browser/download/download_browsertest.cc
diff --git a/content/browser/download/download_browsertest.cc b/content/browser/download/download_browsertest.cc
index 9bad4d3f421135165c7113baa601d8c35b3ba4df..08afa40c5a2ad43de3f23476c21e043d7d759a66 100644
--- a/content/browser/download/download_browsertest.cc
+++ b/content/browser/download/download_browsertest.cc
@@ -5,10 +5,14 @@
 // This file contains download browser tests that are known to be runnable
 // in a pure content context.  Over time tests should be migrated here.
 
+#include <vector>
+
+#include "base/callback_helpers.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/format_macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
@@ -28,6 +32,7 @@
 #include "content/public/test/content_browser_test.h"
 #include "content/public/test/content_browser_test_utils.h"
 #include "content/public/test/download_test_observer.h"
+#include "content/public/test/test_download_request_handler.h"
 #include "content/public/test/test_file_error_injector.h"
 #include "content/public/test/test_utils.h"
 #include "content/shell/browser/shell.h"
@@ -37,7 +42,6 @@
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "net/test/embedded_test_server/http_request.h"
 #include "net/test/embedded_test_server/http_response.h"
-#include "net/test/spawned_test_server/spawned_test_server.h"
 #include "net/test/url_request/url_request_mock_http_job.h"
 #include "net/test/url_request/url_request_slow_download_job.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -378,71 +382,11 @@ class TestShellDownloadManagerDelegate : public ShellDownloadManagerDelegate {
   std::vector<DownloadOpenDelayedCallback> delayed_callbacks_;
 };
 
-// Record all state transitions and byte counts on the observed download.
-class RecordingDownloadObserver : DownloadItem::Observer {
- public:
-  struct RecordStruct {
-    DownloadItem::DownloadState state;
-    int bytes_received;
-  };
-
-  typedef std::vector<RecordStruct> RecordVector;
-
-  RecordingDownloadObserver(DownloadItem* download)
-      : download_(download) {
-    last_state_.state = download->GetState();
-    last_state_.bytes_received = download->GetReceivedBytes();
-    download_->AddObserver(this);
-  }
-
-  ~RecordingDownloadObserver() override { RemoveObserver(); }
-
-  void CompareToExpectedRecord(const RecordStruct expected[], size_t size) {
-    EXPECT_EQ(size, record_.size());
-    int min = size > record_.size() ? record_.size() : size;
-    for (int i = 0; i < min; ++i) {
-      EXPECT_EQ(expected[i].state, record_[i].state) << "Iteration " << i;
-      EXPECT_EQ(expected[i].bytes_received, record_[i].bytes_received)
-          << "Iteration " << i;
-    }
-  }
-
- private:
-  void OnDownloadUpdated(DownloadItem* download) override {
-    DCHECK_EQ(download_, download);
-    DownloadItem::DownloadState state = download->GetState();
-    int bytes = download->GetReceivedBytes();
-    if (last_state_.state != state || last_state_.bytes_received > bytes) {
-      last_state_.state = state;
-      last_state_.bytes_received = bytes;
-      record_.push_back(last_state_);
-    }
-  }
-
-  void OnDownloadDestroyed(DownloadItem* download) override {
-    DCHECK_EQ(download_, download);
-    RemoveObserver();
-  }
-
-  void RemoveObserver() {
-    if (download_) {
-      download_->RemoveObserver(this);
-      download_ = NULL;
-    }
-  }
-
-  DownloadItem* download_;
-  RecordStruct last_state_;
-  RecordVector record_;
-};
-
 // Get the next created download.
 class DownloadCreateObserver : DownloadManager::Observer {
  public:
   DownloadCreateObserver(DownloadManager* manager)
-      : manager_(manager),
-        item_(NULL),
-        waiting_(false) {
+      : manager_(manager), item_(NULL) {
     manager_->AddObserver(this);
   }
 
@@ -463,16 +407,16 @@ class DownloadCreateObserver : DownloadManager::Observer {
     if (!item_)
       item_ = download;
 
-    if (waiting_)
-      base::MessageLoopForUI::current()->QuitWhenIdle();
+    if (!completion_closure_.is_null())
+      base::ResetAndReturn(&completion_closure_).Run();
   }
 
   DownloadItem* WaitForFinished() {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
     if (!item_) {
-      waiting_ = true;
-      RunMessageLoop();
-      waiting_ = false;
+      base::RunLoop run_loop;
+      completion_closure_ = run_loop.QuitClosure();
+      run_loop.Run();
     }
     return item_;
   }
@@ -480,28 +424,20 @@ class DownloadCreateObserver : DownloadManager::Observer {
  private:
   DownloadManager* manager_;
   DownloadItem* item_;
-  bool waiting_;
+  base::Closure completion_closure_;
 };
 
-
-// Filter for waiting for a certain number of bytes.
-bool DataReceivedFilter(int number_of_bytes, DownloadItem* download) {
-  return download->GetReceivedBytes() >= number_of_bytes;
-}
-
 // Filter for download completion.
 bool DownloadCompleteFilter(DownloadItem* download) {
   return download->GetState() == DownloadItem::COMPLETE;

[svaldez, 2015/11/12 20:25:51]

Might just want to merge these into DownloadStateFilter(DownloadItem::State
state, DownloadItem* download) and then Bind appropriately on use.

[asanka, 2015/11/13 21:40:01]

Good call. I changed called it IsDownloadInState() since it's more of a
predicate now that it's not satisfying the Filter interface. 

 }
 
-// Filter for saving the size of the download when the first IN_PROGRESS
-// is hit.
-bool InitialSizeFilter(int* download_size, DownloadItem* download) {
-  if (download->GetState() != DownloadItem::IN_PROGRESS)
-    return false;
+bool DownloadInterruptFilter(DownloadItem* download) {
+  return download->GetState() == DownloadItem::INTERRUPTED;
+}
 
-  *download_size = download->GetReceivedBytes();
-  return true;
+bool DownloadInProgressFilter(DownloadItem* download) {
+  return download->GetState() == DownloadItem::IN_PROGRESS;
 }
 
 // Request handler to be used with CreateRedirectHandler().
@@ -530,12 +466,15 @@ net::EmbeddedTestServer::HandleRequestCallback CreateRedirectHandler(
 // Request handler to be used with CreateBasicResponseHandler().
 scoped_ptr<net::test_server::HttpResponse> HandleRequestAndSendBasicResponse(
     const std::string& relative_url,
+    const base::StringPairs& headers,
     const std::string& content_type,
     const std::string& body,
     const net::test_server::HttpRequest& request) {
   scoped_ptr<net::test_server::BasicHttpResponse> response;
   if (request.relative_url == relative_url) {
     response.reset(new net::test_server::BasicHttpResponse);
+    for (const auto& pair : headers)
+      response->AddCustomHeader(pair.first, pair.second);
     response->set_content_type(content_type);
     response->set_content(body);
   }
@@ -546,27 +485,15 @@ scoped_ptr<net::test_server::HttpResponse> HandleRequestAndSendBasicResponse(
 // HTTP 200 status code, a Content-Type header and a body.
 net::EmbeddedTestServer::HandleRequestCallback CreateBasicResponseHandler(
     const std::string& relative_url,
+    const base::StringPairs& headers,
     const std::string& content_type,
     const std::string& body) {
-  return base::Bind(
-      &HandleRequestAndSendBasicResponse, relative_url, content_type, body);
+  return base::Bind(&HandleRequestAndSendBasicResponse, relative_url, headers,
+                    content_type, body);
 }
 
-}  // namespace
-
 class DownloadContentTest : public ContentBrowserTest {
  protected:
-  // An initial send from a website of at least this size will not be
-  // help up by buffering in the underlying downloads ByteStream data
-  // transfer.  This is important because on resumption tests we wait
-  // until we've gotten the data we expect before allowing the test server
-  // to send its reset, to get around hard close semantics on the Windows
-  // socket layer implementation.
-  int GetSafeBufferChunk() const {
-    return (DownloadResourceHandler::kDownloadByteStreamSize /
-       ByteStreamWriter::kFractionBufferBeforeSending) + 1;
-  }
-
   void SetUpOnMainThread() override {
     ASSERT_TRUE(downloads_directory_.CreateUniqueTempDir());
 
@@ -601,19 +528,19 @@ class DownloadContentTest : public ContentBrowserTest {
         DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL);
   }
 
-  // Create a DownloadTestObserverInProgress that will wait for the
-  // specified number of downloads to start.
-  DownloadCreateObserver* CreateInProgressWaiter(
-      Shell* shell, int num_downloads) {
-    DownloadManager* download_manager = DownloadManagerForShell(shell);
-    return new DownloadCreateObserver(download_manager);
+  void WaitForInterrupt(DownloadItem* download) {
+    DownloadUpdatedObserver(download, base::Bind(&DownloadInterruptFilter))

[svaldez, 2015/11/12 20:25:51]

See above.

[asanka, 2015/11/13 21:40:01]

Done.

+        .WaitForEvent();
   }
 
-  DownloadTestObserver* CreateInterruptedWaiter(
-      Shell* shell, int num_downloads) {
-    DownloadManager* download_manager = DownloadManagerForShell(shell);
-    return new DownloadTestObserverInterrupted(download_manager, num_downloads,
-        DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_FAIL);
+  void WaitForInProgress(DownloadItem* download) {
+    DownloadUpdatedObserver(download, base::Bind(&DownloadInProgressFilter))
+        .WaitForEvent();
+  }
+
+  void WaitForCompletion(DownloadItem* download) {
+    DownloadUpdatedObserver(download, base::Bind(&DownloadCompleteFilter))
+        .WaitForEvent();
   }
 
   // Note: Cannot be used with other alternative DownloadFileFactorys
@@ -675,67 +602,35 @@ class DownloadContentTest : public ContentBrowserTest {
   // Start a download and return the item.
   DownloadItem* StartDownloadAndReturnItem(GURL url) {
     scoped_ptr<DownloadCreateObserver> observer(
-        CreateInProgressWaiter(shell(), 1));
+        new DownloadCreateObserver(DownloadManagerForShell(shell())));
     NavigateToURL(shell(), url);
-    observer->WaitForFinished();
-    std::vector<DownloadItem*> downloads;
-    DownloadManagerForShell(shell())->GetAllDownloads(&downloads);
-    EXPECT_EQ(1u, downloads.size());
-    if (1u != downloads.size())
-      return NULL;
-    return downloads[0];
+    return observer->WaitForFinished();
   }
 
-  // Wait for data
-  void WaitForData(DownloadItem* download, int size) {
-    DownloadUpdatedObserver data_observer(
-        download, base::Bind(&DataReceivedFilter, size));
-    data_observer.WaitForEvent();
-    ASSERT_EQ(size, download->GetReceivedBytes());
-    ASSERT_EQ(DownloadItem::IN_PROGRESS, download->GetState());
-  }
-
-  // Tell the test server to release a pending RST and confirm
-  // that the interrupt is received properly (for download resumption
-  // testing).
-  void ReleaseRSTAndConfirmInterruptForResume(DownloadItem* download) {
-    scoped_ptr<DownloadTestObserver> rst_observer(
-        CreateInterruptedWaiter(shell(), 1));
-    NavigateToURL(shell(), spawned_test_server()->GetURL("download-finish"));
-    rst_observer->WaitForFinished();
-    EXPECT_EQ(DownloadItem::INTERRUPTED, download->GetState());
-  }
-
-  // Confirm file status expected for the given location in a stream
-  // provided by the resume test server.
-  void ConfirmFileStatusForResume(
-      DownloadItem* download, bool file_exists,
-      int received_bytes, int total_bytes,
-      const base::FilePath& expected_filename) {
-    // expected_filename is only known if the file exists.
-    ASSERT_EQ(file_exists, !expected_filename.empty());
-    EXPECT_EQ(received_bytes, download->GetReceivedBytes());
-    EXPECT_EQ(total_bytes, download->GetTotalBytes());
-    EXPECT_EQ(expected_filename.value(),
-              download->GetFullPath().BaseName().value());
-    EXPECT_EQ(file_exists,
-              (!download->GetFullPath().empty() &&
-               base::PathExists(download->GetFullPath())));
-
-    if (file_exists) {
-      std::string file_contents;
-      EXPECT_TRUE(base::ReadFileToString(
-          download->GetFullPath(), &file_contents));
-
-      ASSERT_EQ(static_cast<size_t>(received_bytes), file_contents.size());
-      for (int i = 0; i < received_bytes; ++i) {
-        EXPECT_EQ(static_cast<char>((i * 2 + 15) % 256), file_contents[i])
-            << "File contents diverged at position " << i
-            << " for " << expected_filename.value();
-
-        if (static_cast<char>((i * 2 + 15) % 256) != file_contents[i])
-          return;
-      }
+  static void ReadAndVerifyFileContents(int seed,
+                                        int64_t expected_size,
+                                        const base::FilePath& path) {
+    base::File file(path, base::File::FLAG_OPEN | base::File::FLAG_READ);
+    ASSERT_TRUE(file.IsValid());
+    int64_t file_length = file.GetLength();
+    ASSERT_EQ(expected_size, file_length);
+
+    const int64_t kBufferSize = 64 * 1024;
+    std::vector<char> pattern;
+    std::vector<char> data;
+    pattern.resize(kBufferSize);
+    data.resize(kBufferSize);
+    for (int64_t offset = 0; offset < file_length;) {
+      int bytes_read = file.Read(offset, &data.front(), kBufferSize);
+      ASSERT_LT(0, bytes_read);
+      ASSERT_GE(kBufferSize, bytes_read);
+
+      TestDownloadRequestHandler::GetPatternBytes(seed, offset, bytes_read,
+                                                  &pattern.front());
+      ASSERT_EQ(0, memcmp(&pattern.front(), &data.front(), bytes_read))
+          << "Comparing block at offset " << offset << " and length "
+          << bytes_read;
+      offset += bytes_read;
     }
   }
 
@@ -752,23 +647,19 @@ class DownloadContentTest : public ContentBrowserTest {
   scoped_ptr<TestShellDownloadManagerDelegate> test_delegate_;
 };
 
+}  // namespace
+
 IN_PROC_BROWSER_TEST_F(DownloadContentTest, DownloadCancelled) {
   SetupEnsureNoPendingDownloads();
 
   // Create a download, wait until it's started, and confirm
   // we're in the expected state.
-  scoped_ptr<DownloadCreateObserver> observer(
-      CreateInProgressWaiter(shell(), 1));
-  NavigateToURL(shell(), GURL(net::URLRequestSlowDownloadJob::kUnknownSizeUrl));
-  observer->WaitForFinished();
-
-  std::vector<DownloadItem*> downloads;
-  DownloadManagerForShell(shell())->GetAllDownloads(&downloads);
-  ASSERT_EQ(1u, downloads.size());
-  ASSERT_EQ(DownloadItem::IN_PROGRESS, downloads[0]->GetState());
+  DownloadItem* download = StartDownloadAndReturnItem(
+      GURL(net::URLRequestSlowDownloadJob::kUnknownSizeUrl));
+  ASSERT_EQ(DownloadItem::IN_PROGRESS, download->GetState());

[svaldez, 2015/11/12 20:25:51]

Can this race and be in COMPLETE state?

[asanka, 2015/11/13 21:40:01]

Great catch!
In practice we get lucky, but only because of a couple of implementation details
that we shouldn't depend on. This is especially true since that area of code
(navigation) is currently under flux.

I.e. we are indirectly calling NavigateToURLBlockUntilNavigationsComplete() and
assuming that the DownloadItem hasn't progressed past IN_PROGRESS by the time
the navigation completes. The ResourceLoader stack simulates a cancellation to
the AsyncResourceHandler before the DownloadResourceHandler takes over. This
cancellation propagates into the renderer, and back to the browser and causes
the WeContents to signal a stop loading event. This has to consistently happen
before the DownloadResourceHandler triggers a new download to be created, the
new download determines its target, and that download slurps enough data to
progress past the IN_PROGRESS state.

I'm reluctant to introduce unrelated changes in the same CL, so I'll spin up a
new one and send it your way to change how we handle
StartDownloadAndReturnItem().

[asanka, 2015/11/14 03:32:47]

Actually, no. I misspoke. Here and in the instance below, we are invoking
URLRequestSlowDownloadJob. This particular test fixture is set up to deal with
this kind of race (doh!). The response doesn't progress past a specific point
until another request is made (to kFinishDownloadJob or kErrorDownloadJob). This
ensures that the download item, once created, doesn't make it past the
IN_PROGRESS state.

 
   // Cancel the download and wait for download system quiesce.
-  downloads[0]->Cancel(true);
+  download->Cancel(true);
   scoped_refptr<DownloadTestFlushObserver> flush_observer(
       new DownloadTestFlushObserver(DownloadManagerForShell(shell())));
   flush_observer->WaitForFlush();
@@ -784,28 +675,14 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, MultiDownload) {
 
   // Create a download, wait until it's started, and confirm
   // we're in the expected state.
-  scoped_ptr<DownloadCreateObserver> observer1(
-      CreateInProgressWaiter(shell(), 1));
-  NavigateToURL(shell(), GURL(net::URLRequestSlowDownloadJob::kUnknownSizeUrl));
-  observer1->WaitForFinished();
-
-  std::vector<DownloadItem*> downloads;
-  DownloadManagerForShell(shell())->GetAllDownloads(&downloads);
-  ASSERT_EQ(1u, downloads.size());
-  ASSERT_EQ(DownloadItem::IN_PROGRESS, downloads[0]->GetState());
-  DownloadItem* download1 = downloads[0];  // The only download.
+  DownloadItem* download1 = StartDownloadAndReturnItem(
+      GURL(net::URLRequestSlowDownloadJob::kUnknownSizeUrl));
+  ASSERT_EQ(DownloadItem::IN_PROGRESS, download1->GetState());
 
   // Start the second download and wait until it's done.
   GURL url(net::URLRequestMockHTTPJob::GetMockUrl("download-test.lib"));
-  // Download the file and wait.
-  NavigateToURLAndWaitForDownload(shell(), url, DownloadItem::COMPLETE);
-
-  // Should now have 2 items on the manager.
-  downloads.clear();
-  DownloadManagerForShell(shell())->GetAllDownloads(&downloads);
-  ASSERT_EQ(2u, downloads.size());
-  // We don't know the order of the downloads.
-  DownloadItem* download2 = downloads[(download1 == downloads[0]) ? 1 : 0];
+  DownloadItem* download2 = StartDownloadAndReturnItem(url);
+  WaitForCompletion(download2);
 
   ASSERT_EQ(DownloadItem::IN_PROGRESS, download1->GetState());
   ASSERT_EQ(DownloadItem::COMPLETE, download2->GetState());
@@ -962,21 +839,15 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, CancelAtRelease) {
 // works properly.
 IN_PROC_BROWSER_TEST_F(DownloadContentTest, ShutdownInProgress) {
   // Create a download that won't complete.
-  scoped_ptr<DownloadCreateObserver> observer(
-      CreateInProgressWaiter(shell(), 1));
-  NavigateToURL(shell(), GURL(net::URLRequestSlowDownloadJob::kUnknownSizeUrl));
-  observer->WaitForFinished();
+  DownloadItem* download = StartDownloadAndReturnItem(
+      GURL(net::URLRequestSlowDownloadJob::kUnknownSizeUrl));
 
-  // Get the item.
-  std::vector<DownloadItem*> items;
-  DownloadManagerForShell(shell())->GetAllDownloads(&items);
-  ASSERT_EQ(1u, items.size());
-  EXPECT_EQ(DownloadItem::IN_PROGRESS, items[0]->GetState());
+  EXPECT_EQ(DownloadItem::IN_PROGRESS, download->GetState());

[svaldez, 2015/11/12 20:25:51]

See above.

[asanka, 2015/11/13 21:40:00]

Ditto.

 
   // Shutdown the download manager and make sure we get the right
   // notifications in the right order.
   StrictMock<MockDownloadItemObserver> item_observer;
-  items[0]->AddObserver(&item_observer);
+  download->AddObserver(&item_observer);
   MockDownloadManagerObserver manager_observer(
       DownloadManagerForShell(shell()));
   // Don't care about ModelChanged() events.
@@ -988,11 +859,12 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, ShutdownInProgress) {
     EXPECT_CALL(manager_observer, MockManagerGoingDown(
         DownloadManagerForShell(shell())))
         .WillOnce(Return());
-    EXPECT_CALL(item_observer, OnDownloadUpdated(
-        AllOf(items[0],
-              Property(&DownloadItem::GetState, DownloadItem::CANCELLED))))
+    EXPECT_CALL(
+        item_observer,
+        OnDownloadUpdated(AllOf(download, Property(&DownloadItem::GetState,
+                                                   DownloadItem::CANCELLED))))
         .WillOnce(Return());
-    EXPECT_CALL(item_observer, OnDownloadDestroyed(items[0]))
+    EXPECT_CALL(item_observer, OnDownloadDestroyed(download))
         .WillOnce(Return());
   }
 
@@ -1009,7 +881,6 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, ShutdownInProgress) {
   BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
                           base::Bind(&base::PlatformThread::Sleep,
                                      base::TimeDelta::FromMilliseconds(25)));
-  items.clear();
 }
 
 // Try to shutdown just after we release the download file, by delaying
@@ -1068,288 +939,231 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, ShutdownAtRelease) {
   DownloadManagerForShell(shell())->Shutdown();
 }
 
-IN_PROC_BROWSER_TEST_F(DownloadContentTest, ResumeInterruptedDownload) {
+IN_PROC_BROWSER_TEST_F(DownloadContentTest, Resume_BasicWithStrongValidators) {

[Randy Smith (Not in Mondays), 2015/11/12 00:57:57]

Does this really involve basic auth?  It's not obvious at the test level (or at
the TestDownloadRequestHandler .h file level) if so.

[asanka, 2015/11/13 21:40:00]

Basic meaning it's a basic test with no additional complications. :) I guess
I'll remove the "basic" part since it's confusing.

   base::CommandLine::ForCurrentProcess()->AppendSwitch(
       switches::kEnableDownloadResumption);
-  ASSERT_TRUE(spawned_test_server()->Start());
 
-  GURL url = spawned_test_server()->GetURL(
-      base::StringPrintf("rangereset?size=%d&rst_boundary=%d",
-                         GetSafeBufferChunk() * 3, GetSafeBufferChunk()));
+  TestDownloadRequestHandler request_handler;
+  auto parameters =
+      TestDownloadRequestHandler::Parameters::WithSingleInterruption();
+  auto interruption = parameters.injected_errors.front();

[Randy Smith (Not in Mondays), 2015/11/12 00:57:57]

nit, suggestion: I'm a little uncomfortable with auto here, specifically because
it doesn't surface that you're copying a value rather than a pointer.  Willing
to replace with actual type?

[asanka, 2015/11/13 21:40:01]

Done.

+  request_handler.StartServing(parameters);
 
-  MockDownloadManagerObserver dm_observer(DownloadManagerForShell(shell()));
-  EXPECT_CALL(dm_observer, OnDownloadCreated(_,_)).Times(1);
-
-  DownloadItem* download(StartDownloadAndReturnItem(url));
-  WaitForData(download, GetSafeBufferChunk());
-  ::testing::Mock::VerifyAndClearExpectations(&dm_observer);
+  DownloadItem* download = StartDownloadAndReturnItem(request_handler.url());
+  WaitForInterrupt(download);
 
-  // Confirm resumption while in progress doesn't do anything.
-  download->Resume();
-  ASSERT_EQ(GetSafeBufferChunk(), download->GetReceivedBytes());
-  ASSERT_EQ(DownloadItem::IN_PROGRESS, download->GetState());
+  ASSERT_EQ(interruption.offset, download->GetReceivedBytes());
+  ASSERT_EQ(parameters.size, download->GetTotalBytes());
 
-  // Tell the server to send the RST and confirm the interrupt happens.
-  ReleaseRSTAndConfirmInterruptForResume(download);
-  ConfirmFileStatusForResume(
-      download, true, GetSafeBufferChunk(), GetSafeBufferChunk() * 3,
-      base::FilePath(FILE_PATH_LITERAL("rangereset.crdownload")));
-
-  // Resume, confirming received bytes on resumption is correct.
-  // Make sure no creation calls are included.
-  EXPECT_CALL(dm_observer, OnDownloadCreated(_,_)).Times(0);
-  int initial_size = 0;
-  DownloadUpdatedObserver initial_size_observer(
-      download, base::Bind(&InitialSizeFilter, &initial_size));
-  download->Resume();
-  initial_size_observer.WaitForEvent();
-  EXPECT_EQ(GetSafeBufferChunk(), initial_size);
-  ::testing::Mock::VerifyAndClearExpectations(&dm_observer);
-
-  // and wait for expected data.
-  WaitForData(download, GetSafeBufferChunk() * 2);
-
-  // Tell the server to send the RST and confirm the interrupt happens.
-  ReleaseRSTAndConfirmInterruptForResume(download);
-  ConfirmFileStatusForResume(
-      download, true, GetSafeBufferChunk() * 2, GetSafeBufferChunk() * 3,
-      base::FilePath(FILE_PATH_LITERAL("rangereset.crdownload")));
-
-  // Resume and wait for completion.
-  DownloadUpdatedObserver completion_observer(
-      download, base::Bind(DownloadCompleteFilter));
   download->Resume();
-  completion_observer.WaitForEvent();
-
-  ConfirmFileStatusForResume(
-      download, true, GetSafeBufferChunk() * 3, GetSafeBufferChunk() * 3,
-      base::FilePath(FILE_PATH_LITERAL("rangereset")));
-
-  // Confirm resumption while complete doesn't do anything.
-  download->Resume();
-  ASSERT_EQ(GetSafeBufferChunk() * 3, download->GetReceivedBytes());
-  ASSERT_EQ(DownloadItem::COMPLETE, download->GetState());
-  RunAllPendingInMessageLoop();
-  ASSERT_EQ(GetSafeBufferChunk() * 3, download->GetReceivedBytes());
-  ASSERT_EQ(DownloadItem::COMPLETE, download->GetState());
+  WaitForCompletion(download);
+
+  ASSERT_EQ(parameters.size, download->GetReceivedBytes());
+  ASSERT_EQ(parameters.size, download->GetTotalBytes());
+  ASSERT_NO_FATAL_FAILURE(ReadAndVerifyFileContents(
+      parameters.pattern_generator_seed, parameters.size,
+      download->GetTargetFilePath()));
+
+  // Characterization risk: The next portion of the test examines the requests
+  // that were sent out while downloading our resource. These requests
+  // correspond to the requests that were generated by the browser and the
+  // downloads system and may change as implementation details change.

[Randy Smith (Not in Mondays), 2015/11/12 00:57:56]

Suggestion: I'm not sure if it's worth it, but I think what you really want to
check here is that after the interruption, the next request attempted to
download the range past the interruption (whatever the server did or the
download system did pre-interruption).  I think you could write it that way;
search for the request that failed, and look at what the one after it asked for.
 

(And yay for thinking about characterization vs. behavior.)

[asanka, 2015/11/13 21:40:01]

Yeah. I was thinking of ways to generalize it. The fact that we request the
range from the very next byte onwards is also, IMO, an implementation detail. In
the past we've thrown around doing slightly overlapping ranges or spot checking
to verify that the byte ranges we are getting agrees with the ranges we already
have.

The most general way to check, might be to verify that in the end we have all
the bits for the requested resource and then verify that the sum of the data
transferred over the wire is substantially more economical than starting the
request from scratch. I.e. sum of data transferred < (size received before
resumption + size of entire resource). But that seemed excessively weak.

I'm open for suggestions, but overall, I couldn't convince myself that it's a
win to generalize this further in order to avoid a characterization risk.

[Randy Smith (Not in Mondays), 2015/11/17 22:21:42]

Hmmm.  Maybe have an observer track total bytes transferred and just do a
comparison?  

But I don't have any strong opinions here, I just agree with you that it's worth
some thought to see if we can test behavior rather than characterization.  But I
think we've satisfied honor :-}.

+  TestDownloadRequestHandler::CompletedRequests requests;
+  request_handler.GetCompletedRequestInfo(&requests);
+
+  ASSERT_EQ(2u, requests.size());
+
+  // The first request only transferrs bytes up until the interruption point.
+  EXPECT_EQ(interruption.offset, requests[0].transferred_content_length);
+
+  // The next request should only have transferred the remainder of the
+  // resource.
+  EXPECT_EQ(parameters.size - interruption.offset,
+            requests[1].transferred_content_length);
+
+  std::string value;
+  ASSERT_TRUE(requests[1].request_headers.GetHeader(
+      net::HttpRequestHeaders::kIfRange, &value));
+  EXPECT_EQ(parameters.etag, value);
+
+  ASSERT_TRUE(requests[1].request_headers.GetHeader(
+      net::HttpRequestHeaders::kRange, &value));
+  EXPECT_EQ(base::StringPrintf("bytes=%" PRId64 "-", interruption.offset),
+            value);
 }
 
-// Confirm restart fallback happens if a range request is bounced.
-IN_PROC_BROWSER_TEST_F(DownloadContentTest, ResumeInterruptedDownloadNoRange) {
-  base::CommandLine::ForCurrentProcess()->AppendSwitch(
-      switches::kEnableDownloadResumption);
-  ASSERT_TRUE(spawned_test_server()->Start());
-
-  // Auto-restart if server doesn't handle ranges.
-  GURL url = spawned_test_server()->GetURL(base::StringPrintf(
-      // First download hits an RST, rest don't, no ranges.
-      "rangereset?size=%d&rst_boundary=%d&"
-      "token=NoRange&rst_limit=1&bounce_range",
-      GetSafeBufferChunk() * 3, GetSafeBufferChunk()));
-
-  // Start the download and wait for first data chunk.
-  DownloadItem* download(StartDownloadAndReturnItem(url));
-  WaitForData(download, GetSafeBufferChunk());
-
-  RecordingDownloadObserver recorder(download);
-
-  ReleaseRSTAndConfirmInterruptForResume(download);
-  ConfirmFileStatusForResume(
-      download, true, GetSafeBufferChunk(), GetSafeBufferChunk() * 3,
-      base::FilePath(FILE_PATH_LITERAL("rangereset.crdownload")));
-
-  DownloadUpdatedObserver completion_observer(
-      download, base::Bind(DownloadCompleteFilter));
-  download->Resume();
-  completion_observer.WaitForEvent();
-
-  ConfirmFileStatusForResume(
-      download, true, GetSafeBufferChunk() * 3, GetSafeBufferChunk() * 3,
-      base::FilePath(FILE_PATH_LITERAL("rangereset")));
-
-  static const RecordingDownloadObserver::RecordStruct expected_record[] = {
-    // Result of RST
-    {DownloadItem::INTERRUPTED, GetSafeBufferChunk()},
-    // Starting continuation
-    {DownloadItem::IN_PROGRESS, GetSafeBufferChunk()},
-    // Notification of receiving whole file.
-    {DownloadItem::IN_PROGRESS, 0},
-    // Completion.
-    {DownloadItem::COMPLETE, GetSafeBufferChunk() * 3},
-  };
-
-  recorder.CompareToExpectedRecord(expected_record, arraysize(expected_record));
-}
-
-// Confirm restart fallback happens if a precondition is failed.
+// A partial resumption results in an HTTP 200 response. I.e. ther server

[svaldez, 2015/11/12 20:25:51]

*the

[asanka, 2015/11/13 21:40:01]

Done.

+// ignored the range request and sent the entire resource instead. For If-Range
+// requests (as opposed to If-Match), the behavior for a precondition failure is
+// also to respond with a 200. So this test case covers both validation failure
+// and ignoring the range request.
 IN_PROC_BROWSER_TEST_F(DownloadContentTest,
-                       ResumeInterruptedDownloadBadPrecondition) {
+                       Resume_RestartIfNotPartialResponse) {
   base::CommandLine::ForCurrentProcess()->AppendSwitch(
       switches::kEnableDownloadResumption);
-  ASSERT_TRUE(spawned_test_server()->Start());
+  const int kOriginalPatternGeneratorSeed = 1;
+  const int kNewPatternGeneratorSeed = 2;
 
-  GURL url = spawned_test_server()->GetURL(base::StringPrintf(
-      // First download hits an RST, rest don't, precondition fail.
-      "rangereset?size=%d&rst_boundary=%d&"
-      "token=BadPrecondition&rst_limit=1&fail_precondition=2",
-      GetSafeBufferChunk() * 3, GetSafeBufferChunk()));
+  TestDownloadRequestHandler::Parameters parameters =
+      TestDownloadRequestHandler::Parameters::WithSingleInterruption();
+  parameters.pattern_generator_seed = kOriginalPatternGeneratorSeed;
+  const TestDownloadRequestHandler::InjectedError interruption =

[Randy Smith (Not in Mondays), 2015/11/12 00:57:57]

I would like you to be consistent about use or lack of use of auto in
TestDownloadRequestHandler consumers.

[asanka, 2015/11/13 21:40:01]

Yup. Did a consistency pass.

+      parameters.injected_errors.front();
 
-  // Start the download and wait for first data chunk.
-  DownloadItem* download(StartDownloadAndReturnItem(url));
-  WaitForData(download, GetSafeBufferChunk());
+  TestDownloadRequestHandler request_handler;
+  request_handler.StartServing(parameters);
 
-  RecordingDownloadObserver recorder(download);
+  DownloadItem* download = StartDownloadAndReturnItem(request_handler.url());
+  WaitForInterrupt(download);
 
-  ReleaseRSTAndConfirmInterruptForResume(download);
-  ConfirmFileStatusForResume(
-      download, true, GetSafeBufferChunk(), GetSafeBufferChunk() * 3,
-      base::FilePath(FILE_PATH_LITERAL("rangereset.crdownload")));
-  EXPECT_EQ("BadPrecondition2", download->GetETag());
+  ASSERT_EQ(interruption.offset, download->GetReceivedBytes());
+  ASSERT_EQ(parameters.size, download->GetTotalBytes());
 
-  DownloadUpdatedObserver completion_observer(
-      download, base::Bind(DownloadCompleteFilter));
-  download->Resume();
-  completion_observer.WaitForEvent();
+  parameters = TestDownloadRequestHandler::Parameters();
+  parameters.support_byte_ranges = false;
+  parameters.pattern_generator_seed = kNewPatternGeneratorSeed;
+  request_handler.StartServing(parameters);
 
-  ConfirmFileStatusForResume(
-      download, true, GetSafeBufferChunk() * 3, GetSafeBufferChunk() * 3,
-      base::FilePath(FILE_PATH_LITERAL("rangereset")));
-  EXPECT_EQ("BadPrecondition0", download->GetETag());
-
-  static const RecordingDownloadObserver::RecordStruct expected_record[] = {
-    // Result of RST
-    {DownloadItem::INTERRUPTED, GetSafeBufferChunk()},
-    // Starting continuation
-    {DownloadItem::IN_PROGRESS, GetSafeBufferChunk()},
-    // Server precondition fail.
-    {DownloadItem::INTERRUPTED, 0},
-    // Notification of successful restart.
-    {DownloadItem::IN_PROGRESS, 0},
-    // Completion.
-    {DownloadItem::COMPLETE, GetSafeBufferChunk() * 3},
-  };
-
-  recorder.CompareToExpectedRecord(expected_record, arraysize(expected_record));
+  download->Resume();
+  WaitForCompletion(download);
+
+  ASSERT_EQ(parameters.size, download->GetReceivedBytes());
+  ASSERT_EQ(parameters.size, download->GetTotalBytes());
+  ASSERT_NO_FATAL_FAILURE(
+      ReadAndVerifyFileContents(kNewPatternGeneratorSeed, parameters.size,
+                                download->GetTargetFilePath()));
+
+  // Characterization risk: The next portion of the test examines the requests
+  // that were sent out while downloading our resource. These requests
+  // correspond to the requests that were generated by the browser and the
+  // downloads system and may change as implementation details change.

[Randy Smith (Not in Mondays), 2015/11/12 00:57:57]

This is pretty much the same test as above, right?  Is it worth abstracting it
into a function, so that if it needs to change later it can more easily?

[asanka, 2015/11/13 21:40:00]

This one asserts that the resumption resulted in a restart whereas the previous
test asserts that resumption resulted in a continuation.

After reconsideration, I now believe that it is not a characterization risk to
assume that there will be two requests on the wire:
* The first one that transfers up to the interruption point.
* The second request that transfers the whole resource.

I've updated the test accordingly.

+  TestDownloadRequestHandler::CompletedRequests requests;
+  request_handler.GetCompletedRequestInfo(&requests);
+
+  ASSERT_EQ(2u, requests.size());
+
+  // The first request only sends data up to the interruption point.
+  EXPECT_EQ(interruption.offset, requests[0].transferred_content_length);
+
+  // The second request sends the entire response.
+  EXPECT_EQ(parameters.size, requests[1].transferred_content_length);
+
+  std::string value;
+  ASSERT_TRUE(requests[1].request_headers.GetHeader(
+      net::HttpRequestHeaders::kIfRange, &value));
+  EXPECT_EQ(parameters.etag, value);
+
+  ASSERT_TRUE(requests[1].request_headers.GetHeader(
+      net::HttpRequestHeaders::kRange, &value));
+  EXPECT_EQ(base::StringPrintf("bytes=%" PRId64 "-", interruption.offset),
+            value);
 }
 
 // Confirm we don't try to resume if we don't have a verifier.

[Randy Smith (Not in Mondays), 2015/11/12 00:57:57]

nit, suggestion: I *think* the terminology we settled on was "restart" == start
from beginning, "continue" == range request where we left off, "resume" == union
of those two.

[asanka, 2015/11/13 21:40:00]

You're correct. Updated.

-IN_PROC_BROWSER_TEST_F(DownloadContentTest,
-                       ResumeInterruptedDownloadNoVerifiers) {
+IN_PROC_BROWSER_TEST_F(DownloadContentTest, Resume_RestartIfNoETag) {
   base::CommandLine::ForCurrentProcess()->AppendSwitch(
       switches::kEnableDownloadResumption);
-  ASSERT_TRUE(spawned_test_server()->Start());
-
-  GURL url = spawned_test_server()->GetURL(base::StringPrintf(
-      // First download hits an RST, rest don't, no verifiers.
-      "rangereset?size=%d&rst_boundary=%d&"
-      "token=NoRange&rst_limit=1&no_verifiers",
-      GetSafeBufferChunk() * 3, GetSafeBufferChunk()));
+  const int kOriginalPatternGeneratorSeed = 1;
+  const int kNewPatternGeneratorSeed = 2;
 
-  // Start the download and wait for first data chunk.
-  DownloadItem* download(StartDownloadAndReturnItem(url));
-  WaitForData(download, GetSafeBufferChunk());
+  TestDownloadRequestHandler::Parameters parameters =
+      TestDownloadRequestHandler::Parameters::WithSingleInterruption();
+  ASSERT_EQ(1u, parameters.injected_errors.size());
+  parameters.etag.clear();
+  parameters.pattern_generator_seed = kOriginalPatternGeneratorSeed;
 
-  RecordingDownloadObserver recorder(download);
+  TestDownloadRequestHandler request_handler;
+  request_handler.StartServing(parameters);
+  DownloadItem* download = StartDownloadAndReturnItem(request_handler.url());
+  WaitForInterrupt(download);
 
-  ReleaseRSTAndConfirmInterruptForResume(download);
-  ConfirmFileStatusForResume(
-      download, false, GetSafeBufferChunk(), GetSafeBufferChunk() * 3,
-      base::FilePath());
+  parameters.pattern_generator_seed = kNewPatternGeneratorSeed;
+  parameters.ClearInjectedErrors();
+  request_handler.StartServing(parameters);
 
-  DownloadUpdatedObserver completion_observer(
-      download, base::Bind(DownloadCompleteFilter));
   download->Resume();
-  completion_observer.WaitForEvent();
-
-  ConfirmFileStatusForResume(
-      download, true, GetSafeBufferChunk() * 3, GetSafeBufferChunk() * 3,
-      base::FilePath(FILE_PATH_LITERAL("rangereset")));
-
-  static const RecordingDownloadObserver::RecordStruct expected_record[] = {
-    // Result of RST
-    {DownloadItem::INTERRUPTED, GetSafeBufferChunk()},
-    // Restart for lack of verifiers
-    {DownloadItem::IN_PROGRESS, 0},
-    // Completion.
-    {DownloadItem::COMPLETE, GetSafeBufferChunk() * 3},
-  };
-
-  recorder.CompareToExpectedRecord(expected_record, arraysize(expected_record));
+  WaitForCompletion(download);
+
+  ASSERT_EQ(parameters.size, download->GetReceivedBytes());
+  ASSERT_EQ(parameters.size, download->GetTotalBytes());
+  ASSERT_NO_FATAL_FAILURE(
+      ReadAndVerifyFileContents(kNewPatternGeneratorSeed, parameters.size,
+                                download->GetTargetFilePath()));
+
+  TestDownloadRequestHandler::CompletedRequests requests;
+  request_handler.GetCompletedRequestInfo(&requests);
+
+  // Neither If-Range nor Range headers should be present in the second request.
+  ASSERT_EQ(2u, requests.size());
+  std::string value;
+  EXPECT_FALSE(requests[1].request_headers.GetHeader(
+      net::HttpRequestHeaders::kIfRange, &value));
+  EXPECT_FALSE(requests[1].request_headers.GetHeader(
+      net::HttpRequestHeaders::kRange, &value));
 }
 
-IN_PROC_BROWSER_TEST_F(DownloadContentTest, ResumeWithDeletedFile) {
+IN_PROC_BROWSER_TEST_F(DownloadContentTest, Resume_RestartIfNoPartialFile) {
   base::CommandLine::ForCurrentProcess()->AppendSwitch(
       switches::kEnableDownloadResumption);
-  ASSERT_TRUE(spawned_test_server()->Start());
-
-  GURL url = spawned_test_server()->GetURL(base::StringPrintf(
-      // First download hits an RST, rest don't
-      "rangereset?size=%d&rst_boundary=%d&"
-      "token=NoRange&rst_limit=1",
-      GetSafeBufferChunk() * 3, GetSafeBufferChunk()));
-
-  // Start the download and wait for first data chunk.
-  DownloadItem* download(StartDownloadAndReturnItem(url));
-  WaitForData(download, GetSafeBufferChunk());
+  TestDownloadRequestHandler::Parameters parameters =
+      TestDownloadRequestHandler::Parameters::WithSingleInterruption();
 
-  RecordingDownloadObserver recorder(download);
-
-  ReleaseRSTAndConfirmInterruptForResume(download);
-  ConfirmFileStatusForResume(
-      download, true, GetSafeBufferChunk(), GetSafeBufferChunk() * 3,
-      base::FilePath(FILE_PATH_LITERAL("rangereset.crdownload")));
+  TestDownloadRequestHandler request_handler;
+  request_handler.StartServing(parameters);
+  DownloadItem* download = StartDownloadAndReturnItem(request_handler.url());
+  WaitForInterrupt(download);
 
   // Delete the intermediate file.
-  base::DeleteFile(download->GetFullPath(), false);
+  ASSERT_TRUE(base::PathExists(download->GetFullPath()));
+  ASSERT_TRUE(base::DeleteFile(download->GetFullPath(), false));
 
-  DownloadUpdatedObserver completion_observer(
-      download, base::Bind(DownloadCompleteFilter));
-  download->Resume();
-  completion_observer.WaitForEvent();
+  parameters.ClearInjectedErrors();
+  request_handler.StartServing(parameters);
 
-  ConfirmFileStatusForResume(
-      download, true, GetSafeBufferChunk() * 3, GetSafeBufferChunk() * 3,
-      base::FilePath(FILE_PATH_LITERAL("rangereset")));
-
-  static const RecordingDownloadObserver::RecordStruct expected_record[] = {
-    // Result of RST
-    {DownloadItem::INTERRUPTED, GetSafeBufferChunk()},
-    // Starting continuation
-    {DownloadItem::IN_PROGRESS, GetSafeBufferChunk()},
-    // Error because file isn't there.
-    {DownloadItem::INTERRUPTED, 0},
-    // Restart.
-    {DownloadItem::IN_PROGRESS, 0},
-    // Completion.
-    {DownloadItem::COMPLETE, GetSafeBufferChunk() * 3},
-  };
-
-  recorder.CompareToExpectedRecord(expected_record, arraysize(expected_record));
+  download->Resume();
+  WaitForCompletion(download);
+
+  ASSERT_EQ(parameters.size, download->GetReceivedBytes());
+  ASSERT_EQ(parameters.size, download->GetTotalBytes());
+  ASSERT_NO_FATAL_FAILURE(ReadAndVerifyFileContents(
+      parameters.pattern_generator_seed, parameters.size,
+      download->GetTargetFilePath()));
+
+  // The following verification is definitely a characterization test of
+  // implementation details. We are going to see three requests being made. The
+  // one in the middle is a partial request that is abandoned because the
+  // intermediate file is missing.

[Randy Smith (Not in Mondays), 2015/11/12 00:57:57]

I'm trying to come up with the failure modes we're probing for here.  I'm
inclined to say that, however the system got the whole file, if it did get the
whole file after we deleted the initial bytes out from under it, it's properly
handled the file deletion recovery case.  Does it matter to correctness what the
underlying requests are?

This argument doesn't apply in the other cases because it matters to
*performance* what the underlying requests are; we want to make sure we made
range requests if we could.  Here we can't, and we're explicitly falling back to
the non-performant path.  So I don't think we care about what the on-the-wire
bits looked like, only that we got all the bits correctly.

WDYT?

[asanka, 2015/11/13 21:40:01]

We are verifying that the download logic abandons the validators and issues a
fresh request since it has lost the data, and that it abandons the validators
without too many unnecessary requests in the middle. The characterization aspect
here is that the logic sends a partial request in the middle unnecessarily
because our logic doesn't look at the intermediate file until a response is
seen. I consider this to be a but that we should fix at some point, but one that
is perhaps not relevant to this test.

I've removed the checks since as far as correctness goes, the
ReadAndVerifyFileContents() call is sufficient.

+  TestDownloadRequestHandler::CompletedRequests requests;
+  request_handler.GetCompletedRequestInfo(&requests);
+  ASSERT_EQ(3u, requests.size());
+  EXPECT_EQ(parameters.size, requests[2].transferred_content_length);
+  std::string dummy_value;
+  EXPECT_FALSE(requests[2].request_headers.GetHeader(
+      net::HttpRequestHeaders::kRange, &dummy_value));
+  EXPECT_FALSE(requests[2].request_headers.GetHeader(
+      net::HttpRequestHeaders::kIfRange, &dummy_value));
 }
 
-IN_PROC_BROWSER_TEST_F(DownloadContentTest, ResumeWithFileInitError) {
+IN_PROC_BROWSER_TEST_F(DownloadContentTest, Resume_RecoverFromInitFileError) {
   base::CommandLine::ForCurrentProcess()->AppendSwitch(
       switches::kEnableDownloadResumption);
-  GURL url(net::URLRequestMockHTTPJob::GetMockUrl("download-test.lib"));
+  TestDownloadRequestHandler request_handler;
+  request_handler.StartServing(TestDownloadRequestHandler::Parameters());
 
   // Setup the error injector.
   scoped_refptr<TestFileErrorInjector> injector(
       TestFileErrorInjector::Create(DownloadManagerForShell(shell())));
 
   TestFileErrorInjector::FileErrorInfo err = {
-    url.spec(),
-    TestFileErrorInjector::FILE_OPERATION_INITIALIZE,
-    0,
-    DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE
-  };
+      request_handler.url().spec(),
+      TestFileErrorInjector::FILE_OPERATION_INITIALIZE, 0,
+      DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE};
   injector->AddError(err);
   injector->InjectErrors();
 
   // Start and watch for interrupt.
-  scoped_ptr<DownloadTestObserver> int_observer(
-      CreateInterruptedWaiter(shell(), 1));
-  DownloadItem* download(StartDownloadAndReturnItem(url));
-  int_observer->WaitForFinished();
+  DownloadItem* download(StartDownloadAndReturnItem(request_handler.url()));
+  WaitForInterrupt(download);
   ASSERT_EQ(DownloadItem::INTERRUPTED, download->GetState());
   EXPECT_EQ(DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE,
             download->GetLastReason());
@@ -1377,29 +1191,26 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, ResumeWithFileInitError) {
 }
 
 IN_PROC_BROWSER_TEST_F(DownloadContentTest,
-                       ResumeWithFileIntermediateRenameError) {
+                       Resume_RecoverFromIntermediateFileRenameError) {
   base::CommandLine::ForCurrentProcess()->AppendSwitch(
       switches::kEnableDownloadResumption);
-  GURL url(net::URLRequestMockHTTPJob::GetMockUrl("download-test.lib"));
+  TestDownloadRequestHandler request_handler;
+  request_handler.StartServing(TestDownloadRequestHandler::Parameters());
 
   // Setup the error injector.
   scoped_refptr<TestFileErrorInjector> injector(
       TestFileErrorInjector::Create(DownloadManagerForShell(shell())));
 
   TestFileErrorInjector::FileErrorInfo err = {
-    url.spec(),
-    TestFileErrorInjector::FILE_OPERATION_RENAME_UNIQUIFY,
-    0,
-    DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE
-  };
+      request_handler.url().spec(),
+      TestFileErrorInjector::FILE_OPERATION_RENAME_UNIQUIFY, 0,
+      DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE};
   injector->AddError(err);
   injector->InjectErrors();
 
   // Start and watch for interrupt.
-  scoped_ptr<DownloadTestObserver> int_observer(
-      CreateInterruptedWaiter(shell(), 1));
-  DownloadItem* download(StartDownloadAndReturnItem(url));
-  int_observer->WaitForFinished();
+  DownloadItem* download(StartDownloadAndReturnItem(request_handler.url()));
+  WaitForInterrupt(download);
   ASSERT_EQ(DownloadItem::INTERRUPTED, download->GetState());
   EXPECT_EQ(DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE,
             download->GetLastReason());
@@ -1428,10 +1239,12 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest,
   EXPECT_EQ(download->GetState(), DownloadItem::COMPLETE);
 }
 
-IN_PROC_BROWSER_TEST_F(DownloadContentTest, ResumeWithFileFinalRenameError) {
+IN_PROC_BROWSER_TEST_F(DownloadContentTest,
+                       Resume_RecoverFromFinalRenameError) {

[Randy Smith (Not in Mondays), 2015/11/12 00:57:57]

Why two lines?  (I suspect the answer is "git cl format", but it does look like
it would fit on a single line).

[asanka, 2015/11/13 21:40:00]

'git cl format' :) but it's also 81 characters without the line break.

   base::CommandLine::ForCurrentProcess()->AppendSwitch(
       switches::kEnableDownloadResumption);
-  GURL url(net::URLRequestMockHTTPJob::GetMockUrl("download-test.lib"));
+  TestDownloadRequestHandler request_handler;
+  request_handler.StartServing(TestDownloadRequestHandler::Parameters());
 
   // Setup the error injector.
   scoped_refptr<TestFileErrorInjector> injector(
@@ -1439,19 +1252,15 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, ResumeWithFileFinalRenameError) {
 
   DownloadManagerForShell(shell())->RemoveAllDownloads();
   TestFileErrorInjector::FileErrorInfo err = {
-    url.spec(),
-    TestFileErrorInjector::FILE_OPERATION_RENAME_ANNOTATE,
-    0,
-    DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE
-  };
+      request_handler.url().spec(),
+      TestFileErrorInjector::FILE_OPERATION_RENAME_ANNOTATE, 0,
+      DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE};
   injector->AddError(err);
   injector->InjectErrors();
 
   // Start and watch for interrupt.
-  scoped_ptr<DownloadTestObserver> int_observer(
-      CreateInterruptedWaiter(shell(), 1));
-  DownloadItem* download(StartDownloadAndReturnItem(url));
-  int_observer->WaitForFinished();
+  DownloadItem* download(StartDownloadAndReturnItem(request_handler.url()));
+  WaitForInterrupt(download);
   ASSERT_EQ(DownloadItem::INTERRUPTED, download->GetState());
   EXPECT_EQ(DOWNLOAD_INTERRUPT_REASON_FILE_NO_SPACE,
             download->GetLastReason());
@@ -1483,23 +1292,17 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, ResumeWithFileFinalRenameError) {
 IN_PROC_BROWSER_TEST_F(DownloadContentTest, CancelInterruptedDownload) {
   base::CommandLine::ForCurrentProcess()->AppendSwitch(
       switches::kEnableDownloadResumption);
-  ASSERT_TRUE(spawned_test_server()->Start());
-
-  GURL url1 = spawned_test_server()->GetURL(
-      base::StringPrintf("rangereset?size=%d&rst_boundary=%d",
-                         GetSafeBufferChunk() * 3, GetSafeBufferChunk()));
+  TestDownloadRequestHandler::Parameters parameters =
+      TestDownloadRequestHandler::Parameters::WithSingleInterruption();
+  TestDownloadRequestHandler request_handler;
+  request_handler.StartServing(parameters);
 
-  DownloadItem* download(StartDownloadAndReturnItem(url1));
-  WaitForData(download, GetSafeBufferChunk());
+  DownloadItem* download = StartDownloadAndReturnItem(request_handler.url());
+  WaitForInterrupt(download);
 
-  ReleaseRSTAndConfirmInterruptForResume(download);
-  ConfirmFileStatusForResume(
-      download, true, GetSafeBufferChunk(), GetSafeBufferChunk() * 3,
-      base::FilePath(FILE_PATH_LITERAL("rangereset.crdownload")));
-
-  base::FilePath intermediate_path(download->GetFullPath());
+  base::FilePath intermediate_path = download->GetFullPath();
   ASSERT_FALSE(intermediate_path.empty());
-  EXPECT_TRUE(base::PathExists(intermediate_path));
+  ASSERT_TRUE(base::PathExists(intermediate_path));
 
   download->Cancel(true /* user_cancel */);
   RunAllPendingInMessageLoop(BrowserThread::FILE);
@@ -1510,81 +1313,60 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, CancelInterruptedDownload) {
   EXPECT_TRUE(download->GetFullPath().empty());
 }
 
-IN_PROC_BROWSER_TEST_F(DownloadContentTest, RemoveDownload) {
+IN_PROC_BROWSER_TEST_F(DownloadContentTest, RemoveInterruptedDownload) {
   base::CommandLine::ForCurrentProcess()->AppendSwitch(
       switches::kEnableDownloadResumption);
-  ASSERT_TRUE(spawned_test_server()->Start());
+  TestDownloadRequestHandler::Parameters parameters =
+      TestDownloadRequestHandler::Parameters::WithSingleInterruption();
+  TestDownloadRequestHandler request_handler;
+  request_handler.StartServing(parameters);
 
-  // An interrupted download should remove the intermediate file when it is
-  // removed.
-  {
-    GURL url1 = spawned_test_server()->GetURL(
-        base::StringPrintf("rangereset?size=%d&rst_boundary=%d",
-                           GetSafeBufferChunk() * 3, GetSafeBufferChunk()));
-
-    DownloadItem* download(StartDownloadAndReturnItem(url1));
-    WaitForData(download, GetSafeBufferChunk());
-    ReleaseRSTAndConfirmInterruptForResume(download);
-    ConfirmFileStatusForResume(
-        download, true, GetSafeBufferChunk(), GetSafeBufferChunk() * 3,
-        base::FilePath(FILE_PATH_LITERAL("rangereset.crdownload")));
-
-    base::FilePath intermediate_path(download->GetFullPath());
-    ASSERT_FALSE(intermediate_path.empty());
-    EXPECT_TRUE(base::PathExists(intermediate_path));
-
-    download->Remove();
-    RunAllPendingInMessageLoop(BrowserThread::FILE);
-    RunAllPendingInMessageLoop();
-
-    // The intermediate file should now be gone.
-    EXPECT_FALSE(base::PathExists(intermediate_path));
-  }
+  DownloadItem* download = StartDownloadAndReturnItem(request_handler.url());
+  WaitForInterrupt(download);
 
+  base::FilePath intermediate_path = download->GetFullPath();
+  ASSERT_FALSE(intermediate_path.empty());
+  ASSERT_TRUE(base::PathExists(intermediate_path));
+
+  download->Remove();
+  RunAllPendingInMessageLoop(BrowserThread::FILE);
+  RunAllPendingInMessageLoop();
+
+  // The intermediate file should now be gone.
+  EXPECT_FALSE(base::PathExists(intermediate_path));
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadContentTest, RemoveCompletedDownload) {
   // A completed download shouldn't delete the downloaded file when it is
   // removed.
-  {
-    // Start the second download and wait until it's done.
-    GURL url2(net::URLRequestMockHTTPJob::GetMockUrl("download-test.lib"));
-    scoped_ptr<DownloadTestObserver> completion_observer(
-        CreateWaiter(shell(), 1));
-    DownloadItem* download(StartDownloadAndReturnItem(url2));
-    completion_observer->WaitForFinished();
-
-    // The target path should exist.
-    base::FilePath target_path(download->GetTargetFilePath());
-    EXPECT_TRUE(base::PathExists(target_path));
-    download->Remove();
-    RunAllPendingInMessageLoop(BrowserThread::FILE);
-    RunAllPendingInMessageLoop();
-
-    // The file should still exist.
-    EXPECT_TRUE(base::PathExists(target_path));
-  }
+  TestDownloadRequestHandler request_handler;
+  request_handler.StartServing(TestDownloadRequestHandler::Parameters());
+  scoped_ptr<DownloadTestObserver> completion_observer(
+      CreateWaiter(shell(), 1));
+  DownloadItem* download(StartDownloadAndReturnItem(request_handler.url()));
+  completion_observer->WaitForFinished();
+
+  // The target path should exist.
+  base::FilePath target_path(download->GetTargetFilePath());
+  EXPECT_TRUE(base::PathExists(target_path));
+  download->Remove();
+  RunAllPendingInMessageLoop(BrowserThread::FILE);
+  RunAllPendingInMessageLoop();
+
+  // The file should still exist.
+  EXPECT_TRUE(base::PathExists(target_path));
 }
 
 IN_PROC_BROWSER_TEST_F(DownloadContentTest, RemoveResumingDownload) {
-  SetupEnsureNoPendingDownloads();
   base::CommandLine::ForCurrentProcess()->AppendSwitch(
       switches::kEnableDownloadResumption);
-  ASSERT_TRUE(spawned_test_server()->Start());
-
-  GURL url = spawned_test_server()->GetURL(
-      base::StringPrintf("rangereset?size=%d&rst_boundary=%d",
-                         GetSafeBufferChunk() * 3, GetSafeBufferChunk()));
-
-  MockDownloadManagerObserver dm_observer(DownloadManagerForShell(shell()));
-  EXPECT_CALL(dm_observer, OnDownloadCreated(_,_)).Times(1);
-
-  DownloadItem* download(StartDownloadAndReturnItem(url));
-  WaitForData(download, GetSafeBufferChunk());
-  ::testing::Mock::VerifyAndClearExpectations(&dm_observer);
+  TestDownloadRequestHandler::Parameters parameters =
+      TestDownloadRequestHandler::Parameters::WithSingleInterruption();
+  TestDownloadRequestHandler request_handler;
+  request_handler.StartServing(parameters);
 
-  // Tell the server to send the RST and confirm the interrupt happens.
-  ReleaseRSTAndConfirmInterruptForResume(download);
-  ConfirmFileStatusForResume(
-      download, true, GetSafeBufferChunk(), GetSafeBufferChunk() * 3,
-      base::FilePath(FILE_PATH_LITERAL("rangereset.crdownload")));
+  DownloadItem* download = StartDownloadAndReturnItem(request_handler.url());
+  WaitForInterrupt(download);
 
   base::FilePath intermediate_path(download->GetFullPath());
   ASSERT_FALSE(intermediate_path.empty());
@@ -1592,8 +1374,10 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, RemoveResumingDownload) {
 
   // Resume and remove download. We expect only a single OnDownloadCreated()
   // call, and that's for the second download created below.
+  MockDownloadManagerObserver dm_observer(DownloadManagerForShell(shell()));
   EXPECT_CALL(dm_observer, OnDownloadCreated(_,_)).Times(1);
   download->Resume();
+  EXPECT_EQ(DownloadItem::INTERRUPTED, download->GetState());

[Randy Smith (Not in Mondays), 2015/11/12 00:57:57]

Is this characterization?  Isn't it just because Resume() results in a post to
the IO thread and the download state doesn't get interrupted until the pingback?

(Actually, this seems like an API failure--the download state shouldn't be
interrupted after a resume.  I can imagine the implementation considerations
that drive this, but I'm inclined to think we should fix them?)

[asanka, 2015/11/13 21:40:01]

Correct on both counts. I consider this to be an API failure that we don't
expose RESUMING_INTERNAL as IN_PROGRESS, but it's something I'd like to address
in a separate CL. For this one, I reworked the logic so that we are waiting for
the network request to be initiated, and then removing the download and then
continuing the network request.

   download->Remove();
 
   // The intermediate file should now be gone.
@@ -1601,87 +1385,86 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, RemoveResumingDownload) {
   RunAllPendingInMessageLoop();
   EXPECT_FALSE(base::PathExists(intermediate_path));
 
-  // Start the second download and wait until it's done. The test server is
-  // single threaded. The response to this download request should follow the
-  // response to the previous resumption request.
-  GURL url2(
-      spawned_test_server()->GetURL("rangereset?size=100&rst_limit=0&token=x"));
-  NavigateToURLAndWaitForDownload(shell(), url2, DownloadItem::COMPLETE);
+  parameters.ClearInjectedErrors();
+  request_handler.StartServing(parameters);
 
+  // Start the second download and wait until it's done. This exercises the
+  // entire downloads stack and effectively flushes all of our worker threads.
+  // We are testing whether the URL request created in the previous
+  // DownloadItem::Resume() call reulted in a new download or not.
+  NavigateToURLAndWaitForDownload(shell(), request_handler.url(),
+                                  DownloadItem::COMPLETE);
   EXPECT_TRUE(EnsureNoPendingDownloads());
 }
 
 IN_PROC_BROWSER_TEST_F(DownloadContentTest, CancelResumingDownload) {
-  SetupEnsureNoPendingDownloads();
   base::CommandLine::ForCurrentProcess()->AppendSwitch(
       switches::kEnableDownloadResumption);
-  ASSERT_TRUE(spawned_test_server()->Start());
-
-  GURL url = spawned_test_server()->GetURL(
-      base::StringPrintf("rangereset?size=%d&rst_boundary=%d",
-                         GetSafeBufferChunk() * 3, GetSafeBufferChunk()));
-
-  MockDownloadManagerObserver dm_observer(DownloadManagerForShell(shell()));
-  EXPECT_CALL(dm_observer, OnDownloadCreated(_,_)).Times(1);
-
-  DownloadItem* download(StartDownloadAndReturnItem(url));
-  WaitForData(download, GetSafeBufferChunk());
-  ::testing::Mock::VerifyAndClearExpectations(&dm_observer);
+  TestDownloadRequestHandler::Parameters parameters =
+      TestDownloadRequestHandler::Parameters::WithSingleInterruption();
+  TestDownloadRequestHandler request_handler;
+  request_handler.StartServing(parameters);
 
-  // Tell the server to send the RST and confirm the interrupt happens.
-  ReleaseRSTAndConfirmInterruptForResume(download);
-  ConfirmFileStatusForResume(
-      download, true, GetSafeBufferChunk(), GetSafeBufferChunk() * 3,
-      base::FilePath(FILE_PATH_LITERAL("rangereset.crdownload")));
+  DownloadItem* download = StartDownloadAndReturnItem(request_handler.url());
+  WaitForInterrupt(download);
 
   base::FilePath intermediate_path(download->GetFullPath());
   ASSERT_FALSE(intermediate_path.empty());
   EXPECT_TRUE(base::PathExists(intermediate_path));
 
-  // Resume and cancel download. We expect only a single OnDownloadCreated()
+  // Resume and remove download. We expect only a single OnDownloadCreated()
   // call, and that's for the second download created below.
+  MockDownloadManagerObserver dm_observer(DownloadManagerForShell(shell()));
   EXPECT_CALL(dm_observer, OnDownloadCreated(_,_)).Times(1);

[Randy Smith (Not in Mondays), 2015/11/12 00:57:57]

Possibly we should go over the API contract details around removal; I'm having
the same vague-memory-plus-sinking-feeling looking at this that I had looking at
the Resume->Interrupted lack of transition above.

[asanka, 2015/11/13 21:40:01]

Yup. Reworked in the same manner, but the API should be fixed in another CL.

   download->Resume();
-  download->Cancel(true);
+  EXPECT_EQ(DownloadItem::INTERRUPTED, download->GetState());
+  download->Cancel(true /* user_cancel */);
 
   // The intermediate file should now be gone.
   RunAllPendingInMessageLoop(BrowserThread::FILE);
   RunAllPendingInMessageLoop();
   EXPECT_FALSE(base::PathExists(intermediate_path));
-  EXPECT_TRUE(download->GetFullPath().empty());
 
-  // Start the second download and wait until it's done. The test server is
-  // single threaded. The response to this download request should follow the
-  // response to the previous resumption request.
-  GURL url2(
-      spawned_test_server()->GetURL("rangereset?size=100&rst_limit=0&token=x"));
-  NavigateToURLAndWaitForDownload(shell(), url2, DownloadItem::COMPLETE);
+  parameters.ClearInjectedErrors();
+  request_handler.StartServing(parameters);
 
+  // Start the second download and wait until it's done. This exercises the
+  // entire downloads stack and effectively flushes all of our worker threads.
+  // We are testing whether the URL request created in the previous
+  // DownloadItem::Resume() call reulted in a new download or not.
+  NavigateToURLAndWaitForDownload(shell(), request_handler.url(),
+                                  DownloadItem::COMPLETE);
   EXPECT_TRUE(EnsureNoPendingDownloads());
 }
 
 // Check that the cookie policy is correctly updated when downloading a file
 // that redirects cross origin.
 IN_PROC_BROWSER_TEST_F(DownloadContentTest, CookiePolicy) {
-  ASSERT_TRUE(spawned_test_server()->Start());
-  net::HostPortPair host_port = spawned_test_server()->host_port_pair();
-  DCHECK_EQ(host_port.host(), std::string("127.0.0.1"));
+  net::EmbeddedTestServer origin_one;
+  net::EmbeddedTestServer origin_two;
+  ASSERT_TRUE(origin_one.Start());
+  ASSERT_TRUE(origin_two.Start());
 
   // Block third-party cookies.
   ShellNetworkDelegate::SetAcceptAllCookies(false);
 
   // |url| redirects to a different origin |download| which tries to set a
   // cookie.
-  std::string download(base::StringPrintf(
-      "http://localhost:%d/set-cookie?A=B", host_port.port()));
-  GURL url(spawned_test_server()->GetURL("server-redirect?" + download));
+  base::StringPairs cookie_header;
+  cookie_header.push_back(
+      std::make_pair(std::string("Set-Cookie"), std::string("A=B")));
+  origin_one.RegisterRequestHandler(CreateBasicResponseHandler(
+      "/foo", cookie_header, "application/octet-stream", "abcd"));
+  origin_two.RegisterRequestHandler(
+      CreateRedirectHandler("/bar", origin_one.GetURL("/foo")));
 
   // Download the file.
   SetupEnsureNoPendingDownloads();
-  scoped_ptr<DownloadUrlParameters> dl_params(
-      DownloadUrlParameters::FromWebContents(shell()->web_contents(), url));
+  scoped_ptr<DownloadUrlParameters> download_parameters(
+      DownloadUrlParameters::FromWebContents(shell()->web_contents(),
+                                             origin_two.GetURL("/bar")));
   scoped_ptr<DownloadTestObserver> observer(CreateWaiter(shell(), 1));
-  DownloadManagerForShell(shell())->DownloadUrl(dl_params.Pass());
+  DownloadManagerForShell(shell())->DownloadUrl(download_parameters.Pass());
   observer->WaitForFinished();
 
   // Get the important info from other threads and check it.
@@ -1695,7 +1478,7 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest, CookiePolicy) {
   // Check that the cookies were correctly set.
   EXPECT_EQ("A=B",
             content::GetCookies(shell()->web_contents()->GetBrowserContext(),
-                                GURL(download)));
+                                origin_one.GetURL("/")));
 }
 
 // A filename suggestion specified via a @download attribute should not be
@@ -1727,7 +1510,7 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest,
   origin_one.RegisterRequestHandler(
       CreateRedirectHandler("/ping", origin_two.GetURL("/download")));
   origin_two.RegisterRequestHandler(CreateBasicResponseHandler(
-      "/download", "application/octet-stream", "Hello"));
+      "/download", base::StringPairs(), "application/octet-stream", "Hello"));
 
   NavigateToURLAndWaitForDownload(
       shell(), referrer_url, DownloadItem::COMPLETE);
@@ -1775,7 +1558,7 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest,
   origin_two.RegisterRequestHandler(
       CreateRedirectHandler("/pong", origin_one.GetURL("/download")));
   origin_one.RegisterRequestHandler(CreateBasicResponseHandler(
-      "/download", "application/octet-stream", "Hello"));
+      "/download", base::StringPairs(), "application/octet-stream", "Hello"));
 
   NavigateToURLAndWaitForDownload(
       shell(), referrer_url, DownloadItem::COMPLETE);
@@ -1794,12 +1577,7 @@ IN_PROC_BROWSER_TEST_F(DownloadContentTest,
 // The content body is empty. Make sure this case is handled properly and we
 // don't regress on http://crbug.com/320394.
 IN_PROC_BROWSER_TEST_F(DownloadContentTest, DownloadGZipWithNoContent) {
-  net::EmbeddedTestServer test_server;
-  ASSERT_TRUE(test_server.Start());
-
-  GURL url = test_server.GetURL("/empty.bin");
-  test_server.ServeFilesFromDirectory(GetTestFilePath("download", ""));
-
+  GURL url = net::URLRequestMockHTTPJob::GetMockUrl("empty.bin");
   NavigateToURLAndWaitForDownload(shell(), url, DownloadItem::COMPLETE);
   // That's it. This should work without crashing.
 }