service worker: Check installing registrations in ServiceWorkerRegistry (3 of 3)

This is similar to crrev.com/c/1999974 but for
FindRegistrationForId{,Only}().

ServiceWorkerStorage::DidFindRegistrationForId() became identical
to DidFindRegistration() so it was merged into a single method.

This CL also changes some tests not to use
ServiceWorkerStorage directly. Use ServiceWorkerRegistry
instead.

FindInstallingRegistrationXXX() are now private methods because
ServiceWorkerStorage doesn't use them anymore.

Bug: 1039200
Change-Id: Ia218afc7109b5f697933956719890442de68d51a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2004350Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#732671}

content/browser/background_fetch/background_fetch_test_base.cc

@@ -130,7 +130,7 @@ int64_t BackgroundFetchTestBase::RegisterServiceWorkerForOrigin(
 
   {
     base::RunLoop run_loop;
-    embedded_worker_test_helper_.context()->storage()->FindRegistrationForId(
+    embedded_worker_test_helper_.context()->registry()->FindRegistrationForId(
         service_worker_registration_id, origin.GetURL(),
         base::BindOnce(&DidFindServiceWorkerRegistration,
                        &service_worker_registration, run_loop.QuitClosure()));

content/browser/content_index/content_index_database_unittest.cc

@@ -257,7 +257,7 @@ class ContentIndexDatabaseTest : public ::testing::Test {
 
     {
       base::RunLoop run_loop;
-      embedded_worker_test_helper_.context()->storage()->FindRegistrationForId(
+      embedded_worker_test_helper_.context()->registry()->FindRegistrationForId(
           service_worker_registration_id, origin_.GetURL(),
           base::BindOnce(&DidFindServiceWorkerRegistration,
                          &service_worker_registration_,

content/browser/devtools/devtools_background_services_context_impl_unittest.cc

@@ -224,7 +224,7 @@ class DevToolsBackgroundServicesContextTest
 
     {
       base::RunLoop run_loop;
-      embedded_worker_test_helper_.context()->storage()->FindRegistrationForId(
+      embedded_worker_test_helper_.context()->registry()->FindRegistrationForId(
           service_worker_registration_id, origin_.GetURL(),
           base::BindOnce(&DidFindServiceWorkerRegistration,
                          &service_worker_registration_,

content/browser/notifications/blink_notification_service_impl_unittest.cc

@@ -189,7 +189,7 @@ class BlinkNotificationServiceImplTest : public ::testing::Test {
 
     {
       base::RunLoop run_loop;
-      embedded_worker_helper_->context()->storage()->FindRegistrationForId(
+      embedded_worker_helper_->context()->registry()->FindRegistrationForId(
           service_worker_registration_id, GURL(kTestOrigin),
           base::BindOnce(&BlinkNotificationServiceImplTest::
                              DidFindServiceWorkerRegistration,

content/browser/notifications/notification_storage_unittest.cc

@@ -84,7 +84,7 @@ class NotificationStorageTest : public ::testing::Test {
 
     {
       base::RunLoop run_loop;
-      helper_->context()->storage()->FindRegistrationForId(
+      helper_->context()->registry()->FindRegistrationForId(
           service_worker_registration_id_, origin_,
           base::BindOnce(
               &NotificationStorageTest::DidFindServiceWorkerRegistration,

content/browser/service_worker/service_worker_registry.cc

@@ -88,14 +88,21 @@ void ServiceWorkerRegistry::FindRegistrationForId(
     int64_t registration_id,
     const GURL& origin,
     FindRegistrationCallback callback) {
-  storage()->FindRegistrationForId(registration_id, origin,
-                                   std::move(callback));
+  storage()->FindRegistrationForId(
+      registration_id, origin,
+      base::BindOnce(&ServiceWorkerRegistry::DidFindRegistrationForId,
+                     weak_factory_.GetWeakPtr(), registration_id,
+                     std::move(callback)));
 }
 
 void ServiceWorkerRegistry::FindRegistrationForIdOnly(
     int64_t registration_id,
     FindRegistrationCallback callback) {
-  storage()->FindRegistrationForIdOnly(registration_id, std::move(callback));
+  storage()->FindRegistrationForIdOnly(
+      registration_id,
+      base::BindOnce(&ServiceWorkerRegistry::DidFindRegistrationForId,
+                     weak_factory_.GetWeakPtr(), registration_id,
+                     std::move(callback)));
 }
 
 ServiceWorkerRegistration* ServiceWorkerRegistry::GetUninstallingRegistration(
@@ -280,4 +287,23 @@ void ServiceWorkerRegistry::DidFindRegistrationForScope(
   CompleteFindNow(std::move(registration), status, std::move(callback));
 }
 
+void ServiceWorkerRegistry::DidFindRegistrationForId(
+    int64_t registration_id,
+    FindRegistrationCallback callback,
+    blink::ServiceWorkerStatusCode status,
+    scoped_refptr<ServiceWorkerRegistration> registration) {
+  if (status == blink::ServiceWorkerStatusCode::kErrorNotFound) {
+    // Look for something currently being installed.
+    scoped_refptr<ServiceWorkerRegistration> installing_registration =
+        FindInstallingRegistrationForId(registration_id);
+    if (installing_registration) {
+      CompleteFindNow(std::move(installing_registration),
+                      blink::ServiceWorkerStatusCode::kOk, std::move(callback));
+      return;
+    }
+  }
+
+  CompleteFindNow(std::move(registration), status, std::move(callback));
+}
+
 }  // namespace content

content/browser/service_worker/service_worker_registry.h

@@ -83,16 +83,6 @@ class CONTENT_EXPORT ServiceWorkerRegistry {
       ServiceWorkerRegistration* registration,
       ServiceWorkerRegistration::Status new_status);
 
-  // TODO(crbug.com/1039200): Make these methods private once methods/fields
-  // related to ServiceWorkerRegistration in ServiceWorkerStorage are moved
-  // into this class.
-  ServiceWorkerRegistration* FindInstallingRegistrationForClientUrl(
-      const GURL& client_url);
-  ServiceWorkerRegistration* FindInstallingRegistrationForScope(
-      const GURL& scope);
-  ServiceWorkerRegistration* FindInstallingRegistrationForId(
-      int64_t registration_id);
-
   // TODO(crbug.com/1039200): Make this private once methods/fields related to
   // ServiceWorkerRegistration in ServiceWorkerStorage are moved into this
   // class.
@@ -113,6 +103,13 @@ class CONTENT_EXPORT ServiceWorkerRegistry {
   }
 
  private:
+  ServiceWorkerRegistration* FindInstallingRegistrationForClientUrl(
+      const GURL& client_url);
+  ServiceWorkerRegistration* FindInstallingRegistrationForScope(
+      const GURL& scope);
+  ServiceWorkerRegistration* FindInstallingRegistrationForId(
+      int64_t registration_id);
+
   void DidFindRegistrationForClientUrl(
       const GURL& client_url,
       int64_t trace_event_id,
@@ -124,6 +121,11 @@ class CONTENT_EXPORT ServiceWorkerRegistry {
       FindRegistrationCallback callback,
       blink::ServiceWorkerStatusCode status,
       scoped_refptr<ServiceWorkerRegistration> registration);
+  void DidFindRegistrationForId(
+      int64_t registration_id,
+      FindRegistrationCallback callback,
+      blink::ServiceWorkerStatusCode status,
+      scoped_refptr<ServiceWorkerRegistration> registration);
 
   // The ServiceWorkerContextCore object must outlive this.
   ServiceWorkerContextCore* const context_;

content/browser/service_worker/service_worker_storage.cc

@@ -218,9 +218,8 @@ void ServiceWorkerStorage::FindRegistrationForId(
     FindRegistrationCallback callback) {
   switch (state_) {
     case STORAGE_STATE_DISABLED:
-      CompleteFindNow(scoped_refptr<ServiceWorkerRegistration>(),
-                      blink::ServiceWorkerStatusCode::kErrorAbort,
-                      std::move(callback));
+      std::move(callback).Run(blink::ServiceWorkerStatusCode::kErrorAbort,
+                              nullptr);
       return;
     case STORAGE_STATE_INITIALIZING:  // Fall-through.
     case STORAGE_STATE_UNINITIALIZED:
@@ -233,16 +232,10 @@ void ServiceWorkerStorage::FindRegistrationForId(
       break;
   }
 
-  // See if there are any stored registrations for the origin.
+  // Bypass database lookup when there is no stored registration.
   if (!base::Contains(registered_origins_, origin)) {
-    // Look for something currently being installed.
-    scoped_refptr<ServiceWorkerRegistration> installing_registration =
-        registry_->FindInstallingRegistrationForId(registration_id);
-    CompleteFindNow(installing_registration,
-                    installing_registration
-                        ? blink::ServiceWorkerStatusCode::kOk
-                        : blink::ServiceWorkerStatusCode::kErrorNotFound,
-                    std::move(callback));
+    std::move(callback).Run(blink::ServiceWorkerStatusCode::kErrorNotFound,
+                            nullptr);
     return;
   }
 
@@ -259,7 +252,7 @@ void ServiceWorkerStorage::FindRegistrationForId(
       base::BindOnce(
           &FindForIdInDB, database_.get(), base::ThreadTaskRunnerHandle::Get(),
           registration_id, origin,
-          base::BindOnce(&ServiceWorkerStorage::DidFindRegistrationForId,
+          base::BindOnce(&ServiceWorkerStorage::DidFindRegistration,
                          weak_factory_.GetWeakPtr(), std::move(callback))));
 }
 
@@ -268,8 +261,8 @@ void ServiceWorkerStorage::FindRegistrationForIdOnly(
     FindRegistrationCallback callback) {
   switch (state_) {
     case STORAGE_STATE_DISABLED:
-      CompleteFindNow(nullptr, blink::ServiceWorkerStatusCode::kErrorAbort,
-                      std::move(callback));
+      std::move(callback).Run(blink::ServiceWorkerStatusCode::kErrorAbort,
+                              nullptr);
       return;
     case STORAGE_STATE_INITIALIZING:  // Fall-through.
     case STORAGE_STATE_UNINITIALIZED:
@@ -298,7 +291,7 @@ void ServiceWorkerStorage::FindRegistrationForIdOnly(
       base::BindOnce(
           &FindForIdOnlyInDB, database_.get(),
           base::ThreadTaskRunnerHandle::Get(), registration_id,
-          base::BindOnce(&ServiceWorkerStorage::DidFindRegistrationForId,
+          base::BindOnce(&ServiceWorkerStorage::DidFindRegistration,
                          weak_factory_.GetWeakPtr(), std::move(callback))));
 }
 
@@ -1189,28 +1182,6 @@ void ServiceWorkerStorage::DidFindRegistration(
                           scoped_refptr<ServiceWorkerRegistration>());
 }
 
-void ServiceWorkerStorage::DidFindRegistrationForId(
-    FindRegistrationCallback callback,
-    const ServiceWorkerDatabase::RegistrationData& data,
-    const ResourceList& resources,
-    ServiceWorkerDatabase::Status status) {
-  if (status == ServiceWorkerDatabase::STATUS_OK) {
-    ReturnFoundRegistration(std::move(callback), data, resources);
-    return;
-  }
-
-  if (status == ServiceWorkerDatabase::STATUS_ERROR_NOT_FOUND) {
-    // TODO(nhiroki): Find a registration in |installing_registrations_|.
-    std::move(callback).Run(DatabaseStatusToStatusCode(status),
-                            scoped_refptr<ServiceWorkerRegistration>());
-    return;
-  }
-
-  ScheduleDeleteAndStartOver();
-  std::move(callback).Run(DatabaseStatusToStatusCode(status),
-                          scoped_refptr<ServiceWorkerRegistration>());
-}
-
 void ServiceWorkerStorage::ReturnFoundRegistration(
     FindRegistrationCallback callback,
     const ServiceWorkerDatabase::RegistrationData& data,

content/browser/service_worker/service_worker_storage.h

@@ -389,11 +389,6 @@ class CONTENT_EXPORT ServiceWorkerStorage {
                            const ServiceWorkerDatabase::RegistrationData& data,
                            const ResourceList& resources,
                            ServiceWorkerDatabase::Status status);
-  void DidFindRegistrationForId(
-      FindRegistrationCallback callback,
-      const ServiceWorkerDatabase::RegistrationData& data,
-      const ResourceList& resources,
-      ServiceWorkerDatabase::Status status);
   void DidGetRegistrationsForOrigin(GetRegistrationsCallback callback,
                                     RegistrationList* registration_data_list,
                                     std::vector<ResourceList>* resources_list,