Updated sidebar show options based on channel

fix brave/brave-browser#21726 On Stable, sidebar is not visible by default. Otherwise, it's shown.
brave · Mar 17, 2022 · 5128ce2 · 5128ce2
1 parent 1bca6da
commit 5128ce2
Show file tree

Hide file tree

Showing 6 changed files with 42 additions and 9 deletions.
diff --git a/browser/brave_profile_prefs.cc b/browser/brave_profile_prefs.cc
@@ -403,7 +403,7 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
 #endif
 
 #if BUILDFLAG(ENABLE_SIDEBAR)
-  sidebar::SidebarService::RegisterProfilePrefs(registry);
+  sidebar::SidebarService::RegisterProfilePrefs(registry, chrome::GetChannel());
 #endif
 
 #if !defined(OS_ANDROID)

diff --git a/browser/ui/sidebar/sidebar_browsertest.cc b/browser/ui/sidebar/sidebar_browsertest.cc
@@ -32,6 +32,16 @@ class SidebarBrowserTest : public InProcessBrowserTest,
   SidebarBrowserTest() {}
   ~SidebarBrowserTest() override = default;
 
+  void PreRunTestOnMainThread() override {
+    InProcessBrowserTest::PreRunTestOnMainThread();
+
+    auto* service = SidebarServiceFactory::GetForProfile(browser()->profile());
+    // Enable sidebar explicitely because sidebar option is different based on
+    // channel.
+    service->SetSidebarShowOption(
+        SidebarService::ShowSidebarOption::kShowAlways);
+  }
+
   BraveBrowser* brave_browser() {
     return static_cast<BraveBrowser*>(browser());
   }

diff --git a/components/sidebar/BUILD.gn b/components/sidebar/BUILD.gn
@@ -20,6 +20,7 @@ static_library("sidebar") {
     "//brave/components/resources:strings",
     "//components/keyed_service/core",
     "//components/prefs",
+    "//components/version_info",
     "//ui/base",
     "//url",
   ]
@@ -36,6 +37,7 @@ source_set("unit_tests") {
       "//brave/components/sidebar",
       "//components/prefs",
       "//components/prefs:test_support",
+      "//components/version_info",
       "//content/public/browser",
       "//content/test:test_support",
       "//testing/gtest",

diff --git a/components/sidebar/sidebar_service.cc b/components/sidebar/sidebar_service.cc
@@ -19,6 +19,8 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 
+using version_info::Channel;
+
 namespace sidebar {
 
 namespace {
@@ -89,10 +91,14 @@ std::vector<SidebarItem> GetDefaultSidebarItems() {
 }  // namespace
 
 // static
-void SidebarService::RegisterProfilePrefs(PrefRegistrySimple* registry) {
+void SidebarService::RegisterProfilePrefs(PrefRegistrySimple* registry,
+                                          version_info::Channel channel) {
   registry->RegisterListPref(kSidebarItems);
   registry->RegisterIntegerPref(
-      kSidebarShowOption, static_cast<int>(ShowSidebarOption::kShowAlways));
+      kSidebarShowOption,
+      channel == Channel::STABLE
+          ? static_cast<int>(ShowSidebarOption::kShowNever)
+          : static_cast<int>(ShowSidebarOption::kShowAlways));
   registry->RegisterIntegerPref(kSidebarItemAddedFeedbackBubbleShowCount, 0);
 }
 

diff --git a/components/sidebar/sidebar_service.h b/components/sidebar/sidebar_service.h
@@ -14,6 +14,7 @@
 #include "brave/components/sidebar/sidebar_item.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/prefs/pref_change_registrar.h"
+#include "components/version_info/channel.h"
 
 class PrefRegistrySimple;
 class PrefService;
@@ -42,7 +43,8 @@ class SidebarService : public KeyedService {
     ~Observer() override = default;
   };
 
-  static void RegisterProfilePrefs(PrefRegistrySimple* registry);
+  static void RegisterProfilePrefs(PrefRegistrySimple* registry,
+                                   version_info::Channel channel);
 
   explicit SidebarService(PrefService* prefs);
   ~SidebarService() override;

diff --git a/components/sidebar/sidebar_service_unittest.cc b/components/sidebar/sidebar_service_unittest.cc
@@ -11,8 +11,11 @@
 #include "brave/components/sidebar/sidebar_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
 #include "components/prefs/testing_pref_service.h"
+#include "components/version_info/channel.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using version_info::Channel;
+
 namespace sidebar {
 
 class SidebarServiceTest : public testing::Test,
@@ -22,10 +25,6 @@ class SidebarServiceTest : public testing::Test,
 
   ~SidebarServiceTest() override = default;
 
-  void SetUp() override {
-    SidebarService::RegisterProfilePrefs(prefs_.registry());
-  }
-
   void TearDown() override { service_->RemoveObserver(this); }
 
   void InitService() {
@@ -74,6 +73,7 @@ class SidebarServiceTest : public testing::Test,
 };
 
 TEST_F(SidebarServiceTest, AddRemoveItems) {
+  SidebarService::RegisterProfilePrefs(prefs_.registry(), Channel::CANARY);
   InitService();
 
   // Check the default items count.
@@ -118,6 +118,7 @@ TEST_F(SidebarServiceTest, AddRemoveItems) {
 }
 
 TEST_F(SidebarServiceTest, MoveItem) {
+  SidebarService::RegisterProfilePrefs(prefs_.registry(), Channel::DEV);
   InitService();
 
   // Add one more item to test with 4 items.
@@ -149,6 +150,7 @@ TEST_F(SidebarServiceTest, MoveItem) {
 }
 
 TEST_F(SidebarServiceTest, BuiltInItemUpdateTestWithBuiltInItemTypeKey) {
+  SidebarService::RegisterProfilePrefs(prefs_.registry(), Channel::BETA);
   // Make prefs already have builtin items before service initialization.
   // And it has old url.
   {
@@ -185,6 +187,7 @@ TEST_F(SidebarServiceTest, BuiltInItemUpdateTestWithBuiltInItemTypeKey) {
 }
 
 TEST_F(SidebarServiceTest, BuiltInItemDoesntHaveHistoryItem) {
+  SidebarService::RegisterProfilePrefs(prefs_.registry(), Channel::BETA);
   // Make prefs already have builtin items before service initialization.
   // And it has history item.
   {
@@ -216,6 +219,7 @@ TEST_F(SidebarServiceTest, BuiltInItemDoesntHaveHistoryItem) {
 }
 
 TEST_F(SidebarServiceTest, BuiltInItemUpdateTestWithoutBuiltInItemTypeKey) {
+  SidebarService::RegisterProfilePrefs(prefs_.registry(), Channel::STABLE);
   // Prepare built-in item in prefs w/o setting BuiltInItemType.
   // If not stored, service uses url to get proper latest properties.
   {
@@ -241,6 +245,7 @@ TEST_F(SidebarServiceTest, BuiltInItemUpdateTestWithoutBuiltInItemTypeKey) {
 }
 
 TEST_F(SidebarServiceTest, SidebarShowOptionsDeprecationTest) {
+  SidebarService::RegisterProfilePrefs(prefs_.registry(), Channel::STABLE);
   // Show on click is deprecated.
   // Treat it as a show on mouse over.
   prefs_.SetInteger(
@@ -252,7 +257,15 @@ TEST_F(SidebarServiceTest, SidebarShowOptionsDeprecationTest) {
             service_->GetSidebarShowOption());
 }
 
-TEST_F(SidebarServiceTest, SidebarShowOptionsDefaultTest) {
+TEST_F(SidebarServiceTest, SidebarShowOptionsDefaultTestStable) {
+  SidebarService::RegisterProfilePrefs(prefs_.registry(), Channel::STABLE);
+  InitService();
+  EXPECT_EQ(SidebarService::ShowSidebarOption::kShowNever,
+            service_->GetSidebarShowOption());
+}
+
+TEST_F(SidebarServiceTest, SidebarShowOptionsDefaultTestNonStable) {
+  SidebarService::RegisterProfilePrefs(prefs_.registry(), Channel::BETA);
   InitService();
   EXPECT_EQ(SidebarService::ShowSidebarOption::kShowAlways,
             service_->GetSidebarShowOption());