Handles iframe permissions requests separately, in a subsequent bubble.

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 8816cee26ffea3ff1ebdfce6c6eca5b28a27a6e7..753ff100c03b5e403d7914d1dea65d8124f9ae51 100644
--- a/chrome/browser/ui/website_settings/permission_bubble_manager.cc
+++ b/chrome/browser/ui/website_settings/permission_bubble_manager.cc
@@ -9,6 +9,7 @@
 #include "chrome/browser/ui/website_settings/permission_bubble_request.h"
 #include "chrome/common/chrome_switches.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/navigation_details.h"
 #include "content/public/browser/user_metrics.h"
 
 namespace {
@@ -101,46 +102,38 @@ void PermissionBubbleManager::AddRequest(PermissionBubbleRequest* request) {
   // correct behavior on interstitials -- we probably want to basically queue
   // any request for which GetVisibleURL != GetLastCommittedURL.
   request_url_ = web_contents()->GetLastCommittedURL();
+  bool is_main_frame = request->GetRequestingHostname() == request_url_;

[Greg Billock, 2014/05/21 17:09:19]

Do we need to extract just the hostport from the request_url_ to do this?

[leng, 2014/05/22 00:08:47]

I have no idea.  I don't know enough about main frame vs. iframe requests.

[Greg Billock, 2014/05/22 02:06:31]

I think GetLastCommittedURL() is the full url, and GetRequestingHostname() is
just the hostname portion of the url (formatted as a url), so the root of the
security context. So in our test cases of url == just a host it would work, but
I'm suspicious it won't for urls with paths.

[leng, 2014/05/22 23:57:05]

Got it.  I changed it to use GetOrigin() for both - we only care about same
domain vs. other domain, right?

 
   // 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;
-    // TODO(gbillock): worry about the requesting host name as well.
-    if ((*requests_iter)->GetMessageTextFragment() ==
-        request->GetMessageTextFragment()) {
+  bool exact_duplicate = false;
+  if (ExistingRequest(request, requests_, &exact_duplicate) ||

[Greg Billock, 2014/05/21 17:09:19]

Maybe exact_duplicate --> same_object ?

[leng, 2014/05/22 00:08:47]

Good call.  Done.

+      ExistingRequest(request, queued_requests_, &exact_duplicate) ||
+      ExistingRequest(request, queued_frame_requests_, &exact_duplicate)) {
+    if (!exact_duplicate)
       request->RequestFinished();
-      return;
-    }
-  }
-  for (requests_iter = queued_requests_.begin();
-       requests_iter != queued_requests_.end();
-       requests_iter++) {
-    if (*requests_iter == request)
-      return;
-    if ((*requests_iter)->GetMessageTextFragment() ==
-        request->GetMessageTextFragment()) {
-      request->RequestFinished();
-      return;
-    }
+    return;
   }
 
   if (bubble_showing_) {
     content::RecordAction(
         base::UserMetricsAction("PermissionBubbleRequestQueued"));
-    queued_requests_.push_back(request);
+    if (is_main_frame)
+      queued_requests_.push_back(request);
+    else
+      queued_frame_requests_.push_back(request);
     return;
   }
 
-  requests_.push_back(request);
+  if (is_main_frame)
+    requests_.push_back(request);
+  else
+    queued_frame_requests_.push_back(request);
+
   // TODO(gbillock): do we need to make default state a request property?
   accept_states_.push_back(true);

[Greg Billock, 2014/05/21 17:09:19]

should this only execute if is_main_frame, right?

[leng, 2014/05/22 00:08:47]

Done.

 
   if (request->HasUserGesture())
-    ShowBubble();
+    TriggerShowBubble();

[Greg Billock, 2014/05/21 17:09:19]

Should this be ScheduleShowBubble, right? (and was it an error before?) to let
any simultaneous pending registrations complete? Kind of a race condition, so
maybe no observable difference...

[leng, 2014/05/22 00:08:47]

I see no harm in making it ScheduleShowBubble(), and a slight possible upside if
there is a race.  Changed.

 }
 
 void PermissionBubbleManager::CancelRequest(PermissionBubbleRequest* request) {
@@ -172,7 +165,7 @@ void PermissionBubbleManager::CancelRequest(PermissionBubbleRequest* request) {
       (*requests_iter)->RequestFinished();
       requests_.erase(requests_iter);
       accept_states_.erase(accepts_iter);
-      ShowBubble();  // Will redraw the bubble if it is being shown.
+      TriggerShowBubble();  // Will redraw the bubble if it is being shown.
       return;
     }
 
@@ -203,7 +196,7 @@ void PermissionBubbleManager::SetView(PermissionBubbleView* view) {
     return;
 
   view->SetDelegate(this);
-  ShowBubble();
+  TriggerShowBubble();
 }
 
 void PermissionBubbleManager::DocumentOnLoadCompletedInMainFrame() {
@@ -213,11 +206,15 @@ void PermissionBubbleManager::DocumentOnLoadCompletedInMainFrame() {
   // callbacks finding the UI thread still. This makes sure we allow those
   // scheduled calls to AddRequest to complete before we show the page-load
   // permissions bubble.
-  // TODO(gbillock): make this bind safe with a weak ptr.
-  content::BrowserThread::PostTask(
-      content::BrowserThread::UI, FROM_HERE,
-      base::Bind(&PermissionBubbleManager::ShowBubble,
-                 weak_factory_.GetWeakPtr()));
+  ScheduleShowBubble();
+}
+
+void PermissionBubbleManager::DocumentLoadedInFrame(
+    int64 frame_id,
+    content::RenderViewHost* render_view_host) {
+  if (request_url_has_loaded_) {

[Greg Billock, 2014/05/21 17:09:19]

no braces

[leng, 2014/05/22 00:08:47]

Done.

+    ScheduleShowBubble();
+  }
 }
 
 void PermissionBubbleManager::NavigationEntryCommitted(
@@ -226,8 +223,9 @@ void PermissionBubbleManager::NavigationEntryCommitted(
   if (request_url_.is_empty())
     return;
 
-  // If we have navigated to a new url...
-  if (request_url_ != web_contents()->GetLastCommittedURL()) {
+  // If we have navigated to a new url or reloaded the page...
+  if (request_url_ != web_contents()->GetLastCommittedURL() ||
+      details.type == content::NAVIGATION_TYPE_EXISTING_PAGE) {

[Greg Billock, 2014/05/21 17:09:19]

Makes sense. We then get the re-notifications for any permissions requests
issued by the page, right?

[leng, 2014/05/22 00:08:47]

Correct.  I tested it manually.

     // Kill off existing bubble and cancel any pending requests.
     CancelPendingQueue();
     FinalizeBubble();
@@ -292,16 +290,32 @@ void PermissionBubbleManager::Closing() {
   FinalizeBubble();
 }
 
-// TODO(gbillock): find a better name for this method.
-void PermissionBubbleManager::ShowBubble() {
+void PermissionBubbleManager::ScheduleShowBubble() {
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&PermissionBubbleManager::TriggerShowBubble,
+                 weak_factory_.GetWeakPtr()));
+}
+
+void PermissionBubbleManager::TriggerShowBubble() {
   if (!view_)
     return;
-  if (requests_.empty())
-    return;
   if (bubble_showing_)
     return;
   if (!request_url_has_loaded_)
     return;
+  if (requests_.empty() &&
+      queued_requests_.empty() &&
+      queued_frame_requests_.empty())
+    return;
+
+  if (requests_.empty()) {
+    if (queued_requests_.size())
+      requests_.swap(queued_requests_);
+    else
+      requests_.swap(queued_frame_requests_);
+    accept_states_.resize(requests_.size(), true);

[Greg Billock, 2014/05/21 17:09:19]

Maybe a comment that the 'true' here is setting their value? I felt like I
wanted to look that up to make sure I had it right.

[leng, 2014/05/22 00:08:47]

Done.

+  }
 
   // Note: this should appear above Show() for testing, since in that
   // case we may do in-line calling of finalization.
@@ -322,11 +336,8 @@ void PermissionBubbleManager::FinalizeBubble() {
   }
   requests_.clear();
   accept_states_.clear();
-  if (queued_requests_.size()) {
-    requests_ = queued_requests_;
-    accept_states_.resize(requests_.size(), true);
-    queued_requests_.clear();
-    ShowBubble();
+  if (queued_requests_.size() || queued_frame_requests_.size()) {
+    TriggerShowBubble();
   } else {
     request_url_ = GURL();
   }
@@ -340,3 +351,28 @@ void PermissionBubbleManager::CancelPendingQueue() {
     (*requests_iter)->RequestFinished();
   }
 }
+
+bool PermissionBubbleManager::ExistingRequest(
+    PermissionBubbleRequest* request,
+    const std::vector<PermissionBubbleRequest*>& queue,
+    bool* exact_duplicate) {
+  std::vector<PermissionBubbleRequest*>::const_iterator iter;
+  for (iter = queue.begin();

[Greg Billock, 2014/05/21 17:09:19]

fits on one line?

[leng, 2014/05/22 00:08:47]

Done.

+       iter != queue.end();
+       iter++) {
+    if (*iter == request) {
+      if (exact_duplicate)

[Greg Billock, 2014/05/21 17:09:19]

we can stipulate that CHECK(exact_duplicate)

[leng, 2014/05/22 00:08:47]

Done.

+        *exact_duplicate = true;
+      return true;
+    }
+    if ((*iter)->GetMessageTextFragment() ==
+        request->GetMessageTextFragment() &&
+        (*iter)->GetRequestingHostname() ==
+        request->GetRequestingHostname()) {
+      if (exact_duplicate)
+        *exact_duplicate = false;
+      return true;
+    }
+  }
+  return false;
+}