[WebsiteSettings] Update permission bubble manager policy

This policy is more in accord with the design doc changes. Moves to
showing the dialog upon DOMContentLoaded in source pages, and then
subsequently for user gestures.

R=leng@chromium.org
BUG=332115,364159

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266855

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

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

chrome/browser/download/download_request_limiter_unittest.cc

@@ -71,7 +71,6 @@ class DownloadRequestLimiterTest : public ChromeRenderViewHostTestHarness {
     PermissionBubbleManager* manager =
       PermissionBubbleManager::FromWebContents(web_contents());
     manager->SetView(view_.get());
-    manager->SetCoalesceIntervalForTesting(0);
 
     testing_action_ = ACCEPT;
     ask_allow_count_ = cancel_count_ = continue_count_ = 0;

chrome/browser/ui/website_settings/permission_bubble_manager.cc

@@ -7,17 +7,10 @@
 #include "base/command_line.h"
 #include "chrome/browser/ui/website_settings/permission_bubble_request.h"
 #include "chrome/common/chrome_switches.h"
+#include "content/public/browser/browser_thread.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(
@@ -29,12 +22,9 @@ PermissionBubbleManager::PermissionBubbleManager(
   : 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));
-}
+    request_url_has_loaded_(false),
+    customization_mode_(false),
+    weak_factory_(this) {}
 
 PermissionBubbleManager::~PermissionBubbleManager() {
   if (view_ != NULL)
@@ -97,9 +87,8 @@ void PermissionBubbleManager::AddRequest(PermissionBubbleRequest* 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();
+  if (request->HasUserGesture())
+    ShowBubble();
 }
 
 void PermissionBubbleManager::CancelRequest(PermissionBubbleRequest* request) {
@@ -123,22 +112,22 @@ void PermissionBubbleManager::SetView(PermissionBubbleView* view) {
   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();
+  ShowBubble();
 }
 
-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();
+void PermissionBubbleManager::DocumentOnLoadCompletedInMainFrame(
+    int32 page_id) {
+  request_url_has_loaded_ = true;
+  // This is scheduled because while all calls to the browser have been
+  // issued at DOMContentLoaded, they may be bouncing around in scheduled
+  // 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()));
 }
 
 void PermissionBubbleManager::NavigationEntryCommitted(
@@ -212,7 +201,8 @@ void PermissionBubbleManager::Closing() {
 }
 
 void PermissionBubbleManager::ShowBubble() {
-  if (view_ && !bubble_showing_ && requests_.size()) {
+  if (view_ && !bubble_showing_ && request_url_has_loaded_ &&
+      requests_.size()) {
     // Note: this should appear above Show() for testing, since in that
     // case we may do in-line calling of finalization.
     bubble_showing_ = true;
@@ -237,9 +227,7 @@ void PermissionBubbleManager::FinalizeBubble() {
     requests_ = queued_requests_;
     accept_states_.resize(requests_.size(), true);
     queued_requests_.clear();
-    // TODO(leng):  Explore other options of showing the next bubble.  The
-    // advantage of this is that it uses the same code path as the first bubble.
-    timer_->Reset();
+    ShowBubble();
   } else {
     request_url_ = GURL();
   }
@@ -253,10 +241,3 @@ void PermissionBubbleManager::CancelPendingQueue() {
     (*requests_iter)->RequestFinished();
   }
 }
-
-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));
-}

chrome/browser/ui/website_settings/permission_bubble_manager.h

@@ -7,7 +7,7 @@
 
 #include <vector>
 
-#include "base/timer/timer.h"
+#include "base/memory/weak_ptr.h"
 #include "chrome/browser/ui/website_settings/permission_bubble_view.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
@@ -56,10 +56,6 @@ class PermissionBubbleManager
   // take ownership of the view.
   virtual void SetView(PermissionBubbleView* view) OVERRIDE;
 
- protected:
-  // Sets the coalesce time interval to |interval_ms|. For testing only.
-  void SetCoalesceIntervalForTesting(int interval_ms);
-
  private:
   friend class PermissionBubbleManagerTest;
   friend class DownloadRequestLimiterTest;
@@ -67,15 +63,12 @@ class PermissionBubbleManager
 
   explicit PermissionBubbleManager(content::WebContents* web_contents);
 
-  // contents::WebContentsObserver:
-  // TODO(leng):  Investigate the ordering and timing of page loading and
-  // permission requests with iFrames. DocumentOnLoadCompletedInMainFrame()
-  // and DocumentLoadedInFrame() might be needed as well.
-  virtual void DidFinishLoad(
-      int64 frame_id,
-      const GURL& validated_url,
-      bool is_main_frame,
-      content::RenderViewHost* render_view_host) OVERRIDE;
+  // WebContentsObserver:
+
+  // TODO(leng): Finalize policy for permission requests with iFrames.
+  // DocumentLoadedInFrame() might be needed as well.
+  virtual void DocumentOnLoadCompletedInMainFrame(int32 page_id) OVERRIDE;
+
   // If a page on which permissions requests are pending is navigated,
   // they will be finalized as if canceled by the user.
   virtual void NavigationEntryCommitted(
@@ -90,7 +83,6 @@ class PermissionBubbleManager
   virtual void Deny() OVERRIDE;
   virtual void Closing() OVERRIDE;
 
-  // Called when the coalescing timer is done. Presents the bubble.
   void ShowBubble();
 
   // Finalize the pending permissions request.
@@ -110,10 +102,11 @@ class PermissionBubbleManager
   std::vector<PermissionBubbleRequest*> requests_;
   std::vector<PermissionBubbleRequest*> queued_requests_;
   GURL request_url_;
+  bool request_url_has_loaded_;
   std::vector<bool> accept_states_;
   bool customization_mode_;
 
-  scoped_ptr<base::Timer> timer_;
+  base::WeakPtrFactory<PermissionBubbleManager> weak_factory_;
 };
 
 #endif  // CHROME_BROWSER_UI_WEBSITE_SETTINGS_PERMISSION_BUBBLE_MANAGER_H_

chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc

@@ -70,7 +70,6 @@ class PermissionBubbleManagerTest : public ChromeRenderViewHostTestHarness {
     SetContents(CreateTestWebContents());
 
     manager_.reset(new PermissionBubbleManager(web_contents()));
-    manager_->SetCoalesceIntervalForTesting(0);
   }
 
   virtual void TearDown() OVERRIDE {
@@ -87,6 +86,7 @@ class PermissionBubbleManagerTest : public ChromeRenderViewHostTestHarness {
   }
 
   void WaitForCoalescing() {
+    manager_->DocumentOnLoadCompletedInMainFrame(0);
     base::MessageLoop::current()->RunUntilIdle();
   }
 
@@ -237,9 +237,8 @@ TEST_F(PermissionBubbleManagerTest, TwoRequestsShownInTwoBubbles) {
 
   view_.Hide();
   Accept();
-  EXPECT_FALSE(view_.shown_);
-
   WaitForCoalescing();
+
   EXPECT_TRUE(view_.shown_);
   ASSERT_EQ(1u, view_.permission_requests_.size());
   EXPECT_EQ(&request2_, view_.permission_requests_[0]);
@@ -326,8 +325,8 @@ TEST_F(PermissionBubbleManagerTest, ForgetRequestsOnPageNavigation) {
   ASSERT_EQ(1u, view_.permission_requests_.size());
 
   NavigateAndCommit(GURL("http://www2.google.com/"));
+  WaitForCoalescing();
 
-  EXPECT_FALSE(view_.shown_);
   EXPECT_TRUE(request1_.finished());
   EXPECT_TRUE(request2_.finished());
 }