Giovanni Ortuno Urquidi added 1 comment

Giovanni Ortuno Urquidi . resolved

hta: PTAL!

Related details

Attention is currently required from:

Harald Alvestrand

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

Harald Alvestrand (Gerrit)

unread,

Aug 28, 2025, 3:51:00 PM (11 days ago) Aug 28

to Giovanni Ortuno Urquidi, Chromium LUCI CQ, chromium...@chromium.org, blink-...@chromium.org, network-ser...@chromium.org, video-networking...@google.com

Attention needed from Giovanni Ortuno Urquidi

Harald Alvestrand voted Code-Review+1

Code-Review

+1

Related details

Attention is currently required from:

Giovanni Ortuno Urquidi

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

Giovanni Ortuno Urquidi (Gerrit)

unread,

Aug 28, 2025, 3:52:19 PM (11 days ago) Aug 28

to Giovanni Ortuno Urquidi, Harald Alvestrand, Chromium LUCI CQ, chromium...@chromium.org, blink-...@chromium.org, network-ser...@chromium.org, video-networking...@google.com

Attention needed from mmenke

Giovanni Ortuno Urquidi voted and added 1 comment

Votes added by Giovanni Ortuno Urquidi

Auto-Submit

+1

1 comment

Patchset-level comments

Giovanni Ortuno Urquidi . resolved

mmenke: PTAL at services/network

Related details

Attention is currently required from:

mmenke

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

mmenke (Gerrit)

unread,

Aug 28, 2025, 3:56:41 PM (11 days ago) Aug 28

to Giovanni Ortuno Urquidi, Harald Alvestrand, Chromium LUCI CQ, chromium...@chromium.org, blink-...@chromium.org, network-ser...@chromium.org, video-networking...@google.com

Attention needed from Giovanni Ortuno Urquidi

mmenke added 1 comment

Patchset-level comments

mmenke . resolved

We should have tests for this.

Not my area (and not for this CL, certainly), but is this a decision we should be trusting the renderer process to make?

Related details

Attention is currently required from:

Giovanni Ortuno Urquidi

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

Giovanni Ortuno Urquidi (Gerrit)

unread,

Aug 28, 2025, 4:07:16 PM (11 days ago) Aug 28

to Giovanni Ortuno Urquidi, Harald Alvestrand, Chromium LUCI CQ, chromium...@chromium.org, blink-...@chromium.org, network-ser...@chromium.org, video-networking...@google.com

Attention needed from mmenke

Giovanni Ortuno Urquidi added 1 comment

Patchset-level comments

mmenke . unresolved

We should have tests for this.
Not my area (and not for this CL, certainly), but is this a decision we should be trusting the renderer process to make?

Giovanni Ortuno Urquidi

Are you asking for tests when the killswitch is enabled? The feature is generally tested by web tests and there are tests in WebRTC as well.

File-level comment, Patchset 5:

Related details

Attention is currently required from:

mmenke

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

Giovanni Ortuno Urquidi (Gerrit)

unread,

Sep 3, 2025, 4:40:49 PM (5 days ago) Sep 3

to Giovanni Ortuno Urquidi, Harald Alvestrand, Chromium LUCI CQ, chromium...@chromium.org, blink-...@chromium.org, network-ser...@chromium.org, video-networking...@google.com

Attention needed from Harald Alvestrand and mmenke

Giovanni Ortuno Urquidi voted and added 1 comment

Votes added by Giovanni Ortuno Urquidi

Auto-Submit	+1
Commit-Queue	+0

1 comment

Patchset-level comments

mmenke . resolved

We should have tests for this.
Not my area (and not for this CL, certainly), but is this a decision we should be trusting the renderer process to make?

Giovanni Ortuno Urquidi

Are you asking for tests when the killswitch is enabled? The feature is generally tested by web tests and there are tests in WebRTC as well.

Giovanni Ortuno Urquidi

Done

Related details

Attention is currently required from:

Harald Alvestrand
mmenke

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

Giovanni Ortuno Urquidi (Gerrit)

unread,

Sep 3, 2025, 4:41:13 PM (5 days ago) Sep 3

to Giovanni Ortuno Urquidi, Harald Alvestrand, Chromium LUCI CQ, chromium...@chromium.org, blink-...@chromium.org, network-ser...@chromium.org, video-networking...@google.com

Attention needed from Harald Alvestrand and mmenke

Giovanni Ortuno Urquidi voted and added 1 comment

Votes added by Giovanni Ortuno Urquidi

Auto-Submit

+0

1 comment

Patchset-level comments

Giovanni Ortuno Urquidi . resolved

hta: Mind taking a look again? I added tests so need your +1 for that file.

satisfied_requirement

unsatisfied_requirement

open

diffy

mmenke (Gerrit)

unread,

Sep 3, 2025, 5:00:50 PM (5 days ago) Sep 3

to Giovanni Ortuno Urquidi, Harald Alvestrand, Chromium LUCI CQ, chromium...@chromium.org, blink-...@chromium.org, network-ser...@chromium.org, video-networking...@google.com

Attention needed from Giovanni Ortuno Urquidi and Harald Alvestrand

mmenke voted and added 1 comment

Votes added by mmenke

Code-Review

+1

1 comment

Patchset-level comments

mmenke . resolved

LGTM!

Related details

Attention is currently required from:

Giovanni Ortuno Urquidi
Harald Alvestrand

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

Harald Alvestrand (Gerrit)

unread,

Sep 4, 2025, 5:23:03 PM (4 days ago) Sep 4

to Giovanni Ortuno Urquidi, Chromium LUCI CQ, chromium...@chromium.org, blink-...@chromium.org, network-ser...@chromium.org, video-networking...@google.com

Attention needed from Giovanni Ortuno Urquidi

Harald Alvestrand voted and added 1 comment

Votes added by Harald Alvestrand

Code-Review

+1

1 comment

Patchset-level comments

Harald Alvestrand . resolved

Seems a bit iffy to have this big TestCase struct and then compound it with a boolean... but your style choice.

Related details

Attention is currently required from:

Giovanni Ortuno Urquidi

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

Giovanni Ortuno Urquidi (Gerrit)

unread,

Sep 4, 2025, 6:11:49 PM (4 days ago) Sep 4

to Giovanni Ortuno Urquidi, Harald Alvestrand, Chromium LUCI CQ, chromium...@chromium.org, blink-...@chromium.org, network-ser...@chromium.org, video-networking...@google.com

Attention needed from Harald Alvestrand

Giovanni Ortuno Urquidi added 1 comment

Patchset-level comments

Harald Alvestrand . unresolved

Seems a bit iffy to have this big TestCase struct and then compound it with a boolean... but your style choice.

Giovanni Ortuno Urquidi

That's a fair point. I initially considered adding a `loopback_only_param` boolean to the TestCase struct to control the flag's value. I decided against it because every test case would then need to specify that parameter, which felt overly verbose.

Thinking about it more, we could add a `should_request_permission_when_loopback_only` boolean to the struct. It would hold the expected result of ShouldRequestPermission() when the loopback only flag is enabled. This would allows us to merge the two test case arrays into a single, consolidated one, making it clearer which tests behave differently with the flag enabled.

Does that approach seem better to you?

Related details

Attention is currently required from:

Harald Alvestrand

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

Harald Alvestrand (Gerrit)

unread,

Sep 5, 2025, 2:36:48 AM (4 days ago) Sep 5

to Giovanni Ortuno Urquidi, Chromium LUCI CQ, chromium...@chromium.org, blink-...@chromium.org, network-ser...@chromium.org, video-networking...@google.com

Attention needed from Giovanni Ortuno Urquidi

Harald Alvestrand added 1 comment

Patchset-level comments

Harald Alvestrand . unresolved

Seems a bit iffy to have this big TestCase struct and then compound it with a boolean... but your style choice.

Giovanni Ortuno Urquidi

That's a fair point. I initially considered adding a `loopback_only_param` boolean to the TestCase struct to control the flag's value. I decided against it because every test case would then need to specify that parameter, which felt overly verbose.
Thinking about it more, we could add a `should_request_permission_when_loopback_only` boolean to the struct. It would hold the expected result of ShouldRequestPermission() when the loopback only flag is enabled. This would allows us to merge the two test case arrays into a single, consolidated one, making it clearer which tests behave differently with the flag enabled.
Does that approach seem better to you?

Harald Alvestrand

Yes, that seems like a reasonable approach.

With named-value initialization, it seems that we can avoid cluttering the declaration too much if we add a bool to the struct with a default value.

File-level comment, Patchset 6:

Related details

Attention is currently required from:

Giovanni Ortuno Urquidi

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

Giovanni Ortuno Urquidi (Gerrit)

unread,

Sep 5, 2025, 5:37:26 PM (3 days ago) Sep 5

to Giovanni Ortuno Urquidi, Harald Alvestrand, Chromium LUCI CQ, chromium...@chromium.org, blink-...@chromium.org, network-ser...@chromium.org, video-networking...@google.com

Giovanni Ortuno Urquidi added 1 comment

Patchset-level comments

Harald Alvestrand . resolved

Seems a bit iffy to have this big TestCase struct and then compound it with a boolean... but your style choice.

Giovanni Ortuno Urquidi

That's a fair point. I initially considered adding a `loopback_only_param` boolean to the TestCase struct to control the flag's value. I decided against it because every test case would then need to specify that parameter, which felt overly verbose.
Thinking about it more, we could add a `should_request_permission_when_loopback_only` boolean to the struct. It would hold the expected result of ShouldRequestPermission() when the loopback only flag is enabled. This would allows us to merge the two test case arrays into a single, consolidated one, making it clearer which tests behave differently with the flag enabled.
Does that approach seem better to you?

Harald Alvestrand

Yes, that seems like a reasonable approach.
With named-value initialization, it seems that we can avoid cluttering the declaration too much if we add a bool to the struct with a default value.

Giovanni Ortuno Urquidi

Thanks! Done.