Gabriel Brito (Gerrit)

unread,

Dec 19, 2025, 2:45:27 AM (4 days ago) Dec 19

to Hongchan Choi, chromium...@chromium.org, AyeAye, Chromium LUCI CQ, edgecapab...@microsoft.com, blink-...@chromium.org, feature-me...@chromium.org

Attention needed from Hongchan Choi and Michael Wilson

Gabriel Brito added 1 comment

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Gabriel Brito . resolved

Hey Michael and Hongchan, could you PTAL? Thanks!

Open in Gerrit

Related details

Attention is currently required from:

Hongchan Choi
Michael Wilson

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

unsatisfied_requirement

open

diffy

Hongchan Choi (Gerrit)

unread,

Dec 19, 2025, 11:28:23 AM (4 days ago) Dec 19

to Gabriel Brito, chromium...@chromium.org, AyeAye, Chromium LUCI CQ, edgecapab...@microsoft.com, blink-...@chromium.org, feature-me...@chromium.org

Attention needed from Gabriel Brito and Michael Wilson

Hongchan Choi added 9 comments

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Hongchan Choi . unresolved

Thanks for the good cleaning up! The logic looks pretty simple now.

I’ve added some notes regarding the new tests. It would be great to run them through a formatter/linter. We want to keep these examples clean, as they frequently become the standard reference for the developers.

File third_party/blink/renderer/modules/webaudio/audio_context.cc

Line 1571, Patchset 1 (Latest): DLOG(ERROR) << "AudioContext::OnFrameHidden";

Hongchan Choi . unresolved

DLOG(ERROR) is for debugging. Do you think we need this?

Line 1590, Patchset 1 (Latest): DLOG(ERROR) << "AudioContext::OnFrameShown";

Hongchan Choi . unresolved

Ditto.

File third_party/blink/web_tests/http/tests/webaudio/media-playback-while-not-visible-permission-policy-crossorigin.html

Line 2, Patchset 1 (Latest):<title>Test the behavior of AudioContext with the media-playback-while-not-rendered permission policy</title>

Hongchan Choi . unresolved

Can we do column wrap at 80?

Line 8, Patchset 1 (Latest):

    // The tests in this file depend on
    //resources/media-playback-while-not-visible-permission-policy-iframe.html

Hongchan Choi . unresolved

It looks like we have some formatting issues in test files. Can you format/lint them?

Line 25, Patchset 1 (Latest): let iframe = document.createElement('iframe');

Hongchan Choi . unresolved

Use `const` when possible.

Line 139, Patchset 1 (Latest):</body>

Hongchan Choi . unresolved

Add a new line at the end of the file.

File third_party/blink/web_tests/http/tests/webaudio/resources/media-playback-while-not-visible-permission-policy-iframe.html

Line 22, Patchset 1 (Latest): window.addEventListener('message', (event) => {

Hongchan Choi . unresolved

Can this be async? Also we can use await below.

File third_party/blink/web_tests/http/tests/webaudio/resources/media-playback-while-not-visible-utils.js

Line 16, Patchset 1 (Latest):

    setTimeout(() => {
      window.removeEventListener('message', expectStateChangeEvent);
      resolve('no state change');
    }, 500);

Hongchan Choi . unresolved

I think we want to avoid using setTimeout, and use the primitive provided from testharness here.

Open in Gerrit

Related details

Attention is currently required from:

Gabriel Brito
Michael Wilson

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,

Dec 19, 2025, 6:48:17 PM (4 days ago) Dec 19

to Gabriel Brito, Hongchan Choi, chromium...@chromium.org, AyeAye, Chromium LUCI CQ, edgecapab...@microsoft.com, blink-...@chromium.org, feature-me...@chromium.org

Attention needed from Gabriel Brito

Michael Wilson added 1 comment

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Michael Wilson . resolved

Overall looks good, but I agree with Hongchan's comments about DLOG and formatting the tests.

Open in Gerrit

Related details

Attention is currently required from:

Gabriel Brito

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

Gabriel Brito (Gerrit)

unread,

Dec 19, 2025, 7:46:37 PM (4 days ago) Dec 19

to Hongchan Choi, chromium...@chromium.org, AyeAye, Chromium LUCI CQ, edgecapab...@microsoft.com, blink-...@chromium.org, feature-me...@chromium.org

Attention needed from Hongchan Choi

Gabriel Brito added 9 comments

Patchset-level comments

File-level comment, Patchset 2 (Latest):

Gabriel Brito . resolved

Addressed the comments! PTAL again when convenient. Thanks!

File third_party/blink/renderer/modules/webaudio/audio_context.cc

Line 1571, Patchset 1: DLOG(ERROR) << "AudioContext::OnFrameHidden";

Hongchan Choi . resolved

DLOG(ERROR) is for debugging. Do you think we need this?

Gabriel Brito

Sorry. Forgot to remove after finishing to debug. Done.

Line 1590, Patchset 1: DLOG(ERROR) << "AudioContext::OnFrameShown";

Hongchan Choi . resolved

Ditto.

Gabriel Brito

Done

File third_party/blink/web_tests/http/tests/webaudio/media-playback-while-not-visible-permission-policy-crossorigin.html

Line 2, Patchset 1:<title>Test the behavior of AudioContext with the media-playback-while-not-rendered permission policy</title>

Hongchan Choi . resolved

Can we do column wrap at 80?

Gabriel Brito

Done

Line 8, Patchset 1:

    // The tests in this file depend on
    //resources/media-playback-while-not-visible-permission-policy-iframe.html

Hongchan Choi . resolved

It looks like we have some formatting issues in test files. Can you format/lint them?

Gabriel Brito

Apparently, there is no tool to automatically format HTML files 😞 I did try to look for issues manually though. Hopefully I have been able to address all of them.

Line 25, Patchset 1: let iframe = document.createElement('iframe');

Hongchan Choi . resolved

Use `const` when possible.

Gabriel Brito

Done. Also did the same for the other files.

Line 139, Patchset 1:</body>

Hongchan Choi . resolved

Add a new line at the end of the file.

Gabriel Brito

Done. Also added in the other files.

File third_party/blink/web_tests/http/tests/webaudio/resources/media-playback-while-not-visible-permission-policy-iframe.html

Line 22, Patchset 1: window.addEventListener('message', (event) => {

Hongchan Choi . resolved

Can this be async? Also we can use await below.

Gabriel Brito

Done

File third_party/blink/web_tests/http/tests/webaudio/resources/media-playback-while-not-visible-utils.js

Line 16, Patchset 1: setTimeout(() => {


      window.removeEventListener('message', expectStateChangeEvent);
      resolve('no state change');
    }, 500);

Hongchan Choi . resolved

I think we want to avoid using setTimeout, and use the primitive provided from testharness here.

Gabriel Brito

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

Gabriel Brito (Gerrit)

unread,

Dec 19, 2025, 8:09:06 PM (3 days ago) Dec 19

to Hongchan Choi, chromium...@chromium.org, AyeAye, Chromium LUCI CQ, edgecapab...@microsoft.com, blink-...@chromium.org, feature-me...@chromium.org

Attention needed from Hongchan Choi

Gabriel Brito added 1 comment

Patchset-level comments

File-level comment, Patchset 1:

Hongchan Choi . resolved

Thanks for the good cleaning up! The logic looks pretty simple now.
I’ve added some notes regarding the new tests. It would be great to run them through a formatter/linter. We want to keep these examples clean, as they frequently become the standard reference for the developers.

Gabriel Brito

Totally get it. Pardon for not being as thorough. Hopefully everything has been addressed now.

Open in Gerrit

Related details

Attention is currently required from:

Hongchan Choi

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

unsatisfied_requirement

open

diffy

Hongchan Choi (Gerrit)

unread,

Dec 22, 2025, 1:54:44 PM (18 hours ago) Dec 22

to Gabriel Brito, chromium...@chromium.org, AyeAye, Chromium LUCI CQ, edgecapab...@microsoft.com, blink-...@chromium.org, feature-me...@chromium.org

Attention needed from Gabriel Brito

Hongchan Choi added 13 comments

Patchset-level comments

File-level comment, Patchset 2 (Latest):

Hongchan Choi . resolved

Mostly formatting nits. Thanks for your patience!

File third_party/blink/renderer/modules/webaudio/audio_context.cc

Line 486, Patchset 2 (Latest):

    TRACE_EVENT1("webaudio", "AudioContext::Create - allowed to start", "UUID",
                 audio_context->Uuid());

Hongchan Choi . unresolved

I guess these are artifacts of the linter. I think these are okay, but removing them makes the code review faster! 😊

Line 906, Patchset 2 (Latest):

  TRACE_EVENT2("webaudio", __func__, "UUID", Uuid(), "state",
               static_cast<int>(ContextState()));

Hongchan Choi . unresolved

Ditto. Can we revert these non-functional changes?

File third_party/blink/web_tests/http/tests/webaudio/media-playback-while-not-visible-permission-policy-viewport-crossorigin.html

Line 20, Patchset 2 (Latest): //resources/media-playback-while-not-visible-permission-policy-iframe.html

Hongchan Choi . unresolved

```suggestion
  // resources/media-playback-while-not-visible-permission-policy-iframe.html
```

Line 31, Patchset 2 (Latest): if (document.readyState !=='complete'){

Hongchan Choi . unresolved

```suggestion
    if (document.readyState !== 'complete') {
```

Line 33, Patchset 2 (Latest):

        window.addEventListener('load', resolve)
      })

Hongchan Choi . unresolved

```suggestion
        window.addEventListener('load', resolve);
      });
```

File third_party/blink/web_tests/http/tests/webaudio/media-playback-while-not-visible-permission-policy-visibility-crossorigin.html

Line 22, Patchset 2 (Latest): if (document.readyState !=='complete'){

Hongchan Choi . unresolved

It's a small thing, but the formatting should be:

```suggestion
      if (document.readyState !== 'complete') {
```

Line 23, Patchset 2 (Latest):

        await new Promise(resolve => {
          window.addEventListener('load', resolve)
        })

Hongchan Choi . unresolved

```suggestion
        await new Promise(resolve => {
          window.addEventListener('load', resolve);
        });
```

Line 31, Patchset 2 (Latest):

      iframe.src = 'http://localhost:8000/webaudio/resources/media-playback-while-not-visible-permission-policy-iframe.html';

Hongchan Choi . unresolved

```suggestion
      iframe.src =
          'http://localhost:8000/webaudio/resources/media-playback-while-not-visible-permission-policy-iframe.html';
```

Line 34, Patchset 2 (Latest):

        iframe.addEventListener('load', resolve)
      })

Hongchan Choi . unresolved

```suggestion
        iframe.addEventListener('load', resolve);
      });
```

File third_party/blink/web_tests/http/tests/webaudio/resources/media-playback-while-not-visible-permission-policy-iframe.html

Line 18, Patchset 2 (Latest): if (window.parent !== window){

Hongchan Choi . unresolved

```suggestion
        if (window.parent !== window) {
```

Line 34, Patchset 2 (Latest):

            window.parent.postMessage(
                {
                  operation: 'resume',
                  value: e.name
                },
                '*');

Hongchan Choi . unresolved

Can we make this consistent with L28-L29?

Line 50, Patchset 2 (Latest): const oscillator = audioCtx.createOscillator();

Hongchan Choi . unresolved

I recommend using the newer constructor to the factory method.

```suggestion
      const oscillator = new OscillatorNode(audioCtx);
```

Open in Gerrit

Related details

Attention is currently required from:

Gabriel Brito

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

Gabriel Brito (Gerrit)

unread,

Dec 22, 2025, 7:55:37 PM (12 hours ago) Dec 22

to Hongchan Choi, chromium...@chromium.org, AyeAye, Chromium LUCI CQ, edgecapab...@microsoft.com, blink-...@chromium.org, feature-me...@chromium.org

Attention needed from Hongchan Choi

Gabriel Brito added 13 comments

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Gabriel Brito . resolved

I am sorry for all the lint/formatting mistakes! I ran the JS portion of the CL through the `git cl format` tool. I think that it should look now. Could you PTAL again when you some time? Thanks!

File third_party/blink/renderer/modules/webaudio/audio_context.cc

Line 486, Patchset 2:

    TRACE_EVENT1("webaudio", "AudioContext::Create - allowed to start", "UUID",
                 audio_context->Uuid());

Hongchan Choi . resolved

I guess these are artifacts of the linter. I think these are okay, but removing them makes the code review faster! 😊

Gabriel Brito

Done

Line 906, Patchset 2:

  TRACE_EVENT2("webaudio", __func__, "UUID", Uuid(), "state",
               static_cast<int>(ContextState()));

Hongchan Choi . resolved

Ditto. Can we revert these non-functional changes?

Gabriel Brito

Done

File third_party/blink/web_tests/http/tests/webaudio/media-playback-while-not-visible-permission-policy-viewport-crossorigin.html

Line 20, Patchset 2: //resources/media-playback-while-not-visible-permission-policy-iframe.html

Hongchan Choi . resolved

```suggestion
  // resources/media-playback-while-not-visible-permission-policy-iframe.html
```

Gabriel Brito

Done

Line 31, Patchset 2: if (document.readyState !=='complete'){

Hongchan Choi . resolved

```suggestion
    if (document.readyState !== 'complete') {
```

Gabriel Brito

Done

Line 33, Patchset 2:

        window.addEventListener('load', resolve)
      })

Hongchan Choi . resolved

```suggestion
        window.addEventListener('load', resolve);
      });
```

Gabriel Brito

Done

File third_party/blink/web_tests/http/tests/webaudio/media-playback-while-not-visible-permission-policy-visibility-crossorigin.html

Line 22, Patchset 2: if (document.readyState !=='complete'){

Hongchan Choi . resolved

It's a small thing, but the formatting should be:
```suggestion
      if (document.readyState !== 'complete') {
```

Gabriel Brito

Done

Line 23, Patchset 2: await new Promise(resolve => {


          window.addEventListener('load', resolve)
        })

Hongchan Choi . resolved

```suggestion
        await new Promise(resolve => {
          window.addEventListener('load', resolve);
        });
```

Gabriel Brito

Done

Line 31, Patchset 2:

      iframe.src = 'http://localhost:8000/webaudio/resources/media-playback-while-not-visible-permission-policy-iframe.html';

Hongchan Choi . resolved

```suggestion
      iframe.src =
          'http://localhost:8000/webaudio/resources/media-playback-while-not-visible-permission-policy-iframe.html';
```

Gabriel Brito

Done

Line 34, Patchset 2:

        iframe.addEventListener('load', resolve)
      })

Hongchan Choi . resolved

```suggestion
        iframe.addEventListener('load', resolve);
      });
```

Gabriel Brito

Done

File third_party/blink/web_tests/http/tests/webaudio/resources/media-playback-while-not-visible-permission-policy-iframe.html

Line 18, Patchset 2: if (window.parent !== window){

Hongchan Choi . resolved

```suggestion
        if (window.parent !== window) {
```

Gabriel Brito

Done

Line 34, Patchset 2: window.parent.postMessage(


                {
                  operation: 'resume',
                  value: e.name
                },
                '*');

Hongchan Choi . resolved

Can we make this consistent with L28-L29?

Gabriel Brito

Done

Line 50, Patchset 2: const oscillator = audioCtx.createOscillator();

Hongchan Choi . resolved

I recommend using the newer constructor to the factory method.
```suggestion
      const oscillator = new OscillatorNode(audioCtx);
```

Gabriel Brito

Done

Open in Gerrit

Related details

Attention is currently required from:

Hongchan Choi

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

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

[WebAudio] AudioContext uses new FrameVisibilityObserver methods [chromium/src : main]

Gabriel Brito (Gerrit)

Gabriel Brito added 1 comment

Related details

Hongchan Choi (Gerrit)

Hongchan Choi added 9 comments

Related details

Michael Wilson (Gerrit)

Michael Wilson added 1 comment

Related details

Gabriel Brito (Gerrit)

Gabriel Brito added 9 comments

Related details

Gabriel Brito (Gerrit)

Gabriel Brito added 1 comment

Related details

Hongchan Choi (Gerrit)

Hongchan Choi added 13 comments

Related details

Gabriel Brito (Gerrit)

Gabriel Brito added 13 comments

Related details