brave / brave-browser

Brave browser for Android, iOS, Linux, macOS, Windows.
https://brave.com
Mozilla Public License 2.0
16.99k stars 2.21k forks source link

Decide if we want to employ `Android Tab Group Stable IDs` feature #39218

Open samartnik opened 2 weeks ago

samartnik commented 2 weeks ago

Description

There is a new feature in Chromium Android Tab Group Stable IDs https://source.chromium.org/chromium/chromium/src/+/ed4add002d0322cb110f30ce41bdd58145a2bee6. The main controversy in it is that bottom controls do not disappear when the last tab in group is closed (see video). We should decide whether we want to employ this feature as is or make some modifications or not use it at all.

https://github.com/brave/brave-browser/assets/30602739/a0a9840c-4dc9-4224-b3e9-3117f56a9300

Steps to reproduce

  1. No steps, it's just a new feature

Actual result

None yet

Expected result

None yet

Reproduces how often

Easily reproduced

Brave version

None yet

Device

Channel information

Reproducibility

Miscellaneous information

No response

AlexeyBarabash commented 1 week ago

Comment on how kAndroidTabGroupStableIds affects Sync:

  1. kAndroidTabGroupStableIds is used at tab_groups::IsTabGroupSyncEnabled

  2. Then it is used at ChromeSyncClient::CreateModelTypeControllers to decide if the Tab Group sync will be enabled on Android. Other OS don't depend on this feature.

    // Tab group sync is enabled via separate feature flags on different
    // platforms.
    bool enable_tab_group_sync = false;
    #if BUILDFLAG(IS_CHROMEOS) || BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_MAC) || \
    BUILDFLAG(IS_WIN)
    enable_tab_group_sync = true;
    #elif BUILDFLAG(IS_ANDROID)
    enable_tab_group_sync = tab_groups::IsTabGroupSyncEnabled(GetPrefService());
    tab_groups::TabGroupTrial::OnTabgroupSyncEnabled(enable_tab_group_sync);
    #endif  // BUILDFLAG(IS_CHROMEOS) || BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_MAC) ||
        // BUILDFLAG(IS_WIN)
AlexeyBarabash commented 1 week ago

Here is an example of how tab groups sync looks on real devices. First video - Samsung S22U, where I created a tab group, the second device is Pixel, where the tab group arrived from sync.

Tested On Chrome Dev.

Note, that on Android there is no switch control save this group as Desktop has, and all groups get synced.

Samsung

https://github.com/brave/brave-browser/assets/24739341/eaeb85ee-54b8-461c-8e98-d892f5696597

Pixel

https://github.com/brave/brave-browser/assets/24739341/96eb523a-6435-485c-a7b5-6dcead1e5ec2

AlexeyBarabash commented 1 week ago

It is possible to ungroup tabs and then Chrome gives this message Screenshot_20240624_165708_Chrome Dev

timchilds commented 1 week ago

Needs design input. Decision TBD.