[DoH] Move kForceSecureDnsDohFallback to the network service [chromium/src : main]
Andrew Williams (Gerrit)
Andrew Williams voted and added 2 comments![]( "Open in Gerrit")
Votes added by Andrew Williams
| Commit-Queue | +1 |
2 comments
Hi bashi@, could you take a preliminary look at this CL but ignoring the chrome/browser/net/ changes for now? This should be the last CL needed to run the DoH fallback upgrade experiment in M148. Thank you!
false /* force_check_parental_controls_for_automatic_mode */);Note: Skip this file for now, I split most of these changes off into https://chromium-review.googlesource.com/c/chromium/src/+/7723198 and plan to rebase this CL once that lands
Related details
- Andrew Williams
- Kenichi Ishibashi
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. |
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi added 3 comments![]( "Open in Gerrit")
Looking good, one suggestion.
false /* force_check_parental_controls_for_automatic_mode */);Note: Skip this file for now, I split most of these changes off into https://chromium-review.googlesource.com/c/chromium/src/+/7723198 and plan to rebase this CL once that lands
Acknowledged
if (context->IsDohConfigFromFallbackDohNameservers()) {optional: Can we have a helper function that takes `context` and ensure the check order, and share the helper function in other places?
Probably we need to use `base::expected` to return FallbackFromSecureTransactionPreferredReason.
Related details
- Andrew Williams
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. |
Andrew Williams (Gerrit)
Andrew Williams added 2 comments![]( "Open in Gerrit")
Thanks bashi@, if you could take look at the chrome/browser/net/ changes as well now that'd be great!
Also, Takashi, PTAL at the change to enums.xml. Thanks!
if (context->IsDohConfigFromFallbackDohNameservers()) {optional: Can we have a helper function that takes `context` and ensure the check order, and share the helper function in other places?
Probably we need to use `base::expected` to return FallbackFromSecureTransactionPreferredReason.
It seems like each of the checks is slightly different, so it might be difficult have one method that covers them all... If it's OK with you I'll just leave it this way, especially given that we will be removing all of this code once the experiment is over. Tentatively marking this as resolved.
Related details
- Kenichi Ishibashi
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi voted and added 2 comments![]( "Open in Gerrit")
Votes added by Kenichi Ishibashi
| Code-Review | +1 |
2 comments
lgtm, thanks!
if (context->IsDohConfigFromFallbackDohNameservers()) {Andrew Williamsoptional: Can we have a helper function that takes `context` and ensure the check order, and share the helper function in other places?
Probably we need to use `base::expected` to return FallbackFromSecureTransactionPreferredReason.
It seems like each of the checks is slightly different, so it might be difficult have one method that covers them all... If it's OK with you I'll just leave it this way, especially given that we will be removing all of this code once the experiment is over. Tentatively marking this as resolved.
Ack, it would be helpful if we could a comment saying that these logic will be removed once the experiment is over.
Related details
- Andrew Williams
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Andrew Williams
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Williams (Gerrit)
Andrew Williams voted and added 1 comment![]( "Open in Gerrit")
Votes added by Andrew Williams
| Commit-Queue | +2 |
1 comment
Thanks everyone! I did some manual testing of this and it looks good to me as well.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[DoH] Move kForceSecureDnsDohFallback to the network service
This moves the kForceSecureDnsDohFallback feature flag check to the
network service so that it is checked after we know:
- we can read the system DNS config
- the system isn't using a DNS provider that already supports
auto-upgrade
- the system isn't using a loopback or local network DNS server
- the profile has opted in to Enhanced Safe Browsing
This allows our experiment to only activate users into the experiment
after all of these conditions have evaluated to true, so the metrics
only surface changes from users that are actually eligible for the new
functionality.
NO_IFTTT=Looks like it's not picking up the LINT tag movement for some reason.
- M chrome/browser/net/stub_resolver_config_reader.cc
- M chrome/browser/net/stub_resolver_config_reader_unittest.cc
- M net/dns/canary_domain_service_unittest.cc
- M net/dns/dns_client.cc
- M net/dns/dns_client.h
- M net/dns/dns_client_unittest.cc
- M net/dns/dns_transaction.cc
- M net/dns/dns_transaction_unittest.cc
- M net/dns/resolve_context.cc
- M services/network/network_context_unittest.cc
- M tools/metrics/histograms/metadata/net/enums.xml
Code-Review: +1 by Kenichi Ishibashi, +1 by Takashi Toyoshima
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |