Stop using SpawnedTestServer in DownloadContentTest.*

content/public/test/test_download_request_handler.h

Index: content/public/test/test_download_request_handler.h
diff --git a/content/public/test/test_download_request_handler.h b/content/public/test/test_download_request_handler.h
new file mode 100644
index 0000000000000000000000000000000000000000..d1118d58b053c9fa583a37aa54e0b3e8d428ed8c
--- /dev/null
+++ b/content/public/test/test_download_request_handler.h
@@ -0,0 +1,231 @@
+// Copyright 2015 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.
+
+#ifndef CONTENT_PUBLIC_TEST_TEST_DOWNLOAD_REQUEST_HANDLER_H_
+#define CONTENT_PUBLIC_TEST_TEST_DOWNLOAD_REQUEST_HANDLER_H_
+
+#include <stdint.h>
+#include <queue>
+
+#include "base/files/file.h"
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "net/base/net_errors.h"
+#include "net/http/http_byte_range.h"
+#include "net/http/http_response_headers.h"
+#include "net/http/http_util.h"
+#include "net/url_request/url_request_job.h"
+#include "url/gurl.h"
+
+namespace content {
+
+// A request handler that can be used to mock the behavior a URLRequestJob for a

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

nit: "of a"?

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

Done.

+// download.
+//
+// Testing of download interruption scenarios typically involve simulating
+// errors that occur:
+// 1. on the client, prior to the request being sent out,
+// 2. on the network, between the client and the server,
+// 3. on the server,
+// 4. back on the client, while writing the response to disk,
+// 5. on the client, after the response has been written to disk.
+//
+// This test class is meant to help test failures in #2 and #3 above. The test

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

Wouldn't errors on the server normally show up with a bad HTTP status code
rather than a net:: error?  So isn't this more narrowly targeted at #2?

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

Currently, yes. But it's also controlling the entire server response, and hence
can accomplish #3 as well. I added an OnStartHandler to deal with pausing and
error generation during the Start() loop which effectively makes this fulfill #3
as well.

+// implementation depends on content::BrowserThread and assumes that the
+// thread identified by BrowserThread::IO is the network task runner thread.
+//
+// To use the test request handler:
+//
+//  // Define the request handler. Note that initialiation of the
+//  // TestDownloadRequestHandler object immediately registers it as well and is
+//  // a blocking operation.
+//  TestDownloadRequestHandler request_handler;
+//
+//  // Set up parameters for the partial request handler.
+//  TestDownloadRequestHandler::Parameters parameters;
+//
+//  // Inject an error at offset 100.
+//  parameters.injected_errors.push(TestDownloadRequestHandler::InjectedError(
+//      100, net::ERR_CONNECTION_RESET));
+//
+//  // Start serving.
+//  request_handler.StartServing(parameters);
+//
+// At this point, you can initiate a URLRequest for request_handler.url(). The
+// request will fail when offset 100 is reached with the error specified above.
+class TestDownloadRequestHandler {
+ public:
+  // An injected error.
+  struct InjectedError {
+    InjectedError(int64_t offset, net::Error error);
+
+    int64_t offset;
+    net::Error error;
+  };
+
+  // Parameters used by StartServing().
+  struct Parameters {
+    static Parameters WithSingleInterruption();
+
+    Parameters(const Parameters& other);

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

suggestion: I'm tempted to ask for a comment calling out copy construction, here
or at the end of the class, just because the default for chrome is to disallow
that.  

Also: Copy constructor without assignment operator seems weird; if one can
default, why can't both?  Conversely, if you're going to enable one, enabling
the other seems wise?

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

Thanks for calling this out. During an intermediate stage, the compiler decided
it couldn't generate a default copy constructor. But now it can. Removed the
copy constructor.

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

I read the style guide as recommending but not requiring explicitly declaring
default copy constructors and assignment operators with = default.  Up to you,
just wanted to make sure you were aware of that recommend-but-not-require
(https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types ) 

[asanka, 2015/11/23 22:04:59]

Thanks for pointing this out. I've added the default constructors and operators
for this class.

+    Parameters(const std::string& etag,
+               const std::string& last_modified,
+               const std::string& content_type,
+               int64_t size,
+               int pattern_generator_seed,
+               bool support_byte_ranges);
+
+    // The default constructor initializes the parameters for serving a 100 KB
+    // resource with no interruptions. The response contains an ETag and a
+    // Last-Modified header and the server supports byte range requests.
+    Parameters();
+
+    ~Parameters();
+
+    // Clears the errors in injected_errors.
+    void ClearInjectedErrors();
+
+    // Contents of the ETag header field of the response.  No Etag header is
+    // sent if this field is empty.
+    std::string etag;
+
+    // Contents of the Last-Modified header field of the response. No
+    // Last-Modified header is sent if this field is empty.
+    std::string last_modified;
+
+    // The Content-Type of the response. No Content-Type header is sent if this
+    // field is empty.
+    std::string content_type;
+
+    // The total size of the entity. If the entire entity is requested, then
+    // this would be the same as the value returned in the Content-Length
+    // header.
+    int64_t size;
+
+    // Seed for the pseudo-random sequence that defines the response body
+    // contents. The seed is with GetPatternBytes() to generate the body of the
+    // response.
+    int pattern_generator_seed;
+
+    // If true, the response contains a 'Accept-Ranges: bytes' header.
+    bool support_byte_ranges;
+
+    // Errors to be injected. Each injected error is defined by an offset and an
+    // error. Request handler will successfully fulfil requests to read up to
+    // |offset|. An attempt to read the byte at |offset| will result in the
+    // error defined by the InjectErrors object.
+    //
+    // Note that if a read spans the range containing |offset|, then the portion
+    // of the request preceding |offset| will succeed. The next read would start
+    // at |offset| and hence would result in an error.
+    //
+    // E.g.: injected_errors.push(InjectedError(100, ERR_CONNECTION_RESET));
+    //
+    //    A network read for 1024 bytes at offset 0 would result in successfully
+    //    reading 100 bytes (bytes with offset 0-99). The next read would,
+    //    therefore, start at offset 100 and would result in
+    //    ERR_CONNECTION_RESET.
+    //
+    // Note that distinctions about which read requests signal the error is
+    // often only important at the //net layer. From //content, it would appear
+    // that 100 bytes were read and then request failed with
+    // ERR_CONNECTION_RESET.
+    std::queue<InjectedError> injected_errors;
+  };
+
+  // Details about completed requests returned by GetCompletedRequestInfo().
+  struct CompletedRequest {
+    int64_t transferred_content_length = -1;
+    net::HttpRequestHeaders request_headers;
+  };
+
+  using CompletedRequests = std::vector<CompletedRequest>;
+
+  // Registers a request handler at the default URL. Call url() to determine the
+  // URL.
+  //
+  // Notes:
+  // * The URL used is a constant. Constructing multiple instances of this
+  //   for the same target URL results in unspecified behavior. (The result is
+  //   known, but don't do it).

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

Suggestion: Can you assert/dcheck if they do that?  Or have initialization
automatically use a new URL, JIC?  The undefined but consistent and allowed
behavior seems a bit attractive-nuisance like.  The check wouldn't need to be
general; I'm happy with people explicitly specifying the URL getting what they
deserve, I'd just like the default behavior to be easy to use correctly.  

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

On debug builds, using the same URL to register two interceptors will hit a
DCHECK in net::URLRequestFilter. Assuming tests are built with DCHECKs on, that
should suffice to curb abuse.

I made the default constructor use a generated URL and added a note that callers
shouldn't expect the URL to be the same across different runs of the same test.

+  //
+  // * Initialization of the handler synchronously runs a task on the
+  //   BrowserThread::IO thread using a nested message loop. Only construct an
+  //   instance of this object after browser threads have been initialized.
+  TestDownloadRequestHandler();
+
+  // Similar to the default constructor, but registers the handler at |url|.
+  TestDownloadRequestHandler(const GURL& url);
+
+  // Destroys and posts a task to the IO thread to dismantle the registered URL
+  // request interceptor. Does not wait for the task to return.
+  ~TestDownloadRequestHandler();
+
+  // Returns the URL that this instance is intercepting URLRequests for.
+  const GURL& url() const { return url_; }
+
+  // Start responding to URLRequests for url() with responses based on
+  // |parameters|.
+  //
+  // This method invocation posts a task to the IO thread to update the
+  // URLRequestInterceptor with the new parameters and returns immediately. URL
+  // interception won't be updated until the posted task executes.
+  //
+  // Calling this method does not affect URLRequests that have already started.
+  // The new parameters will only be used to respond to new URLRequests that are
+  // starting.
+  //
+  // StartServing() can be called multiple times to change the operating
+  // parameters of the current URL interceptor.
+  void StartServing(const Parameters& parameters);
+
+  // Start responding to URLRuequests for url() with a static response
+  // containing the headers in |headers|.
+  //
+  // The format of |headers| should comply with the requirements for
+  // net::HttpUtil::AssembleRawHeaders().
+  void StartServingStaticResponse(const base::StringPiece& headers);
+
+  // Get the list of requests that have already completed.
+  //
+  // This method posts a task to the IO thread to collect the list of completed
+  // requests and waits for the task to complete.
+  //
+  // Requests that are currently in progress will not be reflected in
+  // |requests|.
+  void GetCompletedRequestInfo(CompletedRequests* requests);
+
+  // Generate a pseudorandom pattern.
+  //
+  // |seed| is the seed for the pseudorandom sequence.  |offset| is the byte
+  // offset into the sequence.  |length| is a count of bytes to generate.
+  // |data| receives the generated bytes and should be able to store |length|
+  // bytes.
+  //
+  // The pattern has the following properties:
+  //
+  // * For a given |seed|, the entire sequence of bytes is fixed. Any
+  //   subsequence can be generated by specifying the |offset| and |length|.
+  //
+  // * The sequence is aperiodic (at least for the first 1M bytes).
+  //
+  // * |seed| is chaotic.
+  //
+  // These properties make the generated bytes useful for testing partial
+  // requests where the response may need to be built using a sequence of
+  // partial requests.
+  static void GetPatternBytes(int seed, int64 offset, int length, char* data);
+
+ private:
+  struct InterceptorProxy;
+
+  GURL url_;
+  scoped_ptr<InterceptorProxy> interceptor_proxy_;
+  DISALLOW_COPY_AND_ASSIGN(TestDownloadRequestHandler);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_PUBLIC_TEST_TEST_DOWNLOAD_REQUEST_HANDLER_H_