[WebsiteSettings] Change permission bubble API to adapt to new mocks.

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 97864ddde84228af4c4ae0f1570ddf1eac7e6979..2ef02f86f8b291454ca53313de3e3cfda3b547cb 100644
--- a/chrome/browser/ui/website_settings/permission_bubble_manager.cc
+++ b/chrome/browser/ui/website_settings/permission_bubble_manager.cc
@@ -24,23 +24,67 @@ bool PermissionBubbleManager::Enabled() {
       switches::kEnablePermissionsBubbles);
 }
 
+
+
+PermissionBubbleManager::PermissionBubbleManager(
+    content::WebContents* web_contents)
+  : content::WebContentsObserver(web_contents),
+    bubble_showing_(false),
+    view_(NULL),
+    customization_mode_(false) {
+  timer_.reset(new base::Timer(FROM_HERE,
+      base::TimeDelta::FromMilliseconds(kPermissionsCoalesceIntervalMs),
+      base::Bind(&PermissionBubbleManager::ShowBubble, base::Unretained(this)),
+      false));
+}
+
+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();
+  }
+  requests_.clear();

[groby-ooo-7-16, 2014/02/25 22:27:36]

Why clear? The dtor does that already?

[Greg Billock, 2014/02/26 00:28:44]

Just gives me a warm fuzzy. :-) I'll take it out.

+  accept_states_.clear();
+  for (requests_iter = queued_requests_.begin();
+       requests_iter != queued_requests_.end();
+       requests_iter++) {
+    (*requests_iter)->RequestFinished();
+  }
+  queued_requests_.clear();
+}
+
 void PermissionBubbleManager::AddRequest(PermissionBubbleRequest* request) {
-  // Don't re-add an existing request.
+  // Don't re-add an existing request or one with a duplicate text request.
   std::vector<PermissionBubbleRequest*>::iterator requests_iter;
   for (requests_iter = requests_.begin();
        requests_iter != requests_.end();
        requests_iter++) {
     if (*requests_iter == request)
       return;
+    if ((*requests_iter)->GetMessageTextFragment() ==
+        request->GetMessageTextFragment()) {
+      request->RequestFinished();

[groby-ooo-7-16, 2014/02/25 22:27:36]

Is that really "RequestFinished"? Shouldn't that be treated as a still pending
request?

[Greg Billock, 2014/02/26 00:28:44]

This is basically rejecting requests that the user won't be able to distinguish.
I need to add a TODO here to worry about different requesting hosts, though. We
don't have an overall plan how to deal with those yet...

+      return;
+    }
+  }
+  for (requests_iter = queued_requests_.begin();

[groby-ooo-7-16, 2014/02/25 22:27:36]

This is a lot of duplicated code - worth factoring out an internal
AddRequest(std::vector<>, Request*) ?

+       requests_iter != queued_requests_.end();
+       requests_iter++) {
+    if (*requests_iter == request)
+      return;
+    if ((*requests_iter)->GetMessageTextFragment() ==
+        request->GetMessageTextFragment()) {
+      request->RequestFinished();
+      return;
+    }
   }
 
   if (bubble_showing_) {

[groby-ooo-7-16, 2014/02/25 22:27:36]

It's technically outside of this CL's scope, but: Why not ask the view_ if it's
showing?

[Greg Billock, 2014/02/26 00:28:44]

The way I have the handshake working right now, the toolkit makes and owns the
view implementation, which then gets told to the manager. The manager is in
charge of figuring out whether the bubble should be shown, since there's only
one view class per browser, and the decision about bubble showing is per-tab
(the manager is per-tab).

IOW, the same view impl should be shown in tab 1 and not in tab 2 (but with
different contents).

At one point I played with calling this bubble_should_be_shown_, which is
arguably a bit better of a name.

-    for (requests_iter = queued_requests_.begin();
-         requests_iter != queued_requests_.end();
-         requests_iter++) {
-      if (*requests_iter == request)
-        return;
-    }
     queued_requests_.push_back(request);
     return;
   }
@@ -54,6 +98,11 @@ void PermissionBubbleManager::AddRequest(PermissionBubbleRequest* request) {
     timer_->Reset();
 }
 
+void PermissionBubbleManager::CancelRequest(PermissionBubbleRequest* request) {
+  // TODO(gbillock): implement
+  NOTREACHED();
+}

[groby-ooo-7-16, 2014/02/25 22:27:36]

Why add this if it doesn't do anything yet?

[Greg Billock, 2014/02/26 00:28:44]

I was attempting to keep the scope of the CL down. This'll require more than
trivial work at the call sites, so my plan is to do it when I do that. Could
remove the method signature for now, though...

+
 void PermissionBubbleManager::SetView(PermissionBubbleView* view) {
   if (view == view_)
     return;
@@ -78,26 +127,6 @@ void PermissionBubbleManager::SetView(PermissionBubbleView* view) {
     view_->Hide();
 }
 
-PermissionBubbleManager::PermissionBubbleManager(
-    content::WebContents* web_contents)
-  : content::WebContentsObserver(web_contents),
-    bubble_showing_(false),
-    view_(NULL),
-    customization_mode_(false) {
-  timer_.reset(new base::Timer(FROM_HERE,
-      base::TimeDelta::FromMilliseconds(kPermissionsCoalesceIntervalMs),
-      base::Bind(&PermissionBubbleManager::ShowBubble, base::Unretained(this)),
-      false));
-}
-
-PermissionBubbleManager::~PermissionBubbleManager() {
-  if (view_ != NULL) {
-    view_->SetDelegate(NULL);
-    view_->Hide();
-    bubble_showing_ = false;
-  }
-}
-
 void PermissionBubbleManager::DidFinishLoad(
     int64 frame_id,
     const GURL& validated_url,