Revert of [WebsiteSettings] Update permission bubble manager policy...

Revert of [WebsiteSettings] Update permission bubble manager policy (https://codereview.chromium.org/243543003/)

Reason for revert:
This breaks Linux ASAN, heap use after free:
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Tests%20%283%29/builds/2360

Original issue's description:
> [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

TBR=leng@chromium.org,markusheintz@chromium.org,rdsmith@chromium.org,phajdan.jr@chromium.org,gbillock@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=332115,364159

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

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

chrome/browser/download/download_request_limiter_unittest.cc

@@ -71,6 +71,7 @@ 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,10 +7,17 @@
 #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(
@@ -22,8 +29,12 @@ PermissionBubbleManager::PermissionBubbleManager(
   : content::WebContentsObserver(web_contents),
     bubble_showing_(false),
     view_(NULL),
-    request_url_has_loaded_(false),
-    customization_mode_(false) {}
+    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)
@@ -86,8 +97,9 @@ void PermissionBubbleManager::AddRequest(PermissionBubbleRequest* request) {
   // TODO(gbillock): do we need to make default state a request property?
   accept_states_.push_back(true);
 
-  if (request->HasUserGesture())
-    ShowBubble();
+  // Start the timer when there is both a view and a request.
+  if (view_ && !timer_->IsRunning())
+    timer_->Reset();
 }
 
 void PermissionBubbleManager::CancelRequest(PermissionBubbleRequest* request) {
@@ -111,21 +123,22 @@ void PermissionBubbleManager::SetView(PermissionBubbleView* view) {
   else
     return;
 
-  ShowBubble();
+  // 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();
 }
 
-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, base::Unretained(this)));
+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::NavigationEntryCommitted(
@@ -199,8 +212,7 @@ void PermissionBubbleManager::Closing() {
 }
 
 void PermissionBubbleManager::ShowBubble() {
-  if (view_ && !bubble_showing_ && request_url_has_loaded_ &&
-      requests_.size()) {
+  if (view_ && !bubble_showing_ && 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;
@@ -225,7 +237,9 @@ void PermissionBubbleManager::FinalizeBubble() {
     requests_ = queued_requests_;
     accept_states_.resize(requests_.size(), true);
     queued_requests_.clear();
-    ShowBubble();
+    // 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();
   } else {
     request_url_ = GURL();
   }
@@ -239,3 +253,10 @@ 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,6 +7,7 @@
 
 #include <vector>
 
+#include "base/timer/timer.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"
@@ -55,6 +56,10 @@ 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;
@@ -62,12 +67,15 @@ 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(int32 page_id) OVERRIDE;
-
+  // 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;
   // If a page on which permissions requests are pending is navigated,
   // they will be finalized as if canceled by the user.
   virtual void NavigationEntryCommitted(
@@ -82,6 +90,7 @@ 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.
@@ -101,9 +110,10 @@ 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_;
 };
 
 #endif  // CHROME_BROWSER_UI_WEBSITE_SETTINGS_PERMISSION_BUBBLE_MANAGER_H_

chrome/browser/ui/website_settings/permission_bubble_manager_unittest.cc

@@ -70,6 +70,7 @@ class PermissionBubbleManagerTest : public ChromeRenderViewHostTestHarness {
     SetContents(CreateTestWebContents());
 
     manager_.reset(new PermissionBubbleManager(web_contents()));
+    manager_->SetCoalesceIntervalForTesting(0);
   }
 
   virtual void TearDown() OVERRIDE {
@@ -86,7 +87,6 @@ class PermissionBubbleManagerTest : public ChromeRenderViewHostTestHarness {
   }
 
   void WaitForCoalescing() {
-    manager_->DocumentOnLoadCompletedInMainFrame(0);
     base::MessageLoop::current()->RunUntilIdle();
   }
 
@@ -237,8 +237,9 @@ TEST_F(PermissionBubbleManagerTest, TwoRequestsShownInTwoBubbles) {
 
   view_.Hide();
   Accept();
-  WaitForCoalescing();
+  EXPECT_FALSE(view_.shown_);
 
+  WaitForCoalescing();
   EXPECT_TRUE(view_.shown_);
   ASSERT_EQ(1u, view_.permission_requests_.size());
   EXPECT_EQ(&request2_, view_.permission_requests_[0]);
@@ -325,8 +326,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());
 }