Move MimeTypeResourceHandler before ThrottlingResourceHandler

content/browser/loader/mime_sniffing_resource_handler_unittest.cc

 // Copyright 2014 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 "content/browser/loader/mime_type_resource_handler.h"
+#include "content/browser/loader/mime_sniffing_resource_handler.h"
 
 #include <stdint.h>
 
 #include <memory>
 
 #include "base/files/file_path.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "content/browser/loader/intercepting_resource_handler.h"
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
 #include "content/public/browser/resource_controller.h"
 #include "content/public/browser/resource_dispatcher_host_delegate.h"
 #include "content/public/browser/resource_request_info.h"
 #include "content/public/common/resource_response.h"
 #include "content/public/common/webplugininfo.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "content/public/test/test_utils.h"
 #include "content/test/fake_plugin_service.h"
 #include "net/url_request/url_request_context.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
 namespace content {
 
 namespace {
 
 class TestResourceHandler : public ResourceHandler {
  public:
-  TestResourceHandler() : ResourceHandler(nullptr) {}
+  TestResourceHandler(bool response_started,
+                      bool defer_response_started,
+                      bool will_read,
+                      bool read_completed,
+                      bool defer_read_completed)

[mmenke, 2016/07/25 17:39:32]

Think these names can be clearer.  Maybe:

response_started_succeeds,
defer_on_response_started,
will_read_succeeds,
read_completed_succeeds,
defer_on_read_completed

(response_started as a bool could mean whether or not the response is ever
started, whether or not the handler should start the response, etc.  defer_foo
could mean we defer fooing, as opposed to meaning we defer advancing to the next
state after fooing)

[clamy, 2016/07/26 16:24:16]

Done.

+      : ResourceHandler(nullptr),
+        buffer_(new net::IOBuffer(2048)),
+        response_started_(response_started),
+        defer_response_started_(defer_response_started),
+        will_read_(will_read),
+        read_completed_(read_completed),
+        defer_read_completed_(defer_read_completed) {}
 
   void SetController(ResourceController* controller) override {}
 
   bool OnRequestRedirected(const net::RedirectInfo& redirect_info,
                            ResourceResponse* response,
                            bool* defer) override {
     NOTREACHED();
     return false;
   }
 
   bool OnResponseStarted(ResourceResponse* response, bool* defer) override {
-    return false;
+    if (defer_response_started_)
+      *defer = true;
+    return response_started_;
   }
 
-  bool OnWillStart(const GURL& url, bool* defer) override {
-    return false;
-  }
+  bool OnWillStart(const GURL& url, bool* defer) override { return false; }
 
   bool OnWillRead(scoped_refptr<net::IOBuffer>* buf,
                   int* buf_size,
                   int min_size) override {
-    NOTREACHED();
-    return false;
+    *buf = buffer_;
+    *buf_size = 2048;
+    return will_read_;
   }
 
   bool OnReadCompleted(int bytes_read, bool* defer) override {
-    NOTREACHED();
-    return false;
+    DCHECK_LT(bytes_read, 2048);
+    if (defer_read_completed_)
+      *defer = true;
+    return read_completed_;
   }
 
   void OnResponseCompleted(const net::URLRequestStatus& status,
                            const std::string& security_info,
-                           bool* defer) override {
-  }
+                           bool* defer) override {}
 
-  void OnDataDownloaded(int bytes_downloaded) override {
-    NOTREACHED();
+  void OnDataDownloaded(int bytes_downloaded) override { NOTREACHED(); }
+
+  scoped_refptr<net::IOBuffer> buffer() { return buffer_; }
+
+ private:
+  scoped_refptr<net::IOBuffer> buffer_;
+  bool response_started_;
+  bool defer_response_started_;
+  bool will_read_;
+  bool read_completed_;
+  bool defer_read_completed_;
+  DISALLOW_COPY_AND_ASSIGN(TestResourceHandler);

[mmenke, 2016/07/25 17:39:32]

nit:  Blank line before disallow_copy_and_assign

[clamy, 2016/07/26 16:24:15]

Done.

+};
+
+class TestResourceDispatcherHostDelegate
+    : public ResourceDispatcherHostDelegate {
+ public:
+  TestResourceDispatcherHostDelegate(bool must_download)

[mmenke, 2016/07/25 17:39:31]

explicit

[clamy, 2016/07/26 16:24:15]

Done.

+      : must_download_(must_download) {}
+
+  bool ShouldForceDownloadResource(const GURL& url,
+                                   const std::string& mime_type) override {
+    return must_download_;
   }
 
  private:
-  DISALLOW_COPY_AND_ASSIGN(TestResourceHandler);
+  const bool must_download_;
 };
 
 class TestResourceDispatcherHost : public ResourceDispatcherHostImpl {
  public:
-  explicit TestResourceDispatcherHost(bool stream_has_handler)
+  TestResourceDispatcherHost(bool stream_has_handler)

[mmenke, 2016/07/25 17:39:31]

explicit

[clamy, 2016/07/26 16:24:15]

Done.

       : stream_has_handler_(stream_has_handler),
         intercepted_as_stream_(false),
-        intercepted_as_stream_count_(0) {}
+        intercepted_as_stream_count_(0),
+        last_resource_handler_(nullptr) {}
 
   bool intercepted_as_stream() const { return intercepted_as_stream_; }
 
   std::unique_ptr<ResourceHandler> CreateResourceHandlerForDownload(
       net::URLRequest* request,
       bool is_content_initiated,
       bool must_download) override {
-    return std::unique_ptr<ResourceHandler>(new TestResourceHandler);
+    return CreateNewResourceHandler();
   }
 
   std::unique_ptr<ResourceHandler> MaybeInterceptAsStream(
       const base::FilePath& plugin_path,
       net::URLRequest* request,
       ResourceResponse* response,
       std::string* payload) override {
     intercepted_as_stream_count_++;
-    if (stream_has_handler_) {
+    if (stream_has_handler_)
       intercepted_as_stream_ = true;
-      return std::unique_ptr<ResourceHandler>(new TestResourceHandler);
-    } else {
-      return std::unique_ptr<ResourceHandler>();
-    }
+    return CreateNewResourceHandler();
   }
 
   int intercepted_as_stream_count() const {
     return intercepted_as_stream_count_;
   }
 
+  TestResourceHandler* last_resource_handler() const {
+    return last_resource_handler_;
+  }
+
  private:
+  std::unique_ptr<ResourceHandler> CreateNewResourceHandler() {
+    std::unique_ptr<ResourceHandler> new_resource_handler;
+    new_resource_handler.reset(
+        new TestResourceHandler(false, false, true, true, false));

[mmenke, 2016/07/25 17:39:32]

Should merge the above two lines.

[clamy, 2016/07/26 16:24:15]

Done.

+    last_resource_handler_ =
+        static_cast<TestResourceHandler*>(new_resource_handler.get());

[mmenke, 2016/07/25 17:39:32]

Rather than a static_cast, I think it's better to make new_resource_handler a
std::unique_ptr<TestResourceHandler>, and return it with
std::move(new_resource_handler); (May need to return it with
std::move<ResourceHandler>(new_resource_handler), not sure)

[clamy, 2016/07/26 16:24:15]

Done.

+    return new_resource_handler;
+  }
+
   // Whether the URL request should be intercepted as a stream.
   bool stream_has_handler_;
 
   // Whether the URL request has been intercepted as a stream.
   bool intercepted_as_stream_;
 
   // Count of number of times MaybeInterceptAsStream function get called in a
   // test.
   int intercepted_as_stream_count_;
-};
 
-class TestResourceDispatcherHostDelegate
-    : public ResourceDispatcherHostDelegate {
- public:
-  TestResourceDispatcherHostDelegate(bool must_download)
-      : must_download_(must_download) {
-  }
-
-  bool ShouldForceDownloadResource(const GURL& url,
-                                   const std::string& mime_type) override {
-    return must_download_;
-  }
-
- private:
-  const bool must_download_;
-};
-
-class TestResourceController : public ResourceController {
- public:
-  void Cancel() override {}
-
-  void CancelAndIgnore() override {
-    NOTREACHED();
-  }
-
-  void CancelWithError(int error_code) override {
-    NOTREACHED();
-  }
-
-  void Resume() override {
-    NOTREACHED();
-  }
+  // The last TestResourceHandler created by this TestResourceDispatcherHost.
+  TestResourceHandler* last_resource_handler_;

[mmenke, 2016/07/25 17:39:31]

Maybe rename this to new_resource_handler_, and make clearer that it's the
alternative (swapped in?) ResourceHandler?  The RDH normally creates the
original ResourceHandler, too, so seems a bit unclear, as-is.

[clamy, 2016/07/26 16:24:16]

Done.

 };
 
 class TestFakePluginService : public FakePluginService {
  public:
   // If |is_plugin_stale| is true, GetPluginInfo will indicate the plugins are
   // stale until GetPlugins is called.
   TestFakePluginService(bool plugin_available, bool is_plugin_stale)
       : plugin_available_(plugin_available),
         is_plugin_stale_(is_plugin_stale) {}
 
@@ skipping 23 matching lines @@
         FROM_HERE, base::Bind(callback, plugins));
   }
 
  private:
   const bool plugin_available_;
   bool is_plugin_stale_;
 
   DISALLOW_COPY_AND_ASSIGN(TestFakePluginService);
 };
 
-class MimeTypeResourceHandlerTest : public testing::Test {
+class TestResourceController : public ResourceController {
  public:
-  MimeTypeResourceHandlerTest()
+  TestResourceController() : cancel_call_count_(0), resume_call_count_(0) {}
+
+  void Cancel() override { cancel_call_count_++; }
+
+  void CancelAndIgnore() override { NOTREACHED(); }
+
+  void CancelWithError(int error_code) override { NOTREACHED(); }
+
+  void Resume() override { resume_call_count_++; }
+
+  int cancel_call_count() const { return cancel_call_count_; }
+  int resume_call_count() const { return resume_call_count_; }
+
+ private:
+  int cancel_call_count_;
+  int resume_call_count_;
+};
+
+}  // namespace
+
+class MimeSniffingResourceHandlerTest : public testing::Test {
+ public:
+  MimeSniffingResourceHandlerTest()
       : stream_has_handler_(false),
         plugin_available_(false),
         plugin_stale_(false) {}
 
+  std::string TestAcceptHeaderSetting(ResourceType request_resource_type);
+  std::string TestAcceptHeaderSettingWithURLRequest(
+      ResourceType request_resource_type,
+      net::URLRequest* request);

[mmenke, 2016/07/25 17:39:31]

These need comments (What they do, what the return value is)

[clamy, 2016/07/26 16:24:16]

Done.

+
   void set_stream_has_handler(bool stream_has_handler) {
     stream_has_handler_ = stream_has_handler;
   }
 
   void set_plugin_available(bool plugin_available) {
     plugin_available_ = plugin_available;
   }
 
   void set_plugin_stale(bool plugin_stale) { plugin_stale_ = plugin_stale; }
 
   bool TestStreamIsIntercepted(bool allow_download,
                                bool must_download,
                                ResourceType request_resource_type);

[mmenke, 2016/07/25 17:39:31]

Blank line below method.

[clamy, 2016/07/26 16:24:16]

Done.

+  // Tests the operation of the MimeSniffingHandler when it needs to buffer
+  // data.
+  void TestHandlerSniffing(bool response_started,
+                           bool defer_response_started,
+                           bool will_read,
+                           bool read_completed,
+                           bool defer_read_completed);
 
-  std::string TestAcceptHeaderSetting(ResourceType request_resource_type);
-  std::string TestAcceptHeaderSettingWithURLRequest(
-      ResourceType request_resource_type,
-      net::URLRequest* request);
+  // Tests the operation of the MimeSniffingHandler when it doesn't buffer
+  // data.
+  void TestHandlerNoSniffing(bool response_started,
+                             bool defer_response_started,
+                             bool will_read,
+                             bool read_completed,
+                             bool defer_read_completed);
 
  private:
   // Whether the URL request should be intercepted as a stream.
   bool stream_has_handler_;
   bool plugin_available_;
   bool plugin_stale_;
 
   TestBrowserThreadBundle thread_bundle_;
 };
 
-bool MimeTypeResourceHandlerTest::TestStreamIsIntercepted(
+std::string MimeSniffingResourceHandlerTest::TestAcceptHeaderSetting(
+    ResourceType request_resource_type) {
+  net::URLRequestContext context;
+  std::unique_ptr<net::URLRequest> request(context.CreateRequest(
+      GURL("http://www.google.com"), net::DEFAULT_PRIORITY, nullptr));
+  return TestAcceptHeaderSettingWithURLRequest(request_resource_type,
+                                               request.get());
+}
+
+std::string
+MimeSniffingResourceHandlerTest::TestAcceptHeaderSettingWithURLRequest(
+    ResourceType request_resource_type,
+    net::URLRequest* request) {
+  bool is_main_frame = request_resource_type == RESOURCE_TYPE_MAIN_FRAME;
+  ResourceRequestInfo::AllocateForTesting(request, request_resource_type,
+                                          nullptr,        // context
+                                          0,              // render_process_id
+                                          0,              // render_view_id
+                                          0,              // render_frame_id
+                                          is_main_frame,  // is_main_frame
+                                          false,   // parent_is_main_frame
+                                          false,   // allow_download
+                                          true,    // is_async
+                                          false);  // is_using_lofi
+
+  std::unique_ptr<ResourceHandler> mime_sniffing_handler(
+      new MimeSniffingResourceHandler(
+          std::unique_ptr<ResourceHandler>(
+              new TestResourceHandler(false, false, false, false, false)),
+          nullptr, nullptr, nullptr, request));
+
+  bool defer = false;
+  mime_sniffing_handler->OnWillStart(request->url(), &defer);
+  content::RunAllPendingInMessageLoop();
+
+  std::string accept_header;
+  request->extra_request_headers().GetHeader("Accept", &accept_header);
+  return accept_header;
+}
+
+bool MimeSniffingResourceHandlerTest::TestStreamIsIntercepted(
     bool allow_download,
     bool must_download,
     ResourceType request_resource_type) {
   net::URLRequestContext context;
   std::unique_ptr<net::URLRequest> request(context.CreateRequest(
       GURL("http://www.google.com"), net::DEFAULT_PRIORITY, nullptr));
   bool is_main_frame = request_resource_type == RESOURCE_TYPE_MAIN_FRAME;
-  ResourceRequestInfo::AllocateForTesting(
-      request.get(),
-      request_resource_type,
-      nullptr,          // context
-      0,                // render_process_id
-      0,                // render_view_id
-      0,                // render_frame_id
-      is_main_frame,    // is_main_frame
-      false,            // parent_is_main_frame
-      allow_download,   // allow_download
-      true,             // is_async
-      false);           // is_using_lofi
+  ResourceRequestInfo::AllocateForTesting(request.get(), request_resource_type,
+                                          nullptr,        // context
+                                          0,              // render_process_id
+                                          0,              // render_view_id
+                                          0,              // render_frame_id
+                                          is_main_frame,  // is_main_frame
+                                          false,  // parent_is_main_frame
+                                          allow_download,  // allow_download
+                                          true,            // is_async
+                                          false);          // is_using_lofi
 
   TestResourceDispatcherHost host(stream_has_handler_);
   TestResourceDispatcherHostDelegate host_delegate(must_download);
   host.SetDelegate(&host_delegate);
 
   TestFakePluginService plugin_service(plugin_available_, plugin_stale_);
-  std::unique_ptr<ResourceHandler> mime_sniffing_handler(
-      new MimeTypeResourceHandler(
-          std::unique_ptr<ResourceHandler>(new TestResourceHandler()), &host,
-          &plugin_service, request.get()));
+  std::unique_ptr<InterceptingResourceHandler> intercepting_handler(
+      new InterceptingResourceHandler(std::unique_ptr<ResourceHandler>(),
+                                      nullptr));
+  std::unique_ptr<ResourceHandler> mime_handler(new MimeSniffingResourceHandler(
+      std::unique_ptr<ResourceHandler>(
+          new TestResourceHandler(false, false, false, false, false)),
+      &host, &plugin_service, intercepting_handler.get(), request.get()));
+
   TestResourceController resource_controller;
-  mime_sniffing_handler->SetController(&resource_controller);
+  mime_handler->SetController(&resource_controller);
 
   scoped_refptr<ResourceResponse> response(new ResourceResponse);
   // The MIME type isn't important but it shouldn't be empty.
   response->head.mime_type = "application/pdf";
 
   bool defer = false;
-  mime_sniffing_handler->OnResponseStarted(response.get(), &defer);
+  mime_handler->OnResponseStarted(response.get(), &defer);
 
   content::RunAllPendingInMessageLoop();
   EXPECT_LT(host.intercepted_as_stream_count(), 2);
+  if (allow_download)
+    EXPECT_TRUE(intercepting_handler->new_handler_for_testing());
   return host.intercepted_as_stream();
 }
 
-std::string MimeTypeResourceHandlerTest::TestAcceptHeaderSetting(
-    ResourceType request_resource_type) {
+void MimeSniffingResourceHandlerTest::TestHandlerSniffing(
+    bool response_started,
+    bool defer_response_started,
+    bool will_read,
+    bool read_completed,
+    bool defer_read_completed) {
   net::URLRequestContext context;
   std::unique_ptr<net::URLRequest> request(context.CreateRequest(
       GURL("http://www.google.com"), net::DEFAULT_PRIORITY, nullptr));
-  return TestAcceptHeaderSettingWithURLRequest(
-      request_resource_type, request.get());
-}
-
-std::string MimeTypeResourceHandlerTest::TestAcceptHeaderSettingWithURLRequest(
-    ResourceType request_resource_type,
-    net::URLRequest* request) {
-  bool is_main_frame = request_resource_type == RESOURCE_TYPE_MAIN_FRAME;
-  ResourceRequestInfo::AllocateForTesting(
-      request,
-      request_resource_type,
-      nullptr,          // context
-      0,                // render_process_id
-      0,                // render_view_id
-      0,                // render_frame_id
-      is_main_frame,    // is_main_frame
-      false,            // parent_is_main_frame
-      false,            // allow_download
-      true,             // is_async
-      false);           // is_using_lofi
-
-  TestResourceDispatcherHost host(stream_has_handler_);
+  ResourceRequestInfo::AllocateForTesting(request.get(),
+                                          RESOURCE_TYPE_MAIN_FRAME,
+                                          nullptr,  // context
+                                          0,        // render_process_id
+                                          0,        // render_view_id
+                                          0,        // render_frame_id
+                                          true,     // is_main_frame
+                                          false,    // parent_is_main_frame
+                                          false,    // allow_download
+                                          true,     // is_async
+                                          false);   // is_using_lofi
+
+  TestResourceDispatcherHost host(false);
   TestResourceDispatcherHostDelegate host_delegate(false);
   host.SetDelegate(&host_delegate);
 
-  std::unique_ptr<ResourceHandler> mime_sniffing_handler(
-      new MimeTypeResourceHandler(
-          std::unique_ptr<ResourceHandler>(new TestResourceHandler()), &host,
-          nullptr, request));
-
+  TestFakePluginService plugin_service(plugin_available_, plugin_stale_);
+  std::unique_ptr<ResourceHandler> intercepting_handler(
+      new InterceptingResourceHandler(std::unique_ptr<ResourceHandler>(),
+                                      nullptr));
+  std::unique_ptr<ResourceHandler> handler(new MimeSniffingResourceHandler(
+      std::unique_ptr<ResourceHandler>(new TestResourceHandler(
+          response_started, defer_response_started, will_read, read_completed,
+          defer_read_completed)),
+      &host, &plugin_service,
+      static_cast<InterceptingResourceHandler*>(intercepting_handler.get()),
+      request.get()));
+  MimeSniffingResourceHandler* mime_sniffing_handler =
+      static_cast<MimeSniffingResourceHandler*>(handler.get());
+
+  TestResourceController resource_controller;
+  mime_sniffing_handler->SetController(&resource_controller);
+
+  // The response should be sniffed.
+  scoped_refptr<ResourceResponse> response(new ResourceResponse);
+  response->head.mime_type.assign("text/plain");
+
+  CHECK_EQ(MimeSniffingResourceHandler::STATE_STARTING,
+           mime_sniffing_handler->state_);
+
+  // Simulate the response starting. We should start buffering, so the return
+  // value should always be true.
   bool defer = false;
-  mime_sniffing_handler->OnWillStart(request->url(), &defer);
+  CHECK_EQ(true,
+           mime_sniffing_handler->OnResponseStarted(response.get(), &defer));
+  CHECK_EQ(0, resource_controller.cancel_call_count());
+  CHECK_EQ(0, resource_controller.resume_call_count());
+  CHECK_EQ(false, defer);
+  CHECK_EQ(MimeSniffingResourceHandler::STATE_BUFFERING,
+           mime_sniffing_handler->state_);
+
+  // Read some data to sniff the mime type. This will ask the next
+  // ResourceHandler for a buffer.
+  scoped_refptr<net::IOBuffer> read_buffer;
+  int buf_size = 0;
+  CHECK_EQ(will_read,
+           mime_sniffing_handler->OnWillRead(&read_buffer, &buf_size, -1));
+  CHECK_EQ(0, resource_controller.cancel_call_count());
+  CHECK_EQ(MimeSniffingResourceHandler::STATE_BUFFERING,
+           mime_sniffing_handler->state_);
+
+  if (!will_read) {
+    content::RunAllPendingInMessageLoop();
+    return;
+  }
+
+  // Simulate an HTML page. The mime sniffer will identify the MimeType and
+  // proceed with replay.
+  char data[] = "!DOCTYPE html\n<head>\n<title>Foo</title>\n</head>";
+  memcpy(read_buffer->data(), data, sizeof(data));
+
+  defer = false;
+  bool return_value =
+      mime_sniffing_handler->OnReadCompleted(sizeof(data), &defer);
+
+  // If the next handler cancels the response start, we hsould be notified
+  // immediately.
+  if (!response_started) {
+    CHECK_EQ(response_started, return_value);
+    CHECK_EQ(0, resource_controller.cancel_call_count());
+    content::RunAllPendingInMessageLoop();
+    return;
+  }
+
+  // The replay can be deferred both at response started and read replay
+  // stages.
+  CHECK_EQ(defer, defer_response_started || defer_read_completed);
+  if (defer_response_started) {
+    CHECK_EQ(true, defer);
+    CHECK_EQ(true, return_value);
+    CHECK_EQ(MimeSniffingResourceHandler::STATE_REPLAYING_RESPONSE_RECEIVED,
+             mime_sniffing_handler->state_);
+    mime_sniffing_handler->Resume();
+  }
+
+  // The body that was sniffed should be transmitted to the next handler. This
+  // may cancel the request.
+  if (!read_completed) {
+    if (defer_response_started) {
+      CHECK_EQ(1, resource_controller.cancel_call_count());
+    } else {
+      CHECK_EQ(0, resource_controller.cancel_call_count());
+      CHECK_EQ(false, return_value);
+    }
+    content::RunAllPendingInMessageLoop();
+    return;
+  }
+
+  CHECK_EQ(MimeSniffingResourceHandler::STATE_STREAMING,
+           mime_sniffing_handler->state_);
+
+  // The request may be deferred by the next handler once the read is done.
+  if (defer_read_completed) {
+    CHECK_EQ(true, defer);
+    mime_sniffing_handler->Resume();
+  }
+
+  CHECK_EQ(MimeSniffingResourceHandler::STATE_STREAMING,
+           mime_sniffing_handler->state_);
+  CHECK_EQ(0, resource_controller.cancel_call_count());
+
+  // Even if the next handler defers the request twice, the
+  // MimeSniffingResourceHandler should only call Resume on its controller
+  // once.
+  if (defer_response_started || defer_read_completed)
+    CHECK_EQ(1, resource_controller.resume_call_count());
+  else
+    CHECK_EQ(0, resource_controller.resume_call_count());
+
   content::RunAllPendingInMessageLoop();
-
-  std::string accept_header;
-  request->extra_request_headers().GetHeader("Accept", &accept_header);
-  return accept_header;
 }
 
+void MimeSniffingResourceHandlerTest::TestHandlerNoSniffing(
+    bool response_started,
+    bool defer_response_started,
+    bool will_read,
+    bool read_completed,
+    bool defer_read_completed) {
+  net::URLRequestContext context;
+  std::unique_ptr<net::URLRequest> request(context.CreateRequest(
+      GURL("http://www.google.com"), net::DEFAULT_PRIORITY, nullptr));
+  ResourceRequestInfo::AllocateForTesting(request.get(),
+                                          RESOURCE_TYPE_MAIN_FRAME,
+                                          nullptr,  // context
+                                          0,        // render_process_id
+                                          0,        // render_view_id
+                                          0,        // render_frame_id
+                                          true,     // is_main_frame
+                                          false,    // parent_is_main_frame
+                                          false,    // allow_download
+                                          true,     // is_async
+                                          false);   // is_using_lofi
+
+  TestResourceDispatcherHost host(false);
+  TestResourceDispatcherHostDelegate host_delegate(false);
+  host.SetDelegate(&host_delegate);
+
+  TestFakePluginService plugin_service(plugin_available_, plugin_stale_);
+  std::unique_ptr<ResourceHandler> intercepting_handler(

[mmenke, 2016/07/25 17:39:32]

Can make this an InterceptingResourceHandler and get rid of the static_cast.

[clamy, 2016/07/26 16:24:15]

Done.

+      new InterceptingResourceHandler(std::unique_ptr<ResourceHandler>(),
+                                      nullptr));
+
+  std::unique_ptr<ResourceHandler> handler(new MimeSniffingResourceHandler(

[mmenke, 2016/07/25 17:39:32]

Again, can make this a MimeSniffingResourceHandler and get rid of the static
cast.  In general, I think it's best to avoid static_casts unless we really need
them.

[clamy, 2016/07/26 16:24:15]

Yes. I'm not sure why this was a std::unique_ptr<ResourceHandler> (possibly a
leftover from some version of the test where I needed it as a
std::unique_ptr<ResourceHandler> to pass it to something).

+      std::unique_ptr<ResourceHandler>(new TestResourceHandler(
+          response_started, defer_response_started, will_read, read_completed,
+          defer_read_completed)),
+      &host, &plugin_service,
+      static_cast<InterceptingResourceHandler*>(intercepting_handler.get()),
+      request.get()));
+  MimeSniffingResourceHandler* mime_sniffing_handler =
+      static_cast<MimeSniffingResourceHandler*>(handler.get());
+
+  TestResourceController resource_controller;
+  mime_sniffing_handler->SetController(&resource_controller);
+  int expected_resume_calls = 0;
+
+  // The response should not be sniffed.
+  scoped_refptr<ResourceResponse> response(new ResourceResponse);
+  response->head.mime_type.assign("text/html");

[mmenke, 2016/07/25 17:39:32]

I think this behavior is worth noting in the comment for this method.

[clamy, 2016/07/26 16:24:15]

Done.

+
+  CHECK_EQ(MimeSniffingResourceHandler::STATE_STARTING,
+           mime_sniffing_handler->state_);

[mmenke, 2016/07/25 17:39:32]

Do we really get anything out of testing the internal state?  In general, it's
best just to test the public APIs.  Makes tests less likely to break across
refactorings.

[clamy, 2016/07/26 16:24:15]

Done.

+
+  // Simulate the response starting. There should be no need for buffering, so
+  // the return value should be that of the next handler.
+  bool defer = false;
+  CHECK_EQ(response_started,
+           mime_sniffing_handler->OnResponseStarted(response.get(), &defer));
+  CHECK_EQ(0, resource_controller.cancel_call_count());

[mmenke, 2016/07/25 17:39:32]

EXPECT_EQ (Goes for most of this function.  Some could be ASSERTS, but EXPECT
seems fine for most of them)

[clamy, 2016/07/26 16:24:16]

Done. I had put CHECKs since I remembered there were some issues with EXPECTs in
functions called by tests. Since these functions are actually part of the
testing class that seems to work.

+
+  if (!response_started) {

[mmenke, 2016/07/25 17:39:32]

EXPECT_FALSE(defer)?

[clamy, 2016/07/26 16:24:16]

Done.

+    content::RunAllPendingInMessageLoop();

[mmenke, 2016/07/25 17:39:31]

Think these calls need comments as to why they're needed.

[clamy, 2016/07/26 16:24:15]

Done.

+    return;
+  }
+
+  CHECK_EQ(defer_response_started, defer);
+  if (defer) {
+    CHECK_EQ(MimeSniffingResourceHandler::STATE_REPLAYING_RESPONSE_RECEIVED,
+             mime_sniffing_handler->state_);
+    expected_resume_calls++;
+    mime_sniffing_handler->Resume();
+  }
+
+  CHECK_EQ(expected_resume_calls, resource_controller.resume_call_count());
+  CHECK_EQ(MimeSniffingResourceHandler::STATE_STREAMING,
+           mime_sniffing_handler->state_);
+
+  // The MimeSniffingResourceHandler should be acting as a pass-through
+  // ResourceHandler.
+  scoped_refptr<net::IOBuffer> read_buffer;
+  int buf_size = 0;
+  CHECK_EQ(will_read,
+           mime_sniffing_handler->OnWillRead(&read_buffer, &buf_size, -1));
+  CHECK_EQ(0, resource_controller.cancel_call_count());
+  CHECK_EQ(MimeSniffingResourceHandler::STATE_STREAMING,
+           mime_sniffing_handler->state_);
+
+  if (!will_read) {

[mmenke, 2016/07/25 17:39:32]

EXPECT_FALSE(defer)?

[clamy, 2016/07/26 16:24:15]

We don't use defer in OnWillRead, so I added it to if (!read_completed) which
uses it.

+    content::RunAllPendingInMessageLoop();
+    return;
+  }
+
+  defer = false;
+  CHECK_EQ(read_completed,
+           mime_sniffing_handler->OnReadCompleted(2000, &defer));
+  CHECK_EQ(0, resource_controller.cancel_call_count());
+  CHECK_EQ(MimeSniffingResourceHandler::STATE_STREAMING,
+           mime_sniffing_handler->state_);
+
+  if (!read_completed) {
+    content::RunAllPendingInMessageLoop();
+    return;
+  }
+
+  CHECK_EQ(defer_read_completed, defer);
+  if (defer) {
+    expected_resume_calls++;
+    mime_sniffing_handler->Resume();
+  }
+  CHECK_EQ(expected_resume_calls, resource_controller.resume_call_count());
+
+  content::RunAllPendingInMessageLoop();
+}
+
 // Test that the proper Accept: header is set based on the ResourceType
-TEST_F(MimeTypeResourceHandlerTest, AcceptHeaders) {
+TEST_F(MimeSniffingResourceHandlerTest, AcceptHeaders) {
   EXPECT_EQ(
       "text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,"
-          "*/*;q=0.8",
+      "*/*;q=0.8",
       TestAcceptHeaderSetting(RESOURCE_TYPE_MAIN_FRAME));
   EXPECT_EQ(
       "text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,"
-          "*/*;q=0.8",
-    TestAcceptHeaderSetting(RESOURCE_TYPE_SUB_FRAME));
+      "*/*;q=0.8",
+      TestAcceptHeaderSetting(RESOURCE_TYPE_SUB_FRAME));
   EXPECT_EQ("text/css,*/*;q=0.1",
-      TestAcceptHeaderSetting(RESOURCE_TYPE_STYLESHEET));
-  EXPECT_EQ("*/*",
-      TestAcceptHeaderSetting(RESOURCE_TYPE_SCRIPT));
+            TestAcceptHeaderSetting(RESOURCE_TYPE_STYLESHEET));
+  EXPECT_EQ("*/*", TestAcceptHeaderSetting(RESOURCE_TYPE_SCRIPT));
   EXPECT_EQ("image/webp,image/*,*/*;q=0.8",
-      TestAcceptHeaderSetting(RESOURCE_TYPE_IMAGE));
+            TestAcceptHeaderSetting(RESOURCE_TYPE_IMAGE));
   EXPECT_EQ("*/*", TestAcceptHeaderSetting(RESOURCE_TYPE_FONT_RESOURCE));
   EXPECT_EQ("*/*", TestAcceptHeaderSetting(RESOURCE_TYPE_SUB_RESOURCE));
   EXPECT_EQ("*/*", TestAcceptHeaderSetting(RESOURCE_TYPE_OBJECT));
   EXPECT_EQ("*/*", TestAcceptHeaderSetting(RESOURCE_TYPE_MEDIA));
   EXPECT_EQ("*/*", TestAcceptHeaderSetting(RESOURCE_TYPE_WORKER));
   EXPECT_EQ("*/*", TestAcceptHeaderSetting(RESOURCE_TYPE_SHARED_WORKER));
   EXPECT_EQ("*/*", TestAcceptHeaderSetting(RESOURCE_TYPE_PREFETCH));
   EXPECT_EQ("*/*", TestAcceptHeaderSetting(RESOURCE_TYPE_FAVICON));
   EXPECT_EQ("*/*", TestAcceptHeaderSetting(RESOURCE_TYPE_XHR));
   EXPECT_EQ("*/*", TestAcceptHeaderSetting(RESOURCE_TYPE_PING));
   EXPECT_EQ("*/*", TestAcceptHeaderSetting(RESOURCE_TYPE_SERVICE_WORKER));
   EXPECT_EQ("*/*", TestAcceptHeaderSetting(RESOURCE_TYPE_CSP_REPORT));
   EXPECT_EQ("*/*", TestAcceptHeaderSetting(RESOURCE_TYPE_PLUGIN_RESOURCE));
 
   // Ensure that if an Accept header is already set, it is not overwritten.
   net::URLRequestContext context;
   std::unique_ptr<net::URLRequest> request(context.CreateRequest(
       GURL("http://www.google.com"), net::DEFAULT_PRIORITY, nullptr));
   request->SetExtraRequestHeaderByName("Accept", "*", true);
-  EXPECT_EQ("*",
-      TestAcceptHeaderSettingWithURLRequest(RESOURCE_TYPE_XHR, request.get()));
+  EXPECT_EQ("*", TestAcceptHeaderSettingWithURLRequest(RESOURCE_TYPE_XHR,
+                                                       request.get()));
 }
 
 // Test that stream requests are correctly intercepted under the right
 // circumstances. Test is not relevent when plugins are disabled.
 #if defined(ENABLE_PLUGINS)
-TEST_F(MimeTypeResourceHandlerTest, StreamHandling) {
+TEST_F(MimeSniffingResourceHandlerTest, StreamHandling) {
   bool allow_download;
   bool must_download;
   ResourceType resource_type;
 
   // Ensure the stream is handled by MaybeInterceptAsStream in the
   // ResourceDispatcherHost.
   set_stream_has_handler(true);
   set_plugin_available(true);
 
   // Main frame request with no download allowed. Stream shouldn't be
@@ skipping 42 matching lines @@
       TestStreamIsIntercepted(allow_download, must_download, resource_type));
 
   // Test the cases where the stream isn't handled by MaybeInterceptAsStream
   // in the ResourceDispatcherHost.
   set_stream_has_handler(false);
   allow_download = false;
   must_download = false;
   resource_type = RESOURCE_TYPE_OBJECT;
   EXPECT_FALSE(
       TestStreamIsIntercepted(allow_download, must_download, resource_type));
 

[mmenke, 2016/07/25 17:39:32]

Why did you remove a test case here?

[clamy, 2016/07/26 16:24:15]

That wasn't intentional. It's now back.

-  allow_download = true;
-  must_download = false;
-  resource_type = RESOURCE_TYPE_MAIN_FRAME;
-  EXPECT_FALSE(
-      TestStreamIsIntercepted(allow_download, must_download, resource_type));
-
   // Test the cases where the stream handled by MaybeInterceptAsStream
   // with plugin not available. This is the case when intercepting streams for
   // the streamsPrivate extensions API.
   set_stream_has_handler(true);
   set_plugin_available(false);
   allow_download = false;
   must_download = false;
   resource_type = RESOURCE_TYPE_OBJECT;
   EXPECT_TRUE(
       TestStreamIsIntercepted(allow_download, must_download, resource_type));
 
   // Test the cases where the stream handled by MaybeInterceptAsStream
   // with plugin not available. This is the case when intercepting streams for
   // the streamsPrivate extensions API with stale plugin.
   set_plugin_stale(true);
   allow_download = false;
   must_download = false;
   resource_type = RESOURCE_TYPE_OBJECT;
   EXPECT_TRUE(
       TestStreamIsIntercepted(allow_download, must_download, resource_type));
 }
 #endif
 
-}  // namespace
+// Test that the MimeSniffingHandler operates properly when it doesn't sniff
+// resources.
+TEST_F(MimeSniffingResourceHandlerTest, NoSniffing) {
+  // Test simple case.
+  TestHandlerNoSniffing(true, false, true, true, false);

[mmenke, 2016/07/25 17:39:32]

All of these argument need labels.  As-is, it's just too hard to tell what these
calls are doing.  Alternatively, could add enum classes for each argument.  Some
goes for TestHandlerSniffing. 

[clamy, 2016/07/26 16:24:16]

Done.

+
+  // Test deferral in OnResponseStarted and/or in OnReadCompleted.
+  TestHandlerNoSniffing(true, true, true, true, false);
+  TestHandlerNoSniffing(true, false, true, true, true);
+  TestHandlerNoSniffing(true, true, true, true, true);
+
+  // Test cancel in OnResponseStarted, OnWillRead, OnReadCompleted.
+  TestHandlerNoSniffing(false, false, false, false, false);
+  TestHandlerNoSniffing(true, false, false, false, false);
+  TestHandlerNoSniffing(true, false, true, false, false);
+
+  // Test cancel after OnResponseStarted deferral.
+  TestHandlerNoSniffing(true, true, false, false, false);
+  TestHandlerNoSniffing(true, true, true, false, false);
+
+  content::RunAllPendingInMessageLoop();
+}
+
+// Test that the MimeSniffingHandler operates properly when it sniffs
+// resources.
+TEST_F(MimeSniffingResourceHandlerTest, Sniffing) {
+  // Test simple case.
+  TestHandlerSniffing(true, false, true, true, false);
+
+  // Test deferral in OnResponseStarted and/or in OnReadCompleted.
+  TestHandlerSniffing(true, true, true, true, false);
+  TestHandlerSniffing(true, false, true, true, true);
+  TestHandlerSniffing(true, true, true, true, true);
+
+  // Test cancel in OnResponseStarted, OnWillRead, OnReadCompleted.
+  TestHandlerSniffing(false, false, false, false, false);
+  TestHandlerSniffing(true, false, false, false, false);
+  TestHandlerSniffing(true, false, true, false, false);
+
+  // Test cancel after OnResponseStarted deferral.
+  TestHandlerSniffing(true, true, false, false, false);
+  TestHandlerSniffing(true, true, true, false, false);
+
+  content::RunAllPendingInMessageLoop();
+}
+
+// Tests that 304s do not trigger a change in handlers.
+TEST_F(MimeSniffingResourceHandlerTest, 304Handling) {
+  net::URLRequestContext context;
+  std::unique_ptr<net::URLRequest> request(context.CreateRequest(
+      GURL("http://www.google.com"), net::DEFAULT_PRIORITY, nullptr));
+  ResourceRequestInfo::AllocateForTesting(request.get(),
+                                          RESOURCE_TYPE_MAIN_FRAME,
+                                          nullptr,  // context
+                                          0,        // render_process_id
+                                          0,        // render_view_id
+                                          0,        // render_frame_id
+                                          true,     // is_main_frame
+                                          false,    // parent_is_main_frame
+                                          true,     // allow_download
+                                          true,     // is_async
+                                          false);   // is_using_lofi
+
+  TestResourceDispatcherHost host(false);
+  TestResourceDispatcherHostDelegate host_delegate(false);
+  host.SetDelegate(&host_delegate);
+
+  TestFakePluginService plugin_service(false, false);
+  std::unique_ptr<ResourceHandler> intercepting_handler(
+      new InterceptingResourceHandler(std::unique_ptr<ResourceHandler>(),
+                                      nullptr));
+  std::unique_ptr<ResourceHandler> mime_handler(new MimeSniffingResourceHandler(
+      std::unique_ptr<ResourceHandler>(
+          new TestResourceHandler(true, false, true, true, false)),
+      &host, &plugin_service,
+      static_cast<InterceptingResourceHandler*>(intercepting_handler.get()),
+      request.get()));
+
+  TestResourceController resource_controller;
+  mime_handler->SetController(&resource_controller);
+
+  // Simulate a 304 response.
+  scoped_refptr<ResourceResponse> response(new ResourceResponse);
+  // The MIME type isn't important but it shouldn't be empty.
+  response->head.mime_type = "application/pdf";
+  response->head.headers = new net::HttpResponseHeaders("HTTP/1.x 304 OK");
+
+  // The response is received. No new ResourceHandler should be created to
+  // handle the download.
+  bool defer = false;
+  mime_handler->OnResponseStarted(response.get(), &defer);
+  EXPECT_FALSE(defer);
+  TestResourceHandler* new_handler = host.last_resource_handler();
+  EXPECT_TRUE(!new_handler);
+
+  content::RunAllPendingInMessageLoop();
+}
 
 }  // namespace content