permissions: remove PermissionQueueController and introduce PermissionInfoBarManager

chrome/browser/permissions/permission_context_base.cc

Index: chrome/browser/permissions/permission_context_base.cc
diff --git a/chrome/browser/permissions/permission_context_base.cc b/chrome/browser/permissions/permission_context_base.cc
index f329b2bbb640a9b6826ee85bac39ff266dbc7528..2f0bfb9799e8af8c6a665d45505aa7bf6fbd45a7 100644
--- a/chrome/browser/permissions/permission_context_base.cc
+++ b/chrome/browser/permissions/permission_context_base.cc
@@ -9,7 +9,7 @@
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/permissions/permission_bubble_request_impl.h"
 #include "chrome/browser/permissions/permission_context_uma_util.h"
-#include "chrome/browser/permissions/permission_queue_controller.h"
+#include "chrome/browser/permissions/permission_infobar_manager.h"
 #include "chrome/browser/permissions/permission_request_id.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/website_settings/permission_bubble_manager.h"
@@ -26,8 +26,6 @@ PermissionContextBase::PermissionContextBase(
     : profile_(profile),
       permission_type_(permission_type),
       weak_factory_(this) {
-  permission_queue_controller_.reset(
-      new PermissionQueueController(profile_, permission_type_));
 }
 
 PermissionContextBase::~PermissionContextBase() {
@@ -79,10 +77,13 @@ void PermissionContextBase::CancelPermissionRequest(
     const PermissionRequestID& id) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
+  if (!web_contents)
+    return;
+
   if (PermissionBubbleManager::Enabled()) {
     PermissionBubbleRequest* cancelling =
         pending_bubbles_.get(id.ToString());
-    if (cancelling != NULL && web_contents != NULL &&
+    if (cancelling != NULL &&
         PermissionBubbleManager::FromWebContents(web_contents) != NULL) {
       PermissionBubbleManager::FromWebContents(web_contents)->
           CancelRequest(cancelling);
@@ -90,7 +91,10 @@ void PermissionContextBase::CancelPermissionRequest(
     return;
   }
 
-  GetQueueController()->CancelInfoBarRequest(id);
+  if (PermissionInfoBarManager::FromWebContents(web_contents) != NULL) {

[mlamouri (slow - plz ping), 2015/09/28 13:30:14]

Remove |!= NULL| and do !PermissionInfoBarManager::...

[Lalit Maganti, 2015/09/28 15:32:38]

Done.

+    PermissionInfoBarManager::FromWebContents(web_contents)->
+        CancelInfoBarRequest(id);
+  }

[mlamouri (slow - plz ping), 2015/09/28 13:30:14]

What about removing the |return;| from the |PermissionBubbleManager::Enabled|
block and have a |else if| instead? The |return;| in the first block but not the
second sounds slightly error prone.

[Lalit Maganti, 2015/09/28 15:32:38]

Done.

 }
 
 void PermissionContextBase::DecidePermission(
@@ -162,16 +166,18 @@ void PermissionContextBase::DecidePermission(
     return;
   }
 
-  // TODO(gbillock): Delete this and the infobar delegate when
-  // we're using only bubbles. crbug.com/337458
-  GetQueueController()->CreateInfoBarRequest(
-      id, requesting_origin, embedding_origin,
+  PermissionInfoBarManager* infobar_manager =
+      PermissionInfoBarManager::FromWebContents(web_contents);
+  DCHECK(infobar_manager);

[mlamouri (slow - plz ping), 2015/09/28 13:30:14]

What if we are not in a Tab context? Couldn't the WebContents have no
PermissionInfoBarManager?

[Lalit Maganti, 2015/09/28 15:32:38]

You're right this is an issue. Best course of action is to handle this in
permission manager and change IsPermissionBubbleManagerMissing ->
IsPermissionUIManagerMissing.

+  infobar_manager->CreateInfoBarRequest(
+      permission_type_, id,
+      requesting_origin, embedding_origin,
       base::Bind(&PermissionContextBase::PermissionDecided,
                  weak_factory_.GetWeakPtr(), id, requesting_origin,
-                 embedding_origin, callback,
-                 // the queue controller takes care of persisting the
-                 // permission
-                 false));
+                 embedding_origin, callback),
+      base::Bind(&PermissionContextBase::NotifyPermissionSet,

[mlamouri (slow - plz ping), 2015/09/28 13:30:14]

It would be great to unify that with what Bubble does. Also,
::NotifyPermissionSet is a hack given what you are trying to do. We should have
a callback that simply run the callback with the current permission status.

[Lalit Maganti, 2015/09/28 15:32:38]

Changed as discussed.

+                 weak_factory_.GetWeakPtr(), id, requesting_origin,
+                 embedding_origin, callback));
 }
 
 void PermissionContextBase::PermissionDecided(
@@ -181,33 +187,25 @@ void PermissionContextBase::PermissionDecided(
     const BrowserPermissionCallback& callback,
     bool persist,
     ContentSetting content_setting) {
-  // Infobar persistance and its related UMA is tracked on the infobar
-  // controller directly.
-  if (PermissionBubbleManager::Enabled()) {
-    if (persist) {
-      DCHECK(content_setting == CONTENT_SETTING_ALLOW ||
-             content_setting == CONTENT_SETTING_BLOCK);
-      if (content_setting == CONTENT_SETTING_ALLOW)
-        PermissionContextUmaUtil::PermissionGranted(permission_type_,
-                                                    requesting_origin);
-      else
-        PermissionContextUmaUtil::PermissionDenied(permission_type_,
-                                                   requesting_origin);
-    } else {
-      DCHECK_EQ(content_setting, CONTENT_SETTING_DEFAULT);
-      PermissionContextUmaUtil::PermissionDismissed(permission_type_,
-                                                    requesting_origin);
-    }
+  if (persist) {
+    DCHECK(content_setting == CONTENT_SETTING_ALLOW ||
+           content_setting == CONTENT_SETTING_BLOCK);
+    if (content_setting == CONTENT_SETTING_ALLOW)
+      PermissionContextUmaUtil::PermissionGranted(permission_type_,
+                                                  requesting_origin);
+    else
+      PermissionContextUmaUtil::PermissionDenied(permission_type_,
+                                                 requesting_origin);
+  } else {
+    DCHECK_EQ(content_setting, CONTENT_SETTING_DEFAULT);
+    PermissionContextUmaUtil::PermissionDismissed(permission_type_,
+                                                  requesting_origin);
   }
 
   NotifyPermissionSet(id, requesting_origin, embedding_origin, callback,
                       persist, content_setting);
 }
 
-PermissionQueueController* PermissionContextBase::GetQueueController() {
-  return permission_queue_controller_.get();
-}
-
 Profile* PermissionContextBase::profile() const {
   return profile_;
 }