ServiceWorker: Consolidate version change messages

content/browser/service_worker/service_worker_provider_host.cc

 // Copyright 2013 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/service_worker/service_worker_provider_host.h"
 
 #include "base/stl_util.h"
 #include "content/browser/message_port_message_filter.h"
 #include "content/browser/service_worker/service_worker_context_core.h"
 #include "content/browser/service_worker/service_worker_context_request_handler.h"
@@ skipping 30 matching lines @@
     waiting_version_->RemovePotentialControllee(this);
   if (installing_version_)
     installing_version_->RemovePotentialControllee(this);
   if (associated_registration_)
     associated_registration_->RemoveListener(this);
 }
 
 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(
     ServiceWorkerRegistration* registration) {
   DCHECK(associated_registration_);
   DCHECK_EQ(registration->id(), associated_registration_->id());
   UnassociateRegistration();
 }
 
 void ServiceWorkerProviderHost::SetDocumentUrl(const GURL& url) {
   DCHECK(!url.has_ref());
   document_url_ = url;
 }
 
-void ServiceWorkerProviderHost::SetControllerVersion(
+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);
+  }
+
+  type = ChangedVersionAttributesMask::WAITING_VERSION;
+  if (CanSetVersionAttribute(type, waiting_version)) {
+    SetVersionAttributesInternal(type, waiting_version);
+    mask.add(type);
+  }
+
+  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;
+
+  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::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 == controlling_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);
+}
+
+void ServiceWorkerProviderHost::SetControllerVersionAttribute(
+    ServiceWorkerVersion* 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 = 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.
 
   dispatcher_host_->Send(new ServiceWorkerMsg_SetControllerServiceWorker(
       kDocumentMainThreadId, provider_id(), CreateHandleAndPass(version)));
 }
 
-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);
-
-  if (!dispatcher_host_)
-    return;  // Could be NULL in some tests.
-
-  dispatcher_host_->Send(new ServiceWorkerMsg_SetActiveServiceWorker(
-      kDocumentMainThreadId, provider_id(), CreateHandleAndPass(version)));
-}
-
-void ServiceWorkerProviderHost::SetWaitingVersion(
-    ServiceWorkerVersion* version) {
-  DCHECK(CanAssociateVersion(version));
-  if (version == waiting_version_)
-    return;
-  scoped_refptr<ServiceWorkerVersion> previous_version = waiting_version_;
-  waiting_version_ = version;
-  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(
-    ServiceWorkerVersion* version) {
-  DCHECK(CanAssociateVersion(version));
-  if (version == installing_version_)
-    return;
-  scoped_refptr<ServiceWorkerVersion> previous_version = installing_version_;
-  installing_version_ = version;
-  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_SetInstallingServiceWorker(
-      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) {
   if (!context_)
     return true;  // System is shutting down.
   if (active_version_)
     return false;  // Unexpected bad message.
 
   ServiceWorkerVersion* live_version = context_->GetLiveVersion(version_id);
   if (!live_version)
     return true;  // Was deleted before it got started.
 
   ServiceWorkerVersionInfo info = live_version->GetInfo();
   if (info.running_status != ServiceWorkerVersion::STARTING ||
       info.process_id != process_id_) {
     // If we aren't trying to start this version in our process
     // something is amiss.
     return false;
   }
 
   running_hosted_version_ = live_version;
   return true;
 }
 
 void ServiceWorkerProviderHost::AssociateRegistration(
     ServiceWorkerRegistration* registration) {
   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() {
   if (!associated_registration_)
     return;
   associated_registration_->RemoveListener(this);
   associated_registration_ = NULL;
-
-  SetActiveVersion(NULL);
-  SetInstallingVersion(NULL);
-  SetWaitingVersion(NULL);
-  SetControllerVersion(NULL);
+  ClearVersionAttributes();
 }
 
 scoped_ptr<ServiceWorkerRequestHandler>
 ServiceWorkerProviderHost::CreateRequestHandler(
     ResourceType resource_type,
     base::WeakPtr<webkit_blob::BlobStorageContext> blob_storage_context) {
   if (IsHostToRunningServiceWorker()) {
     return scoped_ptr<ServiceWorkerRequestHandler>(
         new ServiceWorkerContextRequestHandler(
             context_, AsWeakPtr(), blob_storage_context, resource_type));
   }
   if (ServiceWorkerUtils::IsMainResourceType(resource_type) ||
       active_version()) {
     return scoped_ptr<ServiceWorkerRequestHandler>(
         new ServiceWorkerControlleeRequestHandler(
             context_, AsWeakPtr(), blob_storage_context, resource_type));
   }
   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(
     ServiceWorkerRegistration* registration) {
   if (!context_)
     return false;
   if (running_hosted_version_)
     return false;
   if (!registration || associated_registration_)
     return false;
@@ skipping 33 matching lines @@
     dispatcher_host_->RegisterServiceWorkerHandle(handle.Pass());
   }
   return info;
 }
 
 bool ServiceWorkerProviderHost::IsContextAlive() {
   return context_ != NULL;
 }
 
 }  // namespace content