Johann Hofmann (Gerrit)

unread,

Mar 10, 2026, 11:43:36 AM (2 days ago) Mar 10

to Marc Treib, chromium...@chromium.org, Andrew Rayskiy, Simon Hangl, aixba+wat...@chromium.org, chromium-a...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, dullweb...@chromium.org, extension...@chromium.org, japhet+...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mek+w...@chromium.org, mgiuca...@chromium.org, msrame...@chromium.org, net-r...@chromium.org, philli...@chromium.org, rmcelra...@chromium.org, rrsilva+wat...@google.com, tbarzi...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org

Attention needed from Marc Treib

Johann Hofmann voted Commit-Queue+1

Commit-Queue

+1

Open in Gerrit

Related details

Attention is currently required from:

Marc Treib

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

Marc Treib (Gerrit)

unread,

Mar 10, 2026, 11:54:22 AM (2 days ago) Mar 10

to Johann Hofmann, Chromium LUCI CQ, Marc Treib, chromium...@chromium.org, Andrew Rayskiy, Simon Hangl, aixba+wat...@chromium.org, chromium-a...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, dullweb...@chromium.org, extension...@chromium.org, japhet+...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mek+w...@chromium.org, mgiuca...@chromium.org, msrame...@chromium.org, net-r...@chromium.org, philli...@chromium.org, rmcelra...@chromium.org, rrsilva+wat...@google.com, tbarzi...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org

Attention needed from Johann Hofmann

Marc Treib added 1 comment

Patchset-level comments

File-level comment, Patchset 1 (Latest):

Marc Treib . unresolved

Have you made any effort to figure out what the "correct" type for all of these cases would be? E.g. I suspect that for many of them, `kHTTP` might make more sense than `kOther`. (And in a way, explicitly specifying `kOther` where some other type would make sense is worse than not specifying the type explicitly at all!)

Open in Gerrit

Related details

Attention is currently required from:

Johann Hofmann

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

Johann Hofmann (Gerrit)

unread,

12:56 AM (11 hours ago) 12:56 AM

to Chromium LUCI CQ, Marc Treib, chromium...@chromium.org, Andrew Rayskiy, Simon Hangl, aixba+wat...@chromium.org, chromium-a...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, dullweb...@chromium.org, extension...@chromium.org, japhet+...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mek+w...@chromium.org, mgiuca...@chromium.org, msrame...@chromium.org, net-r...@chromium.org, philli...@chromium.org, rmcelra...@chromium.org, rrsilva+wat...@google.com, tbarzi...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org

Attention needed from Marc Treib

Johann Hofmann added 1 comment

Patchset-level comments

File-level comment, Patchset 1:

Marc Treib . unresolved

Have you made any effort to figure out what the "correct" type for all of these cases would be? E.g. I suspect that for many of them, `kHTTP` might make more sense than `kOther`. (And in a way, explicitly specifying `kOther` where some other type would make sense is worse than not specifying the type explicitly at all!)

Johann Hofmann

I have not made a particular effort, no, but at a quick glance the usage of kOther in these tests doesn't strike me as wrong.

I'm not sure why exposing that there is in fact a CookieSourceType is worse than having it hidden with the wrong type (that does not exercise production code paths) as the default. :)

Converting all these test usages to their correct type would be a bit much for this entire stack of CLs, and probably unnecessary for 99% of these tests where kOther usage is totally fine.

Open in Gerrit

Related details

Attention is currently required from:

Marc Treib

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

Marc Treib (Gerrit)

unread,

5:07 AM (7 hours ago) 5:07 AM

to Johann Hofmann, Marc Treib, Chromium LUCI CQ, chromium...@chromium.org, Andrew Rayskiy, Simon Hangl, aixba+wat...@chromium.org, chromium-a...@chromium.org, dibyapal+wa...@chromium.org, dmurph+watc...@chromium.org, dullweb...@chromium.org, extension...@chromium.org, japhet+...@chromium.org, kuragin+web-ap...@chromium.org, loyso...@chromium.org, mek+w...@chromium.org, mgiuca...@chromium.org, msrame...@chromium.org, net-r...@chromium.org, philli...@chromium.org, rmcelra...@chromium.org, rrsilva+wat...@google.com, tbarzi...@chromium.org, webap...@microsoft.com, zelin+watch-we...@chromium.org

Attention needed from Johann Hofmann

Marc Treib voted and added 1 comment

Votes added by Marc Treib

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 1:

Marc Treib . resolved

Have you made any effort to figure out what the "correct" type for all of these cases would be? E.g. I suspect that for many of them, `kHTTP` might make more sense than `kOther`. (And in a way, explicitly specifying `kOther` where some other type would make sense is worse than not specifying the type explicitly at all!)

Johann Hofmann

I have not made a particular effort, no, but at a quick glance the usage of kOther in these tests doesn't strike me as wrong.
I'm not sure why exposing that there is in fact a CookieSourceType is worse than having it hidden with the wrong type (that does not exercise production code paths) as the default. :)
Converting all these test usages to their correct type would be a bit much for this entire stack of CLs, and probably unnecessary for 99% of these tests where kOther usage is totally fine.

Marc Treib

I have not made a particular effort, no, but at a quick glance the usage of kOther in these tests doesn't strike me as wrong.

Fair enough, I guess I'll trust your judgement on that.

I'm not sure why exposing that there is in fact a CookieSourceType is worse than having it hidden with the wrong type (that does not exercise production code paths) as the default. :)

Well, purely from reading the code at the call sites, I'd argue it's better to have no information about the type than to have wrong/misleading information. (Of course, if `kOther` is at least kinda-sorta legit, then that argument goes away.)

Converting all these test usages to their correct type would be a bit much for this entire stack of CLs, and probably unnecessary for 99% of these tests where kOther usage is totally fine.

*shrug* fair enough I suppose. The tests are still passing so clearly it doesn't matter for them.

Open in Gerrit

Related details

Attention is currently required from:

Johann Hofmann

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

Reply all

Reply to author

Forward

Remove CookieSourceType::kUnknown (Part 4: chrome/browser tests) [chromium/src : main]

Johann Hofmann (Gerrit)

Johann Hofmann voted Commit-Queue+1

Related details

Marc Treib (Gerrit)

Marc Treib added 1 comment

Related details

Johann Hofmann (Gerrit)

Johann Hofmann added 1 comment

Related details

Marc Treib (Gerrit)

Marc Treib voted and added 1 comment

Votes added by Marc Treib

1 comment

Related details