Mikita Kuchyn (Gerrit)

unread,

Jun 12, 2025, 8:54:51 AM6/12/25

to Dominik Klemba, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org

Mikita Kuchyn added 1 comment

File chrome/browser/ui/views/media_preview/mic_preview/audio_stream_coordinator_browsertest.cc

Line 48, Patchset 3 (Latest): base::OnceClosure quit_closure_;

Mikita Kuchyn . unresolved

I'm not sure that the change from storing the base::RunLoop to QuitClosure is needed here. I think we can leave RunLoop as it was. I'm not sure that the pattern used in this CL is widespread, so let's ask Tom what he thinks.

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
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

Dominik Klemba (Gerrit)

unread,

Jun 13, 2025, 3:10:06 AM6/13/25

to Mikita Kuchyn, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org

Dominik Klemba added 1 comment

File chrome/test/BUILD.gn

Line 5809, Patchset 3 (Latest): if (!is_android && !is_chromeos && (is_linux || is_win || is_mac)) {

Dominik Klemba . unresolved

TODO: remove "!is_android"

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
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

Dominik Klemba (Gerrit)

unread,

Jun 13, 2025, 9:32:38 AM6/13/25

to Mikita Kuchyn, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org

Dominik Klemba abandoned this change

Related details

Attention set is empty

Submit Requirements:

Code-Coverage
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

Mikita Kuchyn (Gerrit)

unread,

Jun 13, 2025, 1:50:29 PM6/13/25

to Tom Lukaszewicz, Dominik Klemba, chromium...@chromium.org, feature-me...@chromium.org

Attention needed from Dominik Klemba and Tom Lukaszewicz

Mikita Kuchyn voted and added 1 comment

Votes added by Mikita Kuchyn

Auto-Submit	+1
Commit-Queue	+2

1 comment

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Mikita Kuchyn . resolved

Hey, PTAL :)

Open in Gerrit

Related details

Attention is currently required from:

Dominik Klemba
Tom Lukaszewicz

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Tom Lukaszewicz (Gerrit)

unread,

Jun 15, 2025, 3:21:35 PM6/15/25

to Mikita Kuchyn, Chromium LUCI CQ, Dominik Klemba, chromium...@chromium.org, feature-me...@chromium.org

Attention needed from Dominik Klemba and Mikita Kuchyn

Tom Lukaszewicz added 1 comment

File chrome/browser/ui/views/media_preview/mic_preview/audio_stream_coordinator_browsertest.cc

Line 100, Patchset 1 (Latest):

  EXPECT_TRUE(
      base::test::RunUntil([&]() { return call_count >= kAudioBusesNumber; }));

Tom Lukaszewicz . unresolved

Avoiding `RunLoop::RunUntilIdle()` is definitely a win here. Your approach does work, though we can simplify this a bit to avoid the need for the `call_count` variable (keeping the setup from the unit test mostly the same).

Instead we can `RunUntil` the expectations have been satisfied, something like
```
  EXPECT_TRUE(base::test::RunUntil(
      [&]() { return testing::Mock::VerifyAndClear(&callback); }));
```

Open in Gerrit

Related details

Attention is currently required from:

Dominik Klemba
Mikita Kuchyn

Submit Requirements:

Code-Coverage
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

Mikita Kuchyn (Gerrit)

unread,

Jun 23, 2025, 5:58:22 AM6/23/25

to Chromium LUCI CQ, Tom Lukaszewicz, Dominik Klemba, chromium...@chromium.org, feature-me...@chromium.org

Attention needed from Dominik Klemba and Tom Lukaszewicz

Mikita Kuchyn voted and added 1 comment

Votes added by Mikita Kuchyn

Auto-Submit	+1
Commit-Queue	+2

1 comment

File chrome/browser/ui/views/media_preview/mic_preview/audio_stream_coordinator_browsertest.cc

Line 100, Patchset 1: EXPECT_TRUE(


      base::test::RunUntil([&]() { return call_count >= kAudioBusesNumber; }));

Tom Lukaszewicz . resolved

Avoiding `RunLoop::RunUntilIdle()` is definitely a win here. Your approach does work, though we can simplify this a bit to avoid the need for the `call_count` variable (keeping the setup from the unit test mostly the same).
Instead we can `RunUntil` the expectations have been satisfied, something like
```
  EXPECT_TRUE(base::test::RunUntil(
      [&]() { return testing::Mock::VerifyAndClear(&callback); }));
```

Mikita Kuchyn

Done

Open in Gerrit

Related details

Attention is currently required from:

Dominik Klemba
Tom Lukaszewicz

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Dominik Klemba (Gerrit)

unread,

Jun 23, 2025, 10:16:52 AM6/23/25

to Mikita Kuchyn, Chromium LUCI CQ, Tom Lukaszewicz, chromium...@chromium.org, feature-me...@chromium.org

Attention needed from Mikita Kuchyn and Tom Lukaszewicz

Dominik Klemba added 1 comment

Patchset-level comments

File-level comment, Patchset 3 (Latest):

Dominik Klemba . resolved

LGTM, but I can't give you +1.

For comparison, same test made by AI: https://chromium-review.googlesource.com/c/chromium/src/+/6592061

Open in Gerrit

Related details

Attention is currently required from:

Mikita Kuchyn
Tom Lukaszewicz

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Tom Lukaszewicz (Gerrit)

unread,

Jun 23, 2025, 2:23:41 PM6/23/25

to Mikita Kuchyn, Keren Zhu, Dominik Klemba, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org

Attention needed from Keren Zhu and Mikita Kuchyn

Tom Lukaszewicz voted and added 1 comment

Votes added by Tom Lukaszewicz

Code-Review	+1
Commit-Queue	+1

1 comment

Patchset-level comments

File-level comment, Patchset 3 (Latest):

Tom Lukaszewicz . resolved

lgtm! Keren could you stamp for committers?

Open in Gerrit

Related details

Attention is currently required from:

Keren Zhu
Mikita Kuchyn

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

Keren Zhu (Gerrit)

unread,

Jun 23, 2025, 2:26:01 PM6/23/25

to Mikita Kuchyn, Tom Lukaszewicz, Dominik Klemba, Chromium LUCI CQ, chromium...@chromium.org, feature-me...@chromium.org

Attention needed from Mikita Kuchyn

Keren Zhu voted

Code-Review	+1
Commit-Queue	+2

Open in Gerrit

Related details

Attention is currently required from:

Mikita Kuchyn

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,

Jun 23, 2025, 3:26:24 PM6/23/25

to Mikita Kuchyn, Keren Zhu, Tom Lukaszewicz, Dominik Klemba, chromium...@chromium.org, feature-me...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

Migrate AudioStreamCoordinatorTest into InProcessBrowserTest

Use of TestWithBrowserView is being deprecated and existing
tests requiring a Browser environment are being migrated to
being browser tests.

This CL updates the AudioStreamCoordinatorTest fixture to an
InProcessBrowserTest and removes the TestWithBrowserView
dependency.

Bug: 417766643

Change-Id: I818159903eca9814deab85db53169c5d4a303919

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

Reviewed-by: Keren Zhu <kere...@chromium.org>

Reviewed-by: Tom Lukaszewicz <tl...@chromium.org>

Commit-Queue: Tom Lukaszewicz <tl...@chromium.org>

Commit-Queue: Keren Zhu <kere...@chromium.org>

Auto-Submit: Mikita Kuchyn <kuc...@chromium.org>

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

Files:

R chrome/browser/ui/views/media_preview/mic_preview/audio_stream_coordinator_browsertest.cc
M chrome/test/BUILD.gn

Change size: M

Delta: 2 files changed, 32 insertions(+), 19 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Tom Lukaszewicz, +1 by Keren Zhu

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

Migrate AudioStreamCoordinatorTest into InProcessBrowserTest [chromium/src : main]

Mikita Kuchyn (Gerrit)

Mikita Kuchyn added 1 comment

Related details

Dominik Klemba (Gerrit)

Dominik Klemba added 1 comment

Related details

Dominik Klemba (Gerrit)

Dominik Klemba abandoned this change

Related details

Mikita Kuchyn (Gerrit)

Mikita Kuchyn voted and added 1 comment

Votes added by Mikita Kuchyn

1 comment

Related details

Tom Lukaszewicz (Gerrit)

Tom Lukaszewicz added 1 comment

Related details

Mikita Kuchyn (Gerrit)

Mikita Kuchyn voted and added 1 comment

Votes added by Mikita Kuchyn

1 comment

Related details

Dominik Klemba (Gerrit)

Dominik Klemba added 1 comment

Related details

Tom Lukaszewicz (Gerrit)

Tom Lukaszewicz voted and added 1 comment

Votes added by Tom Lukaszewicz

1 comment

Related details

Keren Zhu (Gerrit)

Keren Zhu voted

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information