brave · simonhong · Aug 23, 2019 · Jul 5, 2019
@@ -241,6 +241,9 @@ By installing this extension, you are agreeing to the Google Widevine Terms of U
       <message name="IDS_SETTINGS_HIDE_BRAVE_REWARDS_BUTTON_DESC" desc="The description for settings switch controlling the visibility of Brave Rewards button">
         Hides the Brave Rewards button in the location bar when Brave Rewards is not enabled
       </message>
+      <message name="IDS_SETTINGS_ALWAYS_SHOW_BOOKMARK_BAR_ON_NTP" desc="The label for settings switch controlling the visibility of bookmarks bar on NTP">
+        Always show bookmarks bar on New Tab page
+      </message>
       <!-- Tor -->
       <message name="IDS_NEW_TOR_IDENTITY" desc="The text label of a menu item for requesting new Tor identity">
         New Tor Identity

diff --git a/browser/brave_tab_helpers.cc b/browser/brave_tab_helpers.cc
@@ -7,6 +7,7 @@
 
 #include "brave/browser/brave_drm_tab_helper.h"
 #include "brave/browser/greaselion/greaselion_tab_helper.h"
+#include "brave/browser/ui/bookmark/brave_bookmark_tab_helper.h"
 #include "brave/components/brave_ads/browser/ads_tab_helper.h"
 #include "brave/components/brave_rewards/browser/buildflags/buildflags.h"
 #include "brave/components/brave_shields/browser/buildflags/buildflags.h"  // For STP
@@ -36,6 +37,7 @@ void AttachTabHelpers(content::WebContents* web_contents) {
 #endif
   // Add tab helpers here unless they are intended for android too
   BraveDrmTabHelper::CreateForWebContents(web_contents);
+  BraveBookmarkTabHelper::CreateForWebContents(web_contents);
 #endif
 
 #if BUILDFLAG(BRAVE_STP_ENABLED)

diff --git a/browser/browser_context_keyed_service_factories.cc b/browser/browser_context_keyed_service_factories.cc
@@ -13,6 +13,10 @@
 #include "brave/components/brave_ads/browser/ads_service_factory.h"
 #include "brave/components/brave_rewards/browser/rewards_service_factory.h"
 
+#if !defined(OS_ANDROID)
+#include "brave/browser/ui/bookmark/bookmark_prefs_service_factory.h"
+#endif
+
 namespace brave {
 
 void EnsureBrowserContextKeyedServiceFactoriesBuilt() {
@@ -23,6 +27,10 @@ void EnsureBrowserContextKeyedServiceFactoriesBuilt() {
   greaselion::GreaselionServiceFactory::GetInstance();
   TorProfileServiceFactory::GetInstance();
   SearchEngineProviderServiceFactory::GetInstance();
+
+#if !defined(OS_ANDROID)
+  BookmarkPrefsServiceFactory::GetInstance();
+#endif
 }
 
 }  // namespace brave
diff --git a/browser/extensions/api/settings_private/brave_prefs_util.cc b/browser/extensions/api/settings_private/brave_prefs_util.cc
@@ -78,6 +78,8 @@ const PrefsUtil::TypedPrefMap& BravePrefsUtil::GetWhitelistedKeys() {
     settings_api::PrefType::PREF_TYPE_BOOLEAN;
   (*s_brave_whitelist)[browsing_data::prefs::kDeleteHostedAppsDataOnExit] =
     settings_api::PrefType::PREF_TYPE_BOOLEAN;
+  (*s_brave_whitelist)[kAlwaysShowBookmarkBarOnNTP] =
+      settings_api::PrefType::PREF_TYPE_BOOLEAN;
   // WebTorrent pref
   (*s_brave_whitelist)[kWebTorrentEnabled] =
       settings_api::PrefType::PREF_TYPE_BOOLEAN;

diff --git a/browser/resources/settings/brave_appearance_page/brave_appearance_page.html b/browser/resources/settings/brave_appearance_page/brave_appearance_page.html
@@ -37,5 +37,10 @@
       sub-label="$i18n{appearanceSettingsHideBraveRewardsButtonDesc}"
     >
     </settings-toggle-button>
+    <settings-toggle-button
+      pref="{{prefs.brave.always_show_bookmark_bar_on_ntp}}"
+      label="$i18n{appearanceSettingsAlwaysShowBookmarkBarOnNTP}"
+    >
+    </settings-toggle-button>
   </template>
 </dom-module>
diff --git a/browser/ui/BUILD.gn b/browser/ui/BUILD.gn
@@ -31,6 +31,12 @@ source_set("ui") {
 
   if (!is_android) {
     sources += [
+      "bookmark/brave_bookmark_tab_helper.cc",
+      "bookmark/brave_bookmark_tab_helper.h",
+      "bookmark/bookmark_prefs_service.cc",
+      "bookmark/bookmark_prefs_service.h",
+      "bookmark/bookmark_prefs_service_factory.cc",
+      "bookmark/bookmark_prefs_service_factory.h",
       "brave_browser_command_controller.cc",
       "brave_browser_command_controller.h",
       "brave_browser_content_setting_bubble_model_delegate.cc",

diff --git a/browser/ui/bookmark/bookmark_prefs_service.cc b/browser/ui/bookmark/bookmark_prefs_service.cc
@@ -0,0 +1,32 @@
+/* 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/ui/bookmark/bookmark_prefs_service.h"
+
+#include "brave/common/pref_names.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_list.h"
+
+BookmarkPrefsService::BookmarkPrefsService(Profile* profile)
+    : profile_(profile),
+      prefs_(profile->GetPrefs()) {
+  pref_change_registrar_.Init(prefs_);
+  pref_change_registrar_.Add(
+      kAlwaysShowBookmarkBarOnNTP,
+      base::BindRepeating(&BookmarkPrefsService::OnPreferenceChanged,
+                          base::Unretained(this)));
+}
+
+BookmarkPrefsService::~BookmarkPrefsService() = default;
+
+void BookmarkPrefsService::OnPreferenceChanged() {
+  for (Browser* browser : *BrowserList::GetInstance()) {
+    if (profile_->IsSameProfile(browser->profile())) {
+      browser->UpdateBookmarkBarState(
+          Browser::BOOKMARK_BAR_STATE_CHANGE_PREF_CHANGE);
+    }
+  }
+}
diff --git a/browser/ui/bookmark/bookmark_prefs_service.h b/browser/ui/bookmark/bookmark_prefs_service.h
@@ -0,0 +1,31 @@
+/* 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_UI_BOOKMARK_BOOKMARK_PREFS_SERVICE_H_
+#define BRAVE_BROWSER_UI_BOOKMARK_BOOKMARK_PREFS_SERVICE_H_
+
+#include "components/keyed_service/core/keyed_service.h"
+#include "components/prefs/pref_change_registrar.h"
+#include "content/public/browser/browser_thread.h"
+
+class PrefService;
+class Profile;
+
+class BookmarkPrefsService : public KeyedService {
+ public:
+  explicit BookmarkPrefsService(Profile* profile);
+  ~BookmarkPrefsService() override;
+
+ private:
+  void OnPreferenceChanged();
+
+  Profile* profile_;
+  PrefService* prefs_;
+  PrefChangeRegistrar pref_change_registrar_;
+
+  DISALLOW_COPY_AND_ASSIGN(BookmarkPrefsService);
+};
+
+#endif  // BRAVE_BROWSER_UI_BOOKMARK_BOOKMARK_PREFS_SERVICE_H_
diff --git a/browser/ui/bookmark/bookmark_prefs_service_factory.cc b/browser/ui/bookmark/bookmark_prefs_service_factory.cc
@@ -0,0 +1,51 @@
+/* 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/ui/bookmark/bookmark_prefs_service_factory.h"
+
+#include "brave/browser/ui/bookmark/bookmark_prefs_service.h"
+#include "brave/common/pref_names.h"
+#include "chrome/browser/profiles/incognito_helpers.h"
+#include "chrome/browser/profiles/profile.h"
+#include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "components/pref_registry/pref_registry_syncable.h"
+
+// static
+BookmarkPrefsService* BookmarkPrefsServiceFactory::GetForBrowserContext(
+    content::BrowserContext* context) {
+  return static_cast<BookmarkPrefsService*>(
+      GetInstance()->GetServiceForBrowserContext(context, true));
+}
+
+// static
+BookmarkPrefsServiceFactory* BookmarkPrefsServiceFactory::GetInstance() {
+  return base::Singleton<BookmarkPrefsServiceFactory>::get();
+}
+
+BookmarkPrefsServiceFactory::BookmarkPrefsServiceFactory()
+    : BrowserContextKeyedServiceFactory(
+          "BookmarkPrefsService",
+          BrowserContextDependencyManager::GetInstance()) {}
+
+BookmarkPrefsServiceFactory::~BookmarkPrefsServiceFactory() {}
+
+KeyedService* BookmarkPrefsServiceFactory::BuildServiceInstanceFor(
+    content::BrowserContext* context) const {
+  return new BookmarkPrefsService(Profile::FromBrowserContext(context));
+}
+
+content::BrowserContext* BookmarkPrefsServiceFactory::GetBrowserContextToUse(
+    content::BrowserContext* context) const {
+  return chrome::GetBrowserContextRedirectedInIncognito(context);
+}
+
+bool BookmarkPrefsServiceFactory::ServiceIsCreatedWithBrowserContext() const {
+  return true;
+}
+
+void BookmarkPrefsServiceFactory::RegisterProfilePrefs(
+    user_prefs::PrefRegistrySyncable* registry) {
+  registry->RegisterBooleanPref(kAlwaysShowBookmarkBarOnNTP, false);
+}
diff --git a/browser/ui/bookmark/bookmark_prefs_service_factory.h b/browser/ui/bookmark/bookmark_prefs_service_factory.h
@@ -0,0 +1,39 @@
+/* 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_UI_BOOKMARK_BOOKMARK_PREFS_SERVICE_FACTORY_H_
+#define BRAVE_BROWSER_UI_BOOKMARK_BOOKMARK_PREFS_SERVICE_FACTORY_H_
+
+#include "base/memory/singleton.h"
+#include "components/keyed_service/content/browser_context_keyed_service_factory.h"
+
+class BookmarkPrefsService;
+
+class BookmarkPrefsServiceFactory : public BrowserContextKeyedServiceFactory {
+ public:
+  static BookmarkPrefsService* GetForBrowserContext(
+      content::BrowserContext* context);
+
+  static BookmarkPrefsServiceFactory* GetInstance();
+
+ private:
+  friend struct base::DefaultSingletonTraits<BookmarkPrefsServiceFactory>;
+
+  BookmarkPrefsServiceFactory();
+  ~BookmarkPrefsServiceFactory() override;
+
+  // BrowserContextKeyedServiceFactory:
+  KeyedService* BuildServiceInstanceFor(
+      content::BrowserContext* profile) const override;
+  content::BrowserContext* GetBrowserContextToUse(
+      content::BrowserContext* context) const override;
+  bool ServiceIsCreatedWithBrowserContext() const override;
+  void RegisterProfilePrefs(
+      user_prefs::PrefRegistrySyncable* registry) override;
+
+  DISALLOW_COPY_AND_ASSIGN(BookmarkPrefsServiceFactory);
+};
+
+#endif  // BRAVE_BROWSER_UI_BOOKMARK_BOOKMARK_PREFS_SERVICE_FACTORY_H_
diff --git a/browser/ui/bookmark/bookmark_tab_helper_browsertest.cc b/browser/ui/bookmark/bookmark_tab_helper_browsertest.cc
@@ -5,6 +5,7 @@
 
 #include <vector>
 
+#include "brave/common/pref_names.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search/search.h"
@@ -16,6 +17,7 @@
 #include "chrome/test/base/in_process_browser_test.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/browser/bookmark_utils.h"
+#include "components/prefs/pref_service.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test_utils.h"
@@ -36,6 +38,20 @@ bool IsNTP(content::WebContents* web_contents) {
          search::NavEntryIsInstantNTP(web_contents, entry);
 }
 
+void AddBookmarkNode(Profile* profile) {
+  const GURL url = GURL("https://www.brave.com");
+  bookmarks::BookmarkModel* bookmark_model =
+      BookmarkModelFactory::GetForBrowserContext(profile);
+
+  std::vector<const bookmarks::BookmarkNode*> nodes;
+  bookmark_model->GetNodesByURL(url, &nodes);
+  EXPECT_EQ(0UL, nodes.size());
+
+  bookmarks::AddIfNotBookmarked(bookmark_model, url, base::string16());
+  bookmark_model->GetNodesByURL(url, &nodes);
+  EXPECT_EQ(1UL, nodes.size());
+}
+
 }  // namespace
 
 IN_PROC_BROWSER_TEST_F(BookmarkTabHelperBrowserTest,
@@ -47,18 +63,33 @@ IN_PROC_BROWSER_TEST_F(BookmarkTabHelperBrowserTest,
   chrome::ToggleBookmarkBar(browser());
   EXPECT_EQ(BookmarkBar::SHOW, browser()->bookmark_bar_state());
 
-  const GURL url = GURL("https://www.brave.com");
-  bookmarks::BookmarkModel* bookmark_model =
-      BookmarkModelFactory::GetForBrowserContext(browser()->profile());
+  AddBookmarkNode(browser()->profile());
 
-  std::vector<const bookmarks::BookmarkNode*> nodes;
-  bookmark_model->GetNodesByURL(url, &nodes);
-  EXPECT_EQ(0UL, nodes.size());
+  chrome::ToggleBookmarkBar(browser());
 
-  bookmarks::AddIfNotBookmarked(bookmark_model, url, base::string16());
-  bookmark_model->GetNodesByURL(url, &nodes);
-  EXPECT_EQ(1UL, nodes.size());
+  // Check bookmark is still hidden on NTP.
+  EXPECT_EQ(BookmarkBar::HIDDEN, browser()->bookmark_bar_state());
+}
 
+IN_PROC_BROWSER_TEST_F(BookmarkTabHelperBrowserTest,
+                       AlwaysShowBookmarkBarOnNTPTest) {
+  auto* profile = browser()->profile();
+  // Check default is false.
+  EXPECT_FALSE(profile->GetPrefs()->GetBoolean(kAlwaysShowBookmarkBarOnNTP));
+  auto* contents = browser()->tab_strip_model()->GetActiveWebContents();
+  EXPECT_TRUE(content::NavigateToURL(contents,
+                                     GURL(chrome::kChromeUINewTabURL)));
+  EXPECT_TRUE(IsNTP(contents));
+  AddBookmarkNode(profile);
+  profile->GetPrefs()->SetBoolean(kAlwaysShowBookmarkBarOnNTP, true);
+
+  // Check bookmark is visible on NTP.
+  EXPECT_EQ(BookmarkBar::SHOW, browser()->bookmark_bar_state());
+
+  // Check bookmark is still visible on NTP regardless of kBookmarkBar pref
+  // change.
   chrome::ToggleBookmarkBar(browser());
-  EXPECT_EQ(BookmarkBar::HIDDEN, browser()->bookmark_bar_state());
+  EXPECT_EQ(BookmarkBar::SHOW, browser()->bookmark_bar_state());
+  chrome::ToggleBookmarkBar(browser());
+  EXPECT_EQ(BookmarkBar::SHOW, browser()->bookmark_bar_state());
 }
diff --git a/browser/ui/bookmark/brave_bookmark_tab_helper.cc b/browser/ui/bookmark/brave_bookmark_tab_helper.cc
@@ -0,0 +1,48 @@
+/* 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/ui/bookmark/brave_bookmark_tab_helper.h"
+
+#include "brave/common/pref_names.h"
+#include "chrome/browser/profiles/profile.h"
+#include "components/prefs/pref_service.h"
+
+BraveBookmarkTabHelper::BraveBookmarkTabHelper(
+    content::WebContents* web_contents)
+    : web_contents_(web_contents) {
+}
+
+BraveBookmarkTabHelper::~BraveBookmarkTabHelper() {
+}
+
+void BraveBookmarkTabHelper::AddObserver(BookmarkTabHelperObserver* observer) {
+  BookmarkTabHelper::FromWebContents(web_contents_)->AddObserver(observer);
+}
+
+void BraveBookmarkTabHelper::RemoveObserver(
+    BookmarkTabHelperObserver* observer) {
+  BookmarkTabHelper::FromWebContents(web_contents_)->RemoveObserver(observer);
+}
+
+bool BraveBookmarkTabHelper::ShouldShowBookmarkBar() {
+  BookmarkTabHelper* helper =
+      BookmarkTabHelper::FromWebContents(web_contents_);
+  if (!helper)
+    return false;
+
+  // Originally, bookmark is visible for NTP when bookmarks are non empty.
+  // In that case, we want to hide bookmark bar on NTP if user chooses to hide.
+  // When bookmark should be hidden, we do not change it.
+  bool should_show = helper->ShouldShowBookmarkBar();
+  if (should_show) {
+    Profile* profile =
+        Profile::FromBrowserContext(web_contents_->GetBrowserContext());
+    should_show = profile->GetPrefs()->GetBoolean(kAlwaysShowBookmarkBarOnNTP);
+  }
+
+  return should_show;
+}
+
+WEB_CONTENTS_USER_DATA_KEY_IMPL(BraveBookmarkTabHelper)