[Tree Tabs] Draft #31914

sangwoo108 · 2025-10-20T04:43:28Z

Resolves

github-actions · 2025-10-20T04:50:28Z

browser/ui/views/tabs/brave_tab_container.cc

+    tree_tabs_enabled_.Init(
+        brave_tabs::kTreeTabsEnabled, prefs,
+        base::BindRepeating(&BraveTabContainer::UpdateLayoutOrientation,
+                            base::Unretained(this)));


_{reported by reviewdog 🐶}
[semgrep] base::Unretained is most of the time unrequited, and a weak reference is better suited for secure coding.
Consider swapping Unretained for a weak reference.
base::Unretained usage may be acceptable when a callback owner is guaranteed
to be destroyed with the object base::Unretained is pointing to, for example:

- PrefChangeRegistrar
- base::*Timer
- mojo::Receiver
- any other class member destroyed when the class is deallocated

Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/chromium-uaf.yaml

Cc @thypon @goodov

sangwoo108 · 2025-10-20T05:13:20Z

browser/ui/tabs/brave_tab_strip_model.h

+  // Returns tree height that the `tab` at the given index belongs to.
+  int GetTreeHeightOfTab(int index) const;
+
+  // Returns tree node level of the `tab` at the given index.
+  int GetTreeNodeLevel(int index) const;


Merge these into TreeTabVisual(int index) ?

sangwoo108 · 2025-10-20T05:13:57Z

browser/ui/tabs/brave_tab_strip_model.cc

 void BraveTabStripModel::BuildTreeTabs() {
  CHECK(base::FeatureList::IsEnabled(tabs::features::kBraveTreeTab));
-  CHECK(!in_tree_mode_);
+  CHECK(!contents_data()->in_tree_tab_mode());


Maybe we need TreeTabModel

sangwoo108 · 2025-10-20T05:18:29Z

chromium_src/chrome/browser/ui/views/tabs/tab_strip_layout_helper.h

+  void set_use_vertical_tabs(bool vertical) {                         \
+    use_vertical_tabs_ = vertical;                                    \
+  }                                                                   \
+  void set_tab_strip(TabStrip* tab_strip) {                           \
+    tab_strip_ = tab_strip;                                           \
+  }                                                                   \
+  void set_use_tree_tabs(bool tree) {                                 \
+    use_tree_tabs_ = tree;                                            \
+  }                                                                   \


Not a good ideal - having many conditionals would be painful

sangwoo108 · 2025-10-20T05:33:33Z

components/tabs/public/brave_tab_strip_collection.h

+  void set_in_tree_tab_mode(bool in_tree_tab_mode) {
+    in_tree_tab_mode_ = in_tree_tab_mode;
+  }
+  bool in_tree_tab_mode() const { return in_tree_tab_mode_; }


Not a good idea to have these here.

sangwoo108 · 2025-10-20T05:33:58Z

components/tabs/public/brave_tab_strip_collection.h

+  void AddTabRecursive(std::unique_ptr<TabInterface> tab,
+                       size_t index,
+                       std::optional<tab_groups::TabGroupId> new_group_id,
+                       bool new_pinned_state,
+                       TabInterface* opener) override;
+  void MoveTabRecursive(size_t initial_index,
+                        size_t final_index,
+                        std::optional<tab_groups::TabGroupId> new_group_id,
+                        bool new_pinned_state) override;
+  void MoveTabsRecursive(
+      const std::vector<int>& tab_indices,
+      size_t destination_index,
+      std::optional<tab_groups::TabGroupId> new_group_id,
+      bool new_pinned_state,
+      const std::set<TabCollection::Type>& retain_collection_types) override;
+  std::unique_ptr<TabInterface> RemoveTabAtIndexRecursive(
+      size_t index) override;
+  std::unique_ptr<TabInterface> RemoveTabRecursive(
+      TabInterface* tab,
+      bool close_empty_group_collection) override;


These only should route events to delegate or some other classes.

sangwoo108 · 2025-10-20T05:35:32Z

components/tabs/public/brave_tab_strip_collection.h

+//   and insert it into target collection. The target collection can be another
+//   TreeTabNode in case new tab has opener relationship with the previous tab.
+//   Otherwise, the target collection can be anything but pinned collection.
+class BraveTabStripCollection : public TabStripCollection {


So, the responsibility of this class is to make a room for doing some actions in a certain evetns.

sangwoo108 · 2025-10-20T05:36:32Z

browser/ui/tabs/brave_tab_strip_model.cc

+    if (delegate->IsNormalWindow()) {
+      tree_tabs_enabled_.Init(
+          brave_tabs::kTreeTabsEnabled, profile->GetPrefs(),
+          base::BindRepeating(&BraveTabStripModel::OnTreeTabRelatedPrefChanged,
+                              base::Unretained(this)));
+      vertical_tabs_enabled_.Init(
+          brave_tabs::kVerticalTabsEnabled, profile->GetPrefs(),
+          base::BindRepeating(&BraveTabStripModel::OnTreeTabRelatedPrefChanged,
+                              base::Unretained(this)));
+      OnTreeTabRelatedPrefChanged();
+    }


These also should be responsibility of other class.

This commit introduces core functionality for the Tree Tabs feature, enabling tab creation and node wrapping capabilities when adding new tabs. Key changes: - Added tab creation logic with proper node wrapping in tree structure - When adding a new tab with a opener tab and the opener tab is the previous tab, the new tab's node will be set as a child of the opener tab's node. - In this way, we'll have a tree structure like below when we open multiple tabs with the previous tab as opener sequentially: ``` - Tab 1 - Tab 2 - Tab 3 - Tab 4 ```

This node will hold metadata about tree tab's node such as level and height.

github-actions · 2025-10-21T08:56:41Z

components/tabs/impl/tree_tab_node_tab_collection.cc

-      base::BindRepeating(&TreeTabNode::OnWillDetach, base::Unretained(this)));
+  will_detach_tab_subscription_ =
+      current_tab_->RegisterWillDetach(base::BindRepeating(
+          &TreeTabNodeTabCollection::OnWillDetach, base::Unretained(this)));


_{reported by reviewdog 🐶}
[semgrep] base::Unretained is most of the time unrequited, and a weak reference is better suited for secure coding.
Consider swapping Unretained for a weak reference.
base::Unretained usage may be acceptable when a callback owner is guaranteed
to be destroyed with the object base::Unretained is pointing to, for example:

- PrefChangeRegistrar
- base::*Timer
- mojo::Receiver
- any other class member destroyed when the class is deallocated

Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/chromium-uaf.yaml

Cc @thypon @goodov

github-actions · 2025-10-21T08:56:41Z

components/tabs/impl/tree_tab_node_tab_collection.cc

+
+const TreeTabNodeTabCollection* TreeTabNodeTabCollection::GetTopLevelAncestor()
+    const {
+  return const_cast<TreeTabNodeTabCollection*>(this)->GetTopLevelAncestor();


_{reported by reviewdog 🐶}
[semgrep] Avoid const_cast to remove const, except when implementing non-const getters in terms of const getters.

Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/const_cast.yaml

Cc @bridiver @cdesouza-chromium

sangwoo108 added the CI/skip Do not run CI builds (except noplatform) label Oct 20, 2025

github-actions bot assigned sangwoo108 Oct 20, 2025

github-actions bot reviewed Oct 20, 2025

View reviewed changes

github-actions bot added the needs-security-review label Oct 20, 2025

github-actions bot assigned bridiver, cdesouza-chromium, goodov and thypon Oct 20, 2025

sangwoo108 commented Oct 20, 2025

View reviewed changes

sangwoo108 added 4 commits October 21, 2025 12:50

Rename TabTreeNode to tabs::TabTreeNodeTabCollection

5c9b18f

WIP

c5313b4

Introduce TreeTabNode

7bb7f07

This node will hold metadata about tree tab's node such as level and height.

sangwoo108 force-pushed the sko/tree-tab-draft branch from 860f6c6 to c1bdd07 Compare October 21, 2025 08:50

github-actions bot reviewed Oct 21, 2025

View reviewed changes

WIP

22ce69c

sangwoo108 force-pushed the sko/tree-tab-draft branch from c1bdd07 to 22ce69c Compare October 23, 2025 08:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Tree Tabs] Draft #31914

[Tree Tabs] Draft #31914

Uh oh!

sangwoo108 commented Oct 20, 2025

Uh oh!

github-actions bot Oct 20, 2025

Uh oh!

sangwoo108 Oct 20, 2025

Uh oh!

sangwoo108 Oct 20, 2025

Uh oh!

sangwoo108 Oct 20, 2025

Uh oh!

sangwoo108 Oct 20, 2025

Uh oh!

sangwoo108 Oct 20, 2025

Uh oh!

sangwoo108 Oct 20, 2025

Uh oh!

sangwoo108 Oct 20, 2025

Uh oh!

github-actions bot Oct 21, 2025

Uh oh!

github-actions bot Oct 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[Tree Tabs] Draft #31914

Are you sure you want to change the base?

[Tree Tabs] Draft #31914

Uh oh!

Conversation

sangwoo108 commented Oct 20, 2025

Uh oh!

github-actions bot Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

sangwoo108 Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

sangwoo108 Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

sangwoo108 Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

sangwoo108 Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

sangwoo108 Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

sangwoo108 Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

sangwoo108 Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Oct 21, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot Oct 21, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants