[Partitioned Tabs] Add PartitionedTabVisualData #29408

sangwoo108 · 2025-06-05T10:15:11Z

PartitionedTabVisualData will be used to represent PartionedStorage tabs.

It'll be retrieved through TabInterface and set via TabModel,
following the upstream's tabs architecture.

components/tabs/public/partitioned_tab_visual_data.h

goodov · 2025-06-09T07:47:25Z

chromium_src/chrome/browser/ui/tabs/tab_model.cc

+
+#include "chrome/browser/ui/tabs/tab_model.h"
+
+#include "brave/components/containers/core/common/features.h"


everything related to containers should be under BUILDFLAG(ENABLE_CONTAINERS), but I don't think you should mention the containers feature here at all. Just keep partitioned_tab_visual_data_ getters/setter and remove CHECK calls.

goodov · 2025-06-09T07:58:16Z

components/tabs/public/tab_interface.cc

+std::optional<PartitionedTabVisualData>
+TabInterface::GetPartitionedTabVisualData() const {
+  return std::nullopt;
+}


I see what you did here and I think this is interesting, but I'd rather have these default implementations in brave/chromium_src override for consistency as we have in other places where we add new functions to chromium classes.

Yeah, that's what I'd love to do, but tab_interface.cc doesn't exist in Chromium, as they conist of pure virtual methods. As you already know, inlining non-emptybody isn't allowed by the chromium-style.

There're several derived classes, I tried to make these new methods have default implementation in this way. Off the top of my head, it might be possible to make "BraveTabInterface" that extends TabInterface and make others extends BraveTabInterface instead of TabInterface, but could be a little bit complex than the current implementation. Do you think it's worth trying to make BraveTabInterface?

ah, I see now. Thanks for the details!

I'd say having BraveTabInterface would be a nice touch here, but if it will require a lot of changes in different places, then we might stick with tab_interface.cc as you currently have it implemented.

Another take here - declare our methods with = 0 and implement them where required in Chromium (tab_model.cc, tab_android.cc). Maybe even declare them only on desktop (via buildflag guard) and implement only in tab_model.cc for now.

goodov · 2025-06-09T09:04:53Z

chromium_src/chrome/browser/ui/tabs/tab_model.cc

+}
+
+void TabModel::SetPartitionedTabVisualData(
+    const std::optional<PartitionedTabVisualData>& data) {


std::optional<PartitionedTabVisualData> data and partitioned_tab_visual_data_ = std::move(data);

goodov · 2025-06-09T12:20:01Z

chromium_src/chrome/browser/ui/tabs/tab_model.h

 #include "components/tabs/public/tab_interface.h"

 // Extends to support getter and setter for partitioned tabs.
+#if BUILDFLAG(ENABLE_CONTAINERS)


pls replace containers reference with #if !BUILDFLAG(IS_ANDROID).

this will also be used by Leo, so it should just be bound to "desktop" platform. again, no need to pull "containers" into this tab model extension.

this PR should not add any use of //brave/components/containers files.

goodov · 2025-06-09T14:18:34Z

browser/ui/views/tabs/BUILD.gn

@@ -54,4 +55,13 @@ source_set("unit_tests") {
    "//testing/gmock",
    "//testing/gtest",
  ]
+
+  if (enable_containers) {


all mentions of containers should be removed from this PR. please use !is_android check everywhere.

I don't think !is_android is appropriate here as this PR only touches desktop code

Maybe it's an old habit (not desktop = !android); but yeah, I think you meant:

if (!is_android && !is_ios) {

But I am a bit confused as a guest reading this PR. You're sharing a recommendation to use the OS check

all mentions of containers should be removed from this PR. please use !is_android check everywhere.

but then also you shared above:

everything related to containers should be under BUILDFLAG(ENABLE_CONTAINERS)

I also noticed your containers UI PR here is using #if BUILDFLAG(ENABLE_CONTAINERS).

We're in Views so this is already desktop-only. So either we need to gate behind a flag or we leave this API in by default since it isn't being called (and we can gate the calls with a flag, higher up).

bridiver · 2025-06-09T20:05:52Z

components/tabs/public/partitioned_tab_visual_data.h

+// Represents the visual data for a partitioned tab in the tab strip.
+// There're two types of partition tabs:
+// 1. Container tabs
+// 2. Leo tabs


I think everything should be using aichat now instead of leo?

also I think we should call these PartitionedStorageTabs so we're using consistent terminology.

bridiver · 2025-06-09T20:16:10Z

chromium_src/chrome/browser/ui/tabs/tab_model.h

+#if !BUILDFLAG(IS_ANDROID)
+#define IsActivated                                                     \
+  IsPartitionedTab() const override;                                    \
+  void SetPartitionedTabVisualData(                                     \


this doesn't seem like the right place for this, where would this be called from?

I think it probably makes sense to have SetPartitionedStorageTab(bool), but I'm concerned about how this is going to integrate with saved tab groups and whether we should have our own subclass of TabModel, but it doesn't seem like the TabModel should be where we store PartitionedTabVisualData

actually looking at this again, I'm not sure that it makes sense to have SetPartitionedStorageTab because that is what creates the need for PartitionedTabVisualData in the first place and seems to make things overly complicated when we know exactly what types of partitioned storage tabs exist in advance. Maybe SetPartitionedStorageTabType with an enum for CONTAINER, AICHAT and DISABLED and then based on that the UI code can display the correct icon, etc... without having to tell the tab model about it.

after discussing with @petemill apparently each container tab can have a different icon so an enum doesn't really work, but then I'm also not sure that we should have a generic PartitionedStorageTab here at all. I think we should just gave SetContainerId (with kNone) or something like that and then the UI code can lookup the correct icon based on that. For aichat it would just be SetAIChatTab(bool)

I think each entry that a person adds can have its own icon (see #29230 for an example where you can add the new containers). Eventually, that listing (add/remove/edit containers) should look like the mock up in brave/brave-browser#46592

etContainerId (with kNone) or something like that and then the UI code can lookup the correct icon based on that. For aichat it would just be SetAIChatTab(bool)

That'd be aligned to other methods like SetSplit() and SetGroup() in the model. Thanks.

Update: We're talking about if it'd be better to keep it as is so that we can work simultaneously.

bridiver · 2025-06-09T20:29:09Z

chromium_src/chrome/browser/ui/tabs/tab_model.h

+#include "components/tabs/public/tab_interface.h"
+
+// Extends to support getter and setter for partitioned tabs.
+#if !BUILDFLAG(IS_ANDROID)


this is not necessary, this class is already desktop only

bridiver · 2025-06-09T20:53:25Z

The PR description should not be a duplicate of the issue, also this PR does not fully implement the issue, it only provides initial support for the tab model. The issue should be updated to describe only the parts you are implementing here and the PR description should just describe what you're doing, not include all the details from the issue.

github-actions · 2025-06-11T22:59:57Z

Chromium major version is behind target branch (137.0.7151.68 vs 138.0.7204.25). Please rebase.

This data will be used to represet partitioned tab-Container tabs and Leo tabs. So it'll be retrieved through TabInterface and set via TabModel.

github-actions bot assigned sangwoo108 Jun 5, 2025

sangwoo108 force-pushed the sko/partition branch 5 times, most recently from 77aaea9 to 20f9bdf Compare June 5, 2025 13:53

sangwoo108 changed the base branch from sko/unittest to master June 5, 2025 14:04

sangwoo108 changed the title ~~Add PartitionedTabVisualData~~ [Partitioned Tabs] Add PartitionedTabVisualData Jun 6, 2025

sangwoo108 marked this pull request as ready for review June 6, 2025 21:51

sangwoo108 requested review from a team and bridiver as code owners June 6, 2025 21:51

sangwoo108 requested review from goodov and petemill June 6, 2025 21:51

goodov reviewed Jun 9, 2025

View reviewed changes

bridiver reviewed Jun 9, 2025

View reviewed changes

github-actions bot added the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Jun 11, 2025

sangwoo108 added 10 commits June 12, 2025 09:42

Add PartitionedTabVisualData

c9cfb18

This data will be used to represet partitioned tab-Container tabs and Leo tabs. So it'll be retrieved through TabInterface and set via TabModel.

Address code review

3307c0a

use move symantic for setter

d24f479

Try making new TabInterface methods pure virtual

c11040d

Replace BUILDFLAG(ENABLE_CONTAINERS) with !BUILDFLAG(IS_ANDROID)

8e234e6

Clean up unnecessary buildflags and deps

89d28de

Replace Leo with AI chat

ab08c04

Remove unnecessary deps

4f139b7

Remove containers related code

6412eab

Add BUILDFLAG(IS_ANDROID) to TabInterface

0f375ba

sangwoo108 force-pushed the sko/partition branch from 5486661 to 0f375ba Compare June 12, 2025 01:21

github-actions bot removed the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Jun 12, 2025

Update patchfile after rebasing

95e518a


		#include "chrome/browser/ui/tabs/tab_model.h"

		#include "brave/components/containers/core/common/features.h"

[Partitioned Tabs] Add PartitionedTabVisualData #29408

Are you sure you want to change the base?

[Partitioned Tabs] Add PartitionedTabVisualData #29408

Conversation

sangwoo108 commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goodov Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goodov Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsclifton Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bridiver Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsclifton Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sangwoo108 Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bridiver commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 11, 2025

Uh oh!

Uh oh!

sangwoo108 commented Jun 5, 2025 •

edited

Loading

goodov Jun 9, 2025 •

edited

Loading

goodov Jun 9, 2025 •

edited

Loading

bsclifton Jun 9, 2025 •

edited

Loading

bridiver Jun 9, 2025 •

edited

Loading

bsclifton Jun 9, 2025 •

edited

Loading

sangwoo108 Jun 9, 2025 •

edited

Loading

bridiver commented Jun 9, 2025 •

edited

Loading