From 9165ea2a6ef876c31f81fc2697f2c8adb047ada2 Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Mon, 12 Aug 2019 11:30:07 -0700 Subject: [PATCH 1/4] Default shield advanced view to TRUE for existing users. Fixes https://github.com/brave/brave-browser/issues/5582 --- browser/brave_profile_prefs.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/browser/brave_profile_prefs.cc b/browser/brave_profile_prefs.cc index 449a64fee6b7..9b887b1d4660 100644 --- a/browser/brave_profile_prefs.cc +++ b/browser/brave_profile_prefs.cc @@ -8,6 +8,7 @@ #include "brave/common/pref_names.h" #include "brave/components/brave_shields/browser/brave_shields_web_contents_observer.h" #include "brave/components/brave_webtorrent/browser/buildflags/buildflags.h" +#include "chrome/browser/first_run/first_run.h" #include "chrome/browser/net/prediction_options.h" #include "chrome/browser/prefs/session_startup_pref.h" #include "chrome/common/pref_names.h" @@ -50,7 +51,10 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) { registry->RegisterBooleanPref(kHTTPSEVerywhereControlType, true); registry->RegisterBooleanPref(kNoScriptControlType, false); registry->RegisterBooleanPref(kAdControlType, true); - registry->RegisterBooleanPref(kShieldsAdvancedViewEnabled, false); + // > advanced view is defaulted to true for EXISTING users; false for new + bool is_new_user = first_run::IsChromeFirstRun(); + registry->RegisterBooleanPref(kShieldsAdvancedViewEnabled, + is_new_user == false); registry->RegisterBooleanPref(kGoogleLoginControlType, true); registry->RegisterBooleanPref(kFBEmbedControlType, true); registry->RegisterBooleanPref(kTwitterEmbedControlType, true); From cebb09e795d1c6a0ca433a43af925cd4b9dafd53 Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Tue, 13 Aug 2019 15:19:18 -0700 Subject: [PATCH 2/4] Added tests for when advanced shields setting should be true (existing users) and false (first run) Pulls in some re-usable code from chrome/browser/first_run/first_run_browsertest.cc --- browser/brave_first_run_browsertest.cc | 44 ++++++++++ browser/brave_first_run_browsertest.h | 99 ++++++++++++++++++++++ browser/brave_profile_prefs_browsertest.cc | 23 ++++- test/BUILD.gn | 2 + 4 files changed, 165 insertions(+), 3 deletions(-) create mode 100644 browser/brave_first_run_browsertest.cc create mode 100644 browser/brave_first_run_browsertest.h diff --git a/browser/brave_first_run_browsertest.cc b/browser/brave_first_run_browsertest.cc new file mode 100644 index 000000000000..c21d3fd64ca6 --- /dev/null +++ b/browser/brave_first_run_browsertest.cc @@ -0,0 +1,44 @@ +/* Copyright (c) 2019 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#include "brave/browser/brave_first_run_browsertest.h" + +FirstRunMasterPrefsBrowserTestBase::FirstRunMasterPrefsBrowserTestBase() {} + +FirstRunMasterPrefsBrowserTestBase::~FirstRunMasterPrefsBrowserTestBase() {} + +void FirstRunMasterPrefsBrowserTestBase::SetUp() { + // All users of this test class need to call SetMasterPreferencesForTest() + // before this class' SetUp() is invoked. + ASSERT_TRUE(text_.get()); + + ASSERT_TRUE(base::CreateTemporaryFile(&prefs_file_)); + EXPECT_EQ(static_cast(text_->size()), + base::WriteFile(prefs_file_, text_->c_str(), text_->size())); + first_run::SetMasterPrefsPathForTesting(prefs_file_); + + // This invokes BrowserMain, and does the import, so must be done last. + InProcessBrowserTest::SetUp(); +} + +void FirstRunMasterPrefsBrowserTestBase::TearDown() { + EXPECT_TRUE(base::DeleteFile(prefs_file_, false)); + InProcessBrowserTest::TearDown(); +} + +void FirstRunMasterPrefsBrowserTestBase::SetUpCommandLine(base::CommandLine* command_line) { + command_line->AppendSwitch(switches::kForceFirstRun); + EXPECT_EQ(first_run::AUTO_IMPORT_NONE, first_run::auto_import_state()); + + extensions::ComponentLoader::EnableBackgroundExtensionsForTesting(); +} + +#if defined(OS_MACOSX) || defined(OS_LINUX) +void FirstRunMasterPrefsBrowserTestBase::SetUpInProcessBrowserTestFixture() { + InProcessBrowserTest::SetUpInProcessBrowserTestFixture(); + // Suppress first run dialog since it blocks test progress. + first_run::internal::ForceFirstRunDialogShownForTesting(false); +} +#endif diff --git a/browser/brave_first_run_browsertest.h b/browser/brave_first_run_browsertest.h new file mode 100644 index 000000000000..99533ce28cfb --- /dev/null +++ b/browser/brave_first_run_browsertest.h @@ -0,0 +1,99 @@ +/* Copyright (c) 2019 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#ifndef BRAVE_BROWSER_BRAVE_FIRST_RUN_BROWSERTEST_H_ +#define BRAVE_BROWSER_BRAVE_FIRST_RUN_BROWSERTEST_H_ + +// Copied from chrome/browser/first_run/first_run_browsertest.cc +// See browser/brave_profile_prefs_browsertest.cc for an example +// Useful for verifying first-run functionality. + +#include + +#include "base/base_switches.h" +#include "base/command_line.h" +#include "base/files/file_path.h" +#include "base/macros.h" +#include "base/memory/ref_counted.h" +#include "base/run_loop.h" +#include "base/strings/string_util.h" +#include "base/strings/utf_string_conversions.h" +#include "base/test/metrics/histogram_tester.h" +#include "build/build_config.h" +#include "chrome/browser/browser_process.h" +#include "chrome/browser/extensions/component_loader.h" +#include "chrome/browser/first_run/first_run.h" +#include "chrome/browser/first_run/first_run_internal.h" +#include "chrome/browser/importer/importer_list.h" +#include "chrome/browser/prefs/chrome_pref_service_factory.h" +#include "chrome/browser/profiles/profile_manager.h" +#include "chrome/browser/ui/browser.h" +#include "chrome/browser/ui/tabs/tab_strip_model.h" +#include "chrome/common/chrome_switches.h" +#include "chrome/common/pref_names.h" +#include "chrome/common/url_constants.h" +#include "chrome/test/base/in_process_browser_test.h" +#include "chrome/test/base/ui_test_utils.h" +#include "components/prefs/pref_service.h" +#include "components/user_prefs/user_prefs.h" +#include "components/variations/metrics.h" +#include "components/variations/pref_names.h" +#include "components/variations/variations_switches.h" +#include "content/public/browser/web_contents.h" +#include "content/public/common/content_switches.h" +#include "content/public/test/browser_test_utils.h" +#include "content/public/test/test_launcher.h" +#include "testing/gtest/include/gtest/gtest.h" + +typedef InProcessBrowserTest FirstRunBrowserTest; + +// A generic test class to be subclassed by test classes testing specific +// master_preferences. All subclasses must call SetMasterPreferencesForTest() +// from their SetUp() method before deferring the remainder of Setup() to this +// class. +class FirstRunMasterPrefsBrowserTestBase : public InProcessBrowserTest { + public: + FirstRunMasterPrefsBrowserTestBase(); + ~FirstRunMasterPrefsBrowserTestBase() override; + + protected: + void SetUp() override; + + void TearDown() override; + + void SetUpCommandLine(base::CommandLine* command_line) override; + +#if defined(OS_MACOSX) || defined(OS_LINUX) + void SetUpInProcessBrowserTestFixture() override; +#endif + + void SetMasterPreferencesForTest(const char text[]) { + text_.reset(new std::string(text)); + } + + private: + base::FilePath prefs_file_; + std::unique_ptr text_; + + DISALLOW_COPY_AND_ASSIGN(FirstRunMasterPrefsBrowserTestBase); +}; + +template +class FirstRunMasterPrefsBrowserTestT + : public FirstRunMasterPrefsBrowserTestBase { + public: + FirstRunMasterPrefsBrowserTestT() {} + + protected: + void SetUp() override { + SetMasterPreferencesForTest(Text); + FirstRunMasterPrefsBrowserTestBase::SetUp(); + } + + private: + DISALLOW_COPY_AND_ASSIGN(FirstRunMasterPrefsBrowserTestT); +}; + +#endif // BRAVE_BROWSER_BRAVE_FIRST_RUN_BROWSERTEST_H_ diff --git a/browser/brave_profile_prefs_browsertest.cc b/browser/brave_profile_prefs_browsertest.cc index e1c917f56df3..06cf64e0b989 100644 --- a/browser/brave_profile_prefs_browsertest.cc +++ b/browser/brave_profile_prefs_browsertest.cc @@ -13,6 +13,7 @@ #include "components/safe_browsing/common/safe_browsing_prefs.h" #include "components/spellcheck/browser/pref_names.h" #include "components/sync/base/pref_names.h" +#include "brave/browser/brave_first_run_browsertest.h" using BraveProfilePrefsBrowserTest = InProcessBrowserTest; @@ -29,9 +30,6 @@ IN_PROC_BROWSER_TEST_F(BraveProfilePrefsBrowserTest, MiscBravePrefs) { kHTTPSEVerywhereControlType)); EXPECT_FALSE( browser()->profile()->GetPrefs()->GetBoolean(kNoScriptControlType)); - EXPECT_FALSE( - browser()->profile()->GetPrefs()->GetBoolean( - kShieldsAdvancedViewEnabled)); EXPECT_TRUE( browser()->profile()->GetPrefs()->GetBoolean(kAdControlType)); EXPECT_TRUE( @@ -52,6 +50,25 @@ IN_PROC_BROWSER_TEST_F(BraveProfilePrefsBrowserTest, MiscBravePrefs) { browser()->profile()->GetPrefs()->GetBoolean(kIPFSCompanionEnabled)); } +// First run of Brave should default Shields to Simple view +const char kFirstRunEmptyPrefs[] = "{}"; +typedef FirstRunMasterPrefsBrowserTestT + BraveProfilePrefsFirstRunBrowserTest; +IN_PROC_BROWSER_TEST_F(BraveProfilePrefsFirstRunBrowserTest, + AdvancedShieldsNewUserValue) { + EXPECT_FALSE( + browser()->profile()->GetPrefs()->GetBoolean( + kShieldsAdvancedViewEnabled)); +} + +// Existing Brave users should default shields to Advanced view +IN_PROC_BROWSER_TEST_F(BraveProfilePrefsBrowserTest, AdvancedShieldsExistingUserValue) { + first_run::CreateSentinelIfNeeded(); + EXPECT_TRUE( + browser()->profile()->GetPrefs()->GetBoolean( + kShieldsAdvancedViewEnabled)); +} + IN_PROC_BROWSER_TEST_F(BraveProfilePrefsBrowserTest, DisableGoogleServicesByDefault) { EXPECT_FALSE(browser()->profile()->GetPrefs()->GetBoolean( diff --git a/test/BUILD.gn b/test/BUILD.gn index cce1c49fc414..e350e313935b 100644 --- a/test/BUILD.gn +++ b/test/BUILD.gn @@ -373,6 +373,8 @@ test("brave_browser_tests") { "//brave/browser/brave_content_browser_client_browsertest.cc", "//brave/browser/brave_features_browsertest.cc", "//brave/browser/brave_profile_prefs_browsertest.cc", + "//brave/browser/brave_first_run_browsertest.h", + "//brave/browser/brave_first_run_browsertest.cc", "//brave/browser/brave_resources_browsertest.cc", "//brave/browser/brave_stats_updater_browsertest.cc", "//brave/browser/browsing_data/brave_clear_browsing_data_browsertest.cc", From 890bee9a18bbbf958c65cf9365149a5f87d2d24a Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Tue, 13 Aug 2019 21:29:18 -0700 Subject: [PATCH 3/4] Fix lint errors --- browser/brave_first_run_browsertest.cc | 3 ++- browser/brave_first_run_browsertest.h | 1 + browser/brave_profile_prefs_browsertest.cc | 3 ++- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/browser/brave_first_run_browsertest.cc b/browser/brave_first_run_browsertest.cc index c21d3fd64ca6..ac89a70aa246 100644 --- a/browser/brave_first_run_browsertest.cc +++ b/browser/brave_first_run_browsertest.cc @@ -28,7 +28,8 @@ void FirstRunMasterPrefsBrowserTestBase::TearDown() { InProcessBrowserTest::TearDown(); } -void FirstRunMasterPrefsBrowserTestBase::SetUpCommandLine(base::CommandLine* command_line) { +void FirstRunMasterPrefsBrowserTestBase::SetUpCommandLine( + base::CommandLine* command_line) { command_line->AppendSwitch(switches::kForceFirstRun); EXPECT_EQ(first_run::AUTO_IMPORT_NONE, first_run::auto_import_state()); diff --git a/browser/brave_first_run_browsertest.h b/browser/brave_first_run_browsertest.h index 99533ce28cfb..77e83edcf9ec 100644 --- a/browser/brave_first_run_browsertest.h +++ b/browser/brave_first_run_browsertest.h @@ -10,6 +10,7 @@ // See browser/brave_profile_prefs_browsertest.cc for an example // Useful for verifying first-run functionality. +#include #include #include "base/base_switches.h" diff --git a/browser/brave_profile_prefs_browsertest.cc b/browser/brave_profile_prefs_browsertest.cc index 06cf64e0b989..69f6cb6666dd 100644 --- a/browser/brave_profile_prefs_browsertest.cc +++ b/browser/brave_profile_prefs_browsertest.cc @@ -62,7 +62,8 @@ IN_PROC_BROWSER_TEST_F(BraveProfilePrefsFirstRunBrowserTest, } // Existing Brave users should default shields to Advanced view -IN_PROC_BROWSER_TEST_F(BraveProfilePrefsBrowserTest, AdvancedShieldsExistingUserValue) { +IN_PROC_BROWSER_TEST_F(BraveProfilePrefsBrowserTest, + AdvancedShieldsExistingUserValue) { first_run::CreateSentinelIfNeeded(); EXPECT_TRUE( browser()->profile()->GetPrefs()->GetBoolean( From fef2fbc5a8d8ab27e088238d0a91789118256725 Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Wed, 14 Aug 2019 17:25:10 -0700 Subject: [PATCH 4/4] Exclude android from shields advanced view This fixes error seen compiling for android core > 23:08:30 ld.lld: error: undefined symbol: first_run::IsChromeFirstRun() > 23:08:30 >>> referenced by brave_profile_prefs.cc:55 (../../brave/browser/brave_profile_prefs.cc:55) > 23:08:30 >>> thinlto-cache/Thin-471adf.tmp.o:(brave::RegisterProfilePrefs(user_prefs::PrefRegistrySyncable*)) --- browser/brave_profile_prefs.cc | 12 ++++++++++-- browser/brave_profile_prefs_browsertest.cc | 2 ++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/browser/brave_profile_prefs.cc b/browser/brave_profile_prefs.cc index 9b887b1d4660..c2b709c01eec 100644 --- a/browser/brave_profile_prefs.cc +++ b/browser/brave_profile_prefs.cc @@ -8,7 +8,6 @@ #include "brave/common/pref_names.h" #include "brave/components/brave_shields/browser/brave_shields_web_contents_observer.h" #include "brave/components/brave_webtorrent/browser/buildflags/buildflags.h" -#include "chrome/browser/first_run/first_run.h" #include "chrome/browser/net/prediction_options.h" #include "chrome/browser/prefs/session_startup_pref.h" #include "chrome/common/pref_names.h" @@ -29,6 +28,10 @@ #include "brave/components/brave_webtorrent/browser/webtorrent_util.h" #endif +#if !defined(OS_ANDROID) +#include "chrome/browser/first_run/first_run.h" +#endif + using extensions::FeatureSwitch; namespace brave { @@ -52,7 +55,12 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) { registry->RegisterBooleanPref(kNoScriptControlType, false); registry->RegisterBooleanPref(kAdControlType, true); // > advanced view is defaulted to true for EXISTING users; false for new - bool is_new_user = first_run::IsChromeFirstRun(); + bool is_new_user = false; + + #if !defined(OS_ANDROID) + is_new_user = first_run::IsChromeFirstRun(); + #endif + registry->RegisterBooleanPref(kShieldsAdvancedViewEnabled, is_new_user == false); registry->RegisterBooleanPref(kGoogleLoginControlType, true); diff --git a/browser/brave_profile_prefs_browsertest.cc b/browser/brave_profile_prefs_browsertest.cc index 69f6cb6666dd..c7c9db1dfb4c 100644 --- a/browser/brave_profile_prefs_browsertest.cc +++ b/browser/brave_profile_prefs_browsertest.cc @@ -51,6 +51,7 @@ IN_PROC_BROWSER_TEST_F(BraveProfilePrefsBrowserTest, MiscBravePrefs) { } // First run of Brave should default Shields to Simple view +#if !defined(OS_ANDROID) const char kFirstRunEmptyPrefs[] = "{}"; typedef FirstRunMasterPrefsBrowserTestT BraveProfilePrefsFirstRunBrowserTest; @@ -69,6 +70,7 @@ IN_PROC_BROWSER_TEST_F(BraveProfilePrefsBrowserTest, browser()->profile()->GetPrefs()->GetBoolean( kShieldsAdvancedViewEnabled)); } +#endif IN_PROC_BROWSER_TEST_F(BraveProfilePrefsBrowserTest, DisableGoogleServicesByDefault) {