[PUSH] Merge notifications and push messaging prompts

chrome/browser/services/gcm/push_messaging_permission_context.cc

Index: chrome/browser/services/gcm/push_messaging_permission_context.cc
diff --git a/chrome/browser/services/gcm/push_messaging_permission_context.cc b/chrome/browser/services/gcm/push_messaging_permission_context.cc
index ad40ff32319f036bc54864967345cab64832a103..fb65f101faf254ed5989f9e53318e866549c8e48 100644
--- a/chrome/browser/services/gcm/push_messaging_permission_context.cc
+++ b/chrome/browser/services/gcm/push_messaging_permission_context.cc
@@ -4,13 +4,91 @@
 
 #include "chrome/browser/services/gcm/push_messaging_permission_context.h"
 
+#include "chrome/browser/profiles/profile.h"
+#include "components/content_settings/core/browser/host_content_settings_map.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_delegate.h"
+
+const ContentSettingsType kPushSettingType =

[fgorski, 2014/11/17 18:24:06]

Can you put these into an anonymous namespace?

[Miguel Garcia, 2014/11/18 10:47:30]

Agreed online that it is not required (and I got the opposite comment from
another reviewer).

+    CONTENT_SETTINGS_TYPE_PUSH_MESSAGING;
+const ContentSettingsType kNotificationSettingType =
+    CONTENT_SETTINGS_TYPE_NOTIFICATIONS;
+
 namespace gcm {
 
 PushMessagingPermissionContext::PushMessagingPermissionContext(Profile* profile)
-    : PermissionContextBase(profile, CONTENT_SETTINGS_TYPE_PUSH_MESSAGING) {
+    : PermissionContextBase(profile, CONTENT_SETTINGS_TYPE_PUSH_MESSAGING),
+      push_profile_(profile) {
 }
 
 PushMessagingPermissionContext::~PushMessagingPermissionContext() {
 }
 
+ContentSetting PushMessagingPermissionContext::GetPermissionStatus(
+    const GURL& requesting_origin,
+    const GURL& embedding_origin) const {
+  ContentSetting push_content_setting =
+      push_profile_->GetHostContentSettingsMap()->GetContentSetting(
+          requesting_origin, embedding_origin, kPushSettingType, std::string());
+
+  ContentSetting notifications_content_setting =
+      push_profile_->GetHostContentSettingsMap()->GetContentSetting(
+          requesting_origin, embedding_origin, kNotificationSettingType,
+          std::string());
+
+  if (notifications_content_setting == CONTENT_SETTING_BLOCK ||
+      push_content_setting == CONTENT_SETTING_BLOCK) {
+    return CONTENT_SETTING_BLOCK;
+  }
+  if (notifications_content_setting == CONTENT_SETTING_ASK ||
+      push_content_setting == CONTENT_SETTING_ASK) {
+    return CONTENT_SETTING_ASK;
+  }
+  DCHECK_EQ(CONTENT_SETTING_ALLOW, notifications_content_setting);
+  DCHECK_EQ(CONTENT_SETTING_ALLOW, push_content_setting);
+  return CONTENT_SETTING_ALLOW;
+}
+
+// Unlike other permissions, push is decided by the following algorithm
+//  - You need to have notification permission granted.
+//  - You need to not have push permission explicitly blocked

[fgorski, 2014/11/17 18:24:06]

nit: dot at the end.

[Miguel Garcia, 2014/11/18 10:47:30]

Done.

+//  - If those two things are true it is granted without prompting.
+// This is done to avoid double prompting for notifications and push.
+void PushMessagingPermissionContext::DecidePermission(
+    content::WebContents* web_contents,
+    const PermissionRequestID& id,
+    const GURL& requesting_origin,
+    const GURL& embedding_origin,
+    bool user_gesture,
+    const BrowserPermissionCallback& callback) {
+  ContentSetting notifications_content_setting =
+      push_profile_->GetHostContentSettingsMap()
+          ->GetContentSettingAndMaybeUpdateLastUsage(
+              requesting_origin, embedding_origin, kNotificationSettingType,
+              std::string());
+
+  if (notifications_content_setting != CONTENT_SETTING_ALLOW) {
+    DVLOG(1) << "Notification permission has not been granted.";
+    NotifyPermissionSet(id, requesting_origin, embedding_origin, callback,
+                        false /* persist */, false /* granted */);

[fgorski, 2014/11/17 18:24:06]

Could you please remind me what the semantics of persist is here?

If it does not lead to behavior of "ask me again later" I am ok with the code,
but I am curious what this thing means, and why you are setting it to true only
when successful.

[Miguel Garcia, 2014/11/18 10:47:30]

We are denying the permission here just because the permission for notifications
has not been granted, so persisting that we don't want to grant push would be
bad, if the user eventually grants notifications we want to make sure they can
request push automatically.

Note that in general there will no prompts for push after this change, we will
only prompt for notifications which will follow the usual pattern.

+    return;
+  }
+
+  ContentSetting push_content_setting =
+      push_profile_->GetHostContentSettingsMap()
+          ->GetContentSettingAndMaybeUpdateLastUsage(
+              requesting_origin, embedding_origin, kPushSettingType,
+              std::string());
+
+  if (push_content_setting == CONTENT_SETTING_BLOCK) {
+    DVLOG(1) << "Push permission was explicitly blocked.";
+    NotifyPermissionSet(id, requesting_origin, embedding_origin, callback,
+                        false /* persist */, false /* granted */);
+    return;
+  }
+
+  NotifyPermissionSet(id, requesting_origin, embedding_origin, callback,
+                      true /* persist */, true /* granted */);
+}
+
 }  // namespace gcm