Cleanup geofence registrations when a service worker is unregistered.

content/browser/geofencing/geofencing_manager.cc

Index: content/browser/geofencing/geofencing_manager.cc
diff --git a/content/browser/geofencing/geofencing_manager.cc b/content/browser/geofencing/geofencing_manager.cc
index e13095444208c55eb6ac973c16c9d916ecf97466..96f55ecd3d0ae70fa75bdc430b3df8b9bccbf005 100644
--- a/content/browser/geofencing/geofencing_manager.cc
+++ b/content/browser/geofencing/geofencing_manager.cc
@@ -83,11 +83,14 @@ void GeofencingManager::Shutdown() {
 
 void GeofencingManager::InitOnIO() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  service_ = GeofencingServiceImpl::GetInstance();
+  service_worker_context_->AddObserver(this);
+  if (!service_)
+    service_ = GeofencingServiceImpl::GetInstance();
 }
 
 void GeofencingManager::ShutdownOnIO() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  service_worker_context_->RemoveObserver(this);
   // Clean up all registrations with the |GeofencingService|.
   // TODO(mek): This will need to change to support geofence registrations that
   //     outlive the browser, although removing the references to this
@@ -105,21 +108,18 @@ void GeofencingManager::RegisterRegion(
 
   // TODO(mek): Validate region_id and region.
 
-  // Look up service worker. In unit tests |service_worker_context_| might not
-  // be set.
-  GURL service_worker_origin;
-  if (service_worker_context_.get()) {
-    ServiceWorkerRegistration* service_worker_registration =
-        service_worker_context_->context()->GetLiveRegistration(
-            service_worker_registration_id);
-    if (!service_worker_registration) {
-      callback.Run(GEOFENCING_STATUS_OPERATION_FAILED_NO_SERVICE_WORKER);
-      return;
-    }
-
-    service_worker_origin = service_worker_registration->pattern().GetOrigin();
+  // Look up service worker.
+  ServiceWorkerRegistration* service_worker_registration =
+      service_worker_context_->context()->GetLiveRegistration(
+          service_worker_registration_id);
+  if (!service_worker_registration) {
+    callback.Run(GEOFENCING_STATUS_OPERATION_FAILED_NO_SERVICE_WORKER);
+    return;
   }
 
+  GURL service_worker_origin =
+      service_worker_registration->pattern().GetOrigin();
+
   if (!service_->IsServiceAvailable()) {
     callback.Run(GEOFENCING_STATUS_OPERATION_FAILED_SERVICE_NOT_AVAILABLE);
     return;
@@ -144,16 +144,13 @@ void GeofencingManager::UnregisterRegion(int64 service_worker_registration_id,
 
   // TODO(mek): Validate region_id.
 
-  // Look up service worker. In unit tests |service_worker_context_| might not
-  // be set.
-  if (service_worker_context_.get()) {
-    ServiceWorkerRegistration* service_worker_registration =
-        service_worker_context_->context()->GetLiveRegistration(
-            service_worker_registration_id);
-    if (!service_worker_registration) {
-      callback.Run(GEOFENCING_STATUS_OPERATION_FAILED_NO_SERVICE_WORKER);
-      return;
-    }
+  // Look up service worker.
+  ServiceWorkerRegistration* service_worker_registration =
+      service_worker_context_->context()->GetLiveRegistration(
+          service_worker_registration_id);
+  if (!service_worker_registration) {
+    callback.Run(GEOFENCING_STATUS_OPERATION_FAILED_NO_SERVICE_WORKER);
+    return;
   }
 
   if (!service_->IsServiceAvailable()) {
@@ -186,15 +183,12 @@ GeofencingStatus GeofencingManager::GetRegisteredRegions(
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   CHECK(result);
 
-  // Look up service worker. In unit tests |service_worker_context_| might not
-  // be set.
-  if (service_worker_context_.get()) {
-    ServiceWorkerRegistration* service_worker_registration =
-        service_worker_context_->context()->GetLiveRegistration(
-            service_worker_registration_id);
-    if (!service_worker_registration) {
-      return GEOFENCING_STATUS_OPERATION_FAILED_NO_SERVICE_WORKER;
-    }
+  // Look up service worker.
+  ServiceWorkerRegistration* service_worker_registration =
+      service_worker_context_->context()->GetLiveRegistration(
+          service_worker_registration_id);
+  if (!service_worker_registration) {
+    return GEOFENCING_STATUS_OPERATION_FAILED_NO_SERVICE_WORKER;
   }
 
   if (!service_->IsServiceAvailable()) {
@@ -245,6 +239,13 @@ void GeofencingManager::SetMockPosition(double latitude, double longitude) {
   mock_service_->SetMockPosition(latitude, longitude);
 }
 
+void GeofencingManager::OnRegistrationDeleted(
+    int64 service_worker_registration_id,
+    const GURL& pattern) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  CleanUpForServiceWorker(service_worker_registration_id);
+}
+
 void GeofencingManager::RegistrationFinished(int64 geofencing_registration_id,
                                              GeofencingStatus status) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
@@ -328,6 +329,23 @@ void GeofencingManager::ClearRegistration(Registration* registration) {
     registrations_.erase(registrations_iterator);
 }
 
+void GeofencingManager::CleanUpForServiceWorker(
+    int64 service_worker_registration_id) {

[michaeln, 2015/01/09 22:58:41]

id here can be 'invalid', not sure if you want to test for that specifically and
return early?

[Marijn Kruisselbrink, 2015/01/09 23:13:15]

I'm not sure if that's worth the extra code. It would save one map lookup, but
nothing bad happens when it is invalid anyway.

+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  ServiceWorkerRegistrationsMap::iterator registrations_iterator =
+      registrations_.find(service_worker_registration_id);
+  if (registrations_iterator == registrations_.end())
+    return;
+
+  for (const auto& registration : registrations_iterator->second) {
+    int geofencing_registration_id =
+        registration.second.geofencing_registration_id;
+    service_->UnregisterRegion(geofencing_registration_id);
+    registrations_by_id_.erase(geofencing_registration_id);
+  }
+  registrations_.erase(service_worker_registration_id);
+}
+
 void GeofencingManager::DispatchGeofencingEvent(
     blink::WebGeofencingEventType event_type,
     int64 geofencing_registration_id) {