Etienne Dechamps (Gerrit)

unread,

Nov 4, 2025, 11:58:13 AM (yesterday) Nov 4

to Sebastian Poreba, Etienne Dechamps, chromium...@chromium.org, Code Review Nudger, Mohannad Farrag, Vladyslav Kasprov, AyeAye, net-r...@chromium.org

Attention needed from Mohannad Farrag and Sebastian Poreba

Etienne Dechamps added 5 comments

Patchset-level comments

File-level comment, Patchset 27 (Latest):

Etienne Dechamps . resolved

Argh, sorry, I built a CL on top of yours to illustrate and git cl upload overwrote yours on upload... feel free to run git cl upload again to get your CL back to where it was.

File components/cronet/android/test/javatests/src/org/chromium/net/impl/UserAgentIntegrationTest.java

Line 164, Patchset 25: mTestRule.getTestFramework().startEngine();

Etienne Dechamps . unresolved

mCronetTestFramework

File components/cronet/android/test/javatests/src/org/chromium/net/impl/UserAgentTest.java

Line 31, Patchset 25: @Rule public final CronetTestRule mTestRule = CronetTestRule.withManualEngineStartup();

Etienne Dechamps . unresolved

What this does is it makes every test run for every implementation in sequence.
But then you have tests that clearly only make sense to run once, such as `testLegacyUserAgent_whenOnPlatformSourceAndMetadataIsFalse`, which hardcodes the CronetSource and will therefore always run the same code for every implementation. Running it N times doesn't seem to make sense.
And then you have tests like `testLegacyUserAgent_whenNotOnPlatformSourceAndMetadataIsFalse`, which seem to reinvent CronetTestRule by iterating through every CronetSource in their own for loop. Which is already what CronetTestRule does, so you end up with N^2 effective test cases.
If this is intentional, please add code comments to explain the rationale, as this code makes me very confused.
If this is not intentional, either use CronetTestRule and `@IgnoreFor`, or use for loops, but please pick one. It would probably be simpler to use for loops, as you don't need most of the machinery CronetTestRule provides.

Sebastian Poreba

This is intentional, you cannot modify context (= write the manifest flag) after the engine has started.

Etienne Dechamps

There is no "engine" that needs to be "started" here. You are testing UserAgent which does not interact with Cronet engines at all.

Please see https://crrev.com/c/7119519. Note this CL does pass tests.

Line 52, Patchset 21:

    @Rule public final CronetTestRule mTestRule = CronetTestRule.withManualEngineStartup();

    public CronetTestFramework mCronetTestFramework;

Etienne Dechamps . unresolved

I don't think you need this - it's overkill. I believe the following should work:

```
private Context getContextWithLegacyUserAgentOptIn(boolean value) {
        Bundle metaData = new Bundle();
        metaData.putBoolean(CronetManifest.USE_LEGACY_DEFAULT_USER_AGENT, value);
        return (new CronetManifestInterceptor(metaData)).interceptContext(ApplicationProvider.getApplicationContext());
    }

UserAgent.from(getContextWithLegacyUserAgentOptIn(...), ...)
```

Sebastian Poreba

I think this comment got attached to the wrong line? If this is re setLegacyDefaultUserAgent - I moved it to the util, but the change you suggested won't work. CronetTestFramework needs to have the same context, so it needs to be passed down.

Etienne Dechamps

The comment is attached to the correct line.

The point I was trying to make is: you do not need CronetTestFramework *at all* in UserAgentTest.

```
public class UserAgentUtil {
    public static ContextInterceptor getContextInterceptorWithLegacyUserAgent(
            boolean value) {
        Bundle metaData = new Bundle();
        metaData.putBoolean(CronetManifest.USE_LEGACY_DEFAULT_USER_AGENT, value);
        return new CronetManifestInterceptor(metaData);
    }
}

class UserAgentTest {
    private Context getContextWithLegacyUserAgent(boolean value) {
      return UserAgentUtil.getContextInterceptorWithLegacyUserAgent(value).interceptContext(ApplicationProvider.getApplicationContext());
    }

    public void testLegacyUserAgent_whenOnPlatformSourceAndMetadataIsFalse()  {
        assertThat(UserAgent.from(getContextWithLegacyUserAgent(false),
                        
                        
class UserAgentIntegrationTest {
  public void testDefaultUserAgent_legacyOptIn() throws Exception {
     mCronetTestFramework.interceptContext(UserAgentUtil.getContextInterceptorWithLegacyUserAgent(true));
```

Forget about CronetTestFramework. It brings in all kinds of code you don't need. The code under test (`UserAgent`) only needs a Context, it doesn't need all the engine-wrangling code.

Sebastian Poreba

I suspect this would suffer from the same problem as above - you cannot intercept the context so late. If you care I can try it and prove it, I'd rather not.

Etienne Dechamps

You don't need an engine in the first place.

You don't need any of this stuff.

The class that you are testing, UserAgent, is a completely self-contained class that does not interact with CronetEngine or the rest of Cronet at all. It can be tested without using any of the heavyweight test infrastructure like CronetTestRule, CronetTestFramework and the like. All this stuff is overkill.

Please see https://crrev.com/c/7119519. Note this CL does pass tests.

File components/cronet/android/test/javatests/src/org/chromium/net/impl/UserAgentUtil.java

Line 16, Patchset 25:public class UserAgentUtil {

Etienne Dechamps . unresolved

`org.chromium.net.impl.UserAgentUtil` looks like utils for production code. I would name this `userAgentTestUtil`.

Sebastian Poreba

Isn't that redundant in Java since tests are under a separate directory?

Etienne Dechamps

That's technically true, but when looking at the fully qualified name of the class (e.g. from references, or in a stack trace) this looks very much like some utils used by production code, it's not clear at all this is purely for tests.

Open in Gerrit

Related details

Attention is currently required from:

Mohannad Farrag
Sebastian Poreba

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

Etienne Dechamps (Gerrit)

unread,

Nov 4, 2025, 12:13:15 PM (yesterday) Nov 4

to Sebastian Poreba, Etienne Dechamps, chromium...@chromium.org, Code Review Nudger, Mohannad Farrag, Vladyslav Kasprov, AyeAye, net-r...@chromium.org

Attention needed from Mohannad Farrag and Sebastian Poreba

Etienne Dechamps added 2 comments

File components/cronet/android/test/javatests/src/org/chromium/net/impl/UserAgentTest.java

Line 31, Patchset 25: @Rule public final CronetTestRule mTestRule = CronetTestRule.withManualEngineStartup();

Etienne Dechamps . unresolved

What this does is it makes every test run for every implementation in sequence.
But then you have tests that clearly only make sense to run once, such as `testLegacyUserAgent_whenOnPlatformSourceAndMetadataIsFalse`, which hardcodes the CronetSource and will therefore always run the same code for every implementation. Running it N times doesn't seem to make sense.
And then you have tests like `testLegacyUserAgent_whenNotOnPlatformSourceAndMetadataIsFalse`, which seem to reinvent CronetTestRule by iterating through every CronetSource in their own for loop. Which is already what CronetTestRule does, so you end up with N^2 effective test cases.
If this is intentional, please add code comments to explain the rationale, as this code makes me very confused.
If this is not intentional, either use CronetTestRule and `@IgnoreFor`, or use for loops, but please pick one. It would probably be simpler to use for loops, as you don't need most of the machinery CronetTestRule provides.

Sebastian Poreba

This is intentional, you cannot modify context (= write the manifest flag) after the engine has started.

Etienne Dechamps

There is no "engine" that needs to be "started" here. You are testing UserAgent which does not interact with Cronet engines at all.
Please see https://crrev.com/c/7119519. Note this CL does pass tests.

Etienne Dechamps

As I mention in my other comment, in the end I'm fine with using CronetTestRule, but you still have the problem that using a for loop over sources (e.g. in `testLegacyUserAgent_whenNotOnPlatformSourceAndMetadataIsFalse`) *in addition to* CronetTestRule makes no sense to me.

If you get rid of these for loops, and keep CronetTestRule, I would be fine with that.

Line 52, Patchset 21:

    @Rule public final CronetTestRule mTestRule = CronetTestRule.withManualEngineStartup();

    public CronetTestFramework mCronetTestFramework;

Etienne Dechamps . resolved

Etienne Dechamps

That said, I realize now that CronetManifestTest (which is similarly disconnected from CronetEngines) does use CronetTestRule, and it does have the advantage of taking care of some subtleties like correctly intercepting Context#getApplicationContext.

So I'd be fine with the current approach as there is now a consistency argument that I was not aware of before.

satisfied_requirement

unsatisfied_requirement

open

diffy

Sebastian Poreba (Gerrit)

unread,

9:55 AM (6 hours ago) 9:55 AM

to Etienne Dechamps, chromium...@chromium.org, Code Review Nudger, Mohannad Farrag, Vladyslav Kasprov, AyeAye, net-r...@chromium.org

Attention needed from Etienne Dechamps and Mohannad Farrag

Sebastian Poreba added 3 comments

File components/cronet/android/test/javatests/src/org/chromium/net/impl/UserAgentIntegrationTest.java

Line 164, Patchset 25: mTestRule.getTestFramework().startEngine();

Etienne Dechamps . resolved

mCronetTestFramework

Sebastian Poreba

Done

File components/cronet/android/test/javatests/src/org/chromium/net/impl/UserAgentTest.java

Line 31, Patchset 25: @Rule public final CronetTestRule mTestRule = CronetTestRule.withManualEngineStartup();

Etienne Dechamps . resolved

What this does is it makes every test run for every implementation in sequence.
But then you have tests that clearly only make sense to run once, such as `testLegacyUserAgent_whenOnPlatformSourceAndMetadataIsFalse`, which hardcodes the CronetSource and will therefore always run the same code for every implementation. Running it N times doesn't seem to make sense.
And then you have tests like `testLegacyUserAgent_whenNotOnPlatformSourceAndMetadataIsFalse`, which seem to reinvent CronetTestRule by iterating through every CronetSource in their own for loop. Which is already what CronetTestRule does, so you end up with N^2 effective test cases.
If this is intentional, please add code comments to explain the rationale, as this code makes me very confused.
If this is not intentional, either use CronetTestRule and `@IgnoreFor`, or use for loops, but please pick one. It would probably be simpler to use for loops, as you don't need most of the machinery CronetTestRule provides.

Sebastian Poreba

This is intentional, you cannot modify context (= write the manifest flag) after the engine has started.

Etienne Dechamps

There is no "engine" that needs to be "started" here. You are testing UserAgent which does not interact with Cronet engines at all.
Please see https://crrev.com/c/7119519. Note this CL does pass tests.

Etienne Dechamps

As I mention in my other comment, in the end I'm fine with using CronetTestRule, but you still have the problem that using a for loop over sources (e.g. in `testLegacyUserAgent_whenNotOnPlatformSourceAndMetadataIsFalse`) *in addition to* CronetTestRule makes no sense to me.
If you get rid of these for loops, and keep CronetTestRule, I would be fine with that.

Sebastian Poreba

Done

File components/cronet/android/test/javatests/src/org/chromium/net/impl/UserAgentUtil.java

Line 16, Patchset 25:public class UserAgentUtil {

Etienne Dechamps . resolved

`org.chromium.net.impl.UserAgentUtil` looks like utils for production code. I would name this `userAgentTestUtil`.

Sebastian Poreba

Isn't that redundant in Java since tests are under a separate directory?

Etienne Dechamps

That's technically true, but when looking at the fully qualified name of the class (e.g. from references, or in a stack trace) this looks very much like some utils used by production code, it's not clear at all this is purely for tests.

Sebastian Poreba

Done

Open in Gerrit

Related details

Attention is currently required from:

Etienne Dechamps
Mohannad Farrag

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

Mohannad Farrag (Gerrit)

unread,

10:17 AM (5 hours ago) 10:17 AM

to Sebastian Poreba, chromium...@chromium.org, Code Review Nudger, Etienne Dechamps, Vladyslav Kasprov, AyeAye, net-r...@chromium.org

Attention needed from Etienne Dechamps and Sebastian Poreba

Mohannad Farrag added 1 comment

File components/cronet/android/test/javatests/src/org/chromium/net/impl/UserAgentTest.java

Line 30, Patchset 29 (Latest):public class UserAgentTest {

Mohannad Farrag . unresolved

Can we add a test that ensures that HttpEngine's default userAgent is equal to the userAgent which we generate here? This is to ensure that HttpEngine's default userAgent does not accidentally change once this new code gets imported to stable.

The test could look something like `new HttpEngineNativeProvider(context).createBuilder().getDefaultUserAgent().equals(UserAgent.from(...)`.

It should ofcourse only run on Android U+.

Open in Gerrit

Related details

Attention is currently required from:

Etienne Dechamps
Sebastian Poreba

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

Etienne Dechamps (Gerrit)

unread,

11:07 AM (4 hours ago) 11:07 AM

to Sebastian Poreba, Etienne Dechamps, chromium...@chromium.org, Code Review Nudger, Mohannad Farrag, Vladyslav Kasprov, AyeAye, net-r...@chromium.org

Attention needed from Mohannad Farrag and Sebastian Poreba

Etienne Dechamps voted and added 2 comments

Votes added by Etienne Dechamps

Code-Review

+1

2 comments

File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java

Line 1202, Patchset 29 (Latest):

Etienne Dechamps . unresolved

Spurious newline.

File components/cronet/android/test/javatests/src/org/chromium/net/impl/UserAgentTest.java

Line 30, Patchset 29 (Latest):public class UserAgentTest {

Mohannad Farrag . resolved

Can we add a test that ensures that HttpEngine's default userAgent is equal to the userAgent which we generate here? This is to ensure that HttpEngine's default userAgent does not accidentally change once this new code gets imported to stable.
The test could look something like `new HttpEngineNativeProvider(context).createBuilder().getDefaultUserAgent().equals(UserAgent.from(...)`.
It should ofcourse only run on Android U+.

Etienne Dechamps

I think `testDefaultUserAgent_noLegacyOptIn` already does this?

Open in Gerrit

Related details

Attention is currently required from:

Mohannad Farrag
Sebastian Poreba

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

Sebastian Poreba (Gerrit)

unread,

11:15 AM (4 hours ago) 11:15 AM

to Etienne Dechamps, chromium...@chromium.org, Code Review Nudger, Mohannad Farrag, Vladyslav Kasprov, AyeAye, net-r...@chromium.org

Attention needed from Etienne Dechamps and Mohannad Farrag

Sebastian Poreba voted and added 1 comment

Votes added by Sebastian Poreba

Auto-Submit

+1

1 comment

File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamTest.java

Line 1202, Patchset 29:

Etienne Dechamps . resolved

Spurious newline.

Sebastian Poreba

Done

Open in Gerrit

Related details

Attention is currently required from:

Etienne Dechamps
Mohannad Farrag

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

Sebastian Poreba (Gerrit)

unread,

11:16 AM (4 hours ago) 11:16 AM

to Etienne Dechamps, chromium...@chromium.org, Code Review Nudger, Mohannad Farrag, Vladyslav Kasprov, AyeAye, net-r...@chromium.org

Attention needed from Etienne Dechamps and Mohannad Farrag

Sebastian Poreba voted

Auto-Submit	+1
Commit-Queue	+1

Open in Gerrit

Related details

Attention is currently required from:

Etienne Dechamps
Mohannad Farrag

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

Etienne Dechamps (Gerrit)

unread,

11:16 AM (4 hours ago) 11:16 AM

to Sebastian Poreba, Etienne Dechamps, Chromium LUCI CQ, chromium...@chromium.org, Code Review Nudger, Mohannad Farrag, Vladyslav Kasprov, AyeAye, net-r...@chromium.org

Attention needed from Mohannad Farrag and Sebastian Poreba

Etienne Dechamps voted

Code-Review	+1
Commit-Queue	+2

Open in Gerrit

Related details

Attention is currently required from:

Mohannad Farrag
Sebastian Poreba

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

Mohannad Farrag (Gerrit)

unread,

11:16 AM (4 hours ago) 11:16 AM

to Sebastian Poreba, Etienne Dechamps, Chromium LUCI CQ, chromium...@chromium.org, Code Review Nudger, Vladyslav Kasprov, AyeAye, net-r...@chromium.org

Attention needed from Sebastian Poreba

Mohannad Farrag voted

Code-Review	+1
Commit-Queue	+2

Open in Gerrit

Related details

Attention is currently required from:

Sebastian Poreba

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

Sebastian Poreba (Gerrit)

unread,

11:20 AM (4 hours ago) 11:20 AM

to Mohannad Farrag, Etienne Dechamps, Chromium LUCI CQ, chromium...@chromium.org, Code Review Nudger, Vladyslav Kasprov, AyeAye, net-r...@chromium.org

Attention needed from Etienne Dechamps and Mohannad Farrag

Sebastian Poreba voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention is currently required from:

Etienne Dechamps
Mohannad Farrag

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

Vladyslav Kasprov (Gerrit)

unread,

11:23 AM (4 hours ago) 11:23 AM

to Sebastian Poreba, Mohannad Farrag, Etienne Dechamps, Chromium LUCI CQ, chromium...@chromium.org, Code Review Nudger, AyeAye, net-r...@chromium.org

Attention needed from Etienne Dechamps and Mohannad Farrag

Vladyslav Kasprov added 1 comment

Patchset-level comments

File-level comment, Patchset 31 (Latest):

Vladyslav Kasprov . resolved

LGTM

satisfied_requirement

open

diffy

Sebastian Poreba (Gerrit)

unread,

11:29 AM (4 hours ago) 11:29 AM

to Mohannad Farrag, Etienne Dechamps, Chromium LUCI CQ, chromium...@chromium.org, Code Review Nudger, Vladyslav Kasprov, AyeAye, net-r...@chromium.org

Attention needed from Etienne Dechamps and Mohannad Farrag

Sebastian Poreba voted

Auto-Submit	+1
Commit-Queue	+1

Open in Gerrit

Related details

Attention is currently required from:

Etienne Dechamps
Mohannad Farrag

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

Mohannad Farrag (Gerrit)

unread,

11:30 AM (4 hours ago) 11:30 AM

to Sebastian Poreba, Etienne Dechamps, Chromium LUCI CQ, chromium...@chromium.org, Code Review Nudger, Vladyslav Kasprov, AyeAye, net-r...@chromium.org

Attention needed from Etienne Dechamps and Sebastian Poreba

Mohannad Farrag voted

Code-Review	+1
Commit-Queue	+2

Open in Gerrit

Related details

Attention is currently required from:

Etienne Dechamps
Sebastian Poreba

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

Etienne Dechamps (Gerrit)

unread,

11:44 AM (4 hours ago) 11:44 AM

to Sebastian Poreba, Etienne Dechamps, Mohannad Farrag, Chromium LUCI CQ, chromium...@chromium.org, Code Review Nudger, Vladyslav Kasprov, AyeAye, net-r...@chromium.org

Attention needed from Mohannad Farrag and Sebastian Poreba

Etienne Dechamps voted

Code-Review	+1
Commit-Queue	+2

Open in Gerrit

Related details

Attention is currently required from:

Mohannad Farrag
Sebastian Poreba

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

Mohannad Farrag (Gerrit)

unread,

12:55 PM (3 hours ago) 12:55 PM

to Sebastian Poreba, Etienne Dechamps, Chromium LUCI CQ, chromium...@chromium.org, Code Review Nudger, Vladyslav Kasprov, AyeAye, net-r...@chromium.org

Attention needed from Etienne Dechamps and Sebastian Poreba

Mohannad Farrag voted

Code-Review	+1
Commit-Queue	+2

Open in Gerrit

Related details

Attention is currently required from:

Etienne Dechamps
Sebastian Poreba

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

Add to Cronet a manifest option to present legacy User Agent string. [chromium/src : main]

Etienne Dechamps (Gerrit)

Etienne Dechamps added 5 comments

Related details

Etienne Dechamps (Gerrit)

Etienne Dechamps added 2 comments

Sebastian Poreba (Gerrit)

Sebastian Poreba added 3 comments

Related details

Mohannad Farrag (Gerrit)

Mohannad Farrag added 1 comment

Related details

Etienne Dechamps (Gerrit)

Etienne Dechamps voted and added 2 comments

Votes added by Etienne Dechamps

2 comments

Related details

Sebastian Poreba (Gerrit)

Sebastian Poreba voted and added 1 comment

Votes added by Sebastian Poreba

1 comment

Related details

Sebastian Poreba (Gerrit)

Sebastian Poreba voted

Related details

Etienne Dechamps (Gerrit)

Etienne Dechamps voted

Related details

Mohannad Farrag (Gerrit)

Mohannad Farrag voted

Related details

Sebastian Poreba (Gerrit)

Sebastian Poreba voted Commit-Queue+2

Related details

Vladyslav Kasprov (Gerrit)

Vladyslav Kasprov added 1 comment

Sebastian Poreba (Gerrit)

Sebastian Poreba voted

Related details

Mohannad Farrag (Gerrit)

Mohannad Farrag voted

Related details

Etienne Dechamps (Gerrit)

Etienne Dechamps voted

Related details

Mohannad Farrag (Gerrit)

Mohannad Farrag voted

Related details