Add BubbleManager to manage bubbles and ChromeBubbleManager for events.

chrome/browser/ui/website_settings/permission_bubble_manager.cc

Index: chrome/browser/ui/website_settings/permission_bubble_manager.cc
diff --git a/chrome/browser/ui/website_settings/permission_bubble_manager.cc b/chrome/browser/ui/website_settings/permission_bubble_manager.cc
index a2ace674da4eeb7e68fd9d0f0648eea5cbe140ae..24eca21236ecec696381e9a22d94c67c82cc47c8 100644
--- a/chrome/browser/ui/website_settings/permission_bubble_manager.cc
+++ b/chrome/browser/ui/website_settings/permission_bubble_manager.cc
@@ -6,6 +6,12 @@
 
 #include "base/command_line.h"
 #include "base/metrics/user_metrics_action.h"
+#include "chrome/browser/profiles/profile.h"

[msw, 2015/08/13 03:37:21]

nit: this probably isn't needed.

+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/chrome_bubble_manager.h"
+#include "chrome/browser/ui/chrome_bubble_manager_factory.h"
+#include "chrome/browser/ui/website_settings/permission_bubble_delegate.h"
 #include "chrome/browser/ui/website_settings/permission_bubble_request.h"
 #include "chrome/common/chrome_switches.h"
 #include "content/public/browser/browser_thread.h"
@@ -77,29 +83,16 @@ PermissionBubbleManager::PermissionBubbleManager(
     : content::WebContentsObserver(web_contents),
       require_user_gesture_(false),
 #if !defined(OS_ANDROID)  // No bubbles in android tests.
-      view_factory_(base::Bind(&PermissionBubbleView::Create)),
+      ui_factory_(base::Bind(&PermissionBubbleDelegate::CreateBubble)),
 #endif
-      view_(nullptr),
       main_frame_has_fully_loaded_(false),
       auto_response_for_test_(NONE),
       weak_factory_(this) {
 }
 
 PermissionBubbleManager::~PermissionBubbleManager() {
-  if (view_ != NULL)
-    view_->SetDelegate(NULL);
-
-  std::vector<PermissionBubbleRequest*>::iterator requests_iter;
-  for (requests_iter = requests_.begin();
-       requests_iter != requests_.end();
-       requests_iter++) {
-    (*requests_iter)->RequestFinished();
-  }
-  for (requests_iter = queued_requests_.begin();
-       requests_iter != queued_requests_.end();
-       requests_iter++) {
-    (*requests_iter)->RequestFinished();
-  }
+  CancelPendingQueues();
+  CloseBubble();
 }
 
 void PermissionBubbleManager::AddRequest(PermissionBubbleRequest* request) {
@@ -172,18 +165,24 @@ void PermissionBubbleManager::CancelRequest(PermissionBubbleRequest* request) {
     if (*requests_iter != request)
       continue;
 
+    BubbleManager* bubble_manager = GetBubbleManager();
+
     // We can simply erase the current entry in the request table if we aren't
     // showing the dialog, or if we are showing it and it can accept the update.
-    bool can_erase = !IsBubbleVisible() || view_->CanAcceptRequestUpdate();
+    bool can_erase =
+        !IsBubbleVisible() ||
+        (bubble_manager && bubble_manager->ShouldUpdateBubble(active_bubble_));

[msw, 2015/08/13 03:37:21]

q: if the bubble_manager is null, that seems to indicate that there's no browser
hosting the web_contents, does that mean that the tab has been closed or could
it just be a background/extension page or something? I'm wondering if
|can_erase| should be true if there is no bubble manager. I guess I'm also
wondering when the bubble manager might not exist or be found... what happens to
those bubbles below that aren't updated or closed?

[hcarmona, 2015/08/13 20:50:35]

This happens when a tab is closed. The bubble manager is watching the
web contents and it will close the bubbles when the web contents are
destroyed.

[msw, 2015/08/13 21:41:37]

So if |bubble_manager| is null, should |can_erase| be true or false?

+
     if (can_erase) {
       (*requests_iter)->RequestFinished();
       requests_.erase(requests_iter);
       accept_states_.erase(accepts_iter);
 
-      if (IsBubbleVisible()) {
-        view_->Hide();
-        // Will redraw the bubble if it is being shown.
-        TriggerShowBubble();
+      if (bubble_manager && IsBubbleVisible()) {
+        if (requests_.empty())
+          bubble_manager->CloseBubble(active_bubble_);
+        else
+          bubble_manager->UpdateBubble(active_bubble_);
       }
       return;
     }
@@ -199,39 +198,16 @@ void PermissionBubbleManager::CancelRequest(PermissionBubbleRequest* request) {
   NOTREACHED();  // Callers should not cancel requests that are not pending.
 }
 
-void PermissionBubbleManager::HideBubble() {
-  // Disengage from the existing view if there is one.
-  if (!view_)
-    return;
-
-  view_->SetDelegate(nullptr);
-  view_->Hide();
-  view_.reset();
-}
-
-void PermissionBubbleManager::DisplayPendingRequests(Browser* browser) {
-  if (IsBubbleVisible())
+void PermissionBubbleManager::CloseBubble() {
+  if (!active_bubble_)
     return;
 
-  view_ = view_factory_.Run(browser);
-  view_->SetDelegate(this);
-
-  TriggerShowBubble();
-}
-
-void PermissionBubbleManager::UpdateAnchorPosition() {
-  if (view_)
-    view_->UpdateAnchorPosition();
+  if (BubbleManager* bubble_manager = GetBubbleManager())

[msw, 2015/08/13 03:37:21]

Again, when could there be no bubble manager and what happens to these abandoned
unclosed bubbles? This shouldn't be possible with good bookkeeping.

[hcarmona, 2015/08/13 20:50:35]

Yes, this seems like a weird state that we should try to eliminate.

+    bubble_manager->CloseBubble(active_bubble_);
 }
 
 bool PermissionBubbleManager::IsBubbleVisible() {
-  return view_ && view_->IsVisible();
-}
-
-gfx::NativeWindow PermissionBubbleManager::GetBubbleWindow() {
-  if (view_)
-    return view_->GetNativeWindow();
-  return nullptr;
+  return active_bubble_;
 }
 
 void PermissionBubbleManager::RequireUserGesture(bool required) {
@@ -262,29 +238,10 @@ void PermissionBubbleManager::DocumentLoadedInFrame(
   ScheduleShowBubble();
 }
 
-void PermissionBubbleManager::NavigationEntryCommitted(
-    const content::LoadCommittedDetails& details) {
-  // No permissions requests pending.
-  if (request_url_.is_empty())
-    return;
-
-  // If we have navigated to a new url or reloaded the page...
-  // GetAsReferrer strips fragment and username/password, meaning
-  // the navigation is really to the same page.
-  if ((request_url_.GetAsReferrer() !=
-       web_contents()->GetLastCommittedURL().GetAsReferrer()) ||
-      (details.type == content::NAVIGATION_TYPE_EXISTING_PAGE &&
-       !details.is_in_page)) {
-    // Kill off existing bubble and cancel any pending requests.
-    CancelPendingQueues();
-    FinalizeBubble();
-  }
-}
-
 void PermissionBubbleManager::WebContentsDestroyed() {
   // If the web contents has been destroyed, treat the bubble as cancelled.
   CancelPendingQueues();
-  FinalizeBubble();
+  CloseBubble();
 
   // The WebContents is going away; be aggressively paranoid and delete
   // ourselves lest other parts of the system attempt to add permission bubbles
@@ -310,27 +267,35 @@ void PermissionBubbleManager::Accept() {
     else
       (*requests_iter)->PermissionDenied();
   }
-  FinalizeBubble();
+  CloseBubble();
 }
 
 void PermissionBubbleManager::Deny() {
-  std::vector<PermissionBubbleRequest*>::iterator requests_iter;
-  for (requests_iter = requests_.begin();
-       requests_iter != requests_.end();
-       requests_iter++) {
-    (*requests_iter)->PermissionDenied();
-  }
-  FinalizeBubble();
+  for (PermissionBubbleRequest* request : requests_)

[msw, 2015/08/13 03:37:21]

I wonder... if the bubble UI wasn't updated for a change in requests, and the
user clicked allow/deny or just dismissed the bubble, does that decision apply
to permissions that they never even saw? (ie. website asks for camera, I hover
bubble, website asks for mic but UI doesn't update, I click deny for the camera
ui being shown, both camera *and mic* are denied... perhaps worse if I accepted
permissions I never saw!)

[groby-ooo-7-16, 2015/08/13 18:44:38]

IIUC, these requests are stored in |queued_requests_| and so will not be marked
as denied. That's existing behavior, and is related to the postponing discussed
above.

FinalizeBubble used to pop up the next bubble with those requests. It's now
called DidClose, and invoked via CloseBubble.

@hcarmona: Maybe renaming FinalizeBubble is a refactor that should be landed
separately, since it doesn't affect functionality and will it make easier to
follow what's happening here.

[hcarmona, 2015/08/13 20:50:35]

Sounds good, we can have other CLs to refactor first.

+    request->PermissionDenied();
+  CloseBubble();
 }
 
 void PermissionBubbleManager::Closing() {
+  for (PermissionBubbleRequest* request : requests_)
+    request->Cancelled();
+  CloseBubble();
+}
+
+void PermissionBubbleManager::DidClose() {
   std::vector<PermissionBubbleRequest*>::iterator requests_iter;
   for (requests_iter = requests_.begin();

[msw, 2015/08/13 03:37:21]

nit: update loop to foreach like above.

[groby-ooo-7-16, 2015/08/13 18:44:38]

Ugh. I'd rather not do that here. We've already got enough changes :)

 Can we do a separate refactor CL to clean up the for loops, and do this before
this CL?

[msw, 2015/08/13 19:18:58]

It's a nit; definitely not required, and welcome to be addressed separately.

[hcarmona, 2015/08/13 20:50:35]

We can have a separate refactor CL. This particular loop cannot be
changed because |RequestFinished| will clean up requests.

        requests_iter != requests_.end();
        requests_iter++) {
-    (*requests_iter)->Cancelled();
+    (*requests_iter)->RequestFinished();
   }
-  FinalizeBubble();
+
+  requests_.clear();
+  accept_states_.clear();
+  if (queued_requests_.size() || queued_frame_requests_.size())

[msw, 2015/08/13 03:37:21]

nit: ask if they're [not] empty, rather than implicitly converting the size to
booleans.

+    TriggerShowBubble();
+  else
+    request_url_ = GURL();
 }
 
 void PermissionBubbleManager::ScheduleShowBubble() {
@@ -347,8 +312,6 @@ void PermissionBubbleManager::ScheduleShowBubble() {
 }
 
 void PermissionBubbleManager::TriggerShowBubble() {
-  if (!view_)
-    return;
   if (IsBubbleVisible())
     return;
   if (!main_frame_has_fully_loaded_)
@@ -376,7 +339,12 @@ void PermissionBubbleManager::TriggerShowBubble() {
     accept_states_.resize(requests_.size(), true);
   }
 
-  view_->Show(requests_, accept_states_);
+  BubbleManager* bubble_manager = GetBubbleManager();
+  DCHECK(bubble_manager);
+
+  active_bubble_ = bubble_manager->ShowBubble(
+      make_scoped_ptr(new PermissionBubbleDelegate(ui_factory_, this)));
+
   NotifyBubbleAdded();
 
   // If in testing mode, automatically respond to the bubble that was shown.
@@ -384,24 +352,6 @@ void PermissionBubbleManager::TriggerShowBubble() {
     DoAutoResponseForTesting();
 }
 
-void PermissionBubbleManager::FinalizeBubble() {
-  if (view_)
-    view_->Hide();
-
-  std::vector<PermissionBubbleRequest*>::iterator requests_iter;
-  for (requests_iter = requests_.begin();
-       requests_iter != requests_.end();
-       requests_iter++) {
-    (*requests_iter)->RequestFinished();
-  }
-  requests_.clear();
-  accept_states_.clear();
-  if (queued_requests_.size() || queued_frame_requests_.size())
-    TriggerShowBubble();
-  else
-    request_url_ = GURL();
-}
-
 void PermissionBubbleManager::CancelPendingQueues() {
   std::vector<PermissionBubbleRequest*>::iterator requests_iter;
   for (requests_iter = queued_requests_.begin();
@@ -457,6 +407,12 @@ void PermissionBubbleManager::RemoveObserver(Observer* observer) {
   observer_list_.RemoveObserver(observer);
 }
 
+BubbleManager* PermissionBubbleManager::GetBubbleManager() {
+  if (Browser* browser = chrome::FindBrowserWithWebContents(web_contents()))
+    return ChromeBubbleManagerFactory::GetForBrowserContext(browser->profile());
+  return nullptr;
+}
+
 void PermissionBubbleManager::NotifyBubbleAdded() {
   FOR_EACH_OBSERVER(Observer, observer_list_, OnBubbleAdded());
 }