James Cook (Gerrit)

unread,

Jan 7, 2026, 6:57:01 PM (2 days ago) Jan 7

to Michael Wojcicka, Achuith Bhandarkar, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention needed from Michael Wojcicka

James Cook added 8 comments

Patchset-level comments

File-level comment, Patchset 1 (Latest):

James Cook . resolved

Michael, please take a look. This looks big, but it's almost all just cut/paste between files. Thanks!

File chrome/browser/extensions/api/tab_groups/tab_groups_api_browsertest.cc

Line 611, Patchset 1 (Latest):

James Cook . unresolved

Most of this is cut/paste, with TEST_F -> IN_PROC_BROWSER_TEST_F and the test suite class name updated. I've called out the differences.

Line 625, Patchset 1 (Latest): Browser* browser2 = CreateBrowser(profile());

James Cook . unresolved

I had to rewrite this block not to use TestBrowserWindow and instead us a real Browser. (I'll fix this up for Android later.)

Line 632, Patchset 1 (Latest): // A new browser starts with 1 tab open, so iterate from 1.

James Cook . unresolved

Likewise I had to change tab creation to use real tabs, not TestWebContents.

Line 640, Patchset 1 (Latest): ASSERT_EQ(kNumTabs2, tab_strip_model2->count());

James Cook . unresolved

Cut/paste resumes here.

Line 733, Patchset 1 (Latest): BrowserWindow* browser_window = browser()->window();

James Cook . unresolved

I had to change this to use a real BrowserWindow instead of a TestBrowserWindow. Otherise the rest of the test is cut/paste.

Line 770, Patchset 1 (Latest):IN_PROC_BROWSER_TEST_F(TabGroupsApiBrowserTest,

James Cook . unresolved

All cut/paste.

File chrome/browser/extensions/api/tab_groups/tab_groups_api_unittest.cc

Line 49, Patchset 1 (Latest):#include "extensions/common/extension_builder.h"

James Cook . unresolved

I'm not cleaning up the CLs because I'm going to delete this file in 1-2 CLs.

Open in Gerrit

Related details

Attention is currently required from:

Michael Wojcicka

Submit Requirements:

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

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Zoraiz Naeem (Gerrit)

unread,

Jan 7, 2026, 8:38:43 PM (2 days ago) Jan 7

to James Cook, Michael Wojcicka, Achuith Bhandarkar, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention needed from James Cook and Michael Wojcicka

Zoraiz Naeem voted and added 1 comment

Votes added by Zoraiz Naeem

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Zoraiz Naeem . resolved

lgtm!

Open in Gerrit

Related details

Attention is currently required from:

James Cook
Michael Wojcicka

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review

No-Unresolved-Comments

Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Zoraiz Naeem (Gerrit)

unread,

Jan 7, 2026, 8:39:47 PM (2 days ago) Jan 7

to James Cook, Michael Wojcicka, Achuith Bhandarkar, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention needed from James Cook and Michael Wojcicka

Zoraiz Naeem voted and added 1 comment

Votes added by Zoraiz Naeem

Code-Review

+0

1 comment

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Zoraiz Naeem . resolved

Ohh shoot, sorry for the review! I went up relation chain reviewing, without realizing I wasn't a reviewer for this CL.

Open in Gerrit

Related details

Attention is currently required from:

James Cook
Michael Wojcicka

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review

No-Unresolved-Comments

Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

James Cook (Gerrit)

unread,

Jan 7, 2026, 9:05:12 PM (2 days ago) Jan 7

to Zoraiz Naeem, Michael Wojcicka, Achuith Bhandarkar, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention needed from Michael Wojcicka

James Cook added 1 comment

Patchset-level comments

File-level comment, Patchset 2 (Latest):

James Cook . resolved

No worries! I sent this to Michael to try to save you some work. :-)

Open in Gerrit

Related details

Attention is currently required from:

Michael Wojcicka

Submit Requirements:

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

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Michael Wojcicka (Gerrit)

unread,

Jan 8, 2026, 5:51:33 AM (yesterday) Jan 8

to James Cook, Zoraiz Naeem, Achuith Bhandarkar, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Attention needed from James Cook

Michael Wojcicka voted and added 7 comments

Votes added by Michael Wojcicka

Code-Review

+1

7 comments

File chrome/browser/extensions/api/tab_groups/tab_groups_api_browsertest.cc

Line 611, Patchset 1:

James Cook . resolved

Most of this is cut/paste, with TEST_F -> IN_PROC_BROWSER_TEST_F and the test suite class name updated. I've called out the differences.

Michael Wojcicka

Acknowledged

Line 625, Patchset 1: Browser* browser2 = CreateBrowser(profile());

James Cook . resolved

I had to rewrite this block not to use TestBrowserWindow and instead us a real Browser. (I'll fix this up for Android later.)

Michael Wojcicka

Acknowledged

Line 632, Patchset 1: // A new browser starts with 1 tab open, so iterate from 1.

James Cook . resolved

Likewise I had to change tab creation to use real tabs, not TestWebContents.

Michael Wojcicka

Acknowledged

Line 640, Patchset 1: ASSERT_EQ(kNumTabs2, tab_strip_model2->count());

James Cook . resolved

Cut/paste resumes here.

Michael Wojcicka

Acknowledged

Line 733, Patchset 1: BrowserWindow* browser_window = browser()->window();

James Cook . resolved

I had to change this to use a real BrowserWindow instead of a TestBrowserWindow. Otherise the rest of the test is cut/paste.

Michael Wojcicka

Acknowledged

Line 770, Patchset 1:IN_PROC_BROWSER_TEST_F(TabGroupsApiBrowserTest,

James Cook . resolved

All cut/paste.

Michael Wojcicka

Acknowledged

File chrome/browser/extensions/api/tab_groups/tab_groups_api_unittest.cc

Line 49, Patchset 1:#include "extensions/common/extension_builder.h"

James Cook . resolved

I'm not cleaning up the CLs because I'm going to delete this file in 1-2 CLs.

Michael Wojcicka

Acknowledged

Open in Gerrit

Related details

Attention is currently required from:

James Cook

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
Review-Enforcement

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

Gerrit

satisfied_requirement

open

diffy

James Cook (Gerrit)

unread,

Jan 8, 2026, 2:21:15 PM (20 hours ago) Jan 8

to Michael Wojcicka, Zoraiz Naeem, Achuith Bhandarkar, Chromium LUCI CQ, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

James Cook voted and added 1 comment

Votes added by James Cook

Commit-Queue

+2

1 comment

Patchset-level comments

File-level comment, Patchset 2 (Latest):

James Cook . resolved

Thanks for the review!

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

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

Gerrit

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Jan 8, 2026, 2:25:27 PM (20 hours ago) Jan 8

to James Cook, Michael Wojcicka, Zoraiz Naeem, Achuith Bhandarkar, chromium...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

extensions: Port tab group API unit tests to browser tests, part 6

The tab groups API unit test suite provides good coverage, but uses
concepts like Browser and BrowserWindow that are not supposed to be
used anymore in unit tests. (See chrome browser design principles in
//docs or the comment in this browser test file.) Converting to browser
test both fixes this design issue and sets up the tests for future
conversion to run on desktop Android.

This CL cut/pastes over all the remaining tests that don't use the
EventRouter. Those will come in a followup.

Bug: 405219902

Change-Id: I360dc1cd829f54ed3d750fd430c8b6a73e3784a0

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

Commit-Queue: James Cook <jame...@chromium.org>

Reviewed-by: Michael Wojcicka <mw...@google.com>

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

Files:

M chrome/browser/extensions/api/tab_groups/tab_groups_api_browsertest.cc
M chrome/browser/extensions/api/tab_groups/tab_groups_api_unittest.cc

Change size: L

Delta: 2 files changed, 199 insertions(+), 205 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Michael Wojcicka

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

extensions: Port tab group API unit tests to browser tests, part 6 [chromium/src : main]

James Cook (Gerrit)

James Cook added 8 comments

Related details

Zoraiz Naeem (Gerrit)

Zoraiz Naeem voted and added 1 comment

Votes added by Zoraiz Naeem

1 comment

Related details

Zoraiz Naeem (Gerrit)

Zoraiz Naeem voted and added 1 comment

Votes added by Zoraiz Naeem

1 comment

Related details

James Cook (Gerrit)

James Cook added 1 comment

Related details

Michael Wojcicka (Gerrit)

Michael Wojcicka voted and added 7 comments

Votes added by Michael Wojcicka

7 comments

Related details

James Cook (Gerrit)

James Cook voted and added 1 comment

Votes added by James Cook

1 comment

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information