ServiceWorker: Bypass SW when the script doesn't have fetch handler

content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc

Index: content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc
diff --git a/content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc b/content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc
index 37330150cf04cd9b1f4d01a96483d190486b95c7..d95c5cd58804c9f3ce9845ed71b25471e2c87ce5 100644
--- a/content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc
+++ b/content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc
@@ -55,6 +55,48 @@ class FailureHelper : public EmbeddedWorkerTestHelper {
 
 class ServiceWorkerControlleeRequestHandlerTest : public testing::Test {
  public:
+  struct ServiceWorkerRequestTestResources {

[falken, 2016/05/31 01:19:51]

The style guide is to use structs only for "passive objects" that carry data.
This object is doing some heavy work with MaybeCreateJob(). It's probably more
suitable as a class.

[shimazu, 2016/05/31 02:18:10]

Done.

+    std::unique_ptr<net::URLRequest> request;
+    std::unique_ptr<ServiceWorkerControlleeRequestHandler> handler;
+    std::unique_ptr<net::URLRequestJob> job;
+
+    ServiceWorkerURLRequestJob* GetJob() {
+      return static_cast<ServiceWorkerURLRequestJob*>(job.get());
+    }
+
+    static std::unique_ptr<ServiceWorkerRequestTestResources> Create(
+        ServiceWorkerControlleeRequestHandlerTest* test,
+        const GURL& url,
+        ResourceType type) {
+      return std::unique_ptr<ServiceWorkerRequestTestResources>(
+          new ServiceWorkerRequestTestResources(test, url, type));
+    }

[falken, 2016/05/31 01:19:51]

Alternatively, you don't need Create and use the ctor directly. The uses below
don't seem to demand a pointer.

[shimazu, 2016/05/31 02:18:10]

Done.

+
+   private:
+    ServiceWorkerRequestTestResources(
+        ServiceWorkerControlleeRequestHandlerTest* test,
+        const GURL& url,
+        ResourceType type)
+        : request(test->url_request_context_.CreateRequest(
+              url,
+              net::DEFAULT_PRIORITY,
+              &test->url_request_delegate_)),
+          handler(new ServiceWorkerControlleeRequestHandler(
+              test->context()->AsWeakPtr(),
+              test->provider_host_,
+              base::WeakPtr<storage::BlobStorageContext>(),
+              FETCH_REQUEST_MODE_NO_CORS,
+              FETCH_CREDENTIALS_MODE_OMIT,
+              FetchRedirectMode::FOLLOW_MODE,
+              type,
+              REQUEST_CONTEXT_TYPE_HYPERLINK,
+              REQUEST_CONTEXT_FRAME_TYPE_TOP_LEVEL,
+              scoped_refptr<ResourceRequestBody>())),
+          job(handler->MaybeCreateJob(request.get(),

[falken, 2016/05/31 01:19:51]

The coding style says to not do heavy work in constructors. Could you move this
MaybeCreateJob() call out of the constructor. Maybe GetJob() can change to
MaybeCreateJob()?

[shimazu, 2016/05/31 02:18:10]

Done.

+                                      nullptr,
+                                      &test->mock_resource_context_)) {}
+  };
+
   ServiceWorkerControlleeRequestHandlerTest()
       : browser_thread_bundle_(TestBrowserThreadBundle::IO_MAINLOOP) {}
 
@@ -131,6 +173,7 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, DisallowServiceWorker) {
 
   // Store an activated worker.
   version_->SetStatus(ServiceWorkerVersion::ACTIVATED);
+  version_->set_has_fetch_handler(true);
   registration_->SetActiveVersion(version_);
   context()->storage()->StoreRegistration(
       registration_.get(),
@@ -139,21 +182,9 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, DisallowServiceWorker) {
   base::RunLoop().RunUntilIdle();
 
   // Conduct a main resource load.
-  const GURL kDocUrl("http://host/scope/doc");
-  std::unique_ptr<net::URLRequest> request = url_request_context_.CreateRequest(
-      kDocUrl, net::DEFAULT_PRIORITY, &url_request_delegate_);
-  std::unique_ptr<ServiceWorkerControlleeRequestHandler> handler(
-      new ServiceWorkerControlleeRequestHandler(
-          context()->AsWeakPtr(), provider_host_,
-          base::WeakPtr<storage::BlobStorageContext>(),
-          FETCH_REQUEST_MODE_NO_CORS, FETCH_CREDENTIALS_MODE_OMIT,
-          FetchRedirectMode::FOLLOW_MODE, RESOURCE_TYPE_MAIN_FRAME,
-          REQUEST_CONTEXT_TYPE_HYPERLINK, REQUEST_CONTEXT_FRAME_TYPE_TOP_LEVEL,
-          scoped_refptr<ResourceRequestBody>()));
-  std::unique_ptr<net::URLRequestJob> job(
-      handler->MaybeCreateJob(request.get(), nullptr, &mock_resource_context_));
-  ServiceWorkerURLRequestJob* sw_job =
-      static_cast<ServiceWorkerURLRequestJob*>(job.get());
+  auto test_resources = ServiceWorkerRequestTestResources::Create(
+      this, GURL("http://host/scope/doc"), RESOURCE_TYPE_MAIN_FRAME);
+  ServiceWorkerURLRequestJob* sw_job = test_resources->GetJob();
 
   EXPECT_FALSE(sw_job->ShouldFallbackToNetwork());
   EXPECT_FALSE(sw_job->ShouldForwardToServiceWorker());
@@ -171,6 +202,7 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, DisallowServiceWorker) {
 TEST_F(ServiceWorkerControlleeRequestHandlerTest, ActivateWaitingVersion) {
   // Store a registration that is installed but not activated yet.
   version_->SetStatus(ServiceWorkerVersion::INSTALLED);
+  version_->set_has_fetch_handler(true);
   registration_->SetWaitingVersion(version_);
   context()->storage()->StoreRegistration(
       registration_.get(),
@@ -179,21 +211,9 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, ActivateWaitingVersion) {
   base::RunLoop().RunUntilIdle();
 
   // Conduct a main resource load.
-  const GURL kDocUrl("http://host/scope/doc");
-  std::unique_ptr<net::URLRequest> request = url_request_context_.CreateRequest(
-      kDocUrl, net::DEFAULT_PRIORITY, &url_request_delegate_);
-  std::unique_ptr<ServiceWorkerControlleeRequestHandler> handler(
-      new ServiceWorkerControlleeRequestHandler(
-          context()->AsWeakPtr(), provider_host_,
-          base::WeakPtr<storage::BlobStorageContext>(),
-          FETCH_REQUEST_MODE_NO_CORS, FETCH_CREDENTIALS_MODE_OMIT,
-          FetchRedirectMode::FOLLOW_MODE, RESOURCE_TYPE_MAIN_FRAME,
-          REQUEST_CONTEXT_TYPE_HYPERLINK, REQUEST_CONTEXT_FRAME_TYPE_TOP_LEVEL,
-          scoped_refptr<ResourceRequestBody>()));
-  std::unique_ptr<net::URLRequestJob> job(
-      handler->MaybeCreateJob(request.get(), nullptr, &mock_resource_context_));
-  ServiceWorkerURLRequestJob* sw_job =
-      static_cast<ServiceWorkerURLRequestJob*>(job.get());
+  auto test_resources = ServiceWorkerRequestTestResources::Create(
+      this, GURL("http://host/scope/doc"), RESOURCE_TYPE_MAIN_FRAME);
+  ServiceWorkerURLRequestJob* sw_job = test_resources->GetJob();
 
   EXPECT_FALSE(sw_job->ShouldFallbackToNetwork());
   EXPECT_FALSE(sw_job->ShouldForwardToServiceWorker());
@@ -208,7 +228,7 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, ActivateWaitingVersion) {
   EXPECT_TRUE(version_->HasControllee());
 
   // Navigations should trigger an update too.
-  handler.reset(NULL);
+  test_resources->handler.reset(nullptr);
   EXPECT_TRUE(version_->update_timer_.IsRunning());
 }
 
@@ -216,23 +236,15 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, ActivateWaitingVersion) {
 TEST_F(ServiceWorkerControlleeRequestHandlerTest, InstallingRegistration) {
   // Create an installing registration.
   version_->SetStatus(ServiceWorkerVersion::INSTALLING);
+  version_->set_has_fetch_handler(true);
   registration_->SetInstallingVersion(version_);
   context()->storage()->NotifyInstallingRegistration(registration_.get());
 
   // Conduct a main resource load.
-  const GURL kDocUrl("http://host/scope/doc");
-  std::unique_ptr<net::URLRequest> request = url_request_context_.CreateRequest(
-      kDocUrl, net::DEFAULT_PRIORITY, &url_request_delegate_);
-  std::unique_ptr<ServiceWorkerControlleeRequestHandler> handler(
-      new ServiceWorkerControlleeRequestHandler(
-          context()->AsWeakPtr(), provider_host_,
-          base::WeakPtr<storage::BlobStorageContext>(),
-          FETCH_REQUEST_MODE_NO_CORS, FETCH_CREDENTIALS_MODE_OMIT,
-          FetchRedirectMode::FOLLOW_MODE, RESOURCE_TYPE_MAIN_FRAME,
-          REQUEST_CONTEXT_TYPE_HYPERLINK, REQUEST_CONTEXT_FRAME_TYPE_TOP_LEVEL,
-          scoped_refptr<ResourceRequestBody>()));
-  std::unique_ptr<net::URLRequestJob> job(
-      handler->MaybeCreateJob(request.get(), nullptr, &mock_resource_context_));
+  auto test_resources = ServiceWorkerRequestTestResources::Create(
+      this, GURL("http://host/scope/doc"), RESOURCE_TYPE_MAIN_FRAME);
+  ServiceWorkerURLRequestJob* job = test_resources->GetJob();
+
   base::RunLoop().RunUntilIdle();
 
   // The handler should have fallen back to network and destroyed the job. The
@@ -250,6 +262,7 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, DeletedProviderHost) {
   // Store a registration so the call to FindRegistrationForDocument will read
   // from the database.
   version_->SetStatus(ServiceWorkerVersion::ACTIVATED);
+  version_->set_has_fetch_handler(true);
   registration_->SetActiveVersion(version_);
   context()->storage()->StoreRegistration(
       registration_.get(),
@@ -260,21 +273,9 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, DeletedProviderHost) {
   registration_ = NULL;
 
   // Conduct a main resource load.
-  const GURL kDocUrl("http://host/scope/doc");
-  std::unique_ptr<net::URLRequest> request = url_request_context_.CreateRequest(
-      kDocUrl, net::DEFAULT_PRIORITY, &url_request_delegate_);
-  std::unique_ptr<ServiceWorkerControlleeRequestHandler> handler(
-      new ServiceWorkerControlleeRequestHandler(
-          context()->AsWeakPtr(), provider_host_,
-          base::WeakPtr<storage::BlobStorageContext>(),
-          FETCH_REQUEST_MODE_NO_CORS, FETCH_CREDENTIALS_MODE_OMIT,
-          FetchRedirectMode::FOLLOW_MODE, RESOURCE_TYPE_MAIN_FRAME,
-          REQUEST_CONTEXT_TYPE_HYPERLINK, REQUEST_CONTEXT_FRAME_TYPE_TOP_LEVEL,
-          scoped_refptr<ResourceRequestBody>()));
-  std::unique_ptr<net::URLRequestJob> job(
-      handler->MaybeCreateJob(request.get(), nullptr, &mock_resource_context_));
-  ServiceWorkerURLRequestJob* sw_job =
-      static_cast<ServiceWorkerURLRequestJob*>(job.get());
+  auto test_resources = ServiceWorkerRequestTestResources::Create(
+      this, GURL("http://host/scope/doc"), RESOURCE_TYPE_MAIN_FRAME);
+  ServiceWorkerURLRequestJob* sw_job = test_resources->GetJob();
 
   EXPECT_FALSE(sw_job->ShouldFallbackToNetwork());
   EXPECT_FALSE(sw_job->ShouldForwardToServiceWorker());
@@ -289,4 +290,37 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, DeletedProviderHost) {
   EXPECT_FALSE(sw_job->ShouldForwardToServiceWorker());
 }
 
+TEST_F(ServiceWorkerControlleeRequestHandlerTest, FallbackWithNoFetchHandler) {
+  version_->SetStatus(ServiceWorkerVersion::ACTIVATED);
+  version_->set_has_fetch_handler(false);
+  registration_->SetActiveVersion(version_);
+  context()->storage()->StoreRegistration(
+      registration_.get(), version_.get(),
+      base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));
+  base::RunLoop().RunUntilIdle();
+
+  auto main_test_resources = ServiceWorkerRequestTestResources::Create(
+      this, GURL("http://host/scope/doc"), RESOURCE_TYPE_MAIN_FRAME);
+  ServiceWorkerURLRequestJob* main_job = main_test_resources->GetJob();
+
+  EXPECT_FALSE(main_job->ShouldFallbackToNetwork());
+  EXPECT_FALSE(main_job->ShouldForwardToServiceWorker());
+  EXPECT_FALSE(version_->HasControllee());
+
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_TRUE(main_job->ShouldFallbackToNetwork());
+  EXPECT_FALSE(main_job->ShouldForwardToServiceWorker());
+  EXPECT_TRUE(version_->HasControllee());
+  EXPECT_EQ(version_, provider_host_->controlling_version());
+
+  auto sub_test_resources = ServiceWorkerRequestTestResources::Create(
+      this, GURL("http://host/scope/doc/subresource"), RESOURCE_TYPE_IMAGE);
+  ServiceWorkerURLRequestJob* sub_job = sub_test_resources->GetJob();
+
+  // This job shouldn't be created because this worker doesn't have fetch
+  // handler.
+  EXPECT_EQ(nullptr, sub_job);
+}
+
 }  // namespace content