Implement sending custom X-Omnibox-Autofocus header. [chromium/src : main]
Jakub Kudzia (Gerrit)
Jakub Kudzia added 1 comment![]( "Open in Gerrit")
Implement sending custom X-Omnibox-Autofocus-Experiment header informing about config of Omnibox Autofocus ExperimentJakub Kudzianit: The first line (which will become the commit title) should be <= 72 chars
Done
Related details
- Arthur Sonzogni
- Kamil Jarosz
- Marc Treib
- Mark Pearson
- Matt Dembski
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. |
Mark Pearson (Gerrit)
Mark Pearson added 10 comments![]( "Open in Gerrit")
I reviewed all of components/variations/... except for the tests. I ran out of time for the day.
1.
Requested access to the link doc.
2.
Please test this interactively with "--enable-features=" and developer tools to make sure the requests look correct (appended in the right times and not appended when not appropriate).
// header is only set to a non-empty value during experimentation.I think it would help to add a comment that the header should only be sent if the value is non-empty.
// provider is not explicitly enabled nor disabled via a field trial, i.e. thenit: What does provider mean in this context? Can you choose a different word?
extern const char kOmniboxAutofocusHeader[];optional nit: I thought this would be the whole header, but it's only the first part of the header (the header name). Please consider renaming or at least commenting.
kNotFirstTab,I'm not sure how to understand this. Is it "do not show on the first tab"? Or show on things besides the first tab? Can you perhaps (a) choose a clearer name and (b) comment what this parameter does?
ditto below: please comment the effect of other parameters as well. People shouldn't have to find and read the implementation of the code to understand what a parameter is/does.
"not_first_tab",nit: Finch parameters names (the strings) are global. For here and below, can you please choose more specific names so they don't risk overlapping with another experiment?
params->cors_exempt_header_list.push_back(kOmniboxAutofocusHeader);Sometimes we don't add this header in the even if in this case. Is it okay to call this function with a header that we tell the network request to apply?
Here should there be a call to ShouldAppendHeader() like there is in AppendOmniboxAutofocusHeaderIfNeeded()?
to_be_removed_headers->push_back(kOmniboxAutofocusHeader);It is okay to add a header here that wasn't added to the original request?
// Copyright 2025 The Chromium AuthorsThis test uses blink. Does it work on iOS? The build file seem to add it for all platforms.
Related details
- Arthur Sonzogni
- Jakub Kudzia
- Kamil Jarosz
- Marc Treib
- Matt Dembski
Marc Treib (Gerrit)
Marc Treib voted Code-Review+1![]( "Open in Gerrit")
- Arthur Sonzogni
- Jakub Kudzia
- Kamil Jarosz
- Matt Dembski
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. |
Kamil Jarosz (Gerrit)
Kamil Jarosz added 2 comments![]( "Open in Gerrit")
"not_first_tab",nit: Finch parameters names (the strings) are global. For here and below, can you please choose more specific names so they don't risk overlapping with another experiment?
Are you sure that's the case? A lot of feature flags have parameters with the same name & in some cases it's explicitly designed this way (e.g. HaTS). Feature flags are globally unique, but I don't think params are.
// Copyright 2025 The Chromium AuthorsThis test uses blink. Does it work on iOS? The build file seem to add it for all platforms.
@jku...@google.com we only care about Android, so we can include this header for Android only.
Related details
- Arthur Sonzogni
- Jakub Kudzia
- Matt Dembski
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted and added 1 comment![]( "Open in Gerrit")
Votes added by Arthur Sonzogni
| Code-Review | +1 |
1 comment
content/ seems to be only about plumbing the throttle in a similar way of UpdateCorsExemptHeaderForVariations. So this part LGTM. I will let OWNER to review the implementation you to fix the compile issue on iOS.
Related details
- Jakub Kudzia
- Matt Dembski
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. |
Jakub Kudzia (Gerrit)
Jakub Kudzia added 6 comments![]( "Open in Gerrit")
// header is only set to a non-empty value during experimentation.I think it would help to add a comment that the header should only be sent if the value is non-empty.
Fair point, I've enhanced comment for AppendOmniboxAutofocusHeaderIfNeeded (below) which is responsible for actually appending the header
// provider is not explicitly enabled nor disabled via a field trial, i.e. theJakub Kudzianit: What does provider mean in this context? Can you choose a different word?
Yeah, it really was unclear. Sorry for that. I've simplified the comment
optional nit: I thought this would be the whole header, but it's only the first part of the header (the header name). Please consider renaming or at least commenting.
Done
params->cors_exempt_header_list.push_back(kOmniboxAutofocusHeader);Sometimes we don't add this header in the even if in this case. Is it okay to call this function with a header that we tell the network request to apply?
I want to make sure I've understood your concern correctly. Are you asking why we add this header to the CORS-exempt list during initialization, even though we may not end up attaching it to every subsequent request?
My understanding is that this allow-listing is not done on per-request basis so I don't really see other way to do it. For variations header (X-Client-Data) it's done the same way: https://source.chromium.org/chromium/chromium/src/+/main:components/variations/net/variations_http_headers.cc;l=401-404;bpv=1;bpt=1?q=variations_http_hea&ss=chromium%2Fchromium%2Fsrc
I've added this allow-listing in every place where X-Client-Data is allow-listed as well.
I've improved the comment to explicitly claim that the header itself might be absent.
Here should there be a call to ShouldAppendHeader() like there is in AppendOmniboxAutofocusHeaderIfNeeded()?
Not really, because `ShouldAppendHeader()` is called per request to verify whether request is done to a Google domain. Here, we're simply verifying whether experiment is active at all(by checking relevant feature flags via `GetHeaderValue()`)to decide whether enabling the throttle makes any sense.
to_be_removed_headers->push_back(kOmniboxAutofocusHeader);It is okay to add a header here that wasn't added to the original request?
yes, it looks like the code is handling it properly: https://source.chromium.org/chromium/chromium/src/+/main:net/http/http_request_headers.cc;l=170-174
Related details
- Arthur Sonzogni
- Marc Treib
- Mark Pearson
- Matt Dembski
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. |
Jakub Kudzia (Gerrit)
Jakub Kudzia added 2 comments![]( "Open in Gerrit")
// header is only set to a non-empty value during experimentation.Jakub KudziaI think it would help to add a comment that the header should only be sent if the value is non-empty.
Fair point, I've enhanced comment for AppendOmniboxAutofocusHeaderIfNeeded (below) which is responsible for actually appending the header
Done
// provider is not explicitly enabled nor disabled via a field trial, i.e. theJakub Kudzianit: What does provider mean in this context? Can you choose a different word?
Yeah, it really was unclear. Sorry for that. I've simplified the comment
Done
Jakub Kudzia (Gerrit)
Jakub Kudzia added 1 comment![]( "Open in Gerrit")
// Copyright 2025 The Chromium AuthorsKamil JaroszThis test uses blink. Does it work on iOS? The build file seem to add it for all platforms.
@jku...@google.com we only care about Android, so we can include this header for Android only.
- Arthur Sonzogni
- Kamil Jarosz
Marc Treib (Gerrit)
Marc Treib voted Code-Review+1![]( "Open in Gerrit")
- Arthur Sonzogni
- Jakub Kudzia
- Kamil Jarosz
- Mark Pearson
- Matt Dembski
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. |
Matt Dembski (Gerrit)
Matt Dembski voted and added 1 comment![]( "Open in Gerrit")
Related details
- Arthur Sonzogni
- Jakub Kudzia
- Kamil Jarosz
- Mark Pearson
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. |
Jakub Kudzia (Gerrit)
Jakub Kudzia added 1 comment![]( "Open in Gerrit")
kNotFirstTab,I'm not sure how to understand this. Is it "do not show on the first tab"? Or show on things besides the first tab? Can you perhaps (a) choose a clearer name and (b) comment what this parameter does?
ditto below: please comment the effect of other parameters as well. People shouldn't have to find and read the implementation of the code to understand what a parameter is/does.
Thank you for the detailed feedback. I completely agree that the parameters need clearer documentation, and I will add detailed comments to fully explain the effect of each one immediately.
However, I would prefer not to rename these specific parameters. These names were established in the original Android feature implementation (https://chromium-review.googlesource.com/c/chromium/src/+/6923827) to enable experimentation. Renaming these established parameters would unnecessarily expand the scope of this CL, introducing more noise.
By adding thorough comments, I believe we fully resolve the concern about clarity without the distraction of an unnecessary rename.
Related details
- Arthur Sonzogni
- Kamil Jarosz
- Marc Treib
- Mark Pearson
- Matt Dembski
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. |
Jakub Kudzia (Gerrit)
Jakub Kudzia added 1 comment![]( "Open in Gerrit")
// Copyright 2025 The Chromium AuthorsKamil JaroszThis test uses blink. Does it work on iOS? The build file seem to add it for all platforms.
Jakub Kudzia@jku...@google.com we only care about Android, so we can include this header for Android only.
Good catch, I'll exempt this test from running on iOS
As suggested I've added preprocessor directives to ensure the code runs only on Android. I just still need to figure out why there is an error on ios-simulator...
Matt Dembski (Gerrit)
Matt Dembski voted Commit-Queue+1![]( "Open in Gerrit")
- Arthur Sonzogni
- Jakub Kudzia
- Kamil Jarosz
- Marc Treib
- Mark Pearson
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. |
Marc Treib (Gerrit)
Marc Treib voted Code-Review+1![]( "Open in Gerrit")
- Arthur Sonzogni
- Jakub Kudzia
- Kamil Jarosz
- Mark Pearson
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. |
Jakub Kudzia (Gerrit)
Jakub Kudzia added 2 comments![]( "Open in Gerrit")
{Jakub KudziaSimilarly here
Jakub KudziaI've parameterized this test to remove duplicated code
Done
{Jakub KudziaCan you split those blocks into separate tests?
Jakub KudziaI've parameterized this test to remove duplicated code
Done
Related details
- Arthur Sonzogni
- Kamil Jarosz
- Mark Pearson
- Matt Dembski
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. |
Kamil Jarosz (Gerrit)
Kamil Jarosz added 2 comments![]( "Open in Gerrit")
I would enforce android here with #error in case it's not
I would enforce android here with #error in case it's not
Related details
- Arthur Sonzogni
- Jakub Kudzia
- Mark Pearson
- Matt Dembski
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. |
Jakub Kudzia (Gerrit)
Jakub Kudzia added 2 comments![]( "Open in Gerrit")
I would enforce android here with #error in case it's not
Done
I would enforce android here with #error in case it's not
- Arthur Sonzogni
- Kamil Jarosz
- Marc Treib
- Mark Pearson
- Matt Dembski
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. |
Kamil Jarosz (Gerrit)
Kamil Jarosz voted Code-Review+1![]( "Open in Gerrit")
- Arthur Sonzogni
- Jakub Kudzia
- Marc Treib
- Mark Pearson
- Matt Dembski
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. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted and added 1 comment![]( "Open in Gerrit")
Votes added by Arthur Sonzogni
| Code-Review | +1 |
1 comment
Adding ` BUILDFLAG(IS_ANDROID)`. Still LGTM on my side, though it still fail to compile on iOS.
Related details
- Jakub Kudzia
- Marc Treib
- Mark Pearson
- Matt Dembski
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. |
Jakub Kudzia (Gerrit)
Jakub Kudzia added 1 comment![]( "Open in Gerrit")
I reviewed all of components/variations/... except for the tests. I ran out of time for the day.
1.
Requested access to the link doc.2.
Please test this interactively with "--enable-features=" and developer tools to make sure the requests look correct (appended in the right times and not appended when not appropriate).
@mpea...@chromium.org access granted ;)
Could you please review once again?
Related details
Matt Dembski (Gerrit)
Matt Dembski voted Code-Review+1![]( "Open in Gerrit")
- Arthur Sonzogni
- Jakub Kudzia
- Kamil Jarosz
- Mark Pearson
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. |
Jakub Kudzia (Gerrit)
Jakub Kudzia added 1 comment![]( "Open in Gerrit")
// Copyright 2025 The Chromium AuthorsKamil JaroszThis test uses blink. Does it work on iOS? The build file seem to add it for all platforms.
Jakub Kudzia@jku...@google.com we only care about Android, so we can include this header for Android only.
Jakub KudziaGood catch, I'll exempt this test from running on iOS
As suggested I've added preprocessor directives to ensure the code runs only on Android. I just still need to figure out why there is an error on ios-simulator...
I've updated build files so that it's added only for android builds.
Related details
- Arthur Sonzogni
- Kamil Jarosz
- Mark Pearson
Kamil Jarosz (Gerrit)
Kamil Jarosz voted Code-Review+1![]( "Open in Gerrit")
- Arthur Sonzogni
- Jakub Kudzia
- Mark Pearson
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. |
Mark Pearson (Gerrit)
Mark Pearson added 16 comments![]( "Open in Gerrit")
Jakub KudziaI reviewed all of components/variations/... except for the tests. I ran out of time for the day.
1.
Requested access to the link doc.2.
Please test this interactively with "--enable-features=" and developer tools to make sure the requests look correct (appended in the right times and not appended when not appropriate).
@mpea...@chromium.org access granted ;)
Could you please review once again?
Please remember to test this interactively with "--enable-features=" and developer tools to make sure the requests look correct (appended in the right times and not appended when not appropriate).
While I'm pretty confident in your tests and am 99% sure the implementation is correct, in the remote chance we're wrong and only discover the bug later, that's a bug pain. The Chrome release cycle is long/slow. It's worth the extra minutes to confirm interactively.
components/variations/... generally lgtm with a series of minor comments.
Cheers,
Mark
// This function whitelists the header at the network context level so that itnit: allowlists
kNotFirstTab,Jakub KudziaI'm not sure how to understand this. Is it "do not show on the first tab"? Or show on things besides the first tab? Can you perhaps (a) choose a clearer name and (b) comment what this parameter does?
ditto below: please comment the effect of other parameters as well. People shouldn't have to find and read the implementation of the code to understand what a parameter is/does.
Thank you for the detailed feedback. I completely agree that the parameters need clearer documentation, and I will add detailed comments to fully explain the effect of each one immediately.
However, I would prefer not to rename these specific parameters. These names were established in the original Android feature implementation (https://chromium-review.googlesource.com/c/chromium/src/+/6923827) to enable experimentation. Renaming these established parameters would unnecessarily expand the scope of this CL, introducing more noise.
By adding thorough comments, I believe we fully resolve the concern about clarity without the distraction of an unnecessary rename.
Thank you for the detailed feedback. I completely agree that the parameters need clearer documentation, and I will add detailed comments to fully explain the effect of each one immediately.
However, I would prefer not to rename these specific parameters. These names were established in the original Android feature implementation (https://chromium-review.googlesource.com/c/chromium/src/+/6923827) to enable experimentation. Renaming these established parameters would unnecessarily expand the scope of this CL, introducing more noise.
Fair enough. I don't feel strongly enough to push this now that you have clearer comments.
By adding thorough comments, I believe we fully resolve the concern about clarity without the distraction of an unnecessary rename.
"not_first_tab",nit: Finch parameters names (the strings) are global. For here and below, can you please choose more specific names so they don't risk overlapping with another experiment?
Are you sure that's the case? A lot of feature flags have parameters with the same name & in some cases it's explicitly designed this way (e.g. HaTS). Feature flags are globally unique, but I don't think params are.
Are you sure that's the case? A lot of feature flags have parameters with the same name & in some cases it's explicitly designed this way (e.g. HaTS). Feature flags are globally unique, but I don't think params are.
It's kind of true. When you look up a parameter by a feature flag, the code finds the group the client is in for the study that sets the feature and finds the parameter. The problems really only arise when, for some reason, someone needs to combine two groups/studies and they have parameters with the same name. Maybe so unlikely to happen for this experiment that you don't need to worry about it.
// incognito NTP text content.All of these parameters read like an AND: if two are true, then both conditions need to be true so that omnibox autofocus is enabled. Is that correct? Or does the logic behave more like an OR: if not_first_tab enabled and not-first-tab then enable omnibox autofocus. Also, if with_hardware_keyboard enabled and hardware keyboard is attached, then enabled autofocus.
If the former, you may want to state explicitly that the AND implied by the language in these description is indeed what is happening here.
If its the latter, please rephrase (probably replace "only when" with "if").
params->cors_exempt_header_list.push_back(kOmniboxAutofocusHeader);Jakub KudziaSometimes we don't add this header in the even if in this case. Is it okay to call this function with a header that we tell the network request to apply?
I want to make sure I've understood your concern correctly. Are you asking why we add this header to the CORS-exempt list during initialization, even though we may not end up attaching it to every subsequent request?
My understanding is that this allow-listing is not done on per-request basis so I don't really see other way to do it. For variations header (X-Client-Data) it's done the same way: https://source.chromium.org/chromium/chromium/src/+/main:components/variations/net/variations_http_headers.cc;l=401-404;bpv=1;bpt=1?q=variations_http_hea&ss=chromium%2Fchromium%2Fsrc
I've added this allow-listing in every place where X-Client-Data is allow-listed as well.I've improved the comment to explicitly claim that the header itself might be absent.
Sound good.
You intuited what my question was getting at and answered it completely.
}));nit: Please add a least one test case to confirm that if a parameter is set to false, it doesn't enable that bit.
Context: I can easily imagine implementations where the presence of a parameter is what's causing the bit to be on rather than the value of the parameter itself.
EXPECT_FALSE(ShouldAppendHeader(GURL("https://www.not-google.com")));nits:
1. please check a subdomain of google.com such as codelabs.developers.google.com
2. please check a domain owned by google.com that's not google.com, such as youtube.com
Context: I don't know if you want the header sent in these conditions, so my request here is partially to make sure you've thought through the intended behavior for these cases.
}request: please add a test to confirm that the header is set when the feature flag is enabled but the behavior is not enabled (no parameters true). That's your control group right? Let's have an explicit test for the behavior of the control group to make sure that's right. The heading value should be 0 I believe.
Jakub KudziaHere should there be a call to ShouldAppendHeader() like there is in AppendOmniboxAutofocusHeaderIfNeeded()?
Not really, because `ShouldAppendHeader()` is called per request to verify whether request is done to a Google domain. Here, we're simply verifying whether experiment is active at all(by checking relevant feature flags via `GetHeaderValue()`)to decide whether enabling the throttle makes any sense.
Thanks for explaining.
optional nit: add an inline comment with your explanation
to_be_removed_headers->push_back(kOmniboxAutofocusHeader);Jakub KudziaIt is okay to add a header here that wasn't added to the original request?
yes, it looks like the code is handling it properly: https://source.chromium.org/chromium/chromium/src/+/main:net/http/http_request_headers.cc;l=170-174
Acknowledged
base::test::ScopedFeatureList feature_list;
feature_list.InitAndEnableFeature(kReportOmniboxAutofocusHeader);
base::test::ScopedFeatureList inner_feature_list;
inner_feature_list.InitAndEnableFeatureWithParameters(
kOmniboxAutofocusOnIncognitoNtp, {{"not_first_tab", "true"}});Having two feature lists within the same function is weird and also seems unnecessary. Can you use one?
CreateThrottle();I'd be happier with this test if it first enables the feature flag and parameters here, then asserts the header is sent to a regular google.com page, *then* checks the state of how headers are removed for redirects. Otherwise, it seems like the first part of this existing test is more like "don't send headers to non-google page", not "remove an existing header when a google page redirects to a non-google one".
variations::OmniboxAutofocusURLLoaderThrottle ::AppendThrottleIfNeeded(nit: errant space I think
variations::OmniboxAutofocusURLLoaderThrottle ::AppendThrottleIfNeeded(nit: errant space, I think
(unless its the required style?)
Related details
- Arthur Sonzogni
- Jakub Kudzia
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. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 2 comments![]( "Open in Gerrit")
#if BUILDFLAG(IS_ANDROID)
variations::UpdateCorsExemptHeaderForOmniboxAutofocus(context_params.get());
#endif // BUILDFLAG(IS_ANDROID)nit: Maybe the `#if BUILDFLAG` could be an early return inside the implementation of the function.
(That' optional, but it think it would avoid adding too many #if outside of this component)
variations::OmniboxAutofocusURLLoaderThrottle ::AppendThrottleIfNeeded(Ditto: `AppendThrottleIfNeeded` could be the one deciding that it is not needed outside of Android.
Related details
- Jakub Kudzia
Jakub Kudzia (Gerrit)
Jakub Kudzia added 5 comments![]( "Open in Gerrit")
// incognito NTP text content.All of these parameters read like an AND: if two are true, then both conditions need to be true so that omnibox autofocus is enabled. Is that correct? Or does the logic behave more like an OR: if not_first_tab enabled and not-first-tab then enable omnibox autofocus. Also, if with_hardware_keyboard enabled and hardware keyboard is attached, then enabled autofocus.
If the former, you may want to state explicitly that the AND implied by the language in these description is indeed what is happening here.
If its the latter, please rephrase (probably replace "only when" with "if").
That's the latter, I'll improve descriptions as suggested. Thank you!
nit: Please add a least one test case to confirm that if a parameter is set to false, it doesn't enable that bit.
Context: I can easily imagine implementations where the presence of a parameter is what's causing the bit to be on rather than the value of the parameter itself.
Good point, added
EXPECT_FALSE(ShouldAppendHeader(GURL("https://www.not-google.com")));nits:
1. please check a subdomain of google.com such as codelabs.developers.google.com
2. please check a domain owned by google.com that's not google.com, such as youtube.comContext: I don't know if you want the header sent in these conditions, so my request here is partially to make sure you've thought through the intended behavior for these cases.
We're only sending it to subdomains of google.com. I've updated tests
request: please add a test to confirm that the header is set when the feature flag is enabled but the behavior is not enabled (no parameters true). That's your control group right? Let's have an explicit test for the behavior of the control group to make sure that's right. The heading value should be 0 I believe.
Added :)
base::test::ScopedFeatureList feature_list;
feature_list.InitAndEnableFeature(kReportOmniboxAutofocusHeader);
base::test::ScopedFeatureList inner_feature_list;
inner_feature_list.InitAndEnableFeatureWithParameters(
kOmniboxAutofocusOnIncognitoNtp, {{"not_first_tab", "true"}});Having two feature lists within the same function is weird and also seems unnecessary. Can you use one?
- Kamil Jarosz
- Mark Pearson
- Matt Dembski
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. |
Jakub Kudzia (Gerrit)
Jakub Kudzia added 9 comments![]( "Open in Gerrit")
// This function whitelists the header at the network context level so that itJakub Kudzianit: allowlists
Done
"not_first_tab",Kamil Jarosznit: Finch parameters names (the strings) are global. For here and below, can you please choose more specific names so they don't risk overlapping with another experiment?
Mark PearsonAre you sure that's the case? A lot of feature flags have parameters with the same name & in some cases it's explicitly designed this way (e.g. HaTS). Feature flags are globally unique, but I don't think params are.
Are you sure that's the case? A lot of feature flags have parameters with the same name & in some cases it's explicitly designed this way (e.g. HaTS). Feature flags are globally unique, but I don't think params are.
It's kind of true. When you look up a parameter by a feature flag, the code finds the group the client is in for the study that sets the feature and finds the parameter. The problems really only arise when, for some reason, someone needs to combine two groups/studies and they have parameters with the same name. Maybe so unlikely to happen for this experiment that you don't need to worry about it.
Done
// incognito NTP text content.Jakub KudziaAll of these parameters read like an AND: if two are true, then both conditions need to be true so that omnibox autofocus is enabled. Is that correct? Or does the logic behave more like an OR: if not_first_tab enabled and not-first-tab then enable omnibox autofocus. Also, if with_hardware_keyboard enabled and hardware keyboard is attached, then enabled autofocus.
If the former, you may want to state explicitly that the AND implied by the language in these description is indeed what is happening here.
If its the latter, please rephrase (probably replace "only when" with "if").
That's the latter, I'll improve descriptions as suggested. Thank you!
Done
Jakub Kudzianit: Please add a least one test case to confirm that if a parameter is set to false, it doesn't enable that bit.
Context: I can easily imagine implementations where the presence of a parameter is what's causing the bit to be on rather than the value of the parameter itself.
Good point, added
Done
EXPECT_FALSE(ShouldAppendHeader(GURL("https://www.not-google.com")));Jakub Kudzianits:
1. please check a subdomain of google.com such as codelabs.developers.google.com
2. please check a domain owned by google.com that's not google.com, such as youtube.comContext: I don't know if you want the header sent in these conditions, so my request here is partially to make sure you've thought through the intended behavior for these cases.
We're only sending it to subdomains of google.com. I've updated tests
Done
Jakub Kudziarequest: please add a test to confirm that the header is set when the feature flag is enabled but the behavior is not enabled (no parameters true). That's your control group right? Let's have an explicit test for the behavior of the control group to make sure that's right. The heading value should be 0 I believe.
Added :)
Done
base::test::ScopedFeatureList feature_list;
feature_list.InitAndEnableFeature(kReportOmniboxAutofocusHeader);
base::test::ScopedFeatureList inner_feature_list;
inner_feature_list.InitAndEnableFeatureWithParameters(
kOmniboxAutofocusOnIncognitoNtp, {{"not_first_tab", "true"}});Jakub KudziaHaving two feature lists within the same function is weird and also seems unnecessary. Can you use one?
You're right. Updated
Done
variations::OmniboxAutofocusURLLoaderThrottle ::AppendThrottleIfNeeded(nit: errant space I think
Done
variations::OmniboxAutofocusURLLoaderThrottle ::AppendThrottleIfNeeded(nit: errant space, I think
(unless its the required style?)
Done
Jakub Kudzia (Gerrit)
Jakub Kudzia added 2 comments![]( "Open in Gerrit")
Jakub KudziaI reviewed all of components/variations/... except for the tests. I ran out of time for the day.
1.
Requested access to the link doc.2.
Please test this interactively with "--enable-features=" and developer tools to make sure the requests look correct (appended in the right times and not appended when not appropriate).
Mark Pearson@mpea...@chromium.org access granted ;)
Could you please review once again?
Please remember to test this interactively with "--enable-features=" and developer tools to make sure the requests look correct (appended in the right times and not appended when not appropriate).
While I'm pretty confident in your tests and am 99% sure the implementation is correct, in the remote chance we're wrong and only discover the bug later, that's a bug pain. The Chrome release cycle is long/slow. It's worth the extra minutes to confirm interactively.
Of course, you can be sure that I'll do it before submitting the CL
CreateThrottle();I'd be happier with this test if it first enables the feature flag and parameters here, then asserts the header is sent to a regular google.com page, *then* checks the state of how headers are removed for redirects. Otherwise, it seems like the first part of this existing test is more like "don't send headers to non-google page", not "remove an existing header when a google page redirects to a non-google one".
To improve the clarity and focus of the tests, I've split the WillRedirectRequest test into two separate, more descriptive test cases: WillRedirectRequest_ToNonGoogleDomain and WillRedirectRequest_ToGoogleDomain.
Each test now has a single, clear responsibility, which makes the test suite easier to read and maintain. This also ensures that we are unit testing the WillRedirectRequest method in isolation, without depending on the state from WillStartRequest, which I believe is the correct approach for a unit test.
Mark Pearson (Gerrit)
Mark Pearson voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mark Pearson
| Code-Review | +1 |
2 comments
CreateThrottle();Jakub KudziaI'd be happier with this test if it first enables the feature flag and parameters here, then asserts the header is sent to a regular google.com page, *then* checks the state of how headers are removed for redirects. Otherwise, it seems like the first part of this existing test is more like "don't send headers to non-google page", not "remove an existing header when a google page redirects to a non-google one".
To improve the clarity and focus of the tests, I've split the WillRedirectRequest test into two separate, more descriptive test cases: WillRedirectRequest_ToNonGoogleDomain and WillRedirectRequest_ToGoogleDomain.
Each test now has a single, clear responsibility, which makes the test suite easier to read and maintain. This also ensures that we are unit testing the WillRedirectRequest method in isolation, without depending on the state from WillStartRequest, which I believe is the correct approach for a unit test.
- Jakub Kudzia
- Kamil Jarosz
- Matt Dembski
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. |
Jakub Kudzia (Gerrit)
Jakub Kudzia added 2 comments![]( "Open in Gerrit")
#if BUILDFLAG(IS_ANDROID)
variations::UpdateCorsExemptHeaderForOmniboxAutofocus(context_params.get());
#endif // BUILDFLAG(IS_ANDROID)nit: Maybe the `#if BUILDFLAG` could be an early return inside the implementation of the function.
(That' optional, but it think it would avoid adding too many #if outside of this component)
Good idea, it's much cleaner this way.
variations::OmniboxAutofocusURLLoaderThrottle ::AppendThrottleIfNeeded(Ditto: `AppendThrottleIfNeeded` could be the one deciding that it is not needed outside of Android.
- Arthur Sonzogni
- Kamil Jarosz
- Mark Pearson
- Matt Dembski
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. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted Code-Review+1![]( "Open in Gerrit")
- Jakub Kudzia
- Kamil Jarosz
- Mark Pearson
- Matt Dembski
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. |
Matt Dembski (Gerrit)
Matt Dembski voted and added 1 comment![]( "Open in Gerrit")
Votes added by Matt Dembski
| Code-Review | +1 |
re-LGTM
Related details
- Jakub Kudzia
- Kamil Jarosz
- Mark Pearson
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. |
Kamil Jarosz (Gerrit)
Kamil Jarosz voted Code-Review+1![]( "Open in Gerrit")
- Jakub Kudzia
- Mark Pearson
Jakub Kudzia (Gerrit)
Jakub Kudzia added 1 comment![]( "Open in Gerrit")
Jakub KudziaI reviewed all of components/variations/... except for the tests. I ran out of time for the day.
1.
Requested access to the link doc.2.
Please test this interactively with "--enable-features=" and developer tools to make sure the requests look correct (appended in the right times and not appended when not appropriate).
Mark Pearson@mpea...@chromium.org access granted ;)
Could you please review once again?
Jakub KudziaPlease remember to test this interactively with "--enable-features=" and developer tools to make sure the requests look correct (appended in the right times and not appended when not appropriate).
While I'm pretty confident in your tests and am 99% sure the implementation is correct, in the remote chance we're wrong and only discover the bug later, that's a bug pain. The Chrome release cycle is long/slow. It's worth the extra minutes to confirm interactively.
Of course, you can be sure that I'll do it before submitting the CL
- Mark Pearson
Jakub Kudzia (Gerrit)
Jakub Kudzia added 1 comment![]( "Open in Gerrit")
Jakub KudziaI reviewed all of components/variations/... except for the tests. I ran out of time for the day.
1.
Requested access to the link doc.2.
Please test this interactively with "--enable-features=" and developer tools to make sure the requests look correct (appended in the right times and not appended when not appropriate).
Mark Pearson@mpea...@chromium.org access granted ;)
Could you please review once again?
Jakub KudziaPlease remember to test this interactively with "--enable-features=" and developer tools to make sure the requests look correct (appended in the right times and not appended when not appropriate).
While I'm pretty confident in your tests and am 99% sure the implementation is correct, in the remote chance we're wrong and only discover the bug later, that's a bug pain. The Chrome release cycle is long/slow. It's worth the extra minutes to confirm interactively.
Jakub KudziaOf course, you can be sure that I'll do it before submitting the CL
I've tested it interactively and it works as expected 😊
Done
Related details
- Mark Pearson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Pearson (Gerrit)
Mark Pearson added 1 comment![]( "Open in Gerrit")
if (is_android) {The story around testing is a little weird now. It's not obvious from this file or looking at the tests why these tests are only enabled for Android.
I'd suggest either
- prominent comments in both test files and here
or
- run the tests on all platforms (where the non-test libraries are compiled and called)
- If you choose this option, please add additional tests to demonstrate the library calls (that are now made in cross-platform code) are indeed no-ops.
- This option is my preference, as it ensures that the cross-platform code behaves as expected (no-ops) when called.
Related details
- Jakub Kudzia
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. |
Kamil Jarosz (Gerrit)
Kamil Jarosz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kamil Jarosz
| Code-Review | +1 |
1 comment
if (use_blink) { # TODO (jkudzia): check if this is needed, in particular ifPlease file an issue and reference it here instead of referencing a person
Related details
- Arthur Sonzogni
- Jakub Kudzia
- Matt Dembski
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. |
Jakub Kudzia (Gerrit)
Jakub Kudzia added 1 comment![]( "Open in Gerrit")
if (use_blink) { # TODO (jkudzia): check if this is needed, in particular ifPlease file an issue and reference it here instead of referencing a person
Thanks for the vigilance, but I'm going to resolve it before submitting this CL. It's just a reminder for me
Related details
- Arthur Sonzogni
- Kamil Jarosz
- Matt Dembski
Kamil Jarosz (Gerrit)
Kamil Jarosz added 1 comment![]( "Open in Gerrit")
if (use_blink) { # TODO (jkudzia): check if this is needed, in particular ifJakub KudziaPlease file an issue and reference it here instead of referencing a person
Thanks for the vigilance, but I'm going to resolve it before submitting this CL. It's just a reminder for me
- Arthur Sonzogni
- Jakub Kudzia
- Matt Dembski
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. |
Kamil Jarosz (Gerrit)
Kamil Jarosz voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Arthur Sonzogni
- Jakub Kudzia
- Matt Dembski
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. |
Jakub Kudzia (Gerrit)
Jakub Kudzia added 1 comment![]( "Open in Gerrit")
if (is_android) {The story around testing is a little weird now. It's not obvious from this file or looking at the tests why these tests are only enabled for Android.
I'd suggest either
- prominent comments in both test files and here
or
- run the tests on all platforms (where the non-test libraries are compiled and called)
- If you choose this option, please add additional tests to demonstrate the library calls (that are now made in cross-platform code) are indeed no-ops.
- This option is my preference, as it ensures that the cross-platform code behaves as expected (no-ops) when called.
Thank you for the thorough review :)
I've added missing tests and enabled them for blink as the library code will be run only on blink.
Related details
- Arthur Sonzogni
- Jakub Kudzia
- Mark Pearson
- Matt Dembski
Matt Dembski (Gerrit)
Matt Dembski voted and added 1 comment![]( "Open in Gerrit")
Votes added by Matt Dembski
| Code-Review | +1 |
1 comment
Related details
- Arthur Sonzogni
- Jakub Kudzia
- Mark Pearson
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. |
Mark Pearson (Gerrit)
Mark Pearson voted and added 3 comments![]( "Open in Gerrit")
Votes added by Mark Pearson
| Code-Review | +1 |
3 comments
components/variations/... lgtm with one last suggestion
if (is_android) {Jakub KudziaThe story around testing is a little weird now. It's not obvious from this file or looking at the tests why these tests are only enabled for Android.
I'd suggest either
- prominent comments in both test files and here
or
- run the tests on all platforms (where the non-test libraries are compiled and called)
- If you choose this option, please add additional tests to demonstrate the library calls (that are now made in cross-platform code) are indeed no-ops.
- This option is my preference, as it ensures that the cross-platform code behaves as expected (no-ops) when called.
Thank you for the thorough review :)
I've added missing tests and enabled them for blink as the library code will be run only on blink.
Thanks!
#endif // BUILDFLAG(IS_ANDROID)Please add a non-Android test for AppendOmniboxAutofocusHeaderIfNeeded, something like
... else if not Android:
```
base::test::ScopedFeatureList feature_list;
feature_list.InitWithFeaturesAndParameters(
{{kReportOmniboxAutofocusHeader, {}},
{kOmniboxAutofocusOnIncognitoNtp, {{"not_first_tab", "true"}}}},
{});
network::ResourceRequest request;
request.url = GURL("https://www.google.com");
AppendOmniboxAutofocusHeaderIfNeeded(request.url, &request);
std::optional<std::string> header_value =
request.cors_exempt_headers.GetHeader(kOmniboxAutofocusHeaderName);
EXPECT_FALSE(header_value.has_value());
```
Related details
- Arthur Sonzogni
- Jakub Kudzia
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. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted Code-Review+1![]( "Open in Gerrit")
- Jakub Kudzia
Jakub Kudzia (Gerrit)
Jakub Kudzia added 1 comment![]( "Open in Gerrit")
Please add a non-Android test for AppendOmniboxAutofocusHeaderIfNeeded, something like
... else if not Android:
```
base::test::ScopedFeatureList feature_list;
feature_list.InitWithFeaturesAndParameters(
{{kReportOmniboxAutofocusHeader, {}},
{kOmniboxAutofocusOnIncognitoNtp, {{"not_first_tab", "true"}}}},
{});network::ResourceRequest request;
request.url = GURL("https://www.google.com");
AppendOmniboxAutofocusHeaderIfNeeded(request.url, &request);
std::optional<std::string> header_value =
request.cors_exempt_headers.GetHeader(kOmniboxAutofocusHeaderName);
EXPECT_FALSE(header_value.has_value());
```
- Arthur Sonzogni
- Kamil Jarosz
- Mark Pearson
- Matt Dembski
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kamil Jarosz (Gerrit)
Kamil Jarosz voted Code-Review+1![]( "Open in Gerrit")
- Arthur Sonzogni
- Jakub Kudzia
- Mark Pearson
- Matt Dembski
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matt Dembski (Gerrit)
Matt Dembski voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Arthur Sonzogni
- Jakub Kudzia
- Mark Pearson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Jakub Kudzia
- Mark Pearson