Merge pull request #7859 from brave/topsite_edit

Improve topsite tiles in NTP

browser/brave_profile_prefs.cc

@@ -247,6 +247,9 @@ void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry) {
   registry->SetDefaultPrefValue(prefs::kCloudPrintSubmitEnabled,
                                 base::Value(false));
 
+  // Disable default webstore icons in topsites or apps.
+  registry->SetDefaultPrefValue(prefs::kHideWebStoreIcon, base::Value(true));
+
   // Importer: selected data types
   registry->RegisterBooleanPref(kImportDialogExtensions, true);
   registry->RegisterBooleanPref(kImportDialogPayments, true);

browser/brave_profile_prefs_browsertest.cc

@@ -117,4 +117,6 @@ IN_PROC_BROWSER_TEST_F(BraveProfilePrefsBrowserTest,
       prefs::kCloudPrintSubmitEnabled));
   EXPECT_TRUE(browser()->profile()->GetPrefs()->GetBoolean(
       prefs::kNtpUseMostVisitedTiles));
+  EXPECT_TRUE(
+      browser()->profile()->GetPrefs()->GetBoolean(prefs::kHideWebStoreIcon));
 }

browser/ui/BUILD.gn

@@ -91,6 +91,8 @@ source_set("ui") {
       "webui/new_tab_page/brave_new_tab_message_handler.h",
       "webui/new_tab_page/brave_new_tab_ui.cc",
       "webui/new_tab_page/brave_new_tab_ui.h",
+      "webui/new_tab_page/brave_new_tab_ui_utils.cc",
+      "webui/new_tab_page/brave_new_tab_ui_utils.h",
       "webui/new_tab_page/instant_service_message_handler.cc",
       "webui/new_tab_page/instant_service_message_handler.h",
       "webui/settings/brave_appearance_handler.cc",

browser/ui/webui/brave_webui_source.cc

@@ -145,8 +145,24 @@ void CustomizeWebUIHTMLSource(const std::string &name,
         { "clockFormatDefault", IDS_BRAVE_NEW_TAB_CLOCK_FORMAT_DEFAULT },
         { "clockFormat12", IDS_BRAVE_NEW_TAB_CLOCK_FORMAT_12 },
         { "clockFormat24", IDS_BRAVE_NEW_TAB_CLOCK_FORMAT_24 },
+        { "addTopSiteDialogTitle", IDS_BRAVE_NEW_TAB_ADD_TOP_SITE_DIALOG_TITLE },  // NOLINT
+        { "editTopSiteDialogTitle", IDS_BRAVE_NEW_TAB_EDIT_TOP_SITE_DIALOG_TITLE },  // NOLINT
+        { "editSiteTileMenuItem", IDS_BRAVE_NEW_TAB_EDIT_SITE_TILE_MENU_ITEM },
+        { "removeTileMenuItem", IDS_BRAVE_NEW_TAB_REMOVE_TILE_MENU_ITEM },
+        { "addTopSiteDialogURLLabel", IDS_BRAVE_NEW_TAB_ADD_TOP_SITE_DIALOG_URL_LABEL },  // NOLINT
+        { "addTopSiteDialogURLInputPlaceHolder", IDS_BRAVE_NEW_TAB_ADD_TOP_SITE_DIALOG_URL_INPUT_PLACEHOLDER },  // NOLINT
+        { "addTopSiteDialogNameLabel", IDS_BRAVE_NEW_TAB_ADD_TOP_SITE_DIALOG_NAME_LABEL },  // NOLINT
+        { "addTopSiteDialogNameInputPlaceHolder", IDS_BRAVE_NEW_TAB_ADD_TOP_SITE_DIALOG_NAME_INPUT_PLACEHOLDER },  // NOLINT
+        { "addTopSiteDialogSaveButtonLabel", IDS_BRAVE_NEW_TAB_ADD_TOP_SITE_DIALOG_SAVE_BUTTON_LABEL },  // NOLINT
+        { "addTopSiteDialogCancelButtonLabel", IDS_BRAVE_NEW_TAB_ADD_TOP_SITE_DIALOG_CANCEL_BUTTON_LABEL },  // NOLINT
         { "showTopSites", IDS_BRAVE_NEW_TAB_SHOW_TOP_SITES },
-        { "topSiteCustomLinksEnabled", IDS_BRAVE_NEW_TAB_CUSTOM_LINKS_ENABLED },
+        { "showFavoritesLabel", IDS_BRAVE_NEW_TAB_SHOW_FAVORITES_LABEL },
+        { "showFavoritesDesc", IDS_BRAVE_NEW_TAB_SHOW_FAVORITES_DESC },
+        { "showFrecencyLabel", IDS_BRAVE_NEW_TAB_SHOW_FRECENCY_LABEL },
+        { "showFrecencyDesc", IDS_BRAVE_NEW_TAB_SHOW_FRECENCY_DESC },
+        { "addSiteMenuLabel", IDS_BRAVE_NEW_TAB_ADD_SITE_MENU_LABEL },
+        { "showFrecencyMenuLabel", IDS_BRAVE_NEW_TAB_SHOW_FRECENCY_MENU_LABEL },
+        { "showFavoritesMenuLabel", IDS_BRAVE_NEW_TAB_SHOW_FAVORITES_MENU_LABEL },  // NOLINT
         { "showRewards", IDS_BRAVE_NEW_TAB_SHOW_REWARDS },
         { "showBinance", IDS_BRAVE_NEW_TAB_SHOW_BINANCE },
         { "showTogether", IDS_BRAVE_NEW_TAB_SHOW_TOGETHER },

browser/ui/webui/new_tab_page/BUILD.gn

@@ -0,0 +1,19 @@
+# Copyright (c) 2021 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/.
+
+source_set("unit_tests") {
+  testonly = true
+
+  if (!is_android) {
+    sources = [ "brave_new_tab_ui_unittest.cc" ]
+
+    deps = [
+      "//brave/browser/ui",
+      "//components/history/core/browser",
+      "//components/ntp_tiles",
+      "//testing/gtest",
+    ]
+  }
+}

browser/ui/webui/new_tab_page/brave_new_tab_ui_unittest.cc

@@ -0,0 +1,46 @@
+/* Copyright (c) 2021 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/webui/new_tab_page/brave_new_tab_ui_utils.h"
+#include "components/history/core/browser/top_sites_impl.h"
+#include "components/ntp_tiles/constants.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+TEST(BraveNewTabUITest, ConstantsTest) {
+  // Make sure history/ntp_tiles module has proper constants for our NTP
+  // requirements.
+  constexpr size_t kBraveMaxTopSitesNumber = 12;
+  constexpr size_t kTopSitesNumber = history::TopSitesImpl::kTopSitesNumber;
+
+  EXPECT_EQ(kBraveMaxTopSitesNumber, kTopSitesNumber);
+  EXPECT_EQ(kBraveMaxTopSitesNumber, ntp_tiles::kMaxNumCustomLinks);
+  EXPECT_EQ(kBraveMaxTopSitesNumber, ntp_tiles::kMaxNumMostVisited);
+  EXPECT_EQ(static_cast<int>(kBraveMaxTopSitesNumber), ntp_tiles::kMaxNumTiles);
+}
+
+TEST(BraveNewTabUITest, TopSiteURLValidation) {
+  std::string url = "a";
+  EXPECT_TRUE(GetValidURLStringForTopSite(&url));
+  EXPECT_EQ("https://a", url);
+
+  url = "http://a";
+  EXPECT_TRUE(GetValidURLStringForTopSite(&url));
+  EXPECT_EQ("http://a", url);
+
+  url = "https://a";
+  EXPECT_TRUE(GetValidURLStringForTopSite(&url));
+  EXPECT_EQ("https://a", url);
+
+  url = "https://www.a.com";
+  EXPECT_TRUE(GetValidURLStringForTopSite(&url));
+  EXPECT_EQ("https://www.a.com", url);
+
+  // Check failed to make vaile url.
+  url = "!@";
+  EXPECT_FALSE(GetValidURLStringForTopSite(&url));
+
+  url = "";
+  EXPECT_FALSE(GetValidURLStringForTopSite(&url));
+}

browser/ui/webui/new_tab_page/brave_new_tab_ui_utils.cc

@@ -0,0 +1,22 @@
+// Copyright (c) 2021 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/webui/new_tab_page/brave_new_tab_ui_utils.h"
+
+#include "url/gurl.h"
+
+bool GetValidURLStringForTopSite(std::string* url) {
+  if (GURL(*url).is_valid())
+    return true;
+
+  // fixup if passed |url| is not valid.
+  const std::string new_url = "https://" + *url;
+  if (GURL(new_url).is_valid()) {
+    *url = new_url;
+    return true;
+  }
+
+  return false;
+}

browser/ui/webui/new_tab_page/brave_new_tab_ui_utils.h

@@ -0,0 +1,16 @@
+// Copyright (c) 2021 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_WEBUI_NEW_TAB_PAGE_BRAVE_NEW_TAB_UI_UTILS_H_
+#define BRAVE_BROWSER_UI_WEBUI_NEW_TAB_PAGE_BRAVE_NEW_TAB_UI_UTILS_H_
+
+#include <string>
+
+// Simply append `https://` scheme to |url| if |url| is not valid.
+// Retruns true if |url| is valid or fixed |url| is valid.
+// Fixed url is passed via |url|.
+bool GetValidURLStringForTopSite(std::string* url);
+
+#endif  // BRAVE_BROWSER_UI_WEBUI_NEW_TAB_PAGE_BRAVE_NEW_TAB_UI_UTILS_H_

browser/ui/webui/new_tab_page/instant_service_message_handler.cc

@@ -14,12 +14,15 @@
 #include "brave/browser/ntp_background_images/view_counter_service_factory.h"
 #include "brave/browser/profiles/profile_util.h"
 #include "brave/browser/ui/webui/new_tab_page/brave_new_tab_ui.h"
+#include "brave/browser/ui/webui/new_tab_page/brave_new_tab_ui_utils.h"
 #include "brave/common/pref_names.h"
 #include "brave/components/ntp_background_images/browser/ntp_background_images_data.h"
 #include "brave/components/ntp_background_images/browser/view_counter_service.h"
+#include "chrome/browser/ntp_tiles/chrome_most_visited_sites_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search/instant_service.h"
 #include "chrome/browser/search/instant_service_factory.h"
+#include "components/ntp_tiles/most_visited_sites.h"
 
 using ntp_background_images::ViewCounterServiceFactory;
 
@@ -29,14 +32,6 @@ using ntp_background_images::ViewCounterServiceFactory;
 // For more info, see:
 // https://bugs.chromium.org/p/chromium/issues/detail?id=1084363
 
-namespace {
-
-bool ShouldExcludeFromTiles(const GURL& url) {
-  return url.spec().find("https://chrome.google.com/webstore") == 0;
-}
-
-}  // namespace
-
 InstantServiceMessageHandler::InstantServiceMessageHandler(Profile* profile)
         : profile_(profile) {
   instant_service_ = InstantServiceFactory::GetForProfile(profile_);
@@ -78,6 +73,14 @@ void InstantServiceMessageHandler::RegisterMessages() {
     base::BindRepeating(
       &InstantServiceMessageHandler::HandleSetMostVisitedSettings,
       base::Unretained(this)));
+  web_ui()->RegisterMessageCallback(
+      "addNewTopSite",
+      base::BindRepeating(&InstantServiceMessageHandler::HandleAddNewTopSite,
+                          base::Unretained(this)));
+  web_ui()->RegisterMessageCallback(
+      "editTopSite",
+      base::BindRepeating(&InstantServiceMessageHandler::HandleEditTopSite,
+                          base::Unretained(this)));
 }
 
 // InstantServiceObserver:
@@ -113,9 +116,6 @@ void InstantServiceMessageHandler::MostVisitedInfoChanged(
 
   // See chrome/common/search/instant_types.h for more info
   for (auto& tile : info.items) {
-    if (ShouldExcludeFromTiles(tile.url))
-      continue;
-
     base::Value tile_value(base::Value::Type::DICTIONARY);
     if (tile.title.empty()) {
       tile_value.SetStringKey("title", tile.url.spec());
@@ -134,6 +134,7 @@ void InstantServiceMessageHandler::MostVisitedInfoChanged(
   result.SetBoolKey("custom_links_enabled", !info.use_most_visited);
   result.SetKey("tiles", std::move(tiles));
   result.SetBoolKey("visible", info.is_visible);
+  result.SetIntKey("custom_links_num", GetCustomLinksNum());
   top_site_tiles_ = std::move(result);
 
   // Notify listeners of this update (ex: new tab page)
@@ -142,6 +143,24 @@ void InstantServiceMessageHandler::MostVisitedInfoChanged(
   }
 }
 
+int InstantServiceMessageHandler::GetCustomLinksNum() const {
+  // Calculate the number of tiles that can be visible in favorites mode.
+  int custom_links_num = 0;
+  auto most_visited_sites =
+      ChromeMostVisitedSitesFactory::NewForProfile(profile_);
+  if (most_visited_sites) {
+    custom_links_num += most_visited_sites->GetCustomLinkNum();
+  }
+
+  // In NTP SR mode, SR tiles are also shown in tiles.
+  auto* service = ViewCounterServiceFactory::GetForProfile(profile_);
+  if (service) {
+    custom_links_num += service->GetTopSitesVectorForWebUI().size();
+  }
+
+  return custom_links_num;
+}
+
 void InstantServiceMessageHandler::HandleUpdateMostVisitedInfo(
     const base::ListValue* args) {
   AllowJavascript();
@@ -237,3 +256,70 @@ void InstantServiceMessageHandler::HandleSetMostVisitedSettings(
     instant_service_->ToggleMostVisitedOrCustomLinks();
   }
 }
+
+void InstantServiceMessageHandler::HandleEditTopSite(
+    const base::ListValue* args) {
+  AllowJavascript();
+
+  std::string url;
+  if (!args->GetString(0, &url))
+    return;
+  DCHECK(!url.empty());
+
+  std::string new_url;
+  if (!args->GetString(1, &new_url))
+    return;
+
+  std::string title;
+  if (!args->GetString(2, &title))
+    return;
+
+  // |new_url| can be empty if user only want to change title.
+  // Stop editing if we can't make |new_url| valid.
+  if (!new_url.empty() && !GetValidURLStringForTopSite(&new_url))
+    return;
+
+  if (title.empty())
+    title = new_url.empty() ? url : new_url;
+
+  // when user modifies current top sites, change to favorite mode.
+  auto pair = instant_service_->GetCurrentShortcutSettings();
+  const bool custom_links_enabled = !pair.first;
+  if (!custom_links_enabled) {
+    instant_service_->ToggleMostVisitedOrCustomLinks();
+
+    // When user tries to edit from frecency mode, we just try to add modified
+    // item to favorites. If modified url is already existed in favorites,
+    // nothing happened.
+    instant_service_->AddCustomLink(new_url.empty() ? GURL(url) : GURL(new_url),
+                                    title);
+  } else {
+    instant_service_->UpdateCustomLink(GURL(url), GURL(new_url), title);
+  }
+}
+
+void InstantServiceMessageHandler::HandleAddNewTopSite(
+    const base::ListValue* args) {
+  AllowJavascript();
+
+  std::string url;
+  if (!args->GetString(0, &url))
+    return;
+  DCHECK(!url.empty());
+
+  std::string title;
+  if (!args->GetString(1, &title))
+    return;
+
+  // Stop adding if we can't make |url| valid.
+  if (!GetValidURLStringForTopSite(&url))
+    return;
+
+  // when user adds new top sites, change to favorite mode.
+  auto pair = instant_service_->GetCurrentShortcutSettings();
+  const bool custom_links_enabled = !pair.first;
+  if (!custom_links_enabled)
+    instant_service_->ToggleMostVisitedOrCustomLinks();
+
+  instant_service_->AddCustomLink(GURL(url), title);
+}

browser/ui/webui/new_tab_page/instant_service_message_handler.h

@@ -38,6 +38,10 @@ class InstantServiceMessageHandler : public content::WebUIMessageHandler,
   void HandleRestoreMostVisitedDefaults(const base::ListValue* args);
   void HandleUndoMostVisitedTileAction(const base::ListValue* args);
   void HandleSetMostVisitedSettings(const base::ListValue* args);
+  void HandleEditTopSite(const base::ListValue* args);
+  void HandleAddNewTopSite(const base::ListValue* args);
+
+  int GetCustomLinksNum() const;
 
   GURL last_blacklisted_;
   // Weak pointer.

chromium_src/components/history/core/browser/DEPS

@@ -0,0 +1,3 @@
+include_rules = [
+  "+../../../../../../components/history/core/browser",
+]

chromium_src/components/history/core/browser/top_sites_impl.h

@@ -0,0 +1,18 @@
+/* Copyright (c) 2021 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_CHROMIUM_SRC_COMPONENTS_HISTORY_CORE_BROWSER_TOP_SITES_IMPL_H_
+#define BRAVE_CHROMIUM_SRC_COMPONENTS_HISTORY_CORE_BROWSER_TOP_SITES_IMPL_H_
+
+// Needs 12 items for our NTP top site tiles.
+#define kTopSitesNumber \
+  kTopSitesNumber = 12; \
+  static constexpr size_t kTopSitesNumber_Unused
+
+#include "../../../../../../components/history/core/browser/top_sites_impl.h"
+
+#undef kTopSitesNumber
+
+#endif  // BRAVE_CHROMIUM_SRC_COMPONENTS_HISTORY_CORE_BROWSER_TOP_SITES_IMPL_H_

chromium_src/components/ntp_tiles/DEPS

@@ -0,0 +1,4 @@
+include_rules = [
+  "+../../../../components/ntp_tiles",
+  "+components/ntp_tiles",
+]

chromium_src/components/ntp_tiles/constants.cc

@@ -0,0 +1,13 @@
+/* Copyright (c) 2021 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 "components/ntp_tiles/constants.h"
+
+namespace ntp_tiles {
+
+const size_t kMaxNumCustomLinks = 12;
+const size_t kMaxNumMostVisited = 12;
+
+}  // namespace ntp_tiles

chromium_src/components/ntp_tiles/constants.h

@@ -0,0 +1,18 @@
+/* Copyright (c) 2021 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_CHROMIUM_SRC_COMPONENTS_NTP_TILES_CONSTANTS_H_
+#define BRAVE_CHROMIUM_SRC_COMPONENTS_NTP_TILES_CONSTANTS_H_
+
+// Needs 12 items for our NTP top site tiles.
+#define kMaxNumTiles \
+  kMaxNumTiles = 12; \
+  const int kMaxNumTiles_Unused
+
+#include "../../../../components/ntp_tiles/constants.h"
+
+#undef kMaxNumTiles
+
+#endif  // BRAVE_CHROMIUM_SRC_COMPONENTS_NTP_TILES_CONSTANTS_H_