Unregister with push service and SW database when permission is lost.

chrome/browser/services/gcm/push_messaging_service_impl.cc

Index: chrome/browser/services/gcm/push_messaging_service_impl.cc
diff --git a/chrome/browser/services/gcm/push_messaging_service_impl.cc b/chrome/browser/services/gcm/push_messaging_service_impl.cc
index a3ad6a2e54c9539da348522329e47ba4776519b0..6ae7ff43f2c72c060fc152c498104f6fb4d0899a 100644
--- a/chrome/browser/services/gcm/push_messaging_service_impl.cc
+++ b/chrome/browser/services/gcm/push_messaging_service_impl.cc
@@ -27,10 +27,12 @@
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/grit/generated_resources.h"
+#include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/content_settings/core/common/permission_request_id.h"
 #include "components/gcm_driver/gcm_driver.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "content/public/browser/browser_context.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/service_worker_context.h"
 #include "content/public/browser/storage_partition.h"
@@ -130,11 +132,17 @@ PushMessagingServiceImpl::PushMessagingServiceImpl(
       push_registration_count_(0),
       pending_push_registration_count_(0),
       weak_factory_(this) {
+  // In some tests, we might end up with |profile_| being null at that point.

[johnme, 2015/02/16 20:27:44]

that -> this

[mlamouri (slow - plz ping), 2015/02/16 20:46:03]

Done.

+  // When that is the case |profile_| will be set in SetProfileForTesting(), at
+  // which point the service will start to observe HostContentSettingsMap.
+  if (profile_)
+    profile_->GetHostContentSettingsMap()->AddObserver(this);
 }
 
 PushMessagingServiceImpl::~PushMessagingServiceImpl() {
   // TODO(johnme): If it's possible for this to be destroyed before GCMDriver,
   // then we should call RemoveAppHandler.
+  profile_->GetHostContentSettingsMap()->RemoveObserver(this);
 }
 
 void PushMessagingServiceImpl::IncreasePushRegistrationCount(int add,
@@ -234,8 +242,37 @@ void PushMessagingServiceImpl::OnMessage(
                  application_id.service_worker_registration_id, message));
 }
 
+void PushMessagingServiceImpl::OnContentSettingChanged(
+    const ContentSettingsPattern& primary_pattern,
+    const ContentSettingsPattern& secondary_pattern,
+    ContentSettingsType content_type,
+    std::string resource_identifier) {
+  if (content_type != CONTENT_SETTINGS_TYPE_PUSH_MESSAGING &&
+      content_type != CONTENT_SETTINGS_TYPE_NOTIFICATIONS) {
+    return;
+  }
+
+  for (const auto& id : PushMessagingApplicationId::GetAll(profile_)) {
+    if (primary_pattern.IsValid() && !primary_pattern.Matches(id.origin))
+      continue;
+
+    // |primary_pattern| might be invalid or it does matches the origin, thus
+    // the origin permission status might have changed.
+    if (HasPermission(id.origin))
+      continue;
+
+    // Unregister the PushMessagingApplicationId with the push service.
+    Unregister(id.app_id_guid, true /* retry */, UnregisterCallback());;

[johnme, 2015/02/16 20:27:45]

;; -> ;

[mlamouri (slow - plz ping), 2015/02/16 20:46:03]

Surprised this didn't trigger a warning.

+
+    // Clear the associated service worker push registration id.
+    PushMessagingService::ClearPushRegistrationID(

[johnme, 2015/02/16 20:27:45]

We should probably also call this from the
PUSH_DELIVERY_STATUS_PERMISSION_DENIED case of DeliverMessageCallback (even
though hopefully that case will be pretty rare after this patch).

[mlamouri (slow - plz ping), 2015/02/16 20:46:03]

I think we should consider having the service doing the PushRegistrationID
clearance. I do not think it's part of that patch though.

+        profile_, id.origin, id.service_worker_registration_id);
+  }
+}
+
 void PushMessagingServiceImpl::SetProfileForTesting(Profile* profile) {
   profile_ = profile;
+  profile_->GetHostContentSettingsMap()->AddObserver(this);

[johnme, 2015/02/16 20:27:45]

Maybe add a DCHECK(profile) to emphasize that only valid profiles should be
passed to SetProfileForTesting?

[mlamouri (slow - plz ping), 2015/02/16 20:46:03]

Not very useful given that it will crash anyway and this is testing code.

 }
 
 void PushMessagingServiceImpl::DeliverMessageCallback(
@@ -624,8 +661,10 @@ void PushMessagingServiceImpl::Unregister(
   PushMessagingApplicationId application_id = PushMessagingApplicationId::Get(
       profile_, requesting_origin, service_worker_registration_id);
   if (!application_id.IsValid()) {
-    callback.Run(
-        content::PUSH_UNREGISTRATION_STATUS_SUCCESS_WAS_NOT_REGISTERED);
+    if (!callback.is_null()) {
+        callback.Run(

[johnme, 2015/02/16 20:27:45]

2-space indent within if please

[mlamouri (slow - plz ping), 2015/02/16 20:46:03]

Done.

+            content::PUSH_UNREGISTRATION_STATUS_SUCCESS_WAS_NOT_REGISTERED);
+    }
     return;
   }
 
@@ -664,8 +703,15 @@ void PushMessagingServiceImpl::DidUnregister(
   if (result == GCMClient::SUCCESS) {
     PushMessagingApplicationId application_id =
         PushMessagingApplicationId::Get(profile_, app_id_guid);
-    if (application_id.IsValid())
-      application_id.DeleteFromDisk(profile_);
+    if (!application_id.IsValid()) {
+      if (!callback.is_null()) {
+        callback.Run(
+          content::PUSH_UNREGISTRATION_STATUS_SUCCESS_WAS_NOT_REGISTERED);

[johnme, 2015/02/16 20:27:45]

4-space indent for wrap please

[mlamouri (slow - plz ping), 2015/02/16 20:46:03]

Done.

+      }
+      return;
+    }
+
+    application_id.DeleteFromDisk(profile_);
     DecreasePushRegistrationCount(1, false /* was_pending */);
   }