Add hooks to permission layer for permission action reporting

chrome/browser/permissions/permission_uma_util_unittest.cc

+// 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/permissions/permission_uma_util.h"
+
+#include "base/command_line.h"
+#include "chrome/browser/signin/fake_signin_manager_builder.h"
+#include "chrome/browser/signin/signin_manager_factory.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/test/base/testing_profile.h"
+#include "components/browser_sync/browser/profile_sync_service.h"
+#include "components/browser_sync/common/browser_sync_switches.h"
+#include "components/prefs/pref_service.h"
+#include "components/signin/core/browser/fake_signin_manager.h"
+#include "components/sync_driver/glue/sync_backend_host_mock.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "sync/internal_api/public/base/model_type.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+const char* kTestingGaiaId = "gaia_id";
+const char* kTestingUsername = "fake_username";
+}  // namespace
+
+class PermissionUmaUtilTest : public testing::Test {
+ protected:
+  PermissionUmaUtilTest() : profile_(new TestingProfile()) {}

[raymes, 2016/06/22 02:11:13]

It would be better to separate each check (in the test below) into a separate
test function (each set of {} that you have is probably a good granularity).
That would allow clearer separation of state and of what is under test.

(note: I put the comment here so that you would see it before the other comments
and hopefully they make more sense)

[stefanocs, 2016/06/22 04:28:11]

Done.

+
+  static bool IsOptedInPermissionActionReporting(Profile* profile) {
+    return PermissionUmaUtil::IsOptedInPermissionActionReporting(profile);
+  }
+
+  bool IsOptedInPermissionActionReporting() {
+    return IsOptedInPermissionActionReporting(profile_.get());

[raymes, 2016/06/22 02:11:13]

nit: I would remove this and just always use the one above

[stefanocs, 2016/06/22 04:28:11]

Done.

+  }
+
+  void ResetCommandLine() {
+    base::CommandLine::Reset();
+    base::CommandLine::Init(0, NULL);
+  }

[raymes, 2016/06/22 02:11:13]

if you have a SetUp function, you could call this from there and reset the state
for each test. You could also reset the profile for each test to ensure we start
in a blank state.

[stefanocs, 2016/06/22 04:28:11]

Done.

+
+  void FakeSignIn() {
+    SigninManagerBase* signin_manager = static_cast<FakeSigninManager*>(
+        SigninManagerFactory::GetForProfile(profile()));
+    signin_manager->SetAuthenticatedAccountInfo(kTestingGaiaId,
+                                                kTestingUsername);
+    AttachFakeSyncBackend();
+  }
+
+  void SetSafeBrowsing(bool value) {

[raymes, 2016/06/22 02:11:13]

nit: value->enabled

[stefanocs, 2016/06/22 04:28:11]

Done.

+    PrefService* preferences = profile_->GetPrefs();
+    preferences->SetBoolean(prefs::kSafeBrowsingEnabled, value);
+  }
+
+  void ResetAndSetPermissionActionReporting(bool value) {
+    ResetCommandLine();
+    if (value)
+      base::CommandLine::ForCurrentProcess()->AppendSwitch(
+          switches::kEnablePermissionActionReporting);
+    else
+      base::CommandLine::ForCurrentProcess()->AppendSwitch(
+          switches::kDisablePermissionActionReporting);

[raymes, 2016/06/22 02:11:13]

nit: I would just call these directly from where they're needed and remove this
function

[stefanocs, 2016/06/22 04:28:11]

Done.

+  }
+
+  void SyncAllDataTypes() {
+    profile_sync_service()->OnUserChoseDatatypes(true,
+                                                 syncer::UserSelectableTypes());
+  }
+
+  void SyncAllDataTypesExcept(syncer::ModelType data_type) {
+    syncer::ModelTypeSet selectable_data_types = syncer::UserSelectableTypes();
+    selectable_data_types.Remove(data_type);
+    profile_sync_service()->OnUserChoseDatatypes(false, selectable_data_types);

[raymes, 2016/06/22 02:11:13]

We should talk to pavely to ensure this is the right way to do this for the
test.

[stefanocs, 2016/06/22 04:28:11]

Acknowledged.

+  }
+
+  Profile* incognito_profile() { return profile_->GetOffTheRecordProfile(); }

[raymes, 2016/06/22 02:11:13]

nit: I would just inline this into the callsites

[stefanocs, 2016/06/22 04:28:11]

Done.

+
+  Profile* profile() { return profile_.get(); }
+
+ private:
+  void AttachFakeSyncBackend() {
+    profile_sync_service()->backend_.reset(
+        new browser_sync::SyncBackendHostMock());
+    profile_sync_service()->backend_initialized_ = true;

[raymes, 2016/06/22 02:11:13]

nit: just inline this since it's only called from one place

[stefanocs, 2016/06/22 04:28:11]

Done.

+  }
+
+  ProfileSyncService* profile_sync_service() {

[raymes, 2016/06/22 02:11:13]

This should probably be GetProfileSyncService. We generally only use the
lowercase-underscore notation for getters of member variables

[stefanocs, 2016/06/22 04:28:11]

Done.

+    return ProfileSyncServiceFactory::GetForProfile(profile());
+  }
+
+  content::TestBrowserThreadBundle thread_bundle_;
+  std::unique_ptr<Profile> profile_;
+};
+
+TEST_F(PermissionUmaUtilTest, IsOptedInPermissionActionReporting) {
+  ResetAndSetPermissionActionReporting(true);
+  SetSafeBrowsing(true);
+
+  // Test can start sync before sign in check.
+  EXPECT_FALSE(IsOptedInPermissionActionReporting());
+
+  FakeSignIn();
+
+  EXPECT_TRUE(IsOptedInPermissionActionReporting());
+
+  // Test incognito check.
+  EXPECT_FALSE(IsOptedInPermissionActionReporting(incognito_profile()));
+
+  {
+    // Test permission action reporting flag check.
+    ResetCommandLine();
+    EXPECT_FALSE(IsOptedInPermissionActionReporting());
+
+    ResetAndSetPermissionActionReporting(false);
+    EXPECT_FALSE(IsOptedInPermissionActionReporting());
+
+    ResetAndSetPermissionActionReporting(true);
+    EXPECT_TRUE(IsOptedInPermissionActionReporting());
+  }
+
+  {
+    // Test safe browsing enabled check.
+    SetSafeBrowsing(false);
+    EXPECT_FALSE(IsOptedInPermissionActionReporting());
+
+    SetSafeBrowsing(true);
+    EXPECT_TRUE(IsOptedInPermissionActionReporting());
+  }
+
+  {
+    // Test can get profile sync service check.
+    base::CommandLine::ForCurrentProcess()->AppendSwitch(
+        switches::kDisableSync);
+    EXPECT_FALSE(IsOptedInPermissionActionReporting());
+
+    ResetAndSetPermissionActionReporting(true);
+    EXPECT_TRUE(IsOptedInPermissionActionReporting());
+  }
+
+  {
+    // Test Chrome Tab Sync enabled check.
+    SyncAllDataTypesExcept(syncer::PROXY_TABS);
+    EXPECT_FALSE(IsOptedInPermissionActionReporting());
+
+    SyncAllDataTypes();
+    EXPECT_TRUE(IsOptedInPermissionActionReporting());
+  }
+
+  {
+    // Test Chrome Pref Sync enabled check.
+    SyncAllDataTypesExcept(syncer::PREFERENCES);
+    EXPECT_FALSE(IsOptedInPermissionActionReporting());
+
+    SyncAllDataTypes();
+    EXPECT_TRUE(IsOptedInPermissionActionReporting());

[raymes, 2016/06/22 02:11:13]

nit: this is duplicated from above

[stefanocs, 2016/06/22 04:28:11]

Done.

+  }
+}