Hongchan Choi (Gerrit)

unread,

Oct 8, 2025, 6:04:00 PM (2 days ago) Oct 8

to Saqlain, Chromium LUCI CQ, chromium...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org

Attention needed from Michael Wilson and Saqlain

Hongchan Choi added 5 comments

Patchset-level comments

File-level comment, Patchset 3 (Latest):

Hongchan Choi . unresolved

I am proposing re-structuring the code with async/await. Please take a look at my general direction on this approach.

File third_party/blink/web_tests/external/wpt/webaudio/the-audio-api/the-mediaelementaudiosourcenode-interface/no-cors.https.html

Line 18, Patchset 3 (Latest): context.suspend();

Hongchan Choi . unresolved

```suggestion
        await context.suspend();
```

Line 42, Patchset 3 (Latest): await new Promise((resolve, reject) => {

Hongchan Choi . unresolved

```suggestion
        const recordingPromise = await new Promise((resolve, reject) => {
```

Line 50, Patchset 3 (Latest): const expected = new Float32Array(channelData.length).fill(0);

Hongchan Choi . unresolved

```suggestion
                  const expected = new Float32Array(channelData.length);
```

Line 62, Patchset 3 (Latest):

          context.resume().then(() => {
            const playPromise = audioElement.play();

Hongchan Choi . unresolved

```suggestion
          await context.resume();
          await audioElement.play();
          const recordBuffer = await recordingPromise;
```

Open in Gerrit

Related details

Attention is currently required from:

Michael Wilson
Saqlain

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 Wilson (Gerrit)

unread,

Oct 8, 2025, 7:25:44 PM (2 days ago) Oct 8

to Saqlain, Chromium LUCI CQ, Hongchan Choi, chromium...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org

Attention needed from Saqlain

Michael Wilson added 1 comment

Patchset-level comments

File-level comment, Patchset 3 (Latest):

Michael Wilson . resolved

I don't have any additional comments on the current patchset. I will continue my review when Hongchan's restructuring proposal is addressed.

Open in Gerrit

Related details

Attention is currently required from:

Saqlain

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

Saqlain (Gerrit)

unread,

Oct 9, 2025, 4:28:12 PM (2 days ago) Oct 9

to Chromium LUCI CQ, Hongchan Choi, chromium...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org

Attention needed from Hongchan Choi

Saqlain added 5 comments

Patchset-level comments

File-level comment, Patchset 3:

Hongchan Choi . unresolved

I am proposing re-structuring the code with async/await. Please take a look at my general direction on this approach.

Saqlain

thanks, I also removed `try/catch` or `.catch()` handlers that were present in the earlier code.

As it will automatically treats any unhandled promise rejections or thrown errors inside a promise_test as test failures.

File third_party/blink/web_tests/external/wpt/webaudio/the-audio-api/the-mediaelementaudiosourcenode-interface/no-cors.https.html

Line 18, Patchset 3: context.suspend();

Hongchan Choi . resolved

```suggestion
        await context.suspend();
```

Saqlain

Done

Line 42, Patchset 3: await new Promise((resolve, reject) => {

Hongchan Choi . resolved

```suggestion
        const recordingPromise = await new Promise((resolve, reject) => {
```

Saqlain

Done

Line 50, Patchset 3: const expected = new Float32Array(channelData.length).fill(0);

Hongchan Choi . resolved

```suggestion
                  const expected = new Float32Array(channelData.length);
```

Saqlain

Done

Line 62, Patchset 3:

          context.resume().then(() => {
            const playPromise = audioElement.play();

Hongchan Choi . resolved

```suggestion
          await context.resume();
          await audioElement.play();
          const recordBuffer = await recordingPromise;
```

Saqlain

Done

Open in Gerrit

Related details

Attention is currently required from:

Hongchan Choi

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 Wilson (Gerrit)

unread,

Oct 10, 2025, 6:21:25 PM (7 hours ago) Oct 10

to Saqlain, Chromium LUCI CQ, Hongchan Choi, chromium...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org

Attention needed from Hongchan Choi and Saqlain

Michael Wilson voted and added 2 comments

Votes added by Michael Wilson

Code-Review

+1

2 comments

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Michael Wilson . resolved

LGTM once the comment is updated.

File third_party/blink/web_tests/external/wpt/webaudio/the-audio-api/the-mediaelementaudiosourcenode-interface/no-cors.https.html

Line 52, Patchset 5 (Latest):


        // The recorded data from MESN must be non-zero. The source file
        // contains 4 channels of sine wave.

Michael Wilson . unresolved

It looks like this comment in the original test was copied from the "cors-check" test; it should say "zero" instead of "non-zero" for this no-cors test.
```suggestion

        // The recorded data from MESN must be zero.
```

Open in Gerrit

Related details

Attention is currently required from:

Hongchan Choi
Saqlain

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

Hongchan Choi (Gerrit)

unread,

Oct 10, 2025, 6:37:36 PM (7 hours ago) Oct 10

to Saqlain, Chromium LUCI CQ, chromium...@chromium.org, blink-revie...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org

Attention needed from Saqlain

Hongchan Choi voted and added 3 comments

Votes added by Hongchan Choi

Code-Review

+1

3 comments

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Hongchan Choi . resolved

LGTM with nits

File third_party/blink/web_tests/external/wpt/webaudio/the-audio-api/the-mediaelementaudiosourcenode-interface/no-cors.https.html

Line 37, Patchset 5 (Latest): context, `recorder-processor`, {channelCount: 4});

Hongchan Choi . unresolved

The convention is to use single quote when you don't have anything to substitute.

Line 42, Patchset 5 (Latest): if (event.data && event.data.type === `recordfinished`) {

Hongchan Choi . unresolved

Ditto.

Open in Gerrit

Related details

Attention is currently required from:

Saqlain

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

Reply all

Reply to author

Forward

[webaudio-testharness] Migrate no-cors.https.html [chromium/src : main]

Hongchan Choi (Gerrit)

Hongchan Choi added 5 comments

Related details

Michael Wilson (Gerrit)

Michael Wilson added 1 comment

Related details

Saqlain (Gerrit)

Saqlain added 5 comments

Related details

Michael Wilson (Gerrit)

Michael Wilson voted and added 2 comments

Votes added by Michael Wilson

2 comments

Related details

Hongchan Choi (Gerrit)

Hongchan Choi voted and added 3 comments

Votes added by Hongchan Choi

3 comments

Related details