Add a feature to display a persistence toggle for permission prompts on Android.

chrome/browser/permissions/permission_queue_controller.cc

Index: chrome/browser/permissions/permission_queue_controller.cc
diff --git a/chrome/browser/permissions/permission_queue_controller.cc b/chrome/browser/permissions/permission_queue_controller.cc
index 3382aaadc0e6deedeec7514712f664eb18699061..4821a5235be1693b3b30a8eb3907f14ee445211e 100644
--- a/chrome/browser/permissions/permission_queue_controller.cc
+++ b/chrome/browser/permissions/permission_queue_controller.cc
@@ -209,7 +209,7 @@ void PermissionQueueController::OnPermissionSet(const PermissionRequestID& id,
                                                 const GURL& embedder,
                                                 bool user_gesture,
                                                 bool update_content_setting,
-                                                bool allowed) {
+                                                PermissionAction decision) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   // TODO(miguelg): move the permission persistence to
@@ -217,20 +217,26 @@ void PermissionQueueController::OnPermissionSet(const PermissionRequestID& id,
   PermissionRequestGestureType gesture_type =
       user_gesture ? PermissionRequestGestureType::GESTURE
                    : PermissionRequestGestureType::NO_GESTURE;
-  if (update_content_setting) {
-    UpdateContentSetting(requesting_frame, embedder, allowed);
-    if (allowed) {
+  switch (decision) {
+    case GRANTED:
       PermissionUmaUtil::PermissionGranted(permission_type_, gesture_type,
                                            requesting_frame, profile_);
-    } else {
+      break;
+    case DENIED:
       PermissionUmaUtil::PermissionDenied(permission_type_, gesture_type,
                                           requesting_frame, profile_);
-    }
-  } else {
-    PermissionUmaUtil::PermissionDismissed(permission_type_, gesture_type,
-                                           requesting_frame, profile_);
+      break;
+    case DISMISSED:
+      PermissionUmaUtil::PermissionDismissed(permission_type_, gesture_type,
+                                             requesting_frame, profile_);
+      break;
+    default:
+      NOTREACHED();
   }

[raymes, 2016/08/16 01:44:53]

The switch statement here seems kind of redundant. I wonder if we should just
expose RecordPermissionAction in permission_uma_util and call it directly?

[dominickn, 2016/08/16 03:06:50]

I'd like to deal with this in a follow up - there's a lot of metrics related
code which could be moved around like this. Filed crbug.com/638076. :)

[raymes, 2016/08/16 03:39:29]

sgtm, thanks!

 
+  if (decision != DISMISSED && update_content_setting)

[raymes, 2016/08/16 01:44:53]

Should this be a DCHECK instead? Actually I think we can move the DCHECK into
UpdateContentSetting and just have this here:

if (update_content_setting)
  UpdateContentSetting(...);


In UpdateContentSetting we can have:
DCHECK(decision == GRANTED || decision == DENIED);

[dominickn, 2016/08/16 03:06:50]

Done.

+    UpdateContentSetting(requesting_frame, embedder, decision);
+
   // Cancel this request first, then notify listeners.  TODO(pkasting): Why
   // is this order important?
   PendingInfobarRequests requests_to_notify;
@@ -267,13 +273,14 @@ void PermissionQueueController::OnPermissionSet(const PermissionRequestID& id,
 
   // PermissionContextBase needs to know about the new ContentSetting value,
   // CONTENT_SETTING_DEFAULT being the value for nothing happened. The callers
-  // of ::OnPermissionSet passes { true, true } for allow, { true, false } for
-  // block and { false, * } for dismissed. The tuple being
-  // { update_content_setting, allowed }.
+  // of ::OnPermissionSet passes { bool, GRANTED } for allow, { bool, DENIED }
+  // for block and { false, DISMISSED } for dismissed. The tuple being
+  // { update_content_setting, decision }.
   ContentSetting content_setting = CONTENT_SETTING_DEFAULT;
-  if (update_content_setting) {
-    content_setting = allowed ? CONTENT_SETTING_ALLOW : CONTENT_SETTING_BLOCK;
-  }
+  if (decision == GRANTED)
+    content_setting = CONTENT_SETTING_ALLOW;
+  else if (decision == DENIED)
+    content_setting = CONTENT_SETTING_BLOCK;

[raymes, 2016/08/16 01:44:53]

optionally:
else
  DCHECK_EQ(DISMISSED, decision);

[dominickn, 2016/08/16 03:06:50]

Done.

 
   // Send out the permission notifications.
   for (PendingInfobarRequests::iterator i = requests_to_notify.begin();
@@ -392,7 +399,7 @@ void PermissionQueueController::UnregisterForInfoBarNotifications(
 void PermissionQueueController::UpdateContentSetting(
     const GURL& requesting_frame,
     const GURL& embedder,
-    bool allowed) {
+    PermissionAction decision) {
   if (requesting_frame.GetOrigin().SchemeIsFile()) {
     // Chrome can be launched with --disable-web-security which allows
     // geolocation requests from file:// URLs. We don't want to store these
@@ -401,7 +408,7 @@ void PermissionQueueController::UpdateContentSetting(
   }
 
   ContentSetting content_setting =
-      allowed ? CONTENT_SETTING_ALLOW : CONTENT_SETTING_BLOCK;
+      (decision == GRANTED) ? CONTENT_SETTING_ALLOW : CONTENT_SETTING_BLOCK;
 
   HostContentSettingsMapFactory::GetForProfile(profile_)
       ->SetContentSettingDefaultScope(