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

chrome/browser/ui/website_settings/permission_bubble_manager.cc

 // Copyright 2014 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/ui/website_settings/permission_bubble_manager.h"
 
 #include "base/command_line.h"
 #include "chrome/browser/ui/website_settings/permission_bubble_request.h"
 #include "chrome/common/chrome_switches.h"
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(PermissionBubbleManager);
 
 namespace {
 
 // This is how many ms to wait to see if there's another permission request
 // we should coalesce.
 const int kPermissionsCoalesceIntervalMs = 400;
 
 }
 
 // static
 bool PermissionBubbleManager::Enabled() {
   return CommandLine::ForCurrentProcess()->HasSwitch(
       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;
   }
 
   requests_.push_back(request);
   // TODO(gbillock): do we need to make default state a request property?
   accept_states_.push_back(true);
 
   // Start the timer when there is both a view and a request.
   if (view_ && !timer_->IsRunning())
     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;
 
   if (view_ != NULL) {
     view_->SetDelegate(NULL);
     view_->Hide();
     bubble_showing_ = false;
   }
 
   view_ = view;
   if (view_)
     view_->SetDelegate(this);
   else
     return;
 
   // Even if there are requests queued up, add a short delay before the bubble
   // appears.
   if (!requests_.empty() && !timer_->IsRunning())
     timer_->Reset();
   else
     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,
     bool is_main_frame,
     content::RenderViewHost* render_view_host) {
   // Allows extra time for additional requests to coalesce.
   if (timer_->IsRunning())
     timer_->Reset();
 }
 
@@ skipping 84 matching lines @@
     timer_->Reset();
   }
 }
 
 void PermissionBubbleManager::SetCoalesceIntervalForTesting(int interval_ms) {
   timer_.reset(new base::Timer(FROM_HERE,
       base::TimeDelta::FromMilliseconds(interval_ms),
       base::Bind(&PermissionBubbleManager::ShowBubble, base::Unretained(this)),
       false));
 }