Dominic Austria (Gerrit)

unread,

Sep 16, 2025, 5:57:23 PM9/16/25

to chromium...@chromium.org

Dominic Austria added 2 comments

File chrome/browser/ui/views/toolbar/app_menu.cc

Line 1262, Patchset 1 (Latest): } else if (command_id == IDC_CREATE_NEW_TAB_GROUP &&

Dominic Austria . resolved

If the everything menu isn't showing, and the command is to create a new tab group, then the user must have clicked on the 'create new tab group' option at the top level.

Line 1298, Patchset 1 (Latest): } else if (!features::IsTabGroupMenuMoreEntryPointsEnabled()) {

Dominic Austria . resolved

Same logic here

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Yuheng Huang (Gerrit)

unread,

Sep 16, 2025, 7:58:20 PM9/16/25

to Dominic Austria, David Pennington, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from David Pennington and Dominic Austria

Yuheng Huang added 2 comments

File chrome/browser/ui/toolbar/app_menu_model.cc

Line 1846, Patchset 1 (Latest): if (features::IsTabGroupMenuMoreEntryPointsEnabled()) {

Yuheng Huang . unresolved

Having 3 dots menu to have 2 Create New Tab Group menu item is confusing(One under top level one on the tab group submenu). Is it a good idea to do that at all? @dpen...@chromium.org what do you think?

File chrome/browser/ui/views/toolbar/app_menu.cc

Line 1261, Patchset 1 (Latest): stg_everything_menu_->ExecuteCommand(command_id, mouse_event_flags);

Yuheng Huang . unresolved

The logic seems to be incorrect for this route. The original code has an early return in this case. Now since the return is removed, entry.first->ActivatedAt is called, causing create new tab group to execute twice.

Open in Gerrit

Related details

Attention is currently required from:

David Pennington
Dominic Austria

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

David Pennington (Gerrit)

unread,

Sep 17, 2025, 11:23:02 AM9/17/25

to Dominic Austria, Yuheng Huang, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from Dominic Austria

David Pennington added 1 comment

File chrome/browser/ui/toolbar/app_menu_model.cc

Line 1846, Patchset 1 (Latest): if (features::IsTabGroupMenuMoreEntryPointsEnabled()) {

Yuheng Huang . unresolved

Having 3 dots menu to have 2 Create New Tab Group menu item is confusing(One under top level one on the tab group submenu). Is it a good idea to do that at all? @dpen...@chromium.org what do you think?

David Pennington

if this feature flag is on, lets move the button up to the top level like in the UI updates request and remove the one in the submenu, but yes youre right i think this might be a bit confusing if we were to have 2

Open in Gerrit

Related details

Attention is currently required from:

Dominic Austria

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Dominic Austria (Gerrit)

unread,

Sep 17, 2025, 12:30:53 PM9/17/25

to David Pennington, Yuheng Huang, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from Yuheng Huang

Dominic Austria added 1 comment

File chrome/browser/ui/views/toolbar/app_menu.cc

Line 1261, Patchset 1 (Latest): stg_everything_menu_->ExecuteCommand(command_id, mouse_event_flags);

Yuheng Huang . unresolved

The logic seems to be incorrect for this route. The original code has an early return in this case. Now since the return is removed, entry.first->ActivatedAt is called, causing create new tab group to execute twice.

Dominic Austria

You're right oops. That probably explains some of the test failures.

Open in Gerrit

Related details

Attention is currently required from:

Yuheng Huang

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Dominic Austria (Gerrit)

unread,

Sep 17, 2025, 12:34:46 PM9/17/25

to David Pennington, Yuheng Huang, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from David Pennington and Yuheng Huang

Dominic Austria added 2 comments

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Dominic Austria . resolved

n

File chrome/browser/ui/toolbar/app_menu_model.cc

Line 1846, Patchset 1 (Latest): if (features::IsTabGroupMenuMoreEntryPointsEnabled()) {

Yuheng Huang . unresolved

Having 3 dots menu to have 2 Create New Tab Group menu item is confusing(One under top level one on the tab group submenu). Is it a good idea to do that at all? @dpen...@chromium.org what do you think?

David Pennington

if this feature flag is on, lets move the button up to the top level like in the UI updates request and remove the one in the submenu, but yes youre right i think this might be a bit confusing if we were to have 2

Dominic Austria

That makes more sense, will make those changes

Open in Gerrit

Related details

Attention is currently required from:

David Pennington
Yuheng Huang

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Dominic Austria (Gerrit)

unread,

Sep 17, 2025, 1:39:22 PM9/17/25

to David Pennington, Yuheng Huang, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from David Pennington and Yuheng Huang

Dominic Austria added 1 comment

File chrome/browser/ui/views/toolbar/app_menu.cc

Line 1261, Patchset 1 (Latest): stg_everything_menu_->ExecuteCommand(command_id, mouse_event_flags);

Yuheng Huang . unresolved

The logic seems to be incorrect for this route. The original code has an early return in this case. Now since the return is removed, entry.first->ActivatedAt is called, causing create new tab group to execute twice.

Dominic Austria

You're right oops. That probably explains some of the test failures.

Dominic Austria

It really is confusing to have two buttons, huh.

satisfied_requirement

unsatisfied_requirement

open

diffy

Yuheng Huang (Gerrit)

unread,

Sep 18, 2025, 11:44:50 AM9/18/25

to Dominic Austria, David Pennington, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from David Pennington and Dominic Austria

Yuheng Huang added 1 comment

File chrome/browser/ui/toolbar/app_menu_model.cc

Line 1846, Patchset 1: if (features::IsTabGroupMenuMoreEntryPointsEnabled()) {

Yuheng Huang . unresolved

Having 3 dots menu to have 2 Create New Tab Group menu item is confusing(One under top level one on the tab group submenu). Is it a good idea to do that at all? @dpen...@chromium.org what do you think?

David Pennington

if this feature flag is on, lets move the button up to the top level like in the UI updates request and remove the one in the submenu, but yes youre right i think this might be a bit confusing if we were to have 2

Dominic Austria

That makes more sense, will make those changes

Yuheng Huang

Based on product feedback, do we still want 2 new tab group buttons from the app menu? If we do, my opinion is we should have another command id, say IDC_CREATE_NEW_TAB_GROUP_TOP_LEVEL to avoid confusion.

Open in Gerrit

Related details

Attention is currently required from:

David Pennington
Dominic Austria

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Dominic Austria (Gerrit)

unread,

Sep 21, 2025, 3:57:58 AM9/21/25

to David Pennington, Yuheng Huang, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from David Pennington and Yuheng Huang

Dominic Austria added 3 comments

File chrome/browser/ui/toolbar/app_menu_model.cc

Line 1846, Patchset 1: if (features::IsTabGroupMenuMoreEntryPointsEnabled()) {

Yuheng Huang . unresolved

Having 3 dots menu to have 2 Create New Tab Group menu item is confusing(One under top level one on the tab group submenu). Is it a good idea to do that at all? @dpen...@chromium.org what do you think?

David Pennington

if this feature flag is on, lets move the button up to the top level like in the UI updates request and remove the one in the submenu, but yes youre right i think this might be a bit confusing if we were to have 2

Dominic Austria

That makes more sense, will make those changes

Yuheng Huang

Based on product feedback, do we still want 2 new tab group buttons from the app menu? If we do, my opinion is we should have another command id, say IDC_CREATE_NEW_TAB_GROUP_TOP_LEVEL to avoid confusion.

Dominic Austria

Yes we now want 2 new tab group buttons. I agree, I think this will simplify code in multiple places. like AppMenu::ExecuteCommand()and AppMenuModel::Build().

File chrome/browser/ui/toolbar/app_menu_model_interactive_uitest.cc

Line 400, Patchset 2:}

Dominic Austria . unresolved

I think this test is now obsolete since I test this behaviour in saved_tab_group_interactive_uitest.cc . Can I remove it?

File chrome/browser/ui/views/toolbar/app_menu.cc

Line 1261, Patchset 1: stg_everything_menu_->ExecuteCommand(command_id, mouse_event_flags);

Yuheng Huang . resolved

The logic seems to be incorrect for this route. The original code has an early return in this case. Now since the return is removed, entry.first->ActivatedAt is called, causing create new tab group to execute twice.

Dominic Austria

You're right oops. That probably explains some of the test failures.

Dominic Austria

It really is confusing to have two buttons, huh.

Dominic Austria

Done

Open in Gerrit

Related details

Attention is currently required from:

David Pennington
Yuheng Huang

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Dominic Austria (Gerrit)

unread,

Sep 23, 2025, 6:40:29 PM9/23/25

to David Pennington, Yuheng Huang, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from David Pennington and Yuheng Huang

Dominic Austria added 2 comments

File chrome/browser/ui/toolbar/app_menu_model.cc

Line 1846, Patchset 1: if (features::IsTabGroupMenuMoreEntryPointsEnabled()) {

Yuheng Huang . resolved

Having 3 dots menu to have 2 Create New Tab Group menu item is confusing(One under top level one on the tab group submenu). Is it a good idea to do that at all? @dpen...@chromium.org what do you think?

David Pennington

if this feature flag is on, lets move the button up to the top level like in the UI updates request and remove the one in the submenu, but yes youre right i think this might be a bit confusing if we were to have 2

Dominic Austria

That makes more sense, will make those changes

Yuheng Huang

Based on product feedback, do we still want 2 new tab group buttons from the app menu? If we do, my opinion is we should have another command id, say IDC_CREATE_NEW_TAB_GROUP_TOP_LEVEL to avoid confusion.

Dominic Austria

Yes we now want 2 new tab group buttons. I agree, I think this will simplify code in multiple places. like AppMenu::ExecuteCommand()and AppMenuModel::Build().

Dominic Austria

Done

File chrome/browser/ui/toolbar/app_menu_model_interactive_uitest.cc

Line 400, Patchset 2:}

Dominic Austria . resolved

I think this test is now obsolete since I test this behaviour in saved_tab_group_interactive_uitest.cc . Can I remove it?

Dominic Austria

Done

Open in Gerrit

Related details

Attention is currently required from:

David Pennington
Yuheng Huang

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Darryl James (Gerrit)

unread,

Sep 23, 2025, 7:35:48 PM9/23/25

to Dominic Austria, David Pennington, Yuheng Huang, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from David Pennington, Dominic Austria and Yuheng Huang

Darryl James added 3 comments

Commit Message

Line 9, Patchset 8 (Latest):NO_IFTT='new command is not gated on fenced frame network status'

Darryl James . unresolved

Put this below change-id 👍

File chrome/app/chrome_command_ids.h

Line 112, Patchset 8 (Latest):#define IDC_CREATE_NEW_TAB_GROUP_TOP_LEVEL 34106

Darryl James . unresolved

question: What does top level mean in this case?

File chrome/browser/ui/views/toolbar/app_menu.cc

Line 1267, Patchset 8 (Latest): if (command_id == IDC_CREATE_NEW_TAB_GROUP_TOP_LEVEL) {

Darryl James . unresolved

Can we add a brief comment explaining why we are early returning and not executing the command here?

Open in Gerrit

Related details

Attention is currently required from:

David Pennington
Dominic Austria
Yuheng Huang

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Yuheng Huang (Gerrit)

unread,

Sep 24, 2025, 12:07:35 PM9/24/25

to Dominic Austria, Darryl James, David Pennington, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from David Pennington and Dominic Austria

Yuheng Huang added 3 comments

Commit Message

Line 9, Patchset 8 (Latest):NO_IFTT='new command is not gated on fenced frame network status'

Darryl James . unresolved

Put this below change-id 👍

Yuheng Huang

What is NO_IFTT by the way? We should have a brief description of what the CL does similar to other CLs.

File chrome/browser/ui/views/toolbar/app_menu.cc

Line 1267, Patchset 8 (Latest): if (command_id == IDC_CREATE_NEW_TAB_GROUP_TOP_LEVEL) {

Darryl James . unresolved

Can we add a brief comment explaining why we are early returning and not executing the command here?

Yuheng Huang

Do we need this block at all? If the feature flag is disabled I don't think we will run into this if case since the menu item will not be created.

Line 1331, Patchset 8 (Latest):

Yuheng Huang . unresolved

nit: remove this new line change

Open in Gerrit

Related details

Attention is currently required from:

David Pennington
Dominic Austria

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Dominic Austria (Gerrit)

unread,

Sep 24, 2025, 12:16:09 PM9/24/25

to Darryl James, David Pennington, Yuheng Huang, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from Darryl James, David Pennington and Yuheng Huang

Dominic Austria added 3 comments

File chrome/app/chrome_command_ids.h

Line 112, Patchset 8 (Latest):#define IDC_CREATE_NEW_TAB_GROUP_TOP_LEVEL 34106

Darryl James . unresolved

question: What does top level mean in this case?

Dominic Austria

So we are putting a 'create new tab group' menu item in the 3dot menu, but in the first layer (it would be between the 'new tab' and 'new window' menu options).

Yuheng and I thought it would be less confusing to create a new command ID for this menu item, instead of having two menu items that correspond to the same command ID

File chrome/browser/ui/views/toolbar/app_menu.cc

Line 1267, Patchset 8 (Latest): if (command_id == IDC_CREATE_NEW_TAB_GROUP_TOP_LEVEL) {

Darryl James . unresolved

Can we add a brief comment explaining why we are early returning and not executing the command here?

Yuheng Huang

Do we need this block at all? If the feature flag is disabled I don't think we will run into this if case since the menu item will not be created.

Dominic Austria

hmm, i think you are right. ill check and see and remove it if I can

Line 1331, Patchset 8 (Latest):

Yuheng Huang . unresolved

nit: remove this new line change

Dominic Austria

my bad

Open in Gerrit

Related details

Attention is currently required from:

Darryl James
David Pennington
Yuheng Huang

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Dominic Austria (Gerrit)

unread,

Sep 24, 2025, 2:30:56 PM9/24/25

to Darryl James, David Pennington, Yuheng Huang, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from Darryl James, David Pennington and Yuheng Huang

Dominic Austria added 1 comment

Commit Message

Line 9, Patchset 8:NO_IFTT='new command is not gated on fenced frame network status'

Darryl James . resolved

Put this below change-id 👍

Yuheng Huang

What is NO_IFTT by the way? We should have a brief description of what the CL does similar to other CLs.

Dominic Austria

There is a linter warning whenever I make changes to chrome/app/chrome_command_ids.h and to silence it I have to put this in my commit description. In any case I added a more detailed CL description

Open in Gerrit

Related details

Attention is currently required from:

Darryl James
David Pennington
Yuheng Huang

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Dominic Austria (Gerrit)

unread,

Sep 24, 2025, 2:40:19 PM9/24/25

to Darryl James, David Pennington, Yuheng Huang, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from Darryl James, David Pennington and Yuheng Huang

Dominic Austria added 2 comments

File chrome/browser/ui/views/toolbar/app_menu.cc

Line 1267, Patchset 8: if (command_id == IDC_CREATE_NEW_TAB_GROUP_TOP_LEVEL) {

Darryl James . resolved

Can we add a brief comment explaining why we are early returning and not executing the command here?

Yuheng Huang

Do we need this block at all? If the feature flag is disabled I don't think we will run into this if case since the menu item will not be created.

Dominic Austria

hmm, i think you are right. ill check and see and remove it if I can

Dominic Austria

Done

Line 1331, Patchset 8:

Yuheng Huang . resolved

nit: remove this new line change

Dominic Austria

my bad

Dominic Austria

Done

Open in Gerrit

Related details

Attention is currently required from:

Darryl James
David Pennington
Yuheng Huang

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Darryl James (Gerrit)

unread,

Sep 24, 2025, 2:41:57 PM9/24/25

to Dominic Austria, David Pennington, Yuheng Huang, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from David Pennington, Dominic Austria and Yuheng Huang

Darryl James added 1 comment

Commit Message

Line 9, Patchset 8:NO_IFTT='new command is not gated on fenced frame network status'

Darryl James . resolved

Put this below change-id 👍

Yuheng Huang

What is NO_IFTT by the way? We should have a brief description of what the CL does similar to other CLs.

Dominic Austria

There is a linter warning whenever I make changes to chrome/app/chrome_command_ids.h and to silence it I have to put this in my commit description. In any case I added a more detailed CL description

Darryl James

This warning is mainly applicable to commands that can navigate the browser in some way other that creating a default new tab. This is because fenced frames (which are supposed to be more secure than iframes in html) want to restrict network access / data sharing between the frame and the embeddings. This is so the data can be isolated.

For the vast majority of cases this is not applicable for new commands but is something to keep in mind.

Some documentation if anyone is interested!:
1) https://github.com/WICG/fenced-frame/blob/master/explainer/fenced_frames_with_local_unpartitioned_data_access.md#revoking-network-access
2) https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/renderer_context_menu/render_view_context_menu.h;drc=2807cfc042c06d4c6bc767ccc33fb194e17e47d2;l=563

Open in Gerrit

Related details

Attention is currently required from:

David Pennington
Dominic Austria
Yuheng Huang

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Darryl James (Gerrit)

unread,

Sep 24, 2025, 2:45:35 PM9/24/25

to Dominic Austria, David Pennington, Yuheng Huang, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from David Pennington, Dominic Austria and Yuheng Huang

Darryl James voted and added 2 comments

Votes added by Darryl James

Code-Review

+1

2 comments

Commit Message

Line 14, Patchset 10 (Latest):NO_IFTT='new command is not gated on fenced frame network status'

Darryl James . unresolved

nit: 1 more T to get rid of the linter error `NO_IFTTT=`

File chrome/app/chrome_command_ids.h

Line 112, Patchset 8:#define IDC_CREATE_NEW_TAB_GROUP_TOP_LEVEL 34106

Darryl James . resolved

question: What does top level mean in this case?

Dominic Austria

So we are putting a 'create new tab group' menu item in the 3dot menu, but in the first layer (it would be between the 'new tab' and 'new window' menu options).
Yuheng and I thought it would be less confusing to create a new command ID for this menu item, instead of having two menu items that correspond to the same command ID

Darryl James

Got it - maybe `IDC_CREATE_NEW_TAB_GROUP_APP_MENU` instead?

I agree with having 2 commands for this, let's leave the name as is for now since I can't come up with something more explicit and don't want to block on naming.

We can revisit and rename this command when we come up with something better 👍

Open in Gerrit

Related details

Attention is currently required from:

David Pennington
Dominic Austria
Yuheng Huang

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Dominic Austria (Gerrit)

unread,

Sep 24, 2025, 2:50:47 PM9/24/25

to Darryl James, David Pennington, Yuheng Huang, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from David Pennington and Yuheng Huang

Dominic Austria added 1 comment

Commit Message

Line 14, Patchset 10:NO_IFTT='new command is not gated on fenced frame network status'

Darryl James . resolved

nit: 1 more T to get rid of the linter error `NO_IFTTT=`

Dominic Austria

good catch, not sure why i missed that

Open in Gerrit

Related details

Attention is currently required from:

David Pennington
Yuheng Huang

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Yuheng Huang (Gerrit)

unread,

Sep 24, 2025, 5:29:52 PM9/24/25

to Dominic Austria, Darryl James, David Pennington, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from David Pennington and Dominic Austria

Yuheng Huang voted and added 2 comments

Votes added by Yuheng Huang

Code-Review

+1

2 comments

File chrome/browser/ui/views/toolbar/app_menu.cc

Line 1216, Patchset 11 (Latest): return features::IsTabGroupMenuMoreEntryPointsEnabled();

Yuheng Huang . unresolved

nit: Can just return true since if the feature flag is not enabled this command will not exist.

Line 1305, Patchset 11 (Latest): if (features::IsTabGroupMenuMoreEntryPointsEnabled()) {

Yuheng Huang . unresolved

nit: No need to check this since if feature flag is not enabled this command does not show anywhere.

Open in Gerrit

Related details

Attention is currently required from:

David Pennington
Dominic Austria

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Darryl James (Gerrit)

unread,

Sep 24, 2025, 7:41:43 PM9/24/25

to Dominic Austria, Yuheng Huang, David Pennington, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from David Pennington, Dominic Austria and Yuheng Huang

Darryl James voted and added 2 comments

Votes added by Darryl James

Code-Review

+1

2 comments

Patchset-level comments

File-level comment, Patchset 12 (Latest):

Darryl James . resolved

lgtm!

File chrome/browser/ui/views/toolbar/app_menu.cc

Line 1308, Patchset 12 (Latest): }

Darryl James . unresolved

super duper nit: Remove trailing whitespace!

Open in Gerrit

Related details

Attention is currently required from:

David Pennington
Dominic Austria
Yuheng Huang

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review

No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Dominic Austria (Gerrit)

unread,

Sep 24, 2025, 7:48:25 PM9/24/25

to Darryl James, Yuheng Huang, David Pennington, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from Darryl James, David Pennington and Yuheng Huang

Dominic Austria added 1 comment

File chrome/browser/ui/views/toolbar/app_menu.cc

Line 1308, Patchset 12 (Latest): }

Darryl James . unresolved

super duper nit: Remove trailing whitespace!

Dominic Austria

haha ok. i am trying vim so its my bad

Open in Gerrit

Related details

Attention is currently required from:

Darryl James
David Pennington
Yuheng Huang

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Darryl James (Gerrit)

unread,

Sep 24, 2025, 7:50:16 PM9/24/25

to Dominic Austria, Yuheng Huang, David Pennington, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from David Pennington, Dominic Austria and Yuheng Huang

Darryl James voted and added 1 comment

Votes added by Darryl James

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 13 (Latest):

Darryl James . resolved

lgtm!

Open in Gerrit

Related details

Attention is currently required from:

David Pennington
Dominic Austria
Yuheng Huang

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Dominic Austria (Gerrit)

unread,

Sep 24, 2025, 7:50:47 PM9/24/25

to Darryl James, Yuheng Huang, David Pennington, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from David Pennington and Yuheng Huang

Dominic Austria added 1 comment

File chrome/browser/ui/views/toolbar/app_menu.cc

Line 1308, Patchset 12: }

Darryl James . resolved

super duper nit: Remove trailing whitespace!

Dominic Austria

haha ok. i am trying vim so its my bad

Dominic Austria

Done

Open in Gerrit

Related details

Attention is currently required from:

David Pennington
Yuheng Huang

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Dominic Austria (Gerrit)

unread,

Sep 25, 2025, 1:01:13 PM9/25/25

to Darryl James, Yuheng Huang, David Pennington, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from David Pennington and Yuheng Huang

Dominic Austria added 2 comments

File chrome/browser/ui/views/toolbar/app_menu.cc

Line 1216, Patchset 11: return features::IsTabGroupMenuMoreEntryPointsEnabled();

Yuheng Huang . resolved

nit: Can just return true since if the feature flag is not enabled this command will not exist.

Dominic Austria

Done

Line 1305, Patchset 11: if (features::IsTabGroupMenuMoreEntryPointsEnabled()) {

Yuheng Huang . resolved

nit: No need to check this since if feature flag is not enabled this command does not show anywhere.

Dominic Austria

Done

Open in Gerrit

Related details

Attention is currently required from:

David Pennington
Yuheng Huang

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Yuheng Huang (Gerrit)

unread,

Sep 25, 2025, 2:56:54 PM9/25/25

to Dominic Austria, Darryl James, David Pennington, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from David Pennington and Dominic Austria

Yuheng Huang voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

David Pennington
Dominic Austria

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Dominic Austria (Gerrit)

unread,

Sep 25, 2025, 3:37:13 PM9/25/25

to Yuheng Huang, Darryl James, David Pennington, Chromium LUCI CQ, chromium...@chromium.org

Attention needed from David Pennington

Dominic Austria voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention is currently required from:

David Pennington

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Sep 25, 2025, 4:27:31 PM9/25/25

to Dominic Austria, Yuheng Huang, Darryl James, David Pennington, chromium...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

[TabGroupMenus] add create new tab group option to app menu top level

Adds a 'Create new tab group' menu item to the three dot menu at the top
level, in between 'New Tab' and 'New Window'

NO_IFTTT='new command is not gated on fenced frame network status'

Bug: 433807039

Change-Id: I008c26c1f5d010d23c642549014ad2a61353b2e0

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6955652

Reviewed-by: Darryl James <dlj...@chromium.org>

Commit-Queue: Dominic Austria <dominic...@google.com>

Reviewed-by: Yuheng Huang <yuh...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1520839}

Files:

M chrome/app/chrome_command_ids.h
M chrome/browser/ui/browser_command_controller.cc
M chrome/browser/ui/toolbar/app_menu_model.cc
M chrome/browser/ui/toolbar/app_menu_model.h
M chrome/browser/ui/toolbar/app_menu_model_interactive_uitest.cc
M chrome/browser/ui/views/bookmarks/saved_tab_groups/saved_tab_group_interactive_uitest.cc
M chrome/browser/ui/views/toolbar/app_menu.cc

Change size: M

Delta: 7 files changed, 61 insertions(+), 4 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Darryl James, +1 by Yuheng Huang

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

[TabGroupMenus] add create new tab group option to app menu top level [chromium/src : main]

Dominic Austria (Gerrit)

Dominic Austria added 2 comments

Related details

Yuheng Huang (Gerrit)

Yuheng Huang added 2 comments

Related details

David Pennington (Gerrit)

David Pennington added 1 comment

Related details

Dominic Austria (Gerrit)

Dominic Austria added 1 comment

Related details

Dominic Austria (Gerrit)

Dominic Austria added 2 comments

Related details

Dominic Austria (Gerrit)

Dominic Austria added 1 comment

Yuheng Huang (Gerrit)

Yuheng Huang added 1 comment

Related details

Dominic Austria (Gerrit)

Dominic Austria added 3 comments

Related details

Dominic Austria (Gerrit)

Dominic Austria added 2 comments

Related details

Darryl James (Gerrit)

Darryl James added 3 comments

Related details

Yuheng Huang (Gerrit)

Yuheng Huang added 3 comments

Related details

Dominic Austria (Gerrit)

Dominic Austria added 3 comments

Related details

Dominic Austria (Gerrit)

Dominic Austria added 1 comment

Related details

Dominic Austria (Gerrit)

Dominic Austria added 2 comments

Related details

Darryl James (Gerrit)

Darryl James added 1 comment

Related details

Darryl James (Gerrit)

Darryl James voted and added 2 comments

Votes added by Darryl James

2 comments

Related details

Dominic Austria (Gerrit)

Dominic Austria added 1 comment

Related details

Yuheng Huang (Gerrit)

Yuheng Huang voted and added 2 comments

Votes added by Yuheng Huang

2 comments

Related details

Darryl James (Gerrit)

Darryl James voted and added 2 comments

Votes added by Darryl James

2 comments

Related details

Dominic Austria (Gerrit)

Dominic Austria added 1 comment

Related details

Darryl James (Gerrit)

Darryl James voted and added 1 comment

Votes added by Darryl James

1 comment

Related details

Dominic Austria (Gerrit)

Dominic Austria added 1 comment

Related details

Dominic Austria (Gerrit)

Dominic Austria added 2 comments

Related details

Yuheng Huang (Gerrit)

Yuheng Huang voted Code-Review+1

Related details