Handles iframe permissions requests separately, in a subsequent bubble.

If the requests are received before the bubble is shown, and there are main
frame requests pending, they will not be shown with the main frame requests.

Once the main frame requests have been handled, the iframe requests will be
shown in a new bubble.

All requests are flushed from the queues when a page is reloaded.

BUG=369685

Review URL: https://codereview.chromium.org/292453009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@273137 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/ui/website_settings/mock_permission_bubble_request.cc

@@ -9,27 +9,53 @@
 #include "grit/theme_resources.h"
 
 MockPermissionBubbleRequest::MockPermissionBubbleRequest()
-    : granted_(false), cancelled_(false), finished_(false) {
+    : granted_(false),
+      cancelled_(false),
+      finished_(false),
+      user_gesture_(false) {
   text_ = base::ASCIIToUTF16("test");
   accept_label_ = base::ASCIIToUTF16("button");
   deny_label_ = base::ASCIIToUTF16("button");
+  hostname_ = GURL("http://www.google.com");
 }
 
 MockPermissionBubbleRequest::MockPermissionBubbleRequest(
     const std::string& text)
-    : granted_(false), cancelled_(false), finished_(false) {
+    : granted_(false),
+      cancelled_(false),
+      finished_(false),
+      user_gesture_(false) {
   text_ = base::UTF8ToUTF16(text);
   accept_label_ = base::ASCIIToUTF16("button");
   deny_label_ = base::ASCIIToUTF16("button");
+  hostname_ = GURL("http://www.google.com");
 }
 
 MockPermissionBubbleRequest::MockPermissionBubbleRequest(
-    const std::string& text, const std::string& accept_label,
+    const std::string& text,
+    const GURL& url)
+    : granted_(false),
+      cancelled_(false),
+      finished_(false),
+      user_gesture_(false) {
+  text_ = base::UTF8ToUTF16(text);
+  accept_label_ = base::ASCIIToUTF16("button");
+  deny_label_ = base::ASCIIToUTF16("button");
+  hostname_ = url;
+}
+
+MockPermissionBubbleRequest::MockPermissionBubbleRequest(
+    const std::string& text,
+    const std::string& accept_label,
     const std::string& deny_label)
-    : granted_(false), cancelled_(false), finished_(false) {
+    : granted_(false),
+      cancelled_(false),
+      finished_(false),
+      user_gesture_(false) {
   text_ = base::UTF8ToUTF16(text);
   accept_label_ = base::UTF8ToUTF16(accept_label);
   deny_label_ = base::UTF8ToUTF16(deny_label);
+  hostname_ = GURL("http://www.google.com");
 }
 
 MockPermissionBubbleRequest::~MockPermissionBubbleRequest() {}
@@ -48,11 +74,11 @@ base::string16 MockPermissionBubbleRequest::GetMessageTextFragment() const {
 }
 
 bool MockPermissionBubbleRequest::HasUserGesture() const {
-  return false;
+  return user_gesture_;
 }
 
 GURL MockPermissionBubbleRequest::GetRequestingHostname() const {
-  return GURL("http://www.google.com");
+  return hostname_;
 }
 
 void MockPermissionBubbleRequest::PermissionGranted() {
@@ -83,3 +109,7 @@ bool MockPermissionBubbleRequest::cancelled() {
 bool MockPermissionBubbleRequest::finished() {
   return finished_;
 }
+
+void MockPermissionBubbleRequest::SetHasUserGesture() {
+  user_gesture_ = true;
+}

chrome/browser/ui/website_settings/mock_permission_bubble_request.h

@@ -13,6 +13,8 @@ class MockPermissionBubbleRequest : public PermissionBubbleRequest {
  public:
   MockPermissionBubbleRequest();
   explicit MockPermissionBubbleRequest(const std::string& text);
+  explicit MockPermissionBubbleRequest(const std::string& text,
+                                       const GURL& url);
   explicit MockPermissionBubbleRequest(const std::string& text,
                                        const std::string& accept_label,
                                        const std::string& deny_label);
@@ -33,14 +35,18 @@ class MockPermissionBubbleRequest : public PermissionBubbleRequest {
   bool cancelled();
   bool finished();
 
+  void SetHasUserGesture();
+
  private:
   bool granted_;
   bool cancelled_;
   bool finished_;
+  bool user_gesture_;
 
   base::string16 text_;
   base::string16 accept_label_;
   base::string16 deny_label_;
+  GURL hostname_;
 };
 
 #endif  // CHROME_BROWSER_UI_WEBSITE_SETTINGS_MOCK_PERMISSION_BUBBLE_REQUEST_H_

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,39 @@ 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().GetOrigin() == request_url_.GetOrigin();
 
   // 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()) {
-      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()) {
+  bool same_object = false;
+  if (ExistingRequest(request, requests_, &same_object) ||
+      ExistingRequest(request, queued_requests_, &same_object) ||
+      ExistingRequest(request, queued_frame_requests_, &same_object)) {
+    if (!same_object)
       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);
-  // TODO(gbillock): do we need to make default state a request property?
-  accept_states_.push_back(true);
+  if (is_main_frame) {
+    requests_.push_back(request);
+    // TODO(gbillock): do we need to make default state a request property?
+    accept_states_.push_back(true);
+  } else {
+    queued_frame_requests_.push_back(request);
+  }
 
   if (request->HasUserGesture())
-    ShowBubble();
+    ScheduleShowBubble();
 }
 
 void PermissionBubbleManager::CancelRequest(PermissionBubbleRequest* request) {
@@ -172,7 +166,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 +197,7 @@ void PermissionBubbleManager::SetView(PermissionBubbleView* view) {
     return;
 
   view->SetDelegate(this);
-  ShowBubble();
+  TriggerShowBubble();
 }
 
 void PermissionBubbleManager::DocumentOnLoadCompletedInMainFrame() {
@@ -213,11 +207,14 @@ 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_)
+    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) {
     // Kill off existing bubble and cancel any pending requests.
     CancelPendingQueue();
     FinalizeBubble();
@@ -292,16 +290,42 @@ 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()) {
+    // Queues containing a user-gesture-generated request have priority.
+    if (HasUserGestureRequest(queued_requests_))
+      requests_.swap(queued_requests_);
+    else if (HasUserGestureRequest(queued_frame_requests_))
+      requests_.swap(queued_frame_requests_);
+    else if (queued_requests_.size())
+      requests_.swap(queued_requests_);
+    else
+      requests_.swap(queued_frame_requests_);
+
+    // Sets the default value for each request to be 'accept'.
+    // TODO(leng):  Currently all requests default to true.  If that changes:
+    // a) Add additional accept_state queues to store default values.
+    // b) Change the request API to provide the default value.
+    accept_states_.resize(requests_.size(), true);
+  }
 
   // Note: this should appear above Show() for testing, since in that
   // case we may do in-line calling of finalization.
@@ -322,11 +346,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 +361,34 @@ void PermissionBubbleManager::CancelPendingQueue() {
     (*requests_iter)->RequestFinished();
   }
 }
+
+bool PermissionBubbleManager::ExistingRequest(
+    PermissionBubbleRequest* request,
+    const std::vector<PermissionBubbleRequest*>& queue,
+    bool* same_object) {
+  CHECK(same_object);
+  *same_object = false;
+  std::vector<PermissionBubbleRequest*>::const_iterator iter;
+  for (iter = queue.begin(); iter != queue.end(); iter++) {
+    if (*iter == request) {
+      *same_object = true;
+      return true;
+    }
+    if ((*iter)->GetMessageTextFragment() ==
+            request->GetMessageTextFragment() &&
+        (*iter)->GetRequestingHostname() == request->GetRequestingHostname()) {
+      return true;
+    }
+  }
+  return false;
+}
+
+bool PermissionBubbleManager::HasUserGestureRequest(
+    const std::vector<PermissionBubbleRequest*>& queue) {
+  std::vector<PermissionBubbleRequest*>::const_iterator iter;
+  for (iter = queue.begin(); iter != queue.end(); iter++) {
+    if ((*iter)->HasUserGesture())
+      return true;
+  }
+  return false;
+}

chrome/browser/ui/website_settings/permission_bubble_manager.h

@@ -65,10 +65,10 @@ class PermissionBubbleManager
   explicit PermissionBubbleManager(content::WebContents* web_contents);
 
   // WebContentsObserver:
-
-  // TODO(leng): Finalize policy for permission requests with iFrames.
-  // DocumentLoadedInFrame() might be needed as well.
   virtual void DocumentOnLoadCompletedInMainFrame() OVERRIDE;
+  virtual void DocumentLoadedInFrame(
+      int64 frame_id,
+      content::RenderViewHost* render_view_host) OVERRIDE;
 
   // If a page on which permissions requests are pending is navigated,
   // they will be finalized as if canceled by the user.
@@ -83,7 +83,12 @@ class PermissionBubbleManager
   virtual void Deny() OVERRIDE;
   virtual void Closing() OVERRIDE;
 
-  void ShowBubble();
+  // Posts a task which will allow the bubble to become visible if it is needed.
+  void ScheduleShowBubble();
+
+  // Shows the bubble if it is not already visible and there are pending
+  // requests.
+  void TriggerShowBubble();
 
   // Finalize the pending permissions request.
   void FinalizeBubble();
@@ -93,6 +98,18 @@ class PermissionBubbleManager
   // from the requesting page.
   void CancelPendingQueue();
 
+  // Returns whether or not |request| has already been added to |queue|.
+  // |same_object| must be non-null.  It will be set to true if |request|
+  // is the same object as an existing request in |queue|, false otherwise.
+  bool ExistingRequest(PermissionBubbleRequest* request,
+                       const std::vector<PermissionBubbleRequest*>& queue,
+                       bool* same_object);
+
+  // Returns true if |queue| contains a request which was generated by a user
+  // gesture.  Returns false otherwise.
+  bool HasUserGestureRequest(
+      const std::vector<PermissionBubbleRequest*>& queue);
+
   // Whether or not we are showing the bubble in this tab.
   bool bubble_showing_;
 
@@ -101,6 +118,7 @@ class PermissionBubbleManager
 
   std::vector<PermissionBubbleRequest*> requests_;
   std::vector<PermissionBubbleRequest*> queued_requests_;
+  std::vector<PermissionBubbleRequest*> queued_frame_requests_;
 
   // URL of the main frame in the WebContents to which this manager is attached.
   // TODO(gbillock): if there are iframes in the page, we need to deal with it.

chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc

@@ -64,12 +64,17 @@ class PermissionBubbleManagerTest : public ChromeRenderViewHostTestHarness {
   PermissionBubbleManagerTest()
       : ChromeRenderViewHostTestHarness(),
         request1_("test1"),
-        request2_("test2") {}
+        request2_("test2"),
+        iframe_request_same_domain_("iframe",
+                                    GURL("http://www.google.com/some/url")),
+        iframe_request_other_domain_("iframe",
+                                     GURL("http://www.youtube.com")) {}
   virtual ~PermissionBubbleManagerTest() {}
 
   virtual void SetUp() OVERRIDE {
     ChromeRenderViewHostTestHarness::SetUp();
     SetContents(CreateTestWebContents());
+    NavigateAndCommit(GURL("http://www.google.com"));
 
     manager_.reset(new PermissionBubbleManager(web_contents()));
   }
@@ -91,6 +96,11 @@ class PermissionBubbleManagerTest : public ChromeRenderViewHostTestHarness {
     manager_->Closing();
   }
 
+  void WaitForFrameLoad() {
+    manager_->DocumentLoadedInFrame(0, NULL);
+    base::MessageLoop::current()->RunUntilIdle();
+  }
+
   void WaitForCoalescing() {
     manager_->DocumentOnLoadCompletedInMainFrame();
     base::MessageLoop::current()->RunUntilIdle();
@@ -104,6 +114,8 @@ class PermissionBubbleManagerTest : public ChromeRenderViewHostTestHarness {
  protected:
   MockPermissionBubbleRequest request1_;
   MockPermissionBubbleRequest request2_;
+  MockPermissionBubbleRequest iframe_request_same_domain_;
+  MockPermissionBubbleRequest iframe_request_other_domain_;
   MockView view_;
   scoped_ptr<PermissionBubbleManager> manager_;
 };
@@ -321,7 +333,6 @@ TEST_F(PermissionBubbleManagerTest, DuplicateQueuedRequest) {
 }
 
 TEST_F(PermissionBubbleManagerTest, ForgetRequestsOnPageNavigation) {
-  NavigateAndCommit(GURL("http://www.google.com/"));
   manager_->SetView(&view_);
   manager_->AddRequest(&request1_);
   WaitForCoalescing();
@@ -391,3 +402,123 @@ TEST_F(PermissionBubbleManagerTest, TestCancelPendingRequest) {
   EXPECT_TRUE(request2_.finished());
 }
 
+TEST_F(PermissionBubbleManagerTest, MainFrameNoRequestIFrameRequest) {
+  manager_->SetView(&view_);
+  manager_->AddRequest(&iframe_request_same_domain_);
+  WaitForCoalescing();
+  WaitForFrameLoad();
+
+  EXPECT_TRUE(view_.shown_);
+  Closing();
+  EXPECT_TRUE(iframe_request_same_domain_.finished());
+}
+
+TEST_F(PermissionBubbleManagerTest, MainFrameAndIFrameRequestSameDomain) {
+  manager_->SetView(&view_);
+  manager_->AddRequest(&request1_);
+  manager_->AddRequest(&iframe_request_same_domain_);
+  WaitForFrameLoad();
+  WaitForCoalescing();
+
+  EXPECT_TRUE(view_.shown_);
+  EXPECT_EQ(2u, view_.permission_requests_.size());
+  Closing();
+  EXPECT_TRUE(request1_.finished());
+  EXPECT_TRUE(iframe_request_same_domain_.finished());
+  EXPECT_FALSE(view_.shown_);
+}
+
+TEST_F(PermissionBubbleManagerTest, MainFrameAndIFrameRequestOtherDomain) {
+  manager_->SetView(&view_);
+  manager_->AddRequest(&request1_);
+  manager_->AddRequest(&iframe_request_other_domain_);
+  WaitForFrameLoad();
+  WaitForCoalescing();
+
+  EXPECT_TRUE(view_.shown_);
+  Closing();
+  EXPECT_TRUE(request1_.finished());
+  EXPECT_FALSE(iframe_request_other_domain_.finished());
+  EXPECT_TRUE(view_.shown_);
+  Closing();
+  EXPECT_TRUE(iframe_request_other_domain_.finished());
+}
+
+TEST_F(PermissionBubbleManagerTest, IFrameRequestWhenMainRequestVisible) {
+  manager_->SetView(&view_);
+  manager_->AddRequest(&request1_);
+  WaitForCoalescing();
+  EXPECT_TRUE(view_.shown_);
+
+  manager_->AddRequest(&iframe_request_same_domain_);
+  WaitForFrameLoad();
+  EXPECT_EQ(1u, view_.permission_requests_.size());
+  Closing();
+  EXPECT_TRUE(request1_.finished());
+  EXPECT_FALSE(iframe_request_same_domain_.finished());
+  EXPECT_TRUE(view_.shown_);
+  EXPECT_EQ(1u, view_.permission_requests_.size());
+  Closing();
+  EXPECT_TRUE(iframe_request_same_domain_.finished());
+}
+
+TEST_F(PermissionBubbleManagerTest,
+       IFrameRequestOtherDomainWhenMainRequestVisible) {
+  manager_->SetView(&view_);
+  manager_->AddRequest(&request1_);
+  WaitForCoalescing();
+  EXPECT_TRUE(view_.shown_);
+
+  manager_->AddRequest(&iframe_request_other_domain_);
+  WaitForFrameLoad();
+  Closing();
+  EXPECT_TRUE(request1_.finished());
+  EXPECT_FALSE(iframe_request_other_domain_.finished());
+  EXPECT_TRUE(view_.shown_);
+  Closing();
+  EXPECT_TRUE(iframe_request_other_domain_.finished());
+}
+
+TEST_F(PermissionBubbleManagerTest, IFrameUserGestureRequest) {
+  iframe_request_other_domain_.SetHasUserGesture();
+  manager_->SetView(&view_);
+  manager_->AddRequest(&request1_);
+  manager_->AddRequest(&iframe_request_other_domain_);
+  WaitForFrameLoad();
+  WaitForCoalescing();
+  manager_->AddRequest(&request2_);
+
+  EXPECT_TRUE(view_.shown_);
+  Closing();
+  EXPECT_TRUE(request1_.finished());
+  EXPECT_FALSE(iframe_request_other_domain_.finished());
+  EXPECT_FALSE(request2_.finished());
+  EXPECT_TRUE(view_.shown_);
+  Closing();
+  EXPECT_TRUE(iframe_request_other_domain_.finished());
+  EXPECT_FALSE(request2_.finished());
+}
+
+TEST_F(PermissionBubbleManagerTest, AllUserGestureRequests) {
+  iframe_request_other_domain_.SetHasUserGesture();
+  request2_.SetHasUserGesture();
+  manager_->SetView(&view_);
+  manager_->AddRequest(&request1_);
+  manager_->AddRequest(&iframe_request_other_domain_);
+  WaitForCoalescing();
+  WaitForFrameLoad();
+  manager_->AddRequest(&request2_);
+
+  EXPECT_TRUE(view_.shown_);
+  Closing();
+  EXPECT_TRUE(request1_.finished());
+  EXPECT_FALSE(request2_.finished());
+  EXPECT_FALSE(iframe_request_other_domain_.finished());
+  EXPECT_TRUE(view_.shown_);
+  Closing();
+  EXPECT_TRUE(request2_.finished());
+  EXPECT_FALSE(iframe_request_other_domain_.finished());
+  Closing();
+  EXPECT_TRUE(iframe_request_other_domain_.finished());
+  EXPECT_FALSE(view_.shown_);
+}