Update DownloadRequestLimiter tests for the permission bubble

chrome/browser/download/download_request_limiter_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/frame_navigate_params.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using content::WebContents;
@@ skipping 39 matching lines @@
     CANCEL,
     WAIT
   };
 
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
     InfoBarService::CreateForWebContents(web_contents());
 
     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;
     download_request_limiter_ = new DownloadRequestLimiter();
     fake_create_callback_ = base::Bind(
         &DownloadRequestLimiterTest::FakeCreate, base::Unretained(this));
     DownloadRequestInfoBarDelegate::SetCallbackForTesting(
         &fake_create_callback_);
     content_settings_ = new HostContentSettingsMap(profile_.GetPrefs(), false);
     DownloadRequestLimiter::SetContentSettingsForTesting(
@@ skipping 91 matching lines @@
 
   void SetHostContentSetting(WebContents* contents, ContentSetting setting) {
     content_settings_->SetContentSetting(
         ContentSettingsPattern::FromURL(contents->GetURL()),
         ContentSettingsPattern::Wildcard(),
         CONTENT_SETTINGS_TYPE_AUTOMATIC_DOWNLOADS,
         std::string(),
         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.
   TestingAction testing_action_;
 
   // Number of times ContinueDownload was invoked.
   int continue_count_;
 
   // Number of times CancelDownload was invoked.
   int cancel_count_;
@@ skipping 19 matching lines @@
     delegate_->Accept();
   } else if (action == DownloadRequestLimiterTest::CANCEL) {
     delegate_->Deny();
   } else if (action == DownloadRequestLimiterTest::WAIT) {
     // do nothing.
   } else {
     delegate_->Closing();
   }
 }
 
-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()));
 
   // Ask if the tab can do a download. This moves to PROMPT_BEFORE_DOWNLOAD.
   CanDownload();
   ASSERT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD,
             download_request_limiter_->GetDownloadStatus(web_contents()));
   // We should have been told we can download.
   ExpectAndResetCounts(1, 0, 0, __LINE__);
 
   // Ask again. This triggers asking the delegate for allow/disallow.
   testing_action_ = ACCEPT;
   CanDownload();
   // This should ask us if the download is allowed.
   // We should have been told we can download.
   ExpectAndResetCounts(1, 0, 1, __LINE__);
   ASSERT_EQ(DownloadRequestLimiter::ALLOW_ALL_DOWNLOADS,
             download_request_limiter_->GetDownloadStatus(web_contents()));
 
   // Ask again and make sure continue is invoked.
   CanDownload();
   // The state is at allow_all, which means the delegate shouldn't be asked.
   // We should have been told we can download.
   ExpectAndResetCounts(1, 0, 0, __LINE__);
   ASSERT_EQ(DownloadRequestLimiter::ALLOW_ALL_DOWNLOADS,
             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();
   ExpectAndResetCounts(1, 0, 0, __LINE__);
   ASSERT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD,
             download_request_limiter_->GetDownloadStatus(web_contents()));
 
   testing_action_ = ACCEPT;
   CanDownload();
   ExpectAndResetCounts(1, 0, 1, __LINE__);
   ASSERT_EQ(DownloadRequestLimiter::ALLOW_ALL_DOWNLOADS,
             download_request_limiter_->GetDownloadStatus(web_contents()));
 
   // 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,
             download_request_limiter_->GetDownloadStatus(web_contents()));
 
   // Do a user gesture, because we're at allow all, this shouldn't change the
   // state.
   OnUserGesture();
   ASSERT_EQ(DownloadRequestLimiter::ALLOW_ALL_DOWNLOADS,
             download_request_limiter_->GetDownloadStatus(web_contents()));
 
   // 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()));
 
   // Do two downloads, allowing the second so that we end up with allow all.
   CanDownload();
   ExpectAndResetCounts(1, 0, 0, __LINE__);
   ASSERT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD,
             download_request_limiter_->GetDownloadStatus(web_contents()));
 
   testing_action_ = CANCEL;
   CanDownload();
   ExpectAndResetCounts(0, 1, 1, __LINE__);
   ASSERT_EQ(DownloadRequestLimiter::DOWNLOADS_NOT_ALLOWED,
             download_request_limiter_->GetDownloadStatus(web_contents()));
 
   // 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();
   ExpectAndResetCounts(1, 0, 0, __LINE__);
   ASSERT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD,
             download_request_limiter_->GetDownloadStatus(web_contents()));
 
   // Do a user gesture, which should reset back to allow one.
   OnUserGesture();
   ASSERT_EQ(DownloadRequestLimiter::ALLOW_ONE_DOWNLOAD,
@@ skipping 17 matching lines @@
   ASSERT_EQ(DownloadRequestLimiter::DOWNLOADS_NOT_ALLOWED,
             download_request_limiter_->GetDownloadStatus(web_contents()));
   // And make sure we really can't download.
   CanDownload();
   ExpectAndResetCounts(0, 1, 0, __LINE__);
   // And the state shouldn't have changed.
   ASSERT_EQ(DownloadRequestLimiter::DOWNLOADS_NOT_ALLOWED,
             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()));
 
   // If the user refreshes the page without responding to the infobar, pretend
   // like the refresh is the initial load: they get 1 free download (probably
   // the same as the actual initial load), then an infobar.
   testing_action_ = WAIT;
 
   CanDownload();
   ExpectAndResetCounts(1, 0, 0, __LINE__);
@@ skipping 25 matching lines @@
   DidNavigateMainFrame();
   base::RunLoop().RunUntilIdle();
   ASSERT_EQ(DownloadRequestLimiter::DOWNLOADS_NOT_ALLOWED,
             download_request_limiter_->GetDownloadStatus(web_contents()));
   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_RawWebContents) {
   scoped_ptr<WebContents> web_contents(CreateTestWebContents());
 
   // DownloadRequestLimiter won't try to make a permission bubble if there's
   // no permission bubble manager, so don't put one on the test WebContents.
 
   // DownloadRequestLimiter won't try to make an infobar if it doesn't have an
   // InfoBarService, and we want to test that it will Cancel() instead of
   // prompting when it doesn't have a InfoBarService, so unset the delegate.
   UnsetDelegate();
@@ skipping 18 matching lines @@
             download_request_limiter_->GetDownloadStatus(web_contents.get()));
   OnUserGestureFor(web_contents.get());
   EXPECT_EQ(DownloadRequestLimiter::ALLOW_ONE_DOWNLOAD,
             download_request_limiter_->GetDownloadStatus(web_contents.get()));
   CanDownloadFor(web_contents.get());
   ExpectAndResetCounts(1, 0, 0, __LINE__);
   EXPECT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD,
             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();
   ExpectAndResetCounts(1, 0, 0, __LINE__);
   ASSERT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD,
             download_request_limiter_->GetDownloadStatus(web_contents()));
 
   CanDownload();
   ExpectAndResetCounts(1, 0, 0, __LINE__);
   ASSERT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD,
             download_request_limiter_->GetDownloadStatus(web_contents()));
 
   SetHostContentSetting(web_contents(), CONTENT_SETTING_BLOCK);
 
   CanDownload();
   ExpectAndResetCounts(0, 1, 0, __LINE__);
   ASSERT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD,
             download_request_limiter_->GetDownloadStatus(web_contents()));
 
   CanDownload();
   ExpectAndResetCounts(0, 1, 0, __LINE__);
   ASSERT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD,
             download_request_limiter_->GetDownloadStatus(web_contents()));
 }
+
+INSTANTIATE_TEST_CASE_P(DownloadRequestLimiterTestsWithAndWithoutBubbles,
+                        DownloadRequestLimiterParamTests,
+                        ::testing::Values(false, true));