Add browsertest for permission action reporting

chrome/browser/safe_browsing/permission_reporter_browsertest.cc

Index: chrome/browser/safe_browsing/permission_reporter_browsertest.cc
diff --git a/chrome/browser/safe_browsing/permission_reporter_browsertest.cc b/chrome/browser/safe_browsing/permission_reporter_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..dcf6c8eb85201e885687754401d9d3a74c1a6c6e
--- /dev/null
+++ b/chrome/browser/safe_browsing/permission_reporter_browsertest.cc
@@ -0,0 +1,212 @@
+// Copyright 2016 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/safe_browsing/permission_reporter.h"
+
+#include "base/command_line.h"
+#include "base/memory/ptr_util.h"
+#include "base/memory/ref_counted.h"
+#include "base/test/simple_test_clock.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/safe_browsing/ping_manager.h"
+#include "chrome/browser/safe_browsing/safe_browsing_service.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/common/safe_browsing/permission_report.pb.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/browser_thread.h"
+#include "net/url_request/report_sender.h"
+
+#if !defined(OS_ANDROID)

[kcarattini, 2016/07/20 03:59:22]

With all these directives it might make sense to have separate tests entirely
for Android and ChromeOS. What do you think?

[stefanocs, 2016/07/20 04:47:26]

I think there would be too much intersecting code if we separate the tests.
Looking at the other browsertests, I think using directives is a common
approach.

+#include "chrome/browser/permissions/permission_request_manager.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/website_settings/mock_permission_bubble_factory.h"
+#endif
+
+#if !defined(OS_CHROMEOS)
+#include "chrome/browser/signin/signin_manager_factory.h"
+#include "components/signin/core/browser/signin_manager.h"
+#endif
+
+namespace safe_browsing {
+
+namespace {
+#if !defined(OS_CHROMEOS)
+constexpr char kTestingGaiaId[] = "gaia_id";
+constexpr char kTestingUsername[] = "fake_username";

[raymes, 2016/07/25 04:31:40]

nit: just move these down to the place where they are used

[stefanocs, 2016/07/25 12:44:05]

Done.

+#endif
+
+// A mock ReportSender that keeps track of the last report sent.
+class MockReportSender : public net::ReportSender {

[raymes, 2016/07/25 04:31:40]

Can we factor this out into a separate file? It looks like a duplicate of what's
in chrome/browser/safe_browsing/permission_reporter_unittest.cc

[stefanocs, 2016/07/25 12:44:05]

Where would be an appropriate place for this?
net/url_request/mock_report_sender.h?

[raymes, 2016/07/26 00:45:02]

chrome/browser/safe_browsing/ should be fine

[stefanocs, 2016/07/26 01:48:58]

Done.

+ public:
+  MockReportSender() : net::ReportSender(nullptr, DO_NOT_SEND_COOKIES) {
+    number_of_reports_sent_ = 0;
+  }
+
+  ~MockReportSender() override {}
+
+  void Send(const GURL& report_uri, const std::string& report) override {
+    number_of_reports_sent_++;
+    latest_report_uri_ = report_uri;
+    latest_report_ = report;
+  }
+
+  int number_of_reports_sent() { return number_of_reports_sent_; }
+
+  const GURL& latest_report_uri() { return latest_report_uri_; }
+
+  const std::string& latest_report() { return latest_report_; }
+
+ private:
+  int number_of_reports_sent_;
+  GURL latest_report_uri_;
+  std::string latest_report_;
+
+  DISALLOW_COPY_AND_ASSIGN(MockReportSender);
+};
+
+}  // namespace
+
+class PermissionReporterBrowserTest : public InProcessBrowserTest {

[raymes, 2016/07/25 04:31:40]

Can we have a base class PermissionReporterBrowserTest and then two separate
subclasses for android and desktop?

[stefanocs, 2016/07/25 12:44:05]

Done.

+ protected:
+  PermissionReporterBrowserTest() = default;
+  ~PermissionReporterBrowserTest() override = default;

[raymes, 2016/07/25 04:31:40]

nit: are these actually needed?

[stefanocs, 2016/07/25 12:44:05]

Done.

+
+  void SetUpOnMainThread() override {
+    InProcessBrowserTest::SetUpOnMainThread();
+
+    mock_report_sender_ = new MockReportSender;

[raymes, 2016/07/25 04:31:40]

nit: perhaps create this in AttachMockReportSenderOnIOThread so the lifetime is
more clear?

[stefanocs, 2016/07/25 12:44:05]

Done.

+
+    base::RunLoop run_loop;
+    content::BrowserThread::PostTaskAndReply(
+        content::BrowserThread::IO, FROM_HERE,
+        base::Bind(
+            &PermissionReporterBrowserTest::AttachMockReportSenderOnIOThread,
+            this,
+            make_scoped_refptr(g_browser_process->safe_browsing_service())),
+        run_loop.QuitClosure());
+    run_loop.Run();
+
+#if !defined(OS_ANDROID)
+    PermissionRequestManager* manager = GetPermissionRequestManager();
+    mock_permission_bubble_factory_.reset(
+        new MockPermissionBubbleFactory(manager));
+    manager->DisplayPendingRequests();
+#else
+    InfoBarService::CreateForWebContents(
+        browser()->tab_strip_model()->GetActiveWebContents());

[raymes, 2016/07/25 04:31:40]

Is this actually needed? I see it only in unittests.

[stefanocs, 2016/07/25 12:44:05]

I'm not sure. Actually I haven't tested this on Android since I have no way of
running it. Where should this file be added to so that it would run in Android
browsertest?

+#endif
+
+#if !defined(OS_CHROMEOS)
+    SigninManager* signin_manager =
+        SigninManagerFactory::GetForProfile(browser()->profile());
+    signin_manager->SetAuthenticatedAccountInfo(kTestingGaiaId,
+                                                kTestingUsername);
+#endif
+  }
+
+  void TearDownOnMainThread() override {
+    InProcessBrowserTest::TearDownOnMainThread();
+#if !defined(OS_ANDROID)
+    mock_permission_bubble_factory_.reset();
+#endif
+  }
+
+  void AttachMockReportSenderOnIOThread(
+      scoped_refptr<SafeBrowsingService> safe_browsing_service) {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+    safe_browsing_service->ping_manager()->permission_reporter_.reset(
+        new PermissionReporter(base::WrapUnique(mock_report_sender_),
+                               base::WrapUnique(new base::SimpleTestClock)));
+  }
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {

[raymes, 2016/07/25 04:31:40]

nit: move this up closer to TearDown

[stefanocs, 2016/07/25 12:44:05]

Done.

+    command_line->AppendSwitch(switches::kEnablePermissionActionReporting);
+  }
+
+#if !defined(OS_ANDROID)
+  PermissionRequestManager* GetPermissionRequestManager() {
+    return PermissionRequestManager::FromWebContents(
+        browser()->tab_strip_model()->GetActiveWebContents());
+  }
+
+  MockPermissionBubbleFactory* bubble_factory() {
+    return mock_permission_bubble_factory_.get();
+  }
+#else
+  InfoBarService* GetInfobarService() {
+    return InfoBarService::FromWebContents(
+        browser()->tab_strip_model()->GetActiveWebContents());
+  }
+#endif
+
+  void AcceptRequest() {
+#if !defined(OS_ANDROID)
+    GetPermissionRequestManager()->Accept();
+#else
+    infobars::InfoBar* infobar = GetInfobarService()->infobar_at(
+        GetInfobarService()->GetActiveEntryID());
+    ConfirmInfoBarDelegate* infobar_delegate =
+        infobar->delegate()->AsConfirmInfoBarDelegate();
+    infobar_delegate->Accept();
+#endif
+  }
+
+  MockReportSender* mock_report_sender() { return mock_report_sender_; }
+
+ private:
+#if !defined(OS_ANDROID)
+  std::unique_ptr<MockPermissionBubbleFactory> mock_permission_bubble_factory_;
+#endif
+
+  // Owned by permission reporter.
+  MockReportSender* mock_report_sender_;
+};
+
+// Test that permission action report will be sent if the user is opted into it.
+IN_PROC_BROWSER_TEST_F(PermissionReporterBrowserTest,
+                       PermissionActionReporting) {

[raymes, 2016/07/25 04:31:40]

We can have separate tests for android/desktop. A bit of duplicate code in test
bodies is quite common. If you need to you could extract it into helper
functions to check results/etc.

[stefanocs, 2016/07/25 12:44:05]

Done.

+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  ui_test_utils::NavigateToURLBlockUntilNavigationsComplete(
+      browser(), embedded_test_server()->GetURL("/permissions/request.html"),
+      1);
+
+#if !defined(OS_ANDROID)
+  bubble_factory()->WaitForPermissionBubble();
+  EXPECT_TRUE(bubble_factory()->is_visible());
+#else
+  EXPECT_EQ(1U, GetInfobarService()->infobar_count());
+#endif
+
+  AcceptRequest();
+
+#if !defined(OS_ANDROID)
+  EXPECT_FALSE(bubble_factory()->is_visible());
+#else
+  EXPECT_EQ(0U, GetInfobarService()->infobar_count());
+#endif
+
+  EXPECT_EQ(1, mock_report_sender()->number_of_reports_sent());
+
+  PermissionReport permission_report;
+  ASSERT_TRUE(
+      permission_report.ParseFromString(mock_report_sender()->latest_report()));
+  EXPECT_EQ(PermissionReport::GEOLOCATION, permission_report.permission());
+  EXPECT_EQ(PermissionReport::GRANTED, permission_report.action());
+  EXPECT_EQ(embedded_test_server()->base_url().spec(),
+            permission_report.origin());
+#if defined(OS_ANDROID)
+  EXPECT_EQ(PermissionReport::ANDROID_PLATFORM,
+            permission_report.platform_type());
+#elif defined(OS_MACOSX) || defined(OS_WIN) || defined(OS_CHROMEOS) || \
+    defined(OS_LINUX)
+  EXPECT_EQ(PermissionReport::DESKTOP_PLATFORM,
+            permission_report.platform_type());
+#endif
+}
+
+}  // namespace safe_browsing