Add preference to en/disable Shared pinned tab #23797

sangwoo108 · 2024-05-23T01:53:24Z

Resolves brave/brave-browser#38509

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

simonhong

++ 👍🏼

simonhong · 2024-05-23T06:14:52Z

browser/ui/toolbar/brave_location_bar_model_delegate.h

@@ -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_;


nit: = nullptr or raw_ref?

simonhong · 2024-05-23T06:49:39Z

browser/ui/tabs/shared_pinned_tab_service.cc

+          [](base::WeakPtr<SharedPinnedTabService> service) {
+            if (UNLIKELY(!service)) {
+              // As BooleanPrefMember is already RAII style subscription,
+              // this is unlikely to happen but theoretically might happen on


Why do you think it's available in theory?
As shared_pinned_tab_enabled_ is member var of this service, it'll be destroyed before service is gone.
Also weak_ptr_factory_ is last declared member, so it should not be null when it's referred by other member vars.

Right, I thought backward.

Oh wait, shared_pinned_tab_enabled_ can be alive while weak_ptr_factory_ is invalidated, as destruction order is from the last? But still unlikely to happen as it's not accessed by multiple threads? I think making a method would be clear.

Ah, right. it's invalidated at first than all others.

mkarolin · 2024-05-23T12:40:00Z

app/brave_settings_strings.grdp

@@ -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">
+    Shared pinned tabs


@sangwoo108 can we be a little more descriptive here as to what this toggle would do? For example, Show pinned tabs in all windows. cc: @rebron do we have a better wording for this feature? "Shared pinned tabs" as a settings toggle is not immediately obvious, imo.

That's much better!

github-actions · 2024-05-24T00:49:49Z

[puLL-Merge] - brave/brave-core@23797

Description

This PR adds a new setting to enable/disable the "Shared Pinned Tab" feature in Brave. When enabled, pinned tabs are shared across all windows. The setting is exposed in the Appearance settings page.

Changes

Changes

app/brave_settings_strings.grdp: Added new string for the "Show pinned tabs in all windows" setting.
browser/extensions/api/settings_private/brave_prefs_util.cc: Registered the new kSharedPinnedTab preference.
browser/prefs/brave_pref_service_incognito_allowlist.cc: Added kSharedPinnedTab to the list of preferences persisted in incognito mode.
browser/resources/settings/brave_appearance_page/tabs.html: Added a toggle button for the new "Show pinned tabs in all windows" setting.
browser/ui/brave_browser.cc: Check the kSharedPinnedTab pref before treating all tabs as shared pinned tabs.
browser/ui/brave_tab_strip_model_delegate.cc: Check the kSharedPinnedTab pref before allowing tabs to be moved between windows.
browser/ui/browser_commands.cc: Check the kSharedPinnedTab pref when bringing all tabs to the current window.
browser/ui/tabs/brave_tab_prefs.cc, .h: Defined the new kSharedPinnedTab preference.
browser/ui/tabs/shared_pinned_tab_service.cc, .h:
- Observe changes to the kSharedPinnedTab pref.
- When pref is enabled, set up tab sharing. When disabled, remove tab sharing.
browser/ui/tabs/test/shared_pinned_tab_service_browsertest.cc: Added a browser test for enabling/disabling the pref.
browser/ui/toolbar/brave_location_bar_model_delegate.cc: Check kSharedPinnedTab pref before getting URL for initial dummy tab.
browser/ui/views/frame/brave_browser_frame_mac.mm: Check kSharedPinnedTab pref before ignoring Ctrl+W on pinned tabs.
browser/ui/views/frame/brave_browser_view.cc: Check kSharedPinnedTab pref before handling Ctrl+W accelerator on pinned tabs.
browser/ui/views/location_bar/brave_location_bar_view.cc: Check kSharedPinnedTab pref before focusing omnibox on dummy tabs.
browser/ui/views/tabs/tab_drag_controller.cc: Check kSharedPinnedTab pref before disallowing detaching pinned tabs.
browser/ui/webui/settings/brave_settings_localized_strings_provider.cc: Added string ID for the new setting text.

The other changes check the kSharedPinnedTab preference at various places to enable/disable the shared pinned tab behavior based on the preference value.

Security Hotspots

None found. The changes handle a user-controlled preference properly without introducing any likely security issues.

mkarolin

strings++

github-actions bot assigned sangwoo108 May 23, 2024

sangwoo108 force-pushed the sko/spt-pref branch 2 times, most recently from 83851e0 to 47b785e Compare May 23, 2024 02:44

This was referenced May 23, 2024

Follow-ups for shared pinned tabs(a.k.a. Persistent pinned tabs) brave/brave-browser#28873

Closed

Experiment with Shared pinned tabs brave/brave-browser#28532

Closed

sangwoo108 marked this pull request as ready for review May 23, 2024 04:28

sangwoo108 requested a review from a team as a code owner May 23, 2024 04:28

sangwoo108 requested a review from simonhong May 23, 2024 04:28

simonhong approved these changes May 23, 2024

View reviewed changes

Add preference to en/disable Shared pinned tab

2c32328

sangwoo108 force-pushed the sko/spt-pref branch from 47b785e to 2c32328 Compare May 23, 2024 07:32

sangwoo108 enabled auto-merge May 23, 2024 07:33

github-actions bot added the puLL-Merge label May 23, 2024

mkarolin reviewed May 23, 2024

View reviewed changes

Update setting string

0b632ab

mkarolin approved these changes May 24, 2024

View reviewed changes

sangwoo108 merged commit d603893 into master May 24, 2024
19 checks passed

sangwoo108 deleted the sko/spt-pref branch May 24, 2024 16:17

github-actions bot added this to the 1.68.x - Nightly milestone May 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add preference to en/disable Shared pinned tab #23797

Add preference to en/disable Shared pinned tab #23797

sangwoo108 commented May 23, 2024 •

edited

simonhong left a comment

simonhong May 23, 2024

simonhong May 23, 2024

sangwoo108 May 23, 2024

sangwoo108 May 23, 2024 •

edited

simonhong May 23, 2024

mkarolin May 23, 2024

sangwoo108 May 24, 2024

github-actions bot commented May 24, 2024

Changes

mkarolin left a comment

Add preference to en/disable Shared pinned tab #23797

Add preference to en/disable Shared pinned tab #23797

Conversation

sangwoo108 commented May 23, 2024 • edited

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

simonhong left a comment

Choose a reason for hiding this comment

simonhong May 23, 2024

Choose a reason for hiding this comment

simonhong May 23, 2024

Choose a reason for hiding this comment

sangwoo108 May 23, 2024

Choose a reason for hiding this comment

sangwoo108 May 23, 2024 • edited

Choose a reason for hiding this comment

simonhong May 23, 2024

Choose a reason for hiding this comment

mkarolin May 23, 2024

Choose a reason for hiding this comment

sangwoo108 May 24, 2024

Choose a reason for hiding this comment

github-actions bot commented May 24, 2024

Description

Changes

Security Hotspots

mkarolin left a comment

Choose a reason for hiding this comment

sangwoo108 commented May 23, 2024 •

edited

sangwoo108 May 23, 2024 •

edited