diff --git a/browser/ui/sidebar/sidebar_unittest.cc b/browser/ui/sidebar/sidebar_unittest.cc index ead275d6aa72..daed550227b9 100644 --- a/browser/ui/sidebar/sidebar_unittest.cc +++ b/browser/ui/sidebar/sidebar_unittest.cc @@ -9,12 +9,17 @@ #include "base/test/scoped_feature_list.h" #include "brave/browser/ui/sidebar/sidebar_model.h" #include "brave/browser/ui/sidebar/sidebar_service_factory.h" +#include "brave/browser/ui/sidebar/sidebar_utils.h" +#include "brave/common/webui_url_constants.h" +#include "brave/components/sidebar/constants.h" #include "brave/components/sidebar/sidebar_service.h" +#include "chrome/common/url_constants.h" #include "chrome/test/base/testing_profile.h" #include "components/prefs/pref_service.h" #include "components/sync_preferences/testing_pref_service_syncable.h" #include "content/public/test/browser_task_environment.h" #include "testing/gtest/include/gtest/gtest.h" +#include "url/gurl.h" namespace sidebar { @@ -118,4 +123,32 @@ TEST_F(SidebarModelTest, ItemsChangedTest) { EXPECT_EQ(3, model()->active_index()); } +TEST_F(SidebarModelTest, CanUseNotAddedBuiltInItemInsteadOfTest) { + GURL talk("https://talk.brave.com/1Ar1vHfLBWX2sAdi"); + // False because builtin talk item is already added. + EXPECT_FALSE(CanUseNotAddedBuiltInItemInsteadOf(service(), talk)); + + // Remove builtin talk item and check builtin talk item will be used + // instead of adding |talk| url. + service()->RemoveItemAt(0); + EXPECT_TRUE(CanUseNotAddedBuiltInItemInsteadOf(service(), talk)); +} + +TEST(SidebarUtilTest, ConvertURLToBuiltInItemURLTest) { + EXPECT_EQ(GURL(kBraveTalkURL), + ConvertURLToBuiltInItemURL(GURL("https://talk.brave.com"))); + EXPECT_EQ(GURL(kBraveTalkURL), + ConvertURLToBuiltInItemURL( + GURL("https://talk.brave.com/1Ar1vHfLBWX2sAdi"))); + EXPECT_EQ(GURL(kSidebarBookmarksURL), + ConvertURLToBuiltInItemURL(GURL(chrome::kChromeUIBookmarksURL))); + EXPECT_EQ( + GURL(kBraveUIWalletPageURL), + ConvertURLToBuiltInItemURL(GURL("chrome://wallet/crypto/onboarding"))); + + // Not converted for url that doesn't relavant builtin item. + GURL brave_com("https://www.brave.com/"); + EXPECT_EQ(brave_com, ConvertURLToBuiltInItemURL(brave_com)); +} + } // namespace sidebar diff --git a/browser/ui/sidebar/sidebar_utils.cc b/browser/ui/sidebar/sidebar_utils.cc index f65bab279637..91b5638423e7 100644 --- a/browser/ui/sidebar/sidebar_utils.cc +++ b/browser/ui/sidebar/sidebar_utils.cc @@ -6,6 +6,8 @@ #include "brave/browser/ui/sidebar/sidebar_utils.h" #include "brave/browser/ui/sidebar/sidebar_service_factory.h" +#include "brave/common/webui_url_constants.h" +#include "brave/components/sidebar/constants.h" #include "brave/components/sidebar/sidebar_item.h" #include "brave/components/sidebar/sidebar_service.h" #include "chrome/browser/search/search.h" @@ -13,8 +15,10 @@ #include "chrome/browser/ui/tabs/tab_strip_model.h" #include "chrome/browser/ui/webui/new_tab_page/new_tab_page_ui.h" #include "chrome/browser/ui/webui/ntp/new_tab_ui.h" +#include "chrome/common/webui_url_constants.h" #include "content/public/browser/navigation_entry.h" #include "content/public/browser/web_contents.h" +#include "content/public/common/url_constants.h" namespace sidebar { @@ -24,24 +28,22 @@ SidebarService* GetSidebarService(Browser* browser) { return SidebarServiceFactory::GetForProfile(browser->profile()); } -bool IsActiveTabNTP(Browser* browser) { - auto* web_contents = browser->tab_strip_model()->GetActiveWebContents(); +bool IsActiveTabNTP(content::WebContents* active_web_contents) { content::NavigationEntry* entry = - web_contents->GetController().GetLastCommittedEntry(); + active_web_contents->GetController().GetLastCommittedEntry(); if (!entry) - entry = web_contents->GetController().GetVisibleEntry(); + entry = active_web_contents->GetController().GetVisibleEntry(); if (!entry) return false; const GURL url = entry->GetURL(); return NewTabUI::IsNewTab(url) || NewTabPageUI::IsNewTabPageOrigin(url) || - search::NavEntryIsInstantNTP(web_contents, entry); + search::NavEntryIsInstantNTP(active_web_contents, entry); } -bool IsActiveTabAlreadyAddedToSidebar(Browser* browser) { - auto* web_contents = browser->tab_strip_model()->GetActiveWebContents(); - const GURL url = web_contents->GetVisibleURL(); - for (const auto& item : GetSidebarService(browser)->items()) { - if (item.url == url) +bool IsURLAlreadyAddedToSidebar(SidebarService* service, const GURL& url) { + const GURL converted_url = ConvertURLToBuiltInItemURL(url); + for (const auto& item : service->items()) { + if (item.url == converted_url) return true; } @@ -50,20 +52,59 @@ bool IsActiveTabAlreadyAddedToSidebar(Browser* browser) { } // namespace +bool CanUseNotAddedBuiltInItemInsteadOf(SidebarService* service, + const GURL& url) { + const auto not_added_default_items = + service->GetNotAddedDefaultSidebarItems(); + if (not_added_default_items.empty()) + return false; + const GURL converted_url = ConvertURLToBuiltInItemURL(url); + for (const auto& item : not_added_default_items) { + if (item.url == converted_url) + return true; + } + return false; +} + bool CanUseSidebar(Browser* browser) { DCHECK(browser); return browser->is_type_normal(); } +// If url is relavant with bulitin items, use builtin item's url. +// Ex, we don't need to add bookmarks manager as a sidebar shortcut +// if sidebar panel already has bookmarks item. +GURL ConvertURLToBuiltInItemURL(const GURL& url) { + if (url == GURL(chrome::kChromeUIBookmarksURL)) + return GURL(kSidebarBookmarksURL); + + if (url.host() == "talk.brave.com") + return GURL(kBraveTalkURL); + + if (url.SchemeIs(content::kChromeUIScheme) && url.host() == kWalletPageHost) { + return GURL(kBraveUIWalletPageURL); + } + return url; +} + bool CanAddCurrentActiveTabToSidebar(Browser* browser) { - auto* web_contents = browser->tab_strip_model()->GetActiveWebContents(); - if (!web_contents) + auto* active_web_contents = + browser->tab_strip_model()->GetActiveWebContents(); + if (!active_web_contents) + return false; + + if (IsActiveTabNTP(active_web_contents)) + return false; + + const GURL url = active_web_contents->GetLastCommittedURL(); + if (!url.is_valid()) return false; - if (IsActiveTabNTP(browser)) + auto* service = GetSidebarService(browser); + if (IsURLAlreadyAddedToSidebar(service, url)) return false; - if (IsActiveTabAlreadyAddedToSidebar(browser)) + if (CanUseNotAddedBuiltInItemInsteadOf(service, url)) return false; return true; diff --git a/browser/ui/sidebar/sidebar_utils.h b/browser/ui/sidebar/sidebar_utils.h index c8ac0398bd3e..57d16def54a6 100644 --- a/browser/ui/sidebar/sidebar_utils.h +++ b/browser/ui/sidebar/sidebar_utils.h @@ -7,12 +7,20 @@ #define BRAVE_BROWSER_UI_SIDEBAR_SIDEBAR_UTILS_H_ class Browser; +class GURL; namespace sidebar { +class SidebarService; + bool CanUseSidebar(Browser* browser); bool CanAddCurrentActiveTabToSidebar(Browser* browser); +// Exported for testing. +bool CanUseNotAddedBuiltInItemInsteadOf(SidebarService* service, + const GURL& url); +GURL ConvertURLToBuiltInItemURL(const GURL& url); + } // namespace sidebar #endif // BRAVE_BROWSER_UI_SIDEBAR_SIDEBAR_UTILS_H_ diff --git a/components/sidebar/constants.h b/components/sidebar/constants.h index 9065a5d2108e..2eedf69f6d49 100644 --- a/components/sidebar/constants.h +++ b/components/sidebar/constants.h @@ -19,6 +19,8 @@ constexpr char kSidebarItemOpenInPanelKey[] = "open_in_panel"; constexpr char kSidebarBookmarksURL[] = "chrome://sidebar-bookmarks.top-chrome/"; +constexpr char kBraveTalkURL[] = "https://talk.brave.com/widget"; + } // namespace sidebar #endif // BRAVE_COMPONENTS_SIDEBAR_CONSTANTS_H_ diff --git a/components/sidebar/sidebar_service.cc b/components/sidebar/sidebar_service.cc index 8ae050af5cb9..3f26ba3d8d20 100644 --- a/components/sidebar/sidebar_service.cc +++ b/components/sidebar/sidebar_service.cc @@ -27,7 +27,7 @@ SidebarItem GetBuiltInItemForType(SidebarItem::BuiltInItemType type) { switch (type) { case SidebarItem::BuiltInItemType::kBraveTalk: return SidebarItem::Create( - GURL("https://talk.brave.com/widget"), + GURL(kBraveTalkURL), l10n_util::GetStringUTF16(IDS_SIDEBAR_BRAVE_TALK_ITEM_TITLE), SidebarItem::Type::kTypeBuiltIn, SidebarItem::BuiltInItemType::kBraveTalk, false); diff --git a/components/sidebar/sidebar_service_unittest.cc b/components/sidebar/sidebar_service_unittest.cc index 97bac12971e2..ed61f4e1e886 100644 --- a/components/sidebar/sidebar_service_unittest.cc +++ b/components/sidebar/sidebar_service_unittest.cc @@ -171,7 +171,7 @@ TEST_F(SidebarServiceTest, BuiltInItemUpdateTestWithBuiltInItemTypeKey) { // Check service has updated built-in item. Previously url was deprecated.xxx. EXPECT_EQ(1UL, service_->items().size()); - EXPECT_EQ("https://talk.brave.com/widget", service_->items()[0].url); + EXPECT_EQ(GURL(kBraveTalkURL), service_->items()[0].url); // Simulate re-launch and check service has still updated items. service_->RemoveObserver(this); @@ -181,7 +181,7 @@ TEST_F(SidebarServiceTest, BuiltInItemUpdateTestWithBuiltInItemTypeKey) { // Check service has updated built-in item. Previously url was deprecated.xxx. EXPECT_EQ(1UL, service_->items().size()); - EXPECT_EQ("https://talk.brave.com/widget", service_->items()[0].url); + EXPECT_EQ(GURL(kBraveTalkURL), service_->items()[0].url); } TEST_F(SidebarServiceTest, BuiltInItemDoesntHaveHistoryItem) {