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

chrome/browser/permissions/permission_queue_controller.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/permissions/permission_queue_controller.h"
 
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/geolocation/geolocation_infobar_delegate_android.h"
 #include "chrome/browser/infobars/infobar_service.h"
@@ skipping 191 matching lines @@
       pending_infobar_requests_.erase(i);
     return;
   }
 }
 
 void PermissionQueueController::OnPermissionSet(const PermissionRequestID& id,
                                                 const GURL& requesting_frame,
                                                 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
   // PermissionContextBase once all the types are moved there.
   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;
   PendingInfobarRequests infobars_to_remove;
   std::vector<PendingInfobarRequests::iterator> pending_requests_to_remove;
   for (PendingInfobarRequests::iterator i = pending_infobar_requests_.begin();
        i != pending_infobar_requests_.end(); ++i) {
     if (!i->IsForPair(requesting_frame, embedder))
       continue;
     requests_to_notify.push_back(*i);
@@ skipping 16 matching lines @@
     infobars_to_remove.push_back(*i);
   }
 
   // Remove all infobars for the same |requesting_frame| and |embedder|.
   for (PendingInfobarRequests::iterator i = infobars_to_remove.begin();
        i != infobars_to_remove.end(); ++i)
     GetInfoBarService(i->id())->RemoveInfoBar(i->infobar());
 
   // 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();
        i != requests_to_notify.end(); ++i)
     i->RunCallback(content_setting);
 
   // Remove the pending requests in reverse order.
   for (int i = pending_requests_to_remove.size() - 1; i >= 0; --i)
     pending_infobar_requests_.erase(pending_requests_to_remove[i]);
 }
@@ skipping 98 matching lines @@
       content::Source<InfoBarService>(infobar_service))) {
     registrar_.Remove(this,
                       chrome::NOTIFICATION_TAB_CONTENTS_INFOBAR_REMOVED,
                       content::Source<InfoBarService>(infobar_service));
   }
 }
 
 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
     // in the host content settings map.
     return;
   }
 
   ContentSetting content_setting =
-      allowed ? CONTENT_SETTING_ALLOW : CONTENT_SETTING_BLOCK;
+      (decision == GRANTED) ? CONTENT_SETTING_ALLOW : CONTENT_SETTING_BLOCK;
 
   HostContentSettingsMapFactory::GetForProfile(profile_)
       ->SetContentSettingDefaultScope(
           requesting_frame.GetOrigin(), embedder.GetOrigin(),
           content_settings_type_, std::string(), content_setting);
 }