Update DownloadRequestLimiter tests for the permission bubble

chrome/browser/download/download_request_limiter_unittest.cc

Index: chrome/browser/download/download_request_limiter_unittest.cc
diff --git a/chrome/browser/download/download_request_limiter_unittest.cc b/chrome/browser/download/download_request_limiter_unittest.cc
index 33c3c21403714d7f80680a9e5a6163e2acbfd7e8..ac5d6f4546a0445cdea2a573e89ba2f3730a9cef 100644
--- a/chrome/browser/download/download_request_limiter_unittest.cc
+++ b/chrome/browser/download/download_request_limiter_unittest.cc
@@ -5,10 +5,13 @@
 #include "chrome/browser/download/download_request_limiter.h"
 
 #include "base/bind.h"
+#include "base/command_line.h"
 #include "base/run_loop.h"
+#include "chrome/browser/download/download_permission_request.h"
 #include "chrome/browser/download/download_request_infobar_delegate.h"
 #include "chrome/browser/infobars/infobar_service.h"
 #include "chrome/browser/ui/website_settings/permission_bubble_manager.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
@@ -68,9 +71,8 @@ class DownloadRequestLimiterTest : public ChromeRenderViewHostTestHarness {
 
     PermissionBubbleManager::CreateForWebContents(web_contents());
     view_.reset(new FakePermissionBubbleView(this));
-    PermissionBubbleManager* manager =
-      PermissionBubbleManager::FromWebContents(web_contents());
-    manager->SetView(view_.get());
+    PermissionBubbleManager::FromWebContents(web_contents())->
+        SetView(view_.get());
 
     testing_action_ = ACCEPT;
     ask_allow_count_ = cancel_count_ = continue_count_ = 0;
@@ -182,6 +184,11 @@ class DownloadRequestLimiterTest : public ChromeRenderViewHostTestHarness {
         setting);
   }
 
+  void BubbleManagerDocumentLoadCompleted() {
+    PermissionBubbleManager::FromWebContents(web_contents())->
+        DocumentOnLoadCompletedInMainFrame();

[asanka, 2014/12/23 20:44:11]

Curious: Why does this need to be called explicitly?

[felt, 2015/02/05 18:33:43]

There's a good comment here that explains what the bubble manager in the method:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

In the tests, that event will not fire on its own.

[asanka, 2015/02/05 19:44:08]

That comment is a bit scary because if there's any cross-thread asynchrony,
posting a task to the UI thread doesn't guarantee that all AddRequest() calls
have been seen by the time TriggerShowBubble() is called.
Got it. My confusion was why NavigateAndCommit() doesn't automatically result in
DocumentOnLoadCompleted being called. It makes sense now that I looked at how
that event is fired.

[felt, 2015/02/06 09:31:28]

OK. I will investigate. I suspect this is causing another bug that I'm seeing,
where sometimes requests are dropped. (I'm not the original author of this code
but I am going around trying to clean it up and make it nice.)

+  }
+
   scoped_refptr<DownloadRequestLimiter> download_request_limiter_;
 
   // The action that FakeCreate() should take.
@@ -221,8 +228,34 @@ void FakePermissionBubbleView::Show(
   }
 }
 
-TEST_F(DownloadRequestLimiterTest,
+class DownloadRequestLimiterParamTests
+    : public DownloadRequestLimiterTest,
+      public ::testing::WithParamInterface<bool> {
+ protected:
+  DownloadRequestLimiterParamTests() {}
+  ~DownloadRequestLimiterParamTests() override {}
+
+  void SetUp() override {
+    DownloadRequestLimiterTest::SetUp();
+#if !defined(OS_ANDROID)
+    if (GetParam()) {
+      base::CommandLine::ForCurrentProcess()->AppendSwitch(
+          switches::kEnablePermissionsBubbles);
+      EXPECT_TRUE(PermissionBubbleManager::Enabled());
+    } else {
+      base::CommandLine::ForCurrentProcess()->AppendSwitch(
+          switches::kDisablePermissionsBubbles);
+    }
+  }
+#endif

[asanka, 2014/12/23 20:44:11]

Mismatched } for OS_ANDROID.

[asanka, 2014/12/23 20:44:11]

Mismatched } for OS_ANDROID.

[felt, 2015/02/05 18:33:43]

Done.

+ private:
+  DISALLOW_COPY_AND_ASSIGN(DownloadRequestLimiterParamTests);
+};
+
+TEST_P(DownloadRequestLimiterParamTests,
        DownloadRequestLimiter_Allow) {
+  BubbleManagerDocumentLoadCompleted();
+
   // All tabs should initially start at ALLOW_ONE_DOWNLOAD.
   ASSERT_EQ(DownloadRequestLimiter::ALLOW_ONE_DOWNLOAD,
             download_request_limiter_->GetDownloadStatus(web_contents()));
@@ -252,9 +285,10 @@ TEST_F(DownloadRequestLimiterTest,
             download_request_limiter_->GetDownloadStatus(web_contents()));
 }
 
-TEST_F(DownloadRequestLimiterTest,
+TEST_P(DownloadRequestLimiterParamTests,
        DownloadRequestLimiter_ResetOnNavigation) {
   NavigateAndCommit(GURL("http://foo.com/bar"));
+  BubbleManagerDocumentLoadCompleted();
 
   // Do two downloads, allowing the second so that we end up with allow all.
   CanDownload();
@@ -271,6 +305,7 @@ TEST_F(DownloadRequestLimiterTest,
   // Navigate to a new URL with the same host, which shouldn't reset the allow
   // all state.
   NavigateAndCommit(GURL("http://foo.com/bar2"));
+  BubbleManagerDocumentLoadCompleted();
   CanDownload();
   ExpectAndResetCounts(1, 0, 0, __LINE__);
   ASSERT_EQ(DownloadRequestLimiter::ALLOW_ALL_DOWNLOADS,
@@ -284,6 +319,7 @@ TEST_F(DownloadRequestLimiterTest,
 
   // Navigate to a completely different host, which should reset the state.
   NavigateAndCommit(GURL("http://fooey.com"));
+  BubbleManagerDocumentLoadCompleted();
   ASSERT_EQ(DownloadRequestLimiter::ALLOW_ONE_DOWNLOAD,
             download_request_limiter_->GetDownloadStatus(web_contents()));
 
@@ -302,15 +338,17 @@ TEST_F(DownloadRequestLimiterTest,
   // Navigate to a new URL with the same host, which shouldn't reset the allow
   // all state.
   NavigateAndCommit(GURL("http://fooey.com/bar2"));
+  BubbleManagerDocumentLoadCompleted();
   CanDownload();
   ExpectAndResetCounts(0, 1, 0, __LINE__);
   ASSERT_EQ(DownloadRequestLimiter::DOWNLOADS_NOT_ALLOWED,
             download_request_limiter_->GetDownloadStatus(web_contents()));
 }
 
-TEST_F(DownloadRequestLimiterTest,
+TEST_P(DownloadRequestLimiterParamTests,
        DownloadRequestLimiter_ResetOnUserGesture) {
   NavigateAndCommit(GURL("http://foo.com/bar"));
+  BubbleManagerDocumentLoadCompleted();
 
   // Do one download, which should change to prompt before download.
   CanDownload();
@@ -348,9 +386,10 @@ TEST_F(DownloadRequestLimiterTest,
             download_request_limiter_->GetDownloadStatus(web_contents()));
 }
 
-TEST_F(DownloadRequestLimiterTest,
+TEST_P(DownloadRequestLimiterParamTests,
        DownloadRequestLimiter_ResetOnReload) {
   NavigateAndCommit(GURL("http://foo.com/bar"));
+  BubbleManagerDocumentLoadCompleted();
   ASSERT_EQ(DownloadRequestLimiter::ALLOW_ONE_DOWNLOAD,
             download_request_limiter_->GetDownloadStatus(web_contents()));
 
@@ -396,7 +435,7 @@ TEST_F(DownloadRequestLimiterTest,
             download_request_limiter_->GetDownloadStatus(web_contents()));
 }
 
-TEST_F(DownloadRequestLimiterTest,
+TEST_P(DownloadRequestLimiterParamTests,
        DownloadRequestLimiter_RawWebContents) {
   scoped_ptr<WebContents> web_contents(CreateTestWebContents());
 
@@ -435,9 +474,10 @@ TEST_F(DownloadRequestLimiterTest,
             download_request_limiter_->GetDownloadStatus(web_contents.get()));
 }
 
-TEST_F(DownloadRequestLimiterTest,
+TEST_P(DownloadRequestLimiterParamTests,
        DownloadRequestLimiter_SetHostContentSetting) {
   NavigateAndCommit(GURL("http://foo.com/bar"));
+  BubbleManagerDocumentLoadCompleted();
   SetHostContentSetting(web_contents(), CONTENT_SETTING_ALLOW);
 
   CanDownload();
@@ -462,3 +502,7 @@ TEST_F(DownloadRequestLimiterTest,
   ASSERT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD,
             download_request_limiter_->GetDownloadStatus(web_contents()));
 }
+
+INSTANTIATE_TEST_CASE_P(DownloadRequestLimiterTestsWithAndWithoutBubbles,
+                        DownloadRequestLimiterParamTests,
+                        ::testing::Values(false, true));