IPP: Don't modify ProxyList for direct-only IPP proxy lists [chromium/src : main]
Giovanni Ortuno Urquidi (Gerrit)
Giovanni Ortuno Urquidi added 1 comment![]( "Open in Gerrit")
Related details
- Andrew Williams
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Williams (Gerrit)
Andrew Williams voted and added 8 comments![]( "Open in Gerrit")
Votes added by Andrew Williams
| Code-Review | +1 |
8 comments
LGTM with some minor feedback and questions. Using the MockIpProtectionCore to enable use of the real IpProtectionProxyDelegate in these higher level tests turned out simpler than I thought it would - thanks for coming up with that!
by Net.HttpJob.IpProtection.TotalTimeNotCached2nit:
```suggestion
by Net.HttpJob.IpProtection.TotalTimeNotCached3
```
const char kSimpleGetMockWrite[] =I think these will get converted to `std::string_view`s for use by `net::MockRead`, which I'm assuming will require the length to be computed at runtime. Would it be better for these to be `constexpr std::string_view` to begin with?
net::ProxyServer::SCHEME_HTTPS, "proxy-a.invalid", 443);Should we just use `proxy-a` and `proxy-b` for these? there's nothing inherently invalid about them (even though the tests inject errors for them)
// IpProtectionCoreshould this be above IsIpProtectionEnabled, all the overrides for this grouped together, and should SetProxyList be in a separate grouping since it's not part of the interface?
// Since we fall back to direct, after trying the proxy chain, we expect aoptional: it might be helpful to verify that the proxy delegate did attempt to proxy the request, possibly by checking that the NetworkService.IpProtection.ProxyResolution histogram had an emission to the kAttemptProxy bucket. The check below does give us that already, but if this test does start failing in the future, looking at whether kAttemptProxy was emitted will let us know whether the mock proxy delegate might be the issue or the logic added in this CL
}optional: we could get the proxy_resolution_service from context_ and use it to ensure that the IPP proxy chain has been marked as bad, since that seems to be the purpose of this. For example: https://source.chromium.org/chromium/chromium/src/+/main:services/network/network_context_unittest.cc;l=6368;drc=5b7a3b2777a0c5ec94d1635a1c7fdc13442a1e9d
optional: it might be worth looking for a kAttemptProxy here too
Related details
- Giovanni Ortuno Urquidi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Ortuno Urquidi (Gerrit)
Giovanni Ortuno Urquidi added 7 comments![]( "Open in Gerrit")
by Net.HttpJob.IpProtection.TotalTimeNotCached2Giovanni Ortuno Urquidinit:
```suggestion
by Net.HttpJob.IpProtection.TotalTimeNotCached3
```
Done
I think these will get converted to `std::string_view`s for use by `net::MockRead`, which I'm assuming will require the length to be computed at runtime. Would it be better for these to be `constexpr std::string_view` to begin with?
Done
net::ProxyServer::SCHEME_HTTPS, "proxy-a.invalid", 443);Should we just use `proxy-a` and `proxy-b` for these? there's nothing inherently invalid about them (even though the tests inject errors for them)
Done
should this be above IsIpProtectionEnabled, all the overrides for this grouped together, and should SetProxyList be in a separate grouping since it's not part of the interface?
Ah, I skimmed too quickly and didn't realize the methods above were overrides. Fixed.
// Since we fall back to direct, after trying the proxy chain, we expect aoptional: it might be helpful to verify that the proxy delegate did attempt to proxy the request, possibly by checking that the NetworkService.IpProtection.ProxyResolution histogram had an emission to the kAttemptProxy bucket. The check below does give us that already, but if this test does start failing in the future, looking at whether kAttemptProxy was emitted will let us know whether the mock proxy delegate might be the issue or the logic added in this CL
Done
optional: we could get the proxy_resolution_service from context_ and use it to ensure that the IPP proxy chain has been marked as bad, since that seems to be the purpose of this. For example: https://source.chromium.org/chromium/chromium/src/+/main:services/network/network_context_unittest.cc;l=6368;drc=5b7a3b2777a0c5ec94d1635a1c7fdc13442a1e9d
Done
optional: it might be worth looking for a kAttemptProxy here too
- Andrew Williams
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Ortuno Urquidi (Gerrit)
Giovanni Ortuno Urquidi added 1 comment![]( "Open in Gerrit")
- Andrew Williams
- David Schinazi
- Nidhi Jaju
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Ortuno Urquidi (Gerrit)
Giovanni Ortuno Urquidi added 1 comment![]( "Open in Gerrit")
actually dschinazi, mind taking a look a histograms.xml as well? Moving nidhijaju to cc.
Related details
- Andrew Williams
- David Schinazi
- Nidhi Jaju
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Schinazi (Gerrit)
David Schinazi voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Schinazi
| Code-Review | +1 |
1 comment
- Andrew Williams
- Giovanni Ortuno Urquidi
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Ortuno Urquidi (Gerrit)
Giovanni Ortuno Urquidi voted and added 1 comment![]( "Open in Gerrit")
Votes added by Giovanni Ortuno Urquidi
| Commit-Queue | +2 |
1 comment
Thanks!
Related details
- Andrew Williams
Code-Coverage
Code-Owners
Code-Review
| 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
IPP: Don't modify ProxyList for direct-only IPP proxy lists
Switches the logic in IpProtectionProxyDelegate to avoid modifying the
ProxyList if:
1. All IPP chains were deprioritized, or 2. IPPCore didn't return any
chains.
Also introduces IpProtectionUrlRequestHttpJobTest which supersede the
tests in net/url_request/url_request_http_job_unittest.cc which used a
TestProxyDelegate. The new tests use a real IpProtectionProxyDelegate
which should improve the test coverage.
OBSOLETE_HISTOGRAM[Net.HttpJob.IpProtection.Fallback.TotalTimeNotCached]=Superseded
by Net.HttpJob.IpProtection.Fallback.TotalTimeNotCached2
OBSOLETE_HISTOGRAM[Net.HttpJob.IpProtection.TotalTimeNotCached2]=Superseded
by Net.HttpJob.IpProtection.TotalTimeNotCached3
- M components/ip_protection/BUILD.gn
- M components/ip_protection/common/BUILD.gn
- M components/ip_protection/common/ip_protection_proxy_delegate.cc
- M components/ip_protection/common/ip_protection_proxy_delegate_unittest.cc
- A components/ip_protection/common/ip_protection_url_request_http_job_unittest.cc
- M net/url_request/url_request_http_job.cc
- M net/url_request/url_request_http_job_unittest.cc
- M tools/metrics/histograms/metadata/net/histograms.xml
Code-Review: +1 by David Schinazi, +1 by Andrew Williams
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |