[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..d76366864208929de2055ec354498ac92fa0d0d0 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"
+
 namespace gcm {
 
 PushMessagingPermissionContext::PushMessagingPermissionContext(Profile* profile)
-    : PermissionContextBase(profile, CONTENT_SETTINGS_TYPE_PUSH_MESSAGING) {
+    : PermissionContextBase(profile, CONTENT_SETTINGS_TYPE_PUSH_MESSAGING),
+      push_profile_(profile),
+      push_setting_type_(CONTENT_SETTINGS_TYPE_PUSH_MESSAGING) {
 }
 
 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, push_setting_type_,
+          std::string());
+
+  ContentSetting notifications_content_setting =
+      push_profile_->GetHostContentSettingsMap()
+          ->GetContentSettingAndMaybeUpdateLastUsage(

[Michael van Ouwerkerk, 2014/11/13 18:35:52]

I'm not sure I understand this part correctly. Why use
GetContentSettingAndMaybeUpdateLastUsage for Notifications, but
GetContentSetting for Push?

[Miguel Garcia, 2014/11/14 11:34:36]

I made it more clear now, I think we want to keep the same logic as in the base
class so that when you Decide a permission it is marked as used but when you
just get the permission status it is not. I originally considered that trying to
figure out the notification permission would be a good enough reason to consider
it used but this seems more clear.

+              requesting_origin, embedding_origin,
+              CONTENT_SETTINGS_TYPE_NOTIFICATIONS, 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(notifications_content_setting == CONTENT_SETTING_ALLOW);

[Michael van Ouwerkerk, 2014/11/13 18:35:52]

Nit: use DCHECK_EQ, here and below.

[Miguel Garcia, 2014/11/14 11:34:36]

Done.

+  DCHECK(push_content_setting == CONTENT_SETTING_ALLOW);
+  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
+ * - 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& embedder_origin,

[Michael van Ouwerkerk, 2014/11/13 18:35:52]

Nit: let's be consistent and call this embedding_origin throughout the file.

[Miguel Garcia, 2014/11/14 11:34:36]

Done.

+    bool user_gesture,
+    const BrowserPermissionCallback& callback) {
+  ContentSetting notifications_content_setting =
+      push_profile_->GetHostContentSettingsMap()
+          ->GetContentSettingAndMaybeUpdateLastUsage(

[johnme, 2014/11/13 18:50:06]

I'm not too familiar with AndMaybeUpdateLastUsage - is that what we want here?

[Miguel Garcia, 2014/11/14 11:34:36]

Yes, the base class does the same. DecidePermission is called when you are
trying to obtain the permission somehow so this is the right moment to mark it
as last used.

+              requesting_origin, embedder_origin,
+              CONTENT_SETTINGS_TYPE_NOTIFICATIONS, std::string());
+
+  if (notifications_content_setting != CONTENT_SETTING_ALLOW) {
+    DLOG(WARNING) << "Notification permission has not been granted.";

[Michael van Ouwerkerk, 2014/11/13 18:35:52]

Delete this?

[Miguel Garcia, 2014/11/14 11:34:36]

I actually meant to leave this one, it will be helpful if we need to debug
things.

[Bernhard Bauer, 2014/11/14 12:31:33]

Could you at least reduce the severity if it's only for debugging?

[Miguel Garcia, 2014/11/17 11:43:22]

Ok, note that there is a presubmit check to avoid (D)LOG(INFO) but it seems
reasonable to bypass in this case.

On 2014/11/14 12:31:33, Bernhard Bauer wrote:

[Bernhard Bauer, 2014/11/17 11:53:37]

Yeah, I think technically you're supposed to use log level DEBUG for debugging
information. Or you could use (D)VLOG, which has the advantage that the logging
threshold can be controlled more granularily.

+    NotifyPermissionSet(id, requesting_origin, embedder_origin, callback,
+                        false /* persist */, false /* granted */);
+    return;
+  }
+
+  ContentSetting push_content_setting =
+      push_profile_->GetHostContentSettingsMap()
+          ->GetContentSettingAndMaybeUpdateLastUsage(
+              requesting_origin, embedder_origin, push_setting_type_,
+              std::string());
+
+  if (push_content_setting == CONTENT_SETTING_BLOCK) {

[johnme, 2014/11/13 18:50:06]

How would we ever get to this state, if only the notifications permission is
ever surfaced in UI?

[Miguel Garcia, 2014/11/14 11:34:36]

I just want to be a bit defensive about it just in case. Our UX might not let us
do it but Opera or ContentShell or WebView can add that option.

+    DLOG(WARNING) << "Push permission was explicitly blocked.";

[Michael van Ouwerkerk, 2014/11/13 18:35:52]

Delete this?

[Miguel Garcia, 2014/11/14 11:34:36]

Same as above.

+    NotifyPermissionSet(id, requesting_origin, embedder_origin, callback,
+                        false /* persist */, false /* granted */);
+    return;
+  }
+
+  NotifyPermissionSet(id, requesting_origin, embedder_origin, callback,
+                      true /* persist */, true /* granted */);
+}
+
 }  // namespace gcm