Add proxy delegation and retry info support to Win Proxy Service [chromium/src : main]
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi added 6 comments![]( "Open in Gerrit")
const ProxyInfo& proxy_info) {nit: Add `DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);`
if (proxy_delegate_) {
proxy_delegate_->OnSuccessfulRequestAfterFailures(new_retry_info);
}
for (const auto& iter : new_retry_info) {
auto existing = proxy_retry_info_.find(iter.first);
if (existing == proxy_retry_info_.end()) {
proxy_retry_info_[iter.first] = iter.second;
if (proxy_delegate_) {
const ProxyChain& bad_proxy = iter.first;
DCHECK(!bad_proxy.is_direct());
const ProxyRetryInfo& proxy_retry_info = iter.second;
proxy_delegate_->OnFallback(bad_proxy, proxy_retry_info.net_error);
}
} else if (existing->second.bad_until < iter.second.bad_until) {
existing->second.bad_until = iter.second.bad_until;
}
}These look a dup of ConfiguredProxyResolutionService::ReportSuccess(). I'd prefer to avoid duplication, but not sure how to avoid that.
One option is to factor out common parts to ProxyResolutionService and make it non-pure virtual. Another option might be adding ProxyResolutionServiceBase class that has the common parts.
Could you add a TODO to avoid duplication for now?
proxy_delegate_->OnResolveProxy(url, network_anonymization_key, method,Can we have a test where a delegate modifies `result`?
class MockProxyDelegateForTesting : public ProxyDelegate {nit: `FakeProxyDelegate` might be a better name.
https://testing.googleblog.com/2013/06/testing-on-toilet-fake-your-way-to.html
int on_resolve_proxy_call_count_ = 0;nit: size_t for counters, ditto below.
<owner>ri...@chromium.org</owner>ricea@: Are you fine with this?
Related details
- Adam Rice
- Mayur Patil
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mayur Patil (Gerrit)
Mayur Patil added 5 comments![]( "Open in Gerrit")
const ProxyInfo& proxy_info) {Mayur Patilnit: Add `DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);`
Done
if (proxy_delegate_) {
proxy_delegate_->OnSuccessfulRequestAfterFailures(new_retry_info);
}
for (const auto& iter : new_retry_info) {
auto existing = proxy_retry_info_.find(iter.first);
if (existing == proxy_retry_info_.end()) {
proxy_retry_info_[iter.first] = iter.second;
if (proxy_delegate_) {
const ProxyChain& bad_proxy = iter.first;
DCHECK(!bad_proxy.is_direct());
const ProxyRetryInfo& proxy_retry_info = iter.second;
proxy_delegate_->OnFallback(bad_proxy, proxy_retry_info.net_error);
}
} else if (existing->second.bad_until < iter.second.bad_until) {
existing->second.bad_until = iter.second.bad_until;
}
}These look a dup of ConfiguredProxyResolutionService::ReportSuccess(). I'd prefer to avoid duplication, but not sure how to avoid that.
One option is to factor out common parts to ProxyResolutionService and make it non-pure virtual. Another option might be adding ProxyResolutionServiceBase class that has the common parts.
Could you add a TODO to avoid duplication for now?
I’ve moved the common code into a static method for better reusability and clarity. I’d appreciate your thoughts on this approach.
Previously, ProxyResolutionService was designed as an interface, but I’ve now added a static method to it. While this change simplifies usage in some cases, I understand there are alternative options—such as moving the logic to an existing utility class or creating a new one.
I’d love to hear your perspective on this and whether you think a different structure might be more appropriate.
Thanks!
proxy_delegate_->OnResolveProxy(url, network_anonymization_key, method,Can we have a test where a delegate modifies `result`?
Done
class MockProxyDelegateForTesting : public ProxyDelegate {nit: `FakeProxyDelegate` might be a better name.
https://testing.googleblog.com/2013/06/testing-on-toilet-fake-your-way-to.html
Done
nit: size_t for counters, ditto below.
- Adam Rice
- Kenichi Ishibashi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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
if (proxy_delegate_) {
proxy_delegate_->OnSuccessfulRequestAfterFailures(new_retry_info);
}
for (const auto& iter : new_retry_info) {
auto existing = proxy_retry_info_.find(iter.first);
if (existing == proxy_retry_info_.end()) {
proxy_retry_info_[iter.first] = iter.second;
if (proxy_delegate_) {
const ProxyChain& bad_proxy = iter.first;
DCHECK(!bad_proxy.is_direct());
const ProxyRetryInfo& proxy_retry_info = iter.second;
proxy_delegate_->OnFallback(bad_proxy, proxy_retry_info.net_error);
}
} else if (existing->second.bad_until < iter.second.bad_until) {
existing->second.bad_until = iter.second.bad_until;
}
}Mayur PatilThese look a dup of ConfiguredProxyResolutionService::ReportSuccess(). I'd prefer to avoid duplication, but not sure how to avoid that.
One option is to factor out common parts to ProxyResolutionService and make it non-pure virtual. Another option might be adding ProxyResolutionServiceBase class that has the common parts.
Could you add a TODO to avoid duplication for now?
I’ve moved the common code into a static method for better reusability and clarity. I’d appreciate your thoughts on this approach.
Previously, ProxyResolutionService was designed as an interface, but I’ve now added a static method to it. While this change simplifies usage in some cases, I understand there are alternative options—such as moving the logic to an existing utility class or creating a new one.
I’d love to hear your perspective on this and whether you think a different structure might be more appropriate.
Thanks!
I suspect we may want to share more code and adding static methods every time might be a bit of work, but the current approach seems reasonable to me. Thank you for reducing duplication.
Related details
- Adam Rice
- Mayur Patil
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi added 1 comment![]( "Open in Gerrit")
- Adam Rice
- Hayato Ito
- Mayur Patil
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mayur Patil (Gerrit)
Mayur Patil added 1 comment![]( "Open in Gerrit")
<owner>ri...@chromium.org</owner>ricea@: Are you fine with this?
- Adam Rice
- Hayato Ito
- Kenichi Ishibashi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hayato Ito (Gerrit)
Hayato Ito voted and added 3 comments![]( "Open in Gerrit")
Votes added by Hayato Ito
| Code-Review | +1 |
3 comments
LGTM % nits.
Thank you for working on this. It addresses several TODOs. Nice work!
raw_ptr<ProxyDelegate> proxy_delegate_ = nullptr;Nit: For consistency within this header, could you please add a comment explaining what this is used for?
TestProxyDelegate proxy_delegate;nit; Could you please clarify why `TestProxyDelegate` is set up in the `ReportSuccessWithRetryInfo` test? It seems the only difference between this test and `ReportSuccessWithoutDelegate` is the presence of `TestProxyDelegate`. I'm guessing this is to ensure both major code paths are tested, but I would appreciate it if you could add a brief comment to clarify the purpose of these tests. Thanks!
Related details
- Adam Rice
- Mayur Patil
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mayur Patil (Gerrit)
Mayur Patil added 2 comments![]( "Open in Gerrit")
Nit: For consistency within this header, could you please add a comment explaining what this is used for?
done
nit; Could you please clarify why `TestProxyDelegate` is set up in the `ReportSuccessWithRetryInfo` test? It seems the only difference between this test and `ReportSuccessWithoutDelegate` is the presence of `TestProxyDelegate`. I'm guessing this is to ensure both major code paths are tested, but I would appreciate it if you could add a brief comment to clarify the purpose of these tests. Thanks!
The ReportSuccessWithRetryInfo test was implemented to validate basic proxy retry functionality.
Additionally, the method previously named ReportSuccessWithoutDelegate (now renamed for clarity) was introduced to cover a specific condition in the ProcessProxyRetryInfo method. This condition involves iterating over the retry map while intentionally bypassing the proxy_delegate code section.
Related details
- Adam Rice
- Hayato Ito
- Kenichi Ishibashi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |