[SG Android] Add Enterprise Proxy Service utilities [chromium/src : main]
Zonghan Xu (Gerrit)
Zonghan Xu voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Sebastien Lalancette
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sebastien Lalancette (Gerrit)
Sebastien Lalancette added 7 comments![]( "Open in Gerrit")
std::vector<std::string> subnets;Are there no better types in `net` for this? Ideally, `ProvisioningDomainProxyConfig` should hold processed data rather than raw data as much as possible.
std::string identity;
std::string protocol;
std::string proxy;Can we add comments to better define what each of these are? Kind of odd to see a `proxy` field in a `Proxy` struct.
// ID of a proxy, this should be unique within a
// `ProvisioningDomainProxyConfig`.Why not enforce this via a map?
struct ChromeConfig {Could we get rid of `ChromeConfig` and just put the values as optional directly in the `Proxy` struct?
std::vector<ProxyExtraHeader> extra_headers;Wouldn't a map be better? It would also guarantee uniqueness of keys.
ProxyAuthConfig auth_config;```suggestion
std::optional<ProxyAuthConfig> auth_config;
```
kNone,Set first values to = 0 like in:
https://google.github.io/styleguide/cppguide.html#Enumerator_Names
In other enums too.
Related details
- Zonghan Xu
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. |
Zonghan Xu (Gerrit)
Zonghan Xu added 7 comments![]( "Open in Gerrit")
std::vector<std::string> subnets;Are there no better types in `net` for this? Ideally, `ProvisioningDomainProxyConfig` should hold processed data rather than raw data as much as possible.
We are converting these to `net::ProxyHostMatchingRules` later and sending them to the network stack, and `net::ProxyHostMatchingRules` takes string values.
However, I made this conversion just happen early here and we can directly use `net::ProxyHostMatchingRules`
PTAL
std::string identity;
std::string protocol;
std::string proxy;Can we add comments to better define what each of these are? Kind of odd to see a `proxy` field in a `Proxy` struct.
Done, on top of the comments, since we switched to using a map (where `identity` is the key), I renamed this struct from plain `Proxy` to `ProxyEndpoint` (since it's now a bunch of identification and auth/protocol information about proxies.
// ID of a proxy, this should be unique within a
// `ProvisioningDomainProxyConfig`.Why not enforce this via a map?
Done
Could we get rid of `ChromeConfig` and just put the values as optional directly in the `Proxy` struct?
Done
Wouldn't a map be better? It would also guarantee uniqueness of keys.
Done
```suggestion
std::optional<ProxyAuthConfig> auth_config;
```
Done
Set first values to = 0 like in:
https://google.github.io/styleguide/cppguide.html#Enumerator_NamesIn other enums too.
- Sebastien Lalancette
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. |
Zonghan Xu (Gerrit)
Zonghan Xu voted Auto-Submit+1![]( "Open in Gerrit")
| Auto-Submit | +1 |
Related details
- Sebastien Lalancette
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. |
Sebastien Lalancette (Gerrit)
Sebastien Lalancette added 6 comments![]( "Open in Gerrit")
// The protocol scheme of the proxy server (e.g., "https").
std::string protocol;
// The URI of the proxy server (e.g., "proxy.example.com:443").
std::string proxy_uri;Do we need to keep them separate? Why not combine into a GURL?
std::vector<ProxyExtraHeader> extra_headers;Why are we using ProxyExtraHeader here, but ExtraHeader (which wraps ProxyExtraHeader) in the ProxyConfig? Aren't both supposed to be the same?
// A list of proxy identifiers.Are they really identifiers, or is this supposed to be the ProxyList that a matching request should use? Like:
https://source.chromium.org/chromium/chromium/src/+/main:net/proxy_resolution/proxy_config.h;l=214;drc=59cc65f624db22d4fc4e61f646a0a721e9e78b26;bpv=0;bpt=1
std::vector<std::string> subnets;Zonghan XuAre there no better types in `net` for this? Ideally, `ProvisioningDomainProxyConfig` should hold processed data rather than raw data as much as possible.
We are converting these to `net::ProxyHostMatchingRules` later and sending them to the network stack, and `net::ProxyHostMatchingRules` takes string values.
However, I made this conversion just happen early here and we can directly use `net::ProxyHostMatchingRules`
PTAL
Works with me.
const char kValidPvdJsonResponse[] = R"({Can you cover additional test cases to capture what we expect our parsing to do?
e.g. all possible protocols, invalid/unsupported protocol, no auth config, no extra headers, etc.
"proxy": "https://proxy1.example.com",For "https-connect" protocol, I don't believe the scheme is part of this field, but the port is.
```suggestion
"proxy": "proxy1.example.com:443",
```
That is defined in Table 2 of section 3.1.
https://datatracker.ietf.org/doc/draft-ietf-intarea-proxy-config
Related details
- Zonghan Xu
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. |