Add the Sec-CH-UA-Form-Factor header [chromium/src : main]
Dustin Mitchell (Gerrit)
Attention is currently required from: Ari Chivukula, Chromium IPC Reviews, Chromium Katabolism Reviews, Victor Tan.
Dustin Mitchell would like Chromium Katabolism Reviews, Ari Chivukula, Chromium IPC Reviews and Victor Tan to review this change.
Dustin Mitchell removed Yoav Weiss and Mike Taylor from this change.
Add the Sec-CH-UA-Form-Factor header
As described in https://wicg.github.io/ua-client-hints/#sec-ch-ua-form-factor
This is protected by the ClientHintsFormFactor feature flag, which is
disabled by default, so the CL introduces no new behavior. There are
still several open questions about the header, but this initial
implementation should make a suitable starting point regardless of how
those are decided.
This does _not_:
* Add the hint to NavigatorUAData (the spec does not require this).
* Add the hint to devtools (this can wait until more questions are answered).
Bug: 1442283
Change-Id: I964200563ad1f3dea3a8a7a59afc03b5d23f9cf3
---
M android_webview/javatests/src/org/chromium/android_webview/test/ClientHintsTest.java
M chrome/browser/client_hints/client_hints_browsertest.cc
M chrome/test/data/client_hints/accept_ch.html.mock-http-headers
M chrome/test/data/client_hints/accept_ch_img_localhost.html.mock-http-headers
M chrome/test/data/client_hints/http_equiv_accept_ch.html
M chrome/test/data/client_hints/http_equiv_accept_ch_delegation_bar.html
M chrome/test/data/client_hints/http_equiv_accept_ch_delegation_foo.html
M chrome/test/data/client_hints/http_equiv_accept_ch_delegation_merge.html
M chrome/test/data/client_hints/http_equiv_accept_ch_delegation_merge.html.mock-http-headers
M chrome/test/data/client_hints/http_equiv_accept_ch_img_localhost.html
M chrome/test/data/client_hints/http_equiv_accept_ch_injection.html
M chrome/test/data/client_hints/http_equiv_accept_ch_merge.html
M chrome/test/data/client_hints/meta_equiv_delegate_ch.html
M chrome/test/data/client_hints/meta_equiv_delegate_ch_delegation_bar.html
M chrome/test/data/client_hints/meta_equiv_delegate_ch_delegation_foo.html
M chrome/test/data/client_hints/meta_equiv_delegate_ch_delegation_merge.html
M chrome/test/data/client_hints/meta_equiv_delegate_ch_delegation_merge.html.mock-http-headers
M chrome/test/data/client_hints/meta_equiv_delegate_ch_img_localhost.html
M chrome/test/data/client_hints/meta_equiv_delegate_ch_injection.html
M chrome/test/data/client_hints/meta_equiv_delegate_ch_merge.html
M components/embedder_support/user_agent_utils.cc
M components/embedder_support/user_agent_utils_unittest.cc
M content/browser/client_hints/client_hints.cc
M content/browser/client_hints/client_hints_unittest.cc
M content/browser/loader/navigation_url_loader_impl_unittest.cc
M content/public/common/common_param_traits_macros.h
M content/shell/browser/shell_content_browser_client.cc
M services/network/public/cpp/client_hints.cc
M services/network/public/cpp/cors/cors.cc
M services/network/public/cpp/cors/cors_unittest.cc
M services/network/public/mojom/web_client_hints_types.mojom
M third_party/blink/common/client_hints/client_hints.cc
M third_party/blink/common/client_hints/client_hints_unittest.cc
M third_party/blink/common/client_hints/enabled_client_hints.cc
M third_party/blink/common/features.cc
M third_party/blink/common/user_agent/user_agent_metadata.cc
M third_party/blink/common/user_agent/user_agent_metadata_unittest.cc
M third_party/blink/common/user_agent/user_agent_mojom_traits.cc
M third_party/blink/public/common/features.h
M third_party/blink/public/common/user_agent/user_agent_metadata.h
M third_party/blink/public/common/user_agent/user_agent_mojom_traits.h
M third_party/blink/public/devtools_protocol/browser_protocol.pdl
M third_party/blink/public/mojom/permissions_policy/permissions_policy_feature.mojom
M third_party/blink/public/mojom/use_counter/metrics/web_feature.mojom
M third_party/blink/public/mojom/user_agent/user_agent_metadata.mojom
M third_party/blink/renderer/core/loader/base_fetch_context.cc
M third_party/blink/renderer/core/loader/frame_client_hints_preferences_context.cc
M third_party/blink/renderer/core/loader/frame_fetch_context_test.cc
M third_party/blink/renderer/core/permissions_policy/permissions_policy_features.json5
M third_party/blink/web_tests/external/wpt/client-hints/resources/export.js
M third_party/blink/web_tests/virtual/stable/webexposed/feature-policy-features-expected.txt
M third_party/blink/web_tests/webexposed/feature-policy-features-expected.txt
M tools/metrics/histograms/enums.xml
53 files changed, 216 insertions(+), 44 deletions(-)
To view, visit change 4558705. To unsubscribe, or for help writing mail filters, visit settings.
Dustin Mitchell (Gerrit)
Attention is currently required from: Ari Chivukula, Chromium IPC Reviews, Chromium Katabolism Reviews, Victor Tan.
gwsq (Gerrit)
Attention is currently required from: Ari Chivukula, Chromium IPC Reviews, Chromium Katabolism Reviews, Ken Buchanan, Victor Tan.
gwsq would like Ken Buchanan to review this change authored by Dustin Mitchell.
To view, visit change 4558705. To unsubscribe, or for help writing mail filters, visit settings.
gwsq (Gerrit)
Attention is currently required from: Ari Chivukula, Brendon Tiszka, Chromium IPC Reviews, Chromium Katabolism Reviews, Ken Buchanan, Victor Tan.
gwsq would like Brendon Tiszka to review this change authored by Dustin Mitchell.
gwsq (Gerrit)
Attention is currently required from: Ari Chivukula, Brendon Tiszka, Chromium Katabolism Reviews, Ken Buchanan, Victor Tan.
Dustin Mitchell has uploaded this change for review.
gwsq (Gerrit)
Attention is currently required from: Ari Chivukula, Brendon Tiszka, Ken Buchanan, Victor Tan.
gwsq (Gerrit)
Attention is currently required from: Ari Chivukula, Brendon Tiszka, Ken Buchanan, Victor Tan.
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
Shadow: tis...@chromium.org; IPC: ke...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
Shadow IPC reviewer(s): tis...@chromium.org. Please conduct an IPC review and CR+1 when satisfied. Remember to add the main reviewers to the attention set if needed.
Main IPC reviewer(s): ke...@chromium.org. Please wait for the shadowed IPC reviewer to CR+1 before reviewing.
Shadowed: tis...@chromium.org
Reviewer source(s):
vict...@chromium.org is from context(chrome/privacy_sandbox/katabolism/reviews.gwsq)
ke...@chromium.org, tis...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Brendon Tiszka (Gerrit)
Attention is currently required from: Ari Chivukula, Dustin Mitchell, Ken Buchanan, Victor Tan.
Patch set 12:Code-Review +1
1 comment:
Patchset:
mojom (shadow) lgtm
To view, visit change 4558705. To unsubscribe, or for help writing mail filters, visit settings.
Blink W3C Test Autoroller (Gerrit)
Attention is currently required from: Ari Chivukula, Dustin Mitchell, Ken Buchanan, Victor Tan.
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/40257.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Ken Buchanan (Gerrit)
Attention is currently required from: Ari Chivukula, Dustin Mitchell, Victor Tan.
Patchset:
lgtm *.mojom and *_traits*
To view, visit change 4558705. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Dustin Mitchell, Victor Tan.
5 comments:
Commit Message:
Patch Set #12, Line 18:
* Add the hint to NavigatorUAData (the spec does not require this).Why not do this now? Should a TODO at least be added?
Patchset:
Just a few notes, it mostly looks good.
File components/embedder_support/user_agent_utils.cc:
Patch Set #12, Line 622:
metadata.form_factor = metadata.mobile ? "Mobile" : "";Might be worth defining an enum now, or at least a const char[]
Patch Set #12, Line 654:
spoofed_ua.ua_metadata_override->form_factor = "desktop";I thought empty string was the 'desktop' indicator?
File third_party/blink/common/user_agent/user_agent_metadata_unittest.cc:
Patch Set #12, Line 31:
to_encode.form_factor = "tubular";I guess you really do want random storage here and not just an enum? I still think the ones the browser sends sans-spoofing should be an enum or at least const char[] somewhere.
To view, visit change 4558705. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Brendon Tiszka, Dustin Mitchell, Ken Buchanan, Victor Tan.
2 comments:
Commit Message:
Patch Set #13, Line 18:
* Add the hint to NavigatorUAData (the spec does not require this).Please remove
Patchset:
Was the question about serving a single string vs array ever answered? This would be useful in the case where multiple form factors are supported by the device
To view, visit change 4558705. To unsubscribe, or for help writing mail filters, visit settings.
Dustin Mitchell (Gerrit)
Attention is currently required from: Brendon Tiszka, Dustin Mitchell, Ken Buchanan, Victor Tan.
Dustin Mitchell uploaded patch set #14 to this change.
Add the Sec-CH-UA-Form-Factor header
As described in https://wicg.github.io/ua-client-hints/#sec-ch-ua-form-factor
This is protected by the ClientHintsFormFactor feature flag, which is
disabled by default, so the CL introduces no new behavior. There are
still several open questions about the header, but this initial
implementation should make a suitable starting point regardless of how
those are decided.
This does _not_:
M third_party/blink/renderer/core/frame/navigator_ua.cc
M third_party/blink/renderer/core/frame/navigator_ua_data.cc
M third_party/blink/renderer/core/frame/navigator_ua_data.h
M third_party/blink/renderer/core/loader/base_fetch_context.cc
M third_party/blink/renderer/core/loader/frame_client_hints_preferences_context.cc
M third_party/blink/renderer/core/loader/frame_fetch_context_test.cc
M third_party/blink/renderer/core/permissions_policy/permissions_policy_features.json5
M third_party/blink/web_tests/external/wpt/client-hints/resources/export.js
M third_party/blink/web_tests/external/wpt/html/webappapis/system-state-and-capabilities/the-navigator-object/navigator_user_agent.https.html
M third_party/blink/web_tests/external/wpt/workers/WorkerNavigator_userAgentData.https.html
M third_party/blink/web_tests/external/wpt/workers/support/WorkerNavigator.js
M third_party/blink/web_tests/virtual/stable/webexposed/feature-policy-features-expected.txt
M third_party/blink/web_tests/webexposed/feature-policy-features-expected.txt
M tools/metrics/histograms/enums.xml
59 files changed, 226 insertions(+), 46 deletions(-)
To view, visit change 4558705. To unsubscribe, or for help writing mail filters, visit settings.
Victor Tan (Gerrit)
Attention is currently required from: Brendon Tiszka, Dustin Mitchell, Ken Buchanan.
4 comments:
File chrome/browser/client_hints/client_hints_browsertest.cc:
Patch Set #13, Line 448:
network::mojom::WebClientHintsType::kUAFormFactor});For the tests in different platforms:
- can we add a positive test to verify what actually returns in the sec-form-factor, either in the unit test or browser_tests, can use android platform to verify whether return `mobile`.
- are you going to add the tests for other platform like fuchsia ?fuchsia_web/webengine/browser/client_hints_browsertest.c
File components/embedder_support/user_agent_utils.cc:
Patch Set #13, Line 625:
metadata.form_factor = metadata.mobile ? kMobileFormFactor : "";what would return if we can't detect the corresponding device, unknown or empty_string? what case we return unknown, what cases we return empty string?
File third_party/blink/common/features.cc:
Patch Set #12, Line 1259:
BASE_FEATURE(kClientHintsFormFactor,all new feature defined moved to `third_party/blink/renderer/platform/runtime_enabled_features.json5` if there are no custom behaviors. https://chromium.googlesource.com/chromium/src/third_party/+/HEAD/blink/renderer/platform/RuntimeEnabledFeatures.md#generate-a-instance-from-a-blink-feature
File third_party/blink/public/devtools_protocol/browser_protocol.pdl:
Patch Set #12, Line 7245:
ch-ua-form-factorif devtools changes are not included, this can be removed or separate to a different cl?
To view, visit change 4558705. To unsubscribe, or for help writing mail filters, visit settings.
Victor Tan (Gerrit)
Attention is currently required from: Ari Chivukula, Brendon Tiszka, Dustin Mitchell, Ken Buchanan.
1 comment:
File third_party/blink/common/features.cc:
Patch Set #12, Line 1259:
BASE_FEATURE(kClientHintsFormFactor,
Done
I don't think we need to define the feature here, a feature will automatically generated if you define the feature in third_party/blink/renderer/platform/runtime_enabled_features.json5
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/runtime_enabled_features.json5;l=118
To view, visit change 4558705. To unsubscribe, or for help writing mail filters, visit settings.
Dustin Mitchell (Gerrit)
Attention is currently required from: Ari Chivukula, Brendon Tiszka, Ken Buchanan, Victor Tan.
1 comment:
File third_party/blink/common/features.cc:
Patch Set #12, Line 1259:
BASE_FEATURE(kClientHintsFormFactor,
I don't think we need to define the feature here, a feature will automatically generated if you defi […]
Oops! I stashed these changes and forgot about them!
However, having made these changes, I don't seem to be able to enable the feature in browser tests. Neither
```
feature_list->InitializeFromCommandLine(
"UserAgentClientHint,ClientHintsFormFactor", ""); ```
nor
```
feature_list->RegisterExtraFeatureOverrides({
{std::cref(blink::features::kClientHintsFormFactor),
base::FeatureList::OverrideState::OVERRIDE_ENABLE_FEATURE},
});
```
does anything -- all of the tests that expect the header to be present fail because it is not present. This worked with the "regular" blink feature. I see in https://chromium.googlesource.com/chromium/src/third_party/+/HEAD/blink/renderer/platform/RuntimeEnabledFeatures.md that enabling features via command-line flag only works for features with no `status` property, which is what I've defined. But setting `status: "test"` does not seem to help, at least with the first form of enablement, above.@victortan, any ideas?
Ari Chivukula (Gerrit)
Attention is currently required from: Brendon Tiszka, Dustin Mitchell, Ken Buchanan, Victor Tan.
6 comments:
File content/browser/client_hints/client_hints.cc:
Patch Set #15, Line 858:
if (base::FeatureList::IsEnabled(blink::features::kClientHintsFormFactor)) {I think this is redundant because of the IsDisabledByFeature check in ShouldAddClientHint?
File third_party/blink/common/features.cc:
Patch Set #12, Line 1259:
BASE_FEATURE(kClientHintsFormFactor,
Oops! I stashed these changes and forgot about them! […]
I don't think you need a runtime enabled feature at all, a simple base::Feature is fine. If you only referenced this in the renderer, then a runtime feature is nice because you can mark it as experimental and have it auto-enable in WPTs, but if it's referenced outside that directory a base::feature is a lot simpler.
File third_party/blink/public/devtools_protocol/browser_protocol.pdl:
Patch Set #12, Line 7245:
ch-ua-form-factor
This seems to be required to support the permissions policy. Without it: […]
It's okay to include this here, devtools-frontend won't break and it's not adding the hint just the permission
File third_party/blink/renderer/core/frame/navigator_ua.cc:
Patch Set #15, Line 29:
ua_data->SetFormFactor(String::FromUTF8(metadata.form_factor));I think you want to protect this with a base::Feature check right?
File third_party/blink/renderer/core/frame/navigator_ua_data.cc:
Patch Set #15, Line 224:
values->setBitness(form_factor_);SetFormFactor right? Shouldn't this have been caught in a test?
File third_party/blink/renderer/core/loader/base_fetch_context.cc:
Patch Set #15, Line 382:
if (base::FeatureList::IsEnabled(blink::features::kClientHintsFormFactor)) {I think this is redundant because of the IsDisabledByFeature check in ShouldSendClientHint?
To view, visit change 4558705. To unsubscribe, or for help writing mail filters, visit settings.
Victor Tan (Gerrit)
Attention is currently required from: Ari Chivukula, Brendon Tiszka, Dustin Mitchell, Ken Buchanan.
2 comments:
File chrome/browser/client_hints/client_hints_browsertest.cc:
Patch Set #13, Line 448:
network::mojom::WebClientHintsType::kUAFormFactor});
`components/embedder_support/user_agent_utils_unittest. […]
ok, I'm fine if the todo already cover those
File third_party/blink/common/features.cc:
Patch Set #12, Line 1259:
BASE_FEATURE(kClientHintsFormFactor,
I don't think you need a runtime enabled feature at all, a simple base::Feature is fine. […]
1. go with runtime feature: to enable a Blink runtime features in content layer from command line: you need to specify the mapping in https://source.chromium.org/chromium/chromium/src/+/main:content/child/runtime_features.cc;l=334, source: https://chromium.googlesource.com/chromium/src/third_party/+/HEAD/blink/renderer/platform/RuntimeEnabledFeatures.md, if you set status to `experimental`, WPT will auto enabled.
2. go with simple base feature: you need to manually set the override in content/public/common/content_switch_dependent_feature_overrides.cc to allow enable WPT to run your tests. https://source.chromium.org/chromium/chromium/src/+/main:content/public/common/content_switch_dependent_feature_overrides.cc;l=29
I'm fine with a simple base feature.
To view, visit change 4558705. To unsubscribe, or for help writing mail filters, visit settings.
Ken Buchanan (Gerrit)
Attention is currently required from: Brendon Tiszka, Dustin Mitchell, Victor Tan.
Patch set 18:Code-Review +1
Brendon Tiszka (Gerrit)
Attention is currently required from: Dustin Mitchell, Victor Tan.
Dustin Mitchell (Gerrit)
Attention is currently required from: Brendon Tiszka, Victor Tan.
Dustin Mitchell has uploaded this change for review.
Add the Sec-CH-UA-Form-Factor header
As described in https://wicg.github.io/ua-client-hints/#sec-ch-ua-form-factor
This is protected by the ClientHintsFormFactor feature flag, which is
disabled by default, so the CL introduces no new behavior. There are
still several open questions about the header, but this initial
implementation should make a suitable starting point regardless of how
those are decided.
This does _not_:
* Add the hint to devtools (this can wait until more questions are answered).
Bug: 1442283
Change-Id: I964200563ad1f3dea3a8a7a59afc03b5d23f9cf3
---
M chrome/browser/client_hints/client_hints_browsertest.cc
M chrome/browser/ui/browser_commands.cc
M chrome/test/data/client_hints/accept_ch.html.mock-http-headers
M chrome/test/data/client_hints/accept_ch_img_localhost.html.mock-http-headers
M chrome/test/data/client_hints/http_equiv_accept_ch.html
M chrome/test/data/client_hints/http_equiv_accept_ch_delegation_bar.html
M chrome/test/data/client_hints/http_equiv_accept_ch_delegation_foo.html
M chrome/test/data/client_hints/http_equiv_accept_ch_delegation_merge.html
M chrome/test/data/client_hints/http_equiv_accept_ch_delegation_merge.html.mock-http-headers
M chrome/test/data/client_hints/http_equiv_accept_ch_img_localhost.html
M chrome/test/data/client_hints/http_equiv_accept_ch_injection.html
M chrome/test/data/client_hints/http_equiv_accept_ch_merge.html
M chrome/test/data/client_hints/meta_equiv_delegate_ch.html
M chrome/test/data/client_hints/meta_equiv_delegate_ch_delegation_bar.html
M chrome/test/data/client_hints/meta_equiv_delegate_ch_delegation_foo.html
M chrome/test/data/client_hints/meta_equiv_delegate_ch_delegation_merge.html
M chrome/test/data/client_hints/meta_equiv_delegate_ch_delegation_merge.html.mock-http-headers
M chrome/test/data/client_hints/meta_equiv_delegate_ch_img_localhost.html
M chrome/test/data/client_hints/meta_equiv_delegate_ch_injection.html
M chrome/test/data/client_hints/meta_equiv_delegate_ch_merge.html
M components/embedder_support/user_agent_utils.cc
M components/embedder_support/user_agent_utils.h
M components/embedder_support/user_agent_utils_unittest.cc
M content/browser/client_hints/client_hints.cc
M content/browser/client_hints/client_hints_unittest.cc
M content/browser/loader/navigation_url_loader_impl_unittest.cc
M content/public/common/common_param_traits_macros.h
M content/public/common/content_switch_dependent_feature_overrides.cc
M content/shell/browser/shell_content_browser_client.cc
M headless/test/headless_browser_user_agent_metadata_browsertest.cc
M services/network/public/cpp/client_hints.cc
M services/network/public/cpp/cors/cors.cc
M services/network/public/cpp/cors/cors_unittest.cc
M services/network/public/mojom/web_client_hints_types.mojom
M third_party/blink/common/client_hints/client_hints.cc
M third_party/blink/common/client_hints/client_hints_unittest.cc
M third_party/blink/common/client_hints/enabled_client_hints.cc
M third_party/blink/common/features.cc
M third_party/blink/common/user_agent/user_agent_metadata.cc
M third_party/blink/common/user_agent/user_agent_metadata_unittest.cc
M third_party/blink/common/user_agent/user_agent_mojom_traits.cc
M third_party/blink/public/common/features.h
M third_party/blink/public/common/user_agent/user_agent_metadata.h
M third_party/blink/public/common/user_agent/user_agent_mojom_traits.h
M third_party/blink/public/devtools_protocol/browser_protocol.pdl
M third_party/blink/public/mojom/permissions_policy/permissions_policy_feature.mojom
M third_party/blink/public/mojom/use_counter/metrics/web_feature.mojom
M third_party/blink/public/mojom/user_agent/user_agent_metadata.mojom
M third_party/blink/renderer/core/frame/navigator_ua.cc
M third_party/blink/renderer/core/frame/navigator_ua_data.cc
M third_party/blink/renderer/core/frame/navigator_ua_data.h
M third_party/blink/renderer/core/frame/ua_data_values.idl
M third_party/blink/renderer/core/loader/base_fetch_context.cc
M third_party/blink/renderer/core/loader/frame_client_hints_preferences_context.cc
M third_party/blink/renderer/core/loader/frame_fetch_context_test.cc
M third_party/blink/renderer/core/permissions_policy/permissions_policy_features.json5
M third_party/blink/web_tests/external/wpt/client-hints/resources/export.js
M third_party/blink/web_tests/external/wpt/html/webappapis/system-state-and-capabilities/the-navigator-object/navigator_user_agent.https.html
M third_party/blink/web_tests/external/wpt/workers/WorkerNavigator_userAgentData.https.html
M third_party/blink/web_tests/external/wpt/workers/support/WorkerNavigator.js
M third_party/blink/web_tests/virtual/stable/webexposed/feature-policy-features-expected.txt
M third_party/blink/web_tests/webexposed/feature-policy-features-expected.txt
M tools/metrics/histograms/enums.xml
63 files changed, 244 insertions(+), 42 deletions(-)
Dustin Mitchell (Gerrit)
Attention is currently required from: Victor Tan.
Dustin Mitchell (Gerrit)
Attention is currently required from: Victor Tan.
1 comment:
Patchset:
Apologies Brendon & Ken -- I think webview won't see a change here, since the hint is disabled by default.
To view, visit change 4558705. To unsubscribe, or for help writing mail filters, visit settings.
Dustin Mitchell (Gerrit)
Attention is currently required from: Ken Buchanan, Victor Tan.
Dustin Mitchell would like Ken Buchanan to review this change.
Add the Sec-CH-UA-Form-Factor header
As described in https://wicg.github.io/ua-client-hints/#sec-ch-ua-form-factor
This is protected by the ClientHintsFormFactor feature flag, which is
disabled by default, so the CL introduces no new behavior. There are
still several open questions about the header, but this initial
implementation should make a suitable starting point regardless of how
those are decided.
This does _not_:
* Add the hint to devtools (this can wait until more questions are answered).
Bug: 1442283
Change-Id: I964200563ad1f3dea3a8a7a59afc03b5d23f9cf3
---
M android_webview/javatests/src/org/chromium/android_webview/test/ClientHintsTest.java
64 files changed, 275 insertions(+), 57 deletions(-)
Dustin Mitchell (Gerrit)
Attention is currently required from: Brendon Tiszka, Ken Buchanan, Victor Tan.
Dustin Mitchell would like Brendon Tiszka to review this change.
Brendon Tiszka (Gerrit)
Attention is currently required from: Dustin Mitchell, Ken Buchanan, Victor Tan.
Patch set 21:Code-Review +1
Victor Tan (Gerrit)
Attention is currently required from: Dustin Mitchell, Ken Buchanan.
1 comment:
File android_webview/javatests/src/org/chromium/android_webview/test/ClientHintsTest.java:
Patch Set #21, Line 95:
"enable-features=ClientHintsFormFactor"})this test seems no need to enable ClientHintsFormFactor because user-agent client hints disabled in this test, enable a subset feature seems a little weird. The test verification also expected not found in the header. We only need to enable the ClientHintsFormFactor when UACH enabled in the tests.
same for test `testClientHintsDefault` and `testCriticalClientHints`.
To view, visit change 4558705. To unsubscribe, or for help writing mail filters, visit settings.
Dustin Mitchell (Gerrit)
Attention is currently required from: Bo Liu, Ken Buchanan, Rob Buis, Victor Tan.
Dustin Mitchell would like Bo Liu and Rob Buis to review this change.
Dustin Mitchell removed Nate Chapin from this change.
64 files changed, 263 insertions(+), 51 deletions(-)
Ken Buchanan (Gerrit)
Attention is currently required from: Bo Liu, Dustin Mitchell, Rob Buis, Victor Tan.
Patch set 22:Code-Review +1
Victor Tan (Gerrit)
Attention is currently required from: Bo Liu, Dustin Mitchell, Rob Buis.
Dustin Mitchell (Gerrit)
Attention is currently required from: Andrey Kosyakov, Bo Liu, Eric Seckler, Kent Tamura, Kinuko Yasuda, Rob Buis, Scott Violet, danakj.
Dustin Mitchell would like Kent Tamura, Kinuko Yasuda, danakj, Eric Seckler, Scott Violet and Andrey Kosyakov to review this change.
To view, visit change 4558705. To unsubscribe, or for help writing mail filters, visit settings.
Dustin Mitchell (Gerrit)
Attention is currently required from: Andrey Kosyakov, Bo Liu, Eric Seckler, Kent Tamura, Kinuko Yasuda, Rob Buis, Scott Violet, danakj.
1 comment:
Patchset:
@Bo, @Rob Buis, PTAL?
Remaining owners: P(also)TAL, thanks!
To view, visit change 4558705. To unsubscribe, or for help writing mail filters, visit settings.
Bo Liu (Gerrit)
Attention is currently required from: Andrey Kosyakov, Dustin Mitchell, Eric Seckler, Kent Tamura, Kinuko Yasuda, Rob Buis, Scott Violet, danakj.
1 comment:
File android_webview/javatests/src/org/chromium/android_webview/test/ClientHintsTest.java:
Patch Set #22, Line 54:
private static final String[] USER_AGENT_CLIENT_HINTS_DISABLED = {"sec-ch-ua-form-factor"};not used?
Scott Violet (Gerrit)
Attention is currently required from: Andrey Kosyakov, Bo Liu, Dustin Mitchell, Eric Seckler, Kent Tamura, Kinuko Yasuda, Rob Buis, danakj.
Patch set 23:Code-Review +1
1 comment:
Patchset:
I'm assuming you added me for chrome/browser/ui/browser_commands.cc . That LGTM
To view, visit change 4558705. To unsubscribe, or for help writing mail filters, visit settings.
Dustin Mitchell (Gerrit)
Attention is currently required from: Andrey Kosyakov, Bo Liu, Eric Seckler, Kent Tamura, Kinuko Yasuda, Rob Buis, danakj.
1 comment:
Patchset:
I'm assuming you added me for chrome/browser/ui/browser_commands.cc . […]
Yes. I apologize that those auto-generated "files you own" messages were not particularly helpful.
Eric Seckler (Gerrit)
Attention is currently required from: Andrey Kosyakov, Ari Chivukula, Bo Liu, Dustin Mitchell, Kent Tamura, Kinuko Yasuda, Rob Buis, danakj.
1 comment:
Patchset:
I'll delegate to caseq@ for headless/, since you've got him in the reviewers list already 😊
To view, visit change 4558705. To unsubscribe, or for help writing mail filters, visit settings.
Ari Chivukula (Gerrit)
Attention is currently required from: Andrey Kosyakov, Bo Liu, Dustin Mitchell, Kent Tamura, Kinuko Yasuda, Rob Buis, danakj.
Patch set 23:Code-Review +1
2 comments:
Patchset:
LGTM % one question
File third_party/blink/renderer/core/frame/navigator_ua_data.cc:
Patch Set #23, Line 244:
ScriptValue NavigatorUAData::toJSON(ScriptState* script_state) const {Does it also need to be added here?
Victor Tan (Gerrit)
Attention is currently required from: Andrey Kosyakov, Ari Chivukula, Bo Liu, Dustin Mitchell, Kent Tamura, Kinuko Yasuda, Rob Buis, danakj.
Patch set 23:Code-Review +1
1 comment:
File third_party/blink/renderer/core/frame/navigator_ua_data.cc:
Patch Set #23, Line 244:
ScriptValue NavigatorUAData::toJSON(ScriptState* script_state) const {Does it also need to be added here?
no, this is only for the low entropy hints. for all high entropy hints called as callback when toJSON. https://source.chromium.org/chromium/chromium/src/+/refs/heads/main:out/Debug/gen/third_party/blink/renderer/bindings/core/v8/v8_navigator_ua_data.cc;l=245
Ari Chivukula (Gerrit)
Attention is currently required from: Andrey Kosyakov, Bo Liu, Dustin Mitchell, Kent Tamura, Kinuko Yasuda, Rob Buis, danakj.
1 comment:
File third_party/blink/renderer/core/frame/navigator_ua_data.cc:
Patch Set #23, Line 244:
ScriptValue NavigatorUAData::toJSON(ScriptState* script_state) const {
no, this is only for the low entropy hints. […]
Done
Dustin Mitchell (Gerrit)
Attention is currently required from: Andrey Kosyakov, Arthur Sonzogni, Bo Liu, Yoav Weiss.
Dustin Mitchell would like Arthur Sonzogni and Yoav Weiss to review this change.
Dustin Mitchell removed Kent Tamura, Kinuko Yasuda, danakj and Rob Buis from this change.
64 files changed, 260 insertions(+), 51 deletions(-)
Dustin Mitchell (Gerrit)
Attention is currently required from: Andrey Kosyakov, Arthur Sonzogni, Bo Liu, Yoav Weiss.
1 comment:
Patchset:
@victortan suggested a better assortment of reviewers than the gerrit UI, so I've adjusted accordingly.
To view, visit change 4558705. To unsubscribe, or for help writing mail filters, visit settings.
Bo Liu (Gerrit)
Attention is currently required from: Andrey Kosyakov, Arthur Sonzogni, Dustin Mitchell, Yoav Weiss.
Patch set 23:Code-Review +1
Arthur Sonzogni (Gerrit)
Attention is currently required from: Andrey Kosyakov, Dustin Mitchell, Yoav Weiss.
2 comments:
Patchset:
Thanks!
There are many files, what should I review? content/, content/public, IPC?
File content/browser/loader/navigation_url_loader_impl_unittest.cc:
base::test::ScopedFeatureList scoped_feature_list;
scoped_feature_list.InitAndEnableFeature(
blink::features::kClientHintsFormFactor);What about this warning:
Andrey Kosyakov (Gerrit)
Attention is currently required from: Arthur Sonzogni, Dustin Mitchell, Yoav Weiss.
1 comment:
File third_party/blink/public/devtools_protocol/browser_protocol.pdl:
Patch Set #24, Line 7246:
ch-ua-form-factorI would expect this to be used somewhere on the back-end. Perhaps you meant this to be added to `PermissionsPolicyFeatureToProtocol()`?
To view, visit change 4558705. To unsubscribe, or for help writing mail filters, visit settings.
Dustin Mitchell (Gerrit)
Attention is currently required from: Andrey Kosyakov, Ari Chivukula, Arthur Sonzogni, Bo Liu, Brendon Tiszka, Ken Buchanan, Scott Violet, Victor Tan, Yoav Weiss.
Patch set 25:Commit-Queue +1
Arthur Sonzogni (Gerrit)
Attention is currently required from: Andrey Kosyakov, Ari Chivukula, Bo Liu, Brendon Tiszka, Dustin Mitchell, Ken Buchanan, Scott Violet, Victor Tan, Yoav Weiss.
Patch set 27:Code-Review +1
2 comments:
File content/public/common/common_param_traits_macros.h:
IPC_STRUCT_TRAITS_BEGIN(blink::UserAgentMetadata)
IPC_STRUCT_TRAITS_MEMBER(brand_version_list)
IPC_STRUCT_TRAITS_MEMBER(brand_full_version_list)
IPC_STRUCT_TRAITS_MEMBER(full_version)
IPC_STRUCT_TRAITS_MEMBER(platform)
IPC_STRUCT_TRAITS_MEMBER(platform_version)
IPC_STRUCT_TRAITS_MEMBER(architecture)
IPC_STRUCT_TRAITS_MEMBER(model)
IPC_STRUCT_TRAITS_MEMBER(mobile)
IPC_STRUCT_TRAITS_MEMBER(bitness)
IPC_STRUCT_TRAITS_MEMBER(wow64)
IPC_STRUCT_TRAITS_MEMBER(form_factor)
IPC_STRUCT_TRAITS_END()Question outside of the scope of the CL:
Are we still sending it through the old IPC system? Is it still needed? Can we remove it?
File headless/test/headless_browser_user_agent_metadata_browsertest.cc:
protected:
raw_ptr<HeadlessWebContents, DanglingUntriaged> web_contents_;
SimpleDevToolsProtocolClient devtools_client_;
base::test::ScopedFeatureList scoped_feature_list_;nit: Move them into the private section?
To view, visit change 4558705. To unsubscribe, or for help writing mail filters, visit settings.
Ken Buchanan (Gerrit)
Attention is currently required from: Andrey Kosyakov, Ari Chivukula, Arthur Sonzogni, Bo Liu, Brendon Tiszka, Dustin Mitchell, Scott Violet, Victor Tan, Yoav Weiss.
Patch set 29:Code-Review +1
Victor Tan (Gerrit)
Attention is currently required from: Andrey Kosyakov, Ari Chivukula, Arthur Sonzogni, Bo Liu, Brendon Tiszka, Dustin Mitchell, Scott Violet, Yoav Weiss.
Patch set 29:Code-Review +1
1 comment:
File android_webview/javatests/src/org/chromium/android_webview/test/ClientHintsTest.java:
Patch Set #29, Line 250:
@CommandLineFlags.Add({"enable-features=UserAgentClientHint",nit: enable-features can combines:
`enable-features=UserAgentClientHint,ClientHintsFormFactor`
Arthur Sonzogni (Gerrit)
Attention is currently required from: Andrey Kosyakov, Ari Chivukula, Bo Liu, Brendon Tiszka, Dustin Mitchell, Scott Violet, Yoav Weiss.
Patch set 29:Code-Review +1
2 comments:
Patchset:
Patchset 27->29: No change I am owning. So still LGTM.
File content/public/common/common_param_traits_macros.h:
IPC_STRUCT_TRAITS_BEGIN(blink::UserAgentMetadata)
IPC_STRUCT_TRAITS_MEMBER(brand_version_list)
IPC_STRUCT_TRAITS_MEMBER(brand_full_version_list)
IPC_STRUCT_TRAITS_MEMBER(full_version)
IPC_STRUCT_TRAITS_MEMBER(platform)
IPC_STRUCT_TRAITS_MEMBER(platform_version)
IPC_STRUCT_TRAITS_MEMBER(architecture)
IPC_STRUCT_TRAITS_MEMBER(model)
IPC_STRUCT_TRAITS_MEMBER(mobile)
IPC_STRUCT_TRAITS_MEMBER(bitness)
IPC_STRUCT_TRAITS_MEMBER(wow64)
IPC_STRUCT_TRAITS_MEMBER(form_factor)
IPC_STRUCT_TRAITS_END()
I don't know the answer to this -- this was a formulaic change following the pattern of other CH's. […]
Okay. Then I will remove and cleanup all the unnecessary content from this file.
Arthur Sonzogni (Gerrit)
Attention is currently required from: Andrey Kosyakov, Ari Chivukula, Bo Liu, Brendon Tiszka, Dustin Mitchell, Scott Violet, Yoav Weiss.
IPC_STRUCT_TRAITS_BEGIN(blink::UserAgentMetadata)
IPC_STRUCT_TRAITS_MEMBER(brand_version_list)
IPC_STRUCT_TRAITS_MEMBER(brand_full_version_list)
IPC_STRUCT_TRAITS_MEMBER(full_version)
IPC_STRUCT_TRAITS_MEMBER(platform)
IPC_STRUCT_TRAITS_MEMBER(platform_version)
IPC_STRUCT_TRAITS_MEMBER(architecture)
IPC_STRUCT_TRAITS_MEMBER(model)
IPC_STRUCT_TRAITS_MEMBER(mobile)
IPC_STRUCT_TRAITS_MEMBER(bitness)
IPC_STRUCT_TRAITS_MEMBER(wow64)
IPC_STRUCT_TRAITS_MEMBER(form_factor)
IPC_STRUCT_TRAITS_END()
Okay. Then I will remove and cleanup all the unnecessary content from this file.
WIP https://chromium-review.googlesource.com/c/chromium/src/+/4615128
Andrey Kosyakov (Gerrit)
Attention is currently required from: Ari Chivukula, Bo Liu, Brendon Tiszka, Dustin Mitchell, Scott Violet, Yoav Weiss.
Patch set 29:Code-Review +1
1 comment:
Patchset:
devtools/ rslgtm
Brendon Tiszka (Gerrit)
Attention is currently required from: Ari Chivukula, Bo Liu, Dustin Mitchell, Scott Violet, Yoav Weiss.
Patchset:
27->29 no change I am owning
Ari Chivukula (Gerrit)
Attention is currently required from: Bo Liu, Dustin Mitchell, Scott Violet, Yoav Weiss.
Patchset:
Changes since last approval seem mostly rebase based except for a few new tests
Yoav Weiss (Gerrit)
Attention is currently required from: Bo Liu, Dustin Mitchell, Scott Violet.
Patchset:
LGTM
Dustin Mitchell (Gerrit)
Attention is currently required from: Bo Liu, Scott Violet, Victor Tan.
Patch set 30:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
29 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: tools/metrics/histograms/enums.xml
Insertions: 5, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: third_party/blink/common/features.cc
Insertions: 78, Deletions: 221.
The diff is too large to show. Please review the diff.
```
```
The name of the file: third_party/blink/public/mojom/use_counter/metrics/web_feature.mojom
Insertions: 5, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: third_party/blink/public/common/features.h
Insertions: 99, Deletions: 641.
The diff is too large to show. Please review the diff.
```
```
The name of the file: android_webview/javatests/src/org/chromium/android_webview/test/ClientHintsTest.java
Insertions: 10, Deletions: 15.
The diff is too large to show. Please review the diff.
```
Add the Sec-CH-UA-Form-Factor header
As described in https://wicg.github.io/ua-client-hints/#sec-ch-ua-form-factor
This is protected by the ClientHintsFormFactor feature flag, which is
disabled by default, so the CL introduces no new behavior. There are
still several open questions about the header, but this initial
implementation should make a suitable starting point regardless of how
those are decided.
This does _not_:
* Add the hint to devtools (this can wait until more questions are answered).
Bug: 1442283
Change-Id: I964200563ad1f3dea3a8a7a59afc03b5d23f9cf3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4558705
Reviewed-by: Ken Buchanan <ke...@chromium.org>
Reviewed-by: Arthur Sonzogni <arthurs...@chromium.org>
Reviewed-by: Yoav Weiss <yoav...@chromium.org>
Commit-Queue: Dustin Mitchell <djmi...@chromium.org>
Reviewed-by: Ari Chivukula <ari...@chromium.org>
Reviewed-by: Andrey Kosyakov <ca...@chromium.org>
Reviewed-by: Brendon Tiszka <tis...@chromium.org>
Reviewed-by: Victor Tan <vict...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1160067}
64 files changed, 257 insertions(+), 51 deletions(-)
Blink W3C Test Autoroller (Gerrit)
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/40257
Hailey Wang (Gerrit)
Hailey Wang has created a revert of this change.
To view, visit change 4558705. To unsubscribe, or for help writing mail filters, visit settings.