ServiceWorker: Rename OnRegistrationStored -> OnRegistrationCompleted.

For historical reasons, OnRegistrationStored() is called when the
promise returned by ServiceWorkerContainer.register() is resolved, which
happens before the registration is written to disk. This means
OnRegistrationStored() implementations cannot assume that
ServiceWorkerContextCore will be able to retrieve the registration.

This CL renames the method to a more appropriate name. A follow-up CL
will add an OnRegistrationStored() method, which will be called when the
registration is written to the database.

TBR=benwells

Bug: 729800
Change-Id: If273b3008ff55e4d5cd8f5be9e0e6090da2e5390
Reviewed-on: https://chromium-review.googlesource.com/1039083
Commit-Queue: Victor Costan <pwnall@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555550}

chrome/browser/installable/installable_manager.cc

@@ -532,8 +532,8 @@ void InstallableManager::OnDidCheckHasServiceWorker(
     case content::ServiceWorkerCapability::NO_SERVICE_WORKER:
       InstallableTask& task = task_queue_.Current();
       if (task.params.wait_for_worker) {
-        // Wait for ServiceWorkerContextObserver::OnRegistrationStored. Set the
-        // param |wait_for_worker| to false so we only wait once per task.
+        // Wait for ServiceWorkerContextObserver::OnRegistrationCompleted. Set
+        // the param |wait_for_worker| to false so we only wait once per task.
         task.params.wait_for_worker = false;
         OnWaitingForServiceWorker();
         task_queue_.PauseCurrent();
@@ -597,7 +597,7 @@ void InstallableManager::OnIconFetched(const GURL icon_url,
   WorkOnTask();
 }
 
-void InstallableManager::OnRegistrationStored(const GURL& pattern) {
+void InstallableManager::OnRegistrationCompleted(const GURL& pattern) {
   // If the scope doesn't match we keep waiting.
   if (!content::ServiceWorkerContext::ScopeMatches(pattern,
                                                    manifest().start_url)) {

chrome/browser/installable/installable_manager.h

@@ -192,7 +192,7 @@ class InstallableManager
                      const SkBitmap& bitmap);
 
   // content::ServiceWorkerContextObserver overrides
-  void OnRegistrationStored(const GURL& pattern) override;
+  void OnRegistrationCompleted(const GURL& pattern) override;
 
   // content::WebContentsObserver overrides
   void DidFinishNavigation(content::NavigationHandle* handle) override;

content/browser/resources/service_worker/serviceworker_internals.js

@@ -220,7 +220,7 @@ cr.define('serviceworker', function() {
     update();
   }
 
-  function onRegistrationStored(scope) {
+  function onRegistrationCompleted(scope) {
     update();
   }
 
@@ -258,7 +258,7 @@ cr.define('serviceworker', function() {
     onErrorReported: onErrorReported,
     onConsoleMessageReported: onConsoleMessageReported,
     onVersionStateChanged: onVersionStateChanged,
-    onRegistrationStored: onRegistrationStored,
+    onRegistrationCompleted: onRegistrationCompleted,
     onRegistrationDeleted: onRegistrationDeleted,
   };
 });

content/browser/service_worker/service_worker_context_core.cc

@@ -550,11 +550,10 @@ void ServiceWorkerContextCore::RegistrationComplete(
 
   DCHECK(registration);
   std::move(callback).Run(status, status_message, registration->id());
-  // TODO(falken): At this point the registration promise is resolved, but we
-  // haven't persisted anything to storage yet. So we should either call
-  // OnRegistrationStored somewhere else or change its name.
+  // At this point the registration promise is resolved, but we haven't
+  // persisted anything to storage yet.
   observer_list_->Notify(
-      FROM_HERE, &ServiceWorkerContextCoreObserver::OnRegistrationStored,
+      FROM_HERE, &ServiceWorkerContextCoreObserver::OnRegistrationCompleted,
       registration->id(), pattern);
 }
 

content/browser/service_worker/service_worker_context_core_observer.h

@@ -89,8 +89,13 @@ class ServiceWorkerContextCoreObserver {
       blink::mojom::ServiceWorkerProviderType type) {}
   virtual void OnControlleeRemoved(int64_t version_id,
                                    const std::string& uuid) {}
-  virtual void OnRegistrationStored(int64_t registration_id,
-                                    const GURL& pattern) {}
+  // Called when the ServiceWorkerContainer.register() promise is resolved.
+  //
+  // This is called before the service worker registration is persisted to disk.
+  // The caller cannot assume that the ServiceWorkerContextCore will find the
+  // registration at this point.
+  virtual void OnRegistrationCompleted(int64_t registration_id,
+                                       const GURL& pattern) {}
   virtual void OnRegistrationDeleted(int64_t registration_id,
                                      const GURL& pattern) {}
 

content/browser/service_worker/service_worker_context_unittest.cc

@@ -111,7 +111,7 @@ class RejectActivateTestHelper : public EmbeddedWorkerTestHelper {
 };
 
 enum NotificationType {
-  REGISTRATION_STORED,
+  REGISTRATION_COMPLETED,
   REGISTRATION_DELETED,
   STORAGE_RECOVERED,
 };
@@ -138,10 +138,10 @@ class ServiceWorkerContextTest : public ServiceWorkerContextCoreObserver,
   void TearDown() override { helper_.reset(); }
 
   // ServiceWorkerContextCoreObserver overrides.
-  void OnRegistrationStored(int64_t registration_id,
-                            const GURL& pattern) override {
+  void OnRegistrationCompleted(int64_t registration_id,
+                               const GURL& pattern) override {
     NotificationLog log;
-    log.type = REGISTRATION_STORED;
+    log.type = REGISTRATION_COMPLETED;
     log.pattern = pattern;
     log.registration_id = registration_id;
     notifications_.push_back(log);
@@ -229,7 +229,7 @@ TEST_F(ServiceWorkerContextTest, Register) {
   EXPECT_NE(blink::mojom::kInvalidServiceWorkerRegistrationId, registration_id);
 
   ASSERT_EQ(1u, notifications_.size());
-  EXPECT_EQ(REGISTRATION_STORED, notifications_[0].type);
+  EXPECT_EQ(REGISTRATION_COMPLETED, notifications_[0].type);
   EXPECT_EQ(pattern, notifications_[0].pattern);
   EXPECT_EQ(registration_id, notifications_[0].registration_id);
 
@@ -278,7 +278,7 @@ TEST_F(ServiceWorkerContextTest, Register_RejectInstall) {
   EXPECT_NE(blink::mojom::kInvalidServiceWorkerRegistrationId, registration_id);
 
   ASSERT_EQ(1u, notifications_.size());
-  EXPECT_EQ(REGISTRATION_STORED, notifications_[0].type);
+  EXPECT_EQ(REGISTRATION_COMPLETED, notifications_[0].type);
   EXPECT_EQ(pattern, notifications_[0].pattern);
   EXPECT_EQ(registration_id, notifications_[0].registration_id);
 
@@ -326,7 +326,7 @@ TEST_F(ServiceWorkerContextTest, Register_RejectActivate) {
   EXPECT_NE(blink::mojom::kInvalidServiceWorkerRegistrationId, registration_id);
 
   ASSERT_EQ(1u, notifications_.size());
-  EXPECT_EQ(REGISTRATION_STORED, notifications_[0].type);
+  EXPECT_EQ(REGISTRATION_COMPLETED, notifications_[0].type);
   EXPECT_EQ(pattern, notifications_[0].pattern);
   EXPECT_EQ(registration_id, notifications_[0].registration_id);
 
@@ -369,7 +369,7 @@ TEST_F(ServiceWorkerContextTest, Unregister) {
   base::RunLoop().RunUntilIdle();
 
   ASSERT_EQ(2u, notifications_.size());
-  EXPECT_EQ(REGISTRATION_STORED, notifications_[0].type);
+  EXPECT_EQ(REGISTRATION_COMPLETED, notifications_[0].type);
   EXPECT_EQ(pattern, notifications_[0].pattern);
   EXPECT_EQ(registration_id, notifications_[0].registration_id);
   EXPECT_EQ(REGISTRATION_DELETED, notifications_[1].type);
@@ -474,16 +474,16 @@ TEST_F(ServiceWorkerContextTest, UnregisterMultiple) {
   base::RunLoop().RunUntilIdle();
 
   ASSERT_EQ(6u, notifications_.size());
-  EXPECT_EQ(REGISTRATION_STORED, notifications_[0].type);
+  EXPECT_EQ(REGISTRATION_COMPLETED, notifications_[0].type);
   EXPECT_EQ(registration_id1, notifications_[0].registration_id);
   EXPECT_EQ(origin1_p1, notifications_[0].pattern);
-  EXPECT_EQ(REGISTRATION_STORED, notifications_[1].type);
+  EXPECT_EQ(REGISTRATION_COMPLETED, notifications_[1].type);
   EXPECT_EQ(origin1_p2, notifications_[1].pattern);
   EXPECT_EQ(registration_id2, notifications_[1].registration_id);
-  EXPECT_EQ(REGISTRATION_STORED, notifications_[2].type);
+  EXPECT_EQ(REGISTRATION_COMPLETED, notifications_[2].type);
   EXPECT_EQ(origin2_p1, notifications_[2].pattern);
   EXPECT_EQ(registration_id3, notifications_[2].registration_id);
-  EXPECT_EQ(REGISTRATION_STORED, notifications_[3].type);
+  EXPECT_EQ(REGISTRATION_COMPLETED, notifications_[3].type);
   EXPECT_EQ(origin3_p1, notifications_[3].pattern);
   EXPECT_EQ(registration_id4, notifications_[3].registration_id);
   EXPECT_EQ(REGISTRATION_DELETED, notifications_[4].type);
@@ -529,10 +529,10 @@ TEST_F(ServiceWorkerContextTest, RegisterNewScript) {
   EXPECT_EQ(old_registration_id, new_registration_id);
 
   ASSERT_EQ(2u, notifications_.size());
-  EXPECT_EQ(REGISTRATION_STORED, notifications_[0].type);
+  EXPECT_EQ(REGISTRATION_COMPLETED, notifications_[0].type);
   EXPECT_EQ(pattern, notifications_[0].pattern);
   EXPECT_EQ(old_registration_id, notifications_[0].registration_id);
-  EXPECT_EQ(REGISTRATION_STORED, notifications_[1].type);
+  EXPECT_EQ(REGISTRATION_COMPLETED, notifications_[1].type);
   EXPECT_EQ(pattern, notifications_[1].pattern);
   EXPECT_EQ(new_registration_id, notifications_[1].registration_id);
 }
@@ -571,10 +571,10 @@ TEST_F(ServiceWorkerContextTest, RegisterDuplicateScript) {
   EXPECT_EQ(old_registration_id, new_registration_id);
 
   ASSERT_EQ(2u, notifications_.size());
-  EXPECT_EQ(REGISTRATION_STORED, notifications_[0].type);
+  EXPECT_EQ(REGISTRATION_COMPLETED, notifications_[0].type);
   EXPECT_EQ(pattern, notifications_[0].pattern);
   EXPECT_EQ(old_registration_id, notifications_[0].registration_id);
-  EXPECT_EQ(REGISTRATION_STORED, notifications_[1].type);
+  EXPECT_EQ(REGISTRATION_COMPLETED, notifications_[1].type);
   EXPECT_EQ(pattern, notifications_[1].pattern);
   EXPECT_EQ(old_registration_id, notifications_[1].registration_id);
 }
@@ -754,11 +754,11 @@ TEST_P(ServiceWorkerContextRecoveryTest, DeleteAndStartOver) {
   EXPECT_EQ(3, context()->GetNewServiceWorkerHandleId());
 
   ASSERT_EQ(3u, notifications_.size());
-  EXPECT_EQ(REGISTRATION_STORED, notifications_[0].type);
+  EXPECT_EQ(REGISTRATION_COMPLETED, notifications_[0].type);
   EXPECT_EQ(pattern, notifications_[0].pattern);
   EXPECT_EQ(registration_id, notifications_[0].registration_id);
   EXPECT_EQ(STORAGE_RECOVERED, notifications_[1].type);
-  EXPECT_EQ(REGISTRATION_STORED, notifications_[2].type);
+  EXPECT_EQ(REGISTRATION_COMPLETED, notifications_[2].type);
   EXPECT_EQ(pattern, notifications_[2].pattern);
   EXPECT_EQ(registration_id, notifications_[2].registration_id);
 }

content/browser/service_worker/service_worker_context_watcher.cc

@@ -362,8 +362,9 @@ void ServiceWorkerContextWatcher::OnControlleeRemoved(int64_t version_id,
   SendVersionInfo(*version);
 }
 
-void ServiceWorkerContextWatcher::OnRegistrationStored(int64_t registration_id,
-                                                       const GURL& pattern) {
+void ServiceWorkerContextWatcher::OnRegistrationCompleted(
+    int64_t registration_id,
+    const GURL& pattern) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   SendRegistrationInfo(registration_id, pattern,
                        ServiceWorkerRegistrationInfo::IS_NOT_DELETED);

content/browser/service_worker/service_worker_context_watcher.h

@@ -112,8 +112,8 @@ class CONTENT_EXPORT ServiceWorkerContextWatcher
       blink::mojom::ServiceWorkerProviderType type) override;
   void OnControlleeRemoved(int64_t version_id,
                            const std::string& uuid) override;
-  void OnRegistrationStored(int64_t registration_id,
-                            const GURL& pattern) override;
+  void OnRegistrationCompleted(int64_t registration_id,
+                               const GURL& pattern) override;
   void OnRegistrationDeleted(int64_t registration_id,
                              const GURL& pattern) override;
 

content/browser/service_worker/service_worker_context_wrapper.cc

@@ -242,10 +242,11 @@ ResourceContext* ServiceWorkerContextWrapper::resource_context() {
   return resource_context_;
 }
 
-void ServiceWorkerContextWrapper::OnRegistrationStored(int64_t registration_id,
-                                                       const GURL& pattern) {
+void ServiceWorkerContextWrapper::OnRegistrationCompleted(
+    int64_t registration_id,
+    const GURL& pattern) {
   for (auto& observer : observer_list_)
-    observer.OnRegistrationStored(pattern);
+    observer.OnRegistrationCompleted(pattern);
 }
 
 void ServiceWorkerContextWrapper::AddObserver(

content/browser/service_worker/service_worker_context_wrapper.h

@@ -98,8 +98,8 @@ class CONTENT_EXPORT ServiceWorkerContextWrapper
   }
 
   // ServiceWorkerContextCoreObserver implementation:
-  void OnRegistrationStored(int64_t registration_id,
-                            const GURL& pattern) override;
+  void OnRegistrationCompleted(int64_t registration_id,
+                               const GURL& pattern) override;
 
   // ServiceWorkerContext implementation:
   void AddObserver(ServiceWorkerContextObserver* observer) override;

content/browser/service_worker/service_worker_internals_ui.cc

@@ -306,11 +306,11 @@ class ServiceWorkerInternalsUI::PartitionObserver
     web_ui_->CallJavascriptFunctionUnsafe(
         "serviceworker.onConsoleMessageReported", ConvertToRawPtrVector(args));
   }
-  void OnRegistrationStored(int64_t registration_id,
-                            const GURL& pattern) override {
+  void OnRegistrationCompleted(int64_t registration_id,
+                               const GURL& pattern) override {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
-    web_ui_->CallJavascriptFunctionUnsafe("serviceworker.onRegistrationStored",
-                                          Value(pattern.spec()));
+    web_ui_->CallJavascriptFunctionUnsafe(
+        "serviceworker.onRegistrationCompleted", Value(pattern.spec()));
   }
   void OnRegistrationDeleted(int64_t registration_id,
                              const GURL& pattern) override {

content/public/browser/service_worker_context_observer.h

@@ -12,7 +12,11 @@ namespace content {
 class ServiceWorkerContextObserver {
  public:
   // Called when a service worker has been registered with scope |pattern|.
-  virtual void OnRegistrationStored(const GURL& pattern) {}
+  //
+  // This is called when the ServiceWorkerContainer.register() promise is
+  // resolved, which happens before the service worker registration is persisted
+  // to disk.
+  virtual void OnRegistrationCompleted(const GURL& pattern) {}
 
  protected:
   virtual ~ServiceWorkerContextObserver() {}