Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add preference to en/disable Shared pinned tab #23797

Merged
merged 2 commits into from
May 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions app/brave_settings_strings.grdp
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,9 @@
<message name="IDS_SETTINGS_APPEARANCE_SETTINGS_BRAVE_TAB_HOVER_MODE_TOOLTIP" desc="The label name of the tab hover mode where a tooltip with the site title is shown.">
Tooltip
</message>
<message name="IDS_SETTINGS_APPEARANCE_SETTINGS_BRAVE_SHARED_PINNED_TAB" desc="The label name of a toggle button for Shared Pinned Tab feature">
Show pinned tabs in all windows
</message>
<message name="IDS_SETTINGS_APPEARANCE_SETTINGS_SHOW_BOOKMARKS_BUTTON" desc="The label for whether the bookmarks button should be shown or not">
Show bookmarks button
</message>
Expand Down
5 changes: 5 additions & 0 deletions browser/extensions/api/settings_private/brave_prefs_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,11 @@ const PrefsUtil::TypedPrefMap& BravePrefsUtil::GetAllowlistedKeys() {
settings_api::PrefType::kBoolean;
#endif

#if !BUILDFLAG(IS_ANDROID)
(*s_brave_allowlist)[brave_tabs::kSharedPinnedTab] =
settings_api::PrefType::kBoolean;
#endif

return *s_brave_allowlist;
}

Expand Down
1 change: 1 addition & 0 deletions browser/prefs/brave_pref_service_incognito_allowlist.cc
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ const std::vector<const char*>& GetBravePersistentPrefNames() {
brave_tabs::kVerticalTabsShowTitleOnWindow,
brave_tabs::kVerticalTabsOnRight,
brave_tabs::kVerticalTabsShowScrollbar,
brave_tabs::kSharedPinnedTab,
#endif
#if defined(TOOLKIT_VIEWS)
sidebar::kSidePanelWidth,
Expand Down
7 changes: 7 additions & 0 deletions browser/resources/settings/brave_appearance_page/tabs.html
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,13 @@
</settings-dropdown-menu>
</div>

<!-- Shared Pinned Tabs -->
<settings-toggle-button
pref="{{prefs.brave.tabs.shared_pinned_tab}}"
class="cr-row"
label="$i18n{appearanceSettingsSharedPinnedTab}">
</settings-toggle-button>

<!-- Tab memory usage -->
<settings-toggle-button id="hoverCardMemoryUsageToggle"
pref="{{prefs.browser.hovercard.memory_usage_enabled}}"
Expand Down
5 changes: 5 additions & 0 deletions browser/ui/brave_browser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "base/functional/callback_helpers.h"
#include "brave/browser/brave_browser_features.h"
#include "brave/browser/ui/brave_browser_window.h"
#include "brave/browser/ui/tabs/brave_tab_prefs.h"
#include "brave/browser/ui/tabs/features.h"
#include "brave/components/constants/pref_names.h"
#include "chrome/browser/lifetime/browser_close_manager.h"
Expand Down Expand Up @@ -252,6 +253,10 @@ bool BraveBrowser::AreAllTabsSharedPinnedTabs() {
return false;
}

if (!profile()->GetPrefs()->GetBoolean(brave_tabs::kSharedPinnedTab)) {
return false;
}

return tab_strip_model()->count() > 0 &&
tab_strip_model()->count() ==
tab_strip_model()->IndexOfFirstNonPinnedTab();
Expand Down
9 changes: 6 additions & 3 deletions browser/ui/brave_tab_strip_model_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,19 @@

#include "brave/browser/ui/brave_tab_strip_model_delegate.h"

#include "brave/browser/ui/tabs/brave_tab_prefs.h"
#include "brave/browser/ui/tabs/features.h"
#include "brave/browser/ui/tabs/shared_pinned_tab_service.h"
#include "brave/browser/ui/tabs/shared_pinned_tab_service_factory.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/ui/browser.h"
#include "components/prefs/pref_service.h"

namespace chrome {

bool BraveTabStripModelDelegate::CanMoveTabsToWindow(
const std::vector<int>& indices) {
if (!base::FeatureList::IsEnabled(tabs::features::kBraveSharedPinnedTabs)) {
if (!base::FeatureList::IsEnabled(tabs::features::kBraveSharedPinnedTabs) ||
!browser_->profile()->GetPrefs()->GetBoolean(
brave_tabs::kSharedPinnedTab)) {
return BrowserTabStripModelDelegate::CanMoveTabsToWindow(indices);
}

Expand Down
13 changes: 8 additions & 5 deletions browser/ui/browser_commands.cc
Original file line number Diff line number Diff line change
Expand Up @@ -694,15 +694,18 @@ void BringAllTabs(Browser* browser) {
std::stack<std::unique_ptr<tabs::TabModel>> detached_pinned_tabs;
std::stack<std::unique_ptr<tabs::TabModel>> detached_unpinned_tabs;

const bool shared_pinned_tab_enabled =
base::FeatureList::IsEnabled(tabs::features::kBraveSharedPinnedTabs) &&
browser->profile()->GetPrefs()->GetBoolean(brave_tabs::kSharedPinnedTab);

base::ranges::for_each(browsers, [&detached_pinned_tabs,
&detached_unpinned_tabs,
&browsers_to_close](auto* other) {
&detached_unpinned_tabs, &browsers_to_close,
shared_pinned_tab_enabled](auto* other) {
auto* tab_strip_model = other->tab_strip_model();
const int pinned_tab_count = tab_strip_model->IndexOfFirstNonPinnedTab();
for (int i = tab_strip_model->count() - 1; i >= 0; --i) {
const bool is_pinned = i < pinned_tab_count;
if (is_pinned && base::FeatureList::IsEnabled(
tabs::features::kBraveSharedPinnedTabs)) {
if (is_pinned && shared_pinned_tab_enabled) {
// SharedPinnedTabService is responsible for synchronizing pinned
// tabs, thus we shouldn't manually detach and attach tabs here.
// Meanwhile, the tab strips don't get empty when they have dummy
Expand Down Expand Up @@ -737,7 +740,7 @@ void BringAllTabs(Browser* browser) {
detached_unpinned_tabs.pop();
}

if (base::FeatureList::IsEnabled(tabs::features::kBraveSharedPinnedTabs)) {
if (shared_pinned_tab_enabled) {
base::ranges::for_each(browsers_to_close,
[](auto* other) { other->window()->Close(); });
}
Expand Down
2 changes: 2 additions & 0 deletions browser/ui/tabs/brave_tab_prefs.cc
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ void RegisterBraveProfilePrefs(PrefRegistrySimple* registry) {
registry->RegisterIntegerPref(kVerticalTabsExpandedWidth, 220);
registry->RegisterBooleanPref(kVerticalTabsOnRight, false);
registry->RegisterBooleanPref(kVerticalTabsShowScrollbar, false);

registry->RegisterBooleanPref(kSharedPinnedTab, false);
}

void MigrateBraveProfilePrefs(PrefService* prefs) {
Expand Down
2 changes: 2 additions & 0 deletions browser/ui/tabs/brave_tab_prefs.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ inline constexpr char kVerticalTabsOnRight[] =
inline constexpr char kVerticalTabsShowScrollbar[] =
"brave.tabs.vertical_tabs_show_scrollbar";

inline constexpr char kSharedPinnedTab[] = "brave.tabs.shared_pinned_tab";

void RegisterBraveProfilePrefs(PrefRegistrySimple* registry);
void MigrateBraveProfilePrefs(PrefService* prefs);

Expand Down
70 changes: 67 additions & 3 deletions browser/ui/tabs/shared_pinned_tab_service.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "base/functional/bind.h"
#include "base/functional/callback_forward.h"
#include "base/notreached.h"
#include "brave/browser/ui/tabs/brave_tab_prefs.h"
#include "brave/browser/ui/tabs/features.h"
#include "brave/browser/ui/tabs/shared_pinned_tab_dummy_view.h"
#include "chrome/browser/profiles/profile.h"
Expand Down Expand Up @@ -196,7 +197,15 @@ SharedPinnedTabService::SharedPinnedTabService(Profile* profile)
: profile_(profile) {
DCHECK(base::FeatureList::IsEnabled(tabs::features::kBraveSharedPinnedTabs));
profile_observation_.Observe(profile_);
browser_list_observation_.Observe(BrowserList::GetInstance());

shared_pinned_tab_enabled_.Init(
brave_tabs::kSharedPinnedTab, profile_->GetPrefs(),
base::BindRepeating(&SharedPinnedTabService::OnSharedPinnedTabPrefChanged,
weak_ptr_factory_.GetWeakPtr()));

if (*shared_pinned_tab_enabled_) {
browser_list_observation_.Observe(BrowserList::GetInstance());
}
}

SharedPinnedTabService::~SharedPinnedTabService() = default;
Expand Down Expand Up @@ -288,8 +297,6 @@ void SharedPinnedTabService::OnBrowserAdded(Browser* browser) {
DVLOG(2) << __FUNCTION__ << " " << browser->tab_strip_model()->count();

browser->tab_strip_model()->AddObserver(this);
DCHECK_EQ(0, browser->tab_strip_model()->count())
<< "We're assuming that browser doesn't have any tabs at this point.";
browsers_.insert(browser);
}

Expand Down Expand Up @@ -838,6 +845,63 @@ void SharedPinnedTabService::MoveSharedWebContentsToBrowser(
}
}

void SharedPinnedTabService::OnSharedPinnedTabPrefChanged() {
if (*shared_pinned_tab_enabled_) {
OnSharedPinnedTabEnabled();
} else {
OnSharedPinnedTabDisabled();
}
}

void SharedPinnedTabService::OnSharedPinnedTabEnabled() {
// Init observers
browser_list_observation_.Observe(BrowserList::GetInstance());
auto browsers = chrome::FindAllTabbedBrowsersWithProfile(profile_);
for (auto* browser : browsers) {
OnBrowserAdded(browser);
}

// Synchronize all pre-existing pinned tabs.
for (auto* browser : browsers) {
auto* tab_strip_model = browser->tab_strip_model();
for (int i = 0; i < tab_strip_model->IndexOfFirstNonPinnedTab(); ++i) {
auto* contents = tab_strip_model->GetWebContentsAt(i);
if (IsDummyContents(contents)) {
// This tab is dummy tab created inside this loop from another
// browser.
continue;
}
TabPinnedStateChanged(tab_strip_model, contents, i);
}
}
}

void SharedPinnedTabService::OnSharedPinnedTabDisabled() {
// Reset observers. Note that we should remove observers first so that
// closing dummy contents won't close shared contents too.
for (auto* browser : browsers_) {
browser->tab_strip_model()->RemoveObserver(this);
}
browser_list_observation_.Reset();

// Remove all dummy contents
for (auto* browser : browsers_) {
auto* tab_strip_model = browser->tab_strip_model();
for (auto i = tab_strip_model->IndexOfFirstNonPinnedTab() - 1; i >= 0;
--i) {
if (IsDummyContents(tab_strip_model->GetWebContentsAt(i))) {
tab_strip_model->CloseWebContentsAt(i, /* close_type */ 0);
}
}
}

// Reset data
browsers_.clear();
last_active_browser_ = nullptr;
closing_browsers_.clear();
pinned_tab_data_.clear();
}

std::unique_ptr<content::WebContents>
SharedPinnedTabService::CreateDummyWebContents(
content::WebContents* shared_contents) {
Expand Down
7 changes: 7 additions & 0 deletions browser/ui/tabs/shared_pinned_tab_service.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
#include "components/keyed_service/core/keyed_service.h"
#include "components/prefs/pref_member.h"

class Profile;
class BrowserList;
Expand Down Expand Up @@ -107,6 +108,10 @@ class SharedPinnedTabService : public KeyedService,
int index,
bool is_last_closing_browser = false);

void OnSharedPinnedTabPrefChanged();
void OnSharedPinnedTabEnabled();
void OnSharedPinnedTabDisabled();

std::unique_ptr<content::WebContents> CreateDummyWebContents(
content::WebContents* shared_contents);

Expand All @@ -131,6 +136,8 @@ class SharedPinnedTabService : public KeyedService,
base::ScopedObservation<BrowserList, BrowserListObserver>
browser_list_observation_{this};

BooleanPrefMember shared_pinned_tab_enabled_;

base::WeakPtrFactory<SharedPinnedTabService> weak_ptr_factory_{this};
};

Expand Down
52 changes: 52 additions & 0 deletions browser/ui/tabs/test/shared_pinned_tab_service_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,14 @@
#include "base/run_loop.h"
#include "base/test/bind.h"
#include "brave/browser/ui/browser_commands.h"
#include "brave/browser/ui/tabs/brave_tab_prefs.h"
#include "brave/browser/ui/tabs/features.h"
#include "brave/browser/ui/tabs/test/shared_pinned_tab_service_browsertest.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/ui/browser_commands.h"
#include "chrome/browser/ui/browser_list.h"
#include "chrome/browser/ui/browser_navigator_params.h"
#include "chrome/browser/ui/browser_tabstrip.h"
#include "chrome/browser/ui/browser_window.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/browser/ui/views/frame/browser_view.h"
Expand Down Expand Up @@ -105,6 +108,9 @@ void SharedPinnedTabServiceBrowserTest::SetUpOnMainThread() {
return response;
}));
ASSERT_TRUE(https_server_->Start());

browser()->profile()->GetPrefs()->SetBoolean(brave_tabs::kSharedPinnedTab,
true);
}

void SharedPinnedTabServiceBrowserTest::SetUpCommandLine(
Expand Down Expand Up @@ -341,6 +347,52 @@ IN_PROC_BROWSER_TEST_F(SharedPinnedTabServiceBrowserTest,
[]() { return BrowserList::GetInstance()->size() == 1; }));
}

IN_PROC_BROWSER_TEST_F(SharedPinnedTabServiceBrowserTest, PreferenceChanged) {
// Given that there're multiple windows with shared pinned tabs
auto* browser_1 = browser();
auto* tab_strip_model_1 = browser_1->tab_strip_model();
tab_strip_model_1->SetTabPinned(0, /* pinned= */ true);
chrome::AddTabAt(browser_1, GURL(), /*index*/ -1, /*foreground*/ true);

auto* browser_2 = CreateNewBrowser();
browser_2->tab_strip_model()->SetTabPinned(1, true);
chrome::AddTabAt(browser_2, GURL(), /*index*/ -1, /*foreground*/ true);

ASSERT_EQ(3, browser_1->tab_strip_model()->count());
ASSERT_TRUE(browser_1->tab_strip_model()->IsTabPinned(0));
ASSERT_TRUE(browser_1->tab_strip_model()->IsTabPinned(1));

ASSERT_EQ(3, browser_2->tab_strip_model()->count());
ASSERT_TRUE(browser_2->tab_strip_model()->IsTabPinned(0));
ASSERT_TRUE(browser_2->tab_strip_model()->IsTabPinned(1));

// When disabling the shared pinned tab preference
browser_1->profile()->GetPrefs()->SetBoolean(brave_tabs::kSharedPinnedTab,
false);

// Then all dummy contents should be gone.
EXPECT_EQ(2, browser_1->tab_strip_model()->count());
EXPECT_TRUE(browser_1->tab_strip_model()->IsTabPinned(0));
EXPECT_FALSE(browser_1->tab_strip_model()->IsTabPinned(1));

EXPECT_EQ(2, browser_2->tab_strip_model()->count());
EXPECT_TRUE(browser_2->tab_strip_model()->IsTabPinned(0));
EXPECT_FALSE(browser_2->tab_strip_model()->IsTabPinned(1));

// When enabling the shared pinned tab preference
browser_1->profile()->GetPrefs()->SetBoolean(brave_tabs::kSharedPinnedTab,
true);

// All pinned tabs should be synchronized
EXPECT_EQ(3, browser_1->tab_strip_model()->count());
EXPECT_TRUE(browser_1->tab_strip_model()->IsTabPinned(0));
EXPECT_TRUE(browser_1->tab_strip_model()->IsTabPinned(1));

EXPECT_EQ(3, browser_2->tab_strip_model()->count());
EXPECT_TRUE(browser_2->tab_strip_model()->IsTabPinned(0));
EXPECT_TRUE(browser_2->tab_strip_model()->IsTabPinned(1));
}

#if !BUILDFLAG(IS_MAC)
IN_PROC_BROWSER_TEST_F(SharedPinnedTabServiceBrowserTest,
CloseTabShortCutShouldBeDisabled) {
Expand Down
8 changes: 6 additions & 2 deletions browser/ui/toolbar/brave_location_bar_model_delegate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "brave/components/ipfs/ipfs_constants.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/ui/browser.h"
#include "components/prefs/pref_service.h"
#include "content/public/browser/navigation_entry.h"
#include "extensions/buildflags/buildflags.h"

Expand All @@ -23,12 +24,13 @@
#endif

#if !BUILDFLAG(IS_ANDROID)
#include "brave/browser/ui/tabs/brave_tab_prefs.h"
#include "brave/browser/ui/tabs/shared_pinned_tab_service.h"
#include "brave/browser/ui/tabs/shared_pinned_tab_service_factory.h"
#endif

BraveLocationBarModelDelegate::BraveLocationBarModelDelegate(Browser* browser)
: BrowserLocationBarModelDelegate(browser) {}
: BrowserLocationBarModelDelegate(browser), browser_(browser) {}

BraveLocationBarModelDelegate::~BraveLocationBarModelDelegate() = default;

Expand Down Expand Up @@ -70,7 +72,9 @@ BraveLocationBarModelDelegate::FormattedStringWithEquivalentMeaning(

bool BraveLocationBarModelDelegate::GetURL(GURL* url) const {
#if !BUILDFLAG(IS_ANDROID)
if (base::FeatureList::IsEnabled(tabs::features::kBraveSharedPinnedTabs)) {
if (base::FeatureList::IsEnabled(tabs::features::kBraveSharedPinnedTabs) &&
browser_->profile()->GetPrefs()->GetBoolean(
brave_tabs::kSharedPinnedTab)) {
content::NavigationEntry* entry = GetNavigationEntry();
if (entry && entry->IsInitialEntry()) {
auto* active_web_contents = GetActiveWebContents();
Expand Down
2 changes: 2 additions & 0 deletions browser/ui/toolbar/brave_location_bar_model_delegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ class BraveLocationBarModelDelegate : public BrowserLocationBarModelDelegate {
const GURL& url,
const std::u16string& formatted_url) const override;
bool GetURL(GURL* url) const override;

raw_ptr<Browser> browser_ = nullptr;
};

#endif // BRAVE_BROWSER_UI_TOOLBAR_BRAVE_LOCATION_BAR_MODEL_DELEGATE_H_
Loading
Loading