ServiceWorker: Consolidate version change messages

content/browser/service_worker/service_worker_provider_host.cc

Index: content/browser/service_worker/service_worker_provider_host.cc
diff --git a/content/browser/service_worker/service_worker_provider_host.cc b/content/browser/service_worker/service_worker_provider_host.cc
index 8112f3dd4977e6d0eafe3a6a726db7488c0bc901..9e29a107af189c8d9e0be81db7876078f2559408 100644
--- a/content/browser/service_worker/service_worker_provider_host.cc
+++ b/content/browser/service_worker/service_worker_provider_host.cc
@@ -49,12 +49,9 @@ void ServiceWorkerProviderHost::OnVersionAttributesChanged(
     ServiceWorkerRegistration* registration,
     ChangedVersionAttributesMask changed_mask,
     const ServiceWorkerRegistrationInfo& info) {

[michaeln, 2014/08/11 22:15:41]

maybe DCHECK(registration == accociated_registration), which is essentially the
gist of the CanSetVersion helper

[nhiroki, 2014/08/12 01:45:56]

Done.

-  if (changed_mask.installing_changed())
-    SetInstallingVersion(registration->installing_version());
-  if (changed_mask.waiting_changed())
-    SetWaitingVersion(registration->waiting_version());
-  if (changed_mask.active_changed())
-    SetActiveVersion(registration->active_version());
+  SetVersionAttributes(registration->installing_version(),
+                       registration->waiting_version(),
+                       registration->active_version());
 }
 
 void ServiceWorkerProviderHost::OnRegistrationFailed(
@@ -69,93 +66,102 @@ void ServiceWorkerProviderHost::SetDocumentUrl(const GURL& url) {
   document_url_ = url;
 }
 
-void ServiceWorkerProviderHost::SetControllerVersion(
-    ServiceWorkerVersion* version) {
-  DCHECK(CanAssociateVersion(version));
-  if (version == controlling_version_)
-    return;
-  scoped_refptr<ServiceWorkerVersion> previous_version = controlling_version_;
-  controlling_version_ = version;
-  if (version)
-    version->AddControllee(this);
-  if (previous_version)
-    previous_version->RemoveControllee(this);
-
-  if (!dispatcher_host_)
-    return;  // Could be NULL in some tests.
+void ServiceWorkerProviderHost::SetVersionAttributes(
+    ServiceWorkerVersion* installing_version,
+    ServiceWorkerVersion* waiting_version,
+    ServiceWorkerVersion* active_version) {
+  ChangedVersionAttributesMask mask;
+
+  ChangedVersionAttributesMask::AttributeType type =
+      ChangedVersionAttributesMask::INSTALLING_VERSION;
+  if (CanSetVersionAttribute(type, installing_version)) {

[michaeln, 2014/08/11 22:15:41]

could this be replace with if (installing_version != installing_version_)?

[nhiroki, 2014/08/12 01:45:56]

Done.

+    SetVersionAttributesInternal(type, installing_version);
+    mask.add(type);
+  }
 
-  dispatcher_host_->Send(new ServiceWorkerMsg_SetControllerServiceWorker(
-      kDocumentMainThreadId, provider_id(), CreateHandleAndPass(version)));
-}
+  type = ChangedVersionAttributesMask::WAITING_VERSION;
+  if (CanSetVersionAttribute(type, waiting_version)) {
+    SetVersionAttributesInternal(type, waiting_version);
+    mask.add(type);
+  }
 
-void ServiceWorkerProviderHost::SetActiveVersion(
-    ServiceWorkerVersion* version) {
-  DCHECK(CanAssociateVersion(version));
-  if (version == active_version_)
-    return;
-  scoped_refptr<ServiceWorkerVersion> previous_version = active_version_;
-  active_version_ = version;
-  if (version)
-    version->AddPotentialControllee(this);
-  if (previous_version)
-    previous_version->RemovePotentialControllee(this);
+  type = ChangedVersionAttributesMask::ACTIVE_VERSION;
+  if (CanSetVersionAttribute(type, active_version)) {
+    SetVersionAttributesInternal(type, active_version);
+    mask.add(type);
+  }
 
   if (!dispatcher_host_)
     return;  // Could be NULL in some tests.
+  if (!mask.changed())
+    return;
 
-  dispatcher_host_->Send(new ServiceWorkerMsg_SetActiveServiceWorker(
-      kDocumentMainThreadId, provider_id(), CreateHandleAndPass(version)));
+  ServiceWorkerVersionAttributes attributes;
+  if (mask.installing_changed())
+    attributes.installing = CreateHandleAndPass(installing_version);
+  if (mask.waiting_changed())
+    attributes.waiting = CreateHandleAndPass(waiting_version);
+  if (mask.active_changed())
+    attributes.active = CreateHandleAndPass(active_version);
+
+  dispatcher_host_->Send(new ServiceWorkerMsg_SetVersionAttributes(
+      kDocumentMainThreadId, provider_id(), mask.changed(), attributes));
 }
 
-void ServiceWorkerProviderHost::SetWaitingVersion(
+void ServiceWorkerProviderHost::SetVersionAttributesInternal(
+    ChangedVersionAttributesMask::AttributeType type,
     ServiceWorkerVersion* version) {

[michaeln, 2014/08/11 22:15:41]

if this method took a ptr to the version and a ptr to the scoped_refptr<>
datamember to set, this method might be simplicified some (no switch statement
or type param required).

[nhiroki, 2014/08/12 01:45:56]

Done.

-  DCHECK(CanAssociateVersion(version));
-  if (version == waiting_version_)
-    return;
-  scoped_refptr<ServiceWorkerVersion> previous_version = waiting_version_;
-  waiting_version_ = version;
+  DCHECK(CanSetVersionAttribute(type, version));
+  scoped_refptr<ServiceWorkerVersion> previous_version;
+
+  switch (type) {
+    case ChangedVersionAttributesMask::INSTALLING_VERSION:
+      previous_version = installing_version_;
+      installing_version_ = version;
+      break;
+    case ChangedVersionAttributesMask::WAITING_VERSION:
+      previous_version = waiting_version_;
+      waiting_version_ = version;
+      break;
+    case ChangedVersionAttributesMask::ACTIVE_VERSION:
+      previous_version = active_version_;
+      active_version_ = version;
+      break;
+    default:
+      NOTREACHED() << "Unsupported attribute type: " << type;
+      break;
+  }
+
   if (version)
     version->AddPotentialControllee(this);
   if (previous_version)
     previous_version->RemovePotentialControllee(this);
-
-  if (!dispatcher_host_)
-    return;  // Could be NULL in some tests.
-
-  dispatcher_host_->Send(new ServiceWorkerMsg_SetWaitingServiceWorker(
-      kDocumentMainThreadId, provider_id(), CreateHandleAndPass(version)));
 }
 
-void ServiceWorkerProviderHost::SetInstallingVersion(
+void ServiceWorkerProviderHost::SetControllerVersionAttribute(
     ServiceWorkerVersion* version) {
-  DCHECK(CanAssociateVersion(version));
-  if (version == installing_version_)
+  if (!CanSetVersionAttribute(ChangedVersionAttributesMask::CONTROLLING_VERSION,

[michaeln, 2014/08/11 22:15:41]

can this be simplified to if (version == controlling_version_) return;

[nhiroki, 2014/08/12 01:45:56]

Done.

+                              version)) {
     return;
-  scoped_refptr<ServiceWorkerVersion> previous_version = installing_version_;
-  installing_version_ = version;
+  }
+
+  scoped_refptr<ServiceWorkerVersion> previous_version = controlling_version_;
+  controlling_version_ = version;
   if (version)
-    version->AddPotentialControllee(this);
+    version->AddControllee(this);
   if (previous_version)
-    previous_version->RemovePotentialControllee(this);
+    previous_version->RemoveControllee(this);
 
   if (!dispatcher_host_)
     return;  // Could be NULL in some tests.
 
-  dispatcher_host_->Send(new ServiceWorkerMsg_SetInstallingServiceWorker(
+  dispatcher_host_->Send(new ServiceWorkerMsg_SetControllerServiceWorker(
       kDocumentMainThreadId, provider_id(), CreateHandleAndPass(version)));
 }
 
-void ServiceWorkerProviderHost::UnsetVersion(ServiceWorkerVersion* version) {
-  if (!version)
-    return;
-  if (installing_version_ == version)
-    SetInstallingVersion(NULL);
-  else if (waiting_version_  == version)
-    SetWaitingVersion(NULL);
-  else if (active_version_ == version)
-    SetActiveVersion(NULL);
-  else if (controlling_version_ == version)
-    SetControllerVersion(NULL);
+void ServiceWorkerProviderHost::ClearVersionAttributes() {
+  SetVersionAttributes(NULL, NULL, NULL);
+  SetControllerVersionAttribute(NULL);
 }
 
 bool ServiceWorkerProviderHost::SetHostedVersionId(int64 version_id) {
@@ -185,10 +191,10 @@ void ServiceWorkerProviderHost::AssociateRegistration(
   DCHECK(CanAssociateRegistration(registration));
   associated_registration_ = registration;
   registration->AddListener(this);
-
-  SetActiveVersion(registration->active_version());
-  SetInstallingVersion(registration->installing_version());
-  SetWaitingVersion(registration->waiting_version());
+  SetVersionAttributes(registration->installing_version(),
+                       registration->waiting_version(),
+                       registration->active_version());
+  SetControllerVersionAttribute(registration->active_version());
 }
 
 void ServiceWorkerProviderHost::UnassociateRegistration() {
@@ -196,11 +202,7 @@ void ServiceWorkerProviderHost::UnassociateRegistration() {
     return;
   associated_registration_->RemoveListener(this);
   associated_registration_ = NULL;
-
-  SetActiveVersion(NULL);
-  SetInstallingVersion(NULL);
-  SetWaitingVersion(NULL);
-  SetControllerVersion(NULL);
+  ClearVersionAttributes();
 }
 
 scoped_ptr<ServiceWorkerRequestHandler>
@@ -221,28 +223,30 @@ ServiceWorkerProviderHost::CreateRequestHandler(
   return scoped_ptr<ServiceWorkerRequestHandler>();
 }
 
-bool ServiceWorkerProviderHost::CanAssociateVersion(
+bool ServiceWorkerProviderHost::CanSetVersionAttribute(

[michaeln, 2014/08/11 22:15:41]

I think this method could maybe be simplified or even removed? The version
setters are all private and only called by other methods if this class. Given
the callsites we know the versions are from our associated registrations so i
think we don't need all the runtime checks for that stuff.

[nhiroki, 2014/08/12 01:45:56]

Ah, good point! I think we can completely remove this function. Done.

+    ChangedVersionAttributesMask::AttributeType type,
     ServiceWorkerVersion* version) {
   if (!context_)
     return false;
   if (running_hosted_version_)
     return false;
-  if (!version)
-    return true;
+  if (!associated_registration_)
+    return false;
+  if (version && version->registration_id() != associated_registration_->id())
+    return false;
 
-  ServiceWorkerVersion* already_associated_version = NULL;
-  if (controlling_version_)
-    already_associated_version = controlling_version_;
-  if (active_version_)
-    already_associated_version = active_version_;
-  else if (waiting_version_)
-    already_associated_version = waiting_version_;
-  else if (installing_version_)
-    already_associated_version = installing_version_;
-
-  return !already_associated_version ||
-         already_associated_version->registration_id() ==
-              version->registration_id();
+  switch (type) {
+    case ChangedVersionAttributesMask::INSTALLING_VERSION:
+      return installing_version_ != version;
+    case ChangedVersionAttributesMask::WAITING_VERSION:
+      return waiting_version_ != version;
+    case ChangedVersionAttributesMask::ACTIVE_VERSION:
+      return active_version_ != version;
+    case ChangedVersionAttributesMask::CONTROLLING_VERSION:
+      return controlling_version_ != version;
+  }
+  NOTREACHED();
+  return false;
 }
 
 bool ServiceWorkerProviderHost::CanAssociateRegistration(