Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2899313006:
Plumb NQP to context and to http_proxy_client_socket_pool (Closed)
DescriptionPlumb NQP to context and to http_proxy_client_socket_pool
Also, refactor http_proxy_client_socket_pool.cc so that proxy
connection timeout is determined by a separate method.
The next CL would use Network Quality Provider (NQP) to determine the
proxy connection timeout in http_proxy_client_socket_pool.
BUG=704339
Review-Url: https://codereview.chromium.org/2899313006
Cr-Commit-Position: refs/heads/master@{#478159}
Committed: https://chromium.googlesource.com/chromium/src/+/16196a1e96e34678cbccaf3d0c0bea0dd2b4fb42
Patch Set 1 : ps #
Total comments: 8
Patch Set 2 : Rebased #
Total comments: 17
Patch Set 3 : mmenke comments #
Total comments: 3
Patch Set 4 : mmenke comments #Patch Set 5 : Use the macro for silencing unused variable warning #Patch Set 6 : fix compile error #Messages
Total messages: 83 (70 generated)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Description was changed from ========== Expose NQE to params BUG= ========== to ========== Expose NQE to params BUG=704339 ==========
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Expose NQE to params BUG=704339 ========== to ========== Expose NQE to params and to http_proxy_client_socket_pool Also, refactor http_proxy_client_socket_pool.cc so that proxy connection timeout is determined by a separate method. The next CL would use network quality estimator to determine the proxy connection timeout. BUG=704339 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #2 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Expose NQE to params and to http_proxy_client_socket_pool Also, refactor http_proxy_client_socket_pool.cc so that proxy connection timeout is determined by a separate method. The next CL would use network quality estimator to determine the proxy connection timeout. BUG=704339 ========== to ========== Expose NQE to context and to http_proxy_client_socket_pool Also, refactor http_proxy_client_socket_pool.cc so that proxy connection timeout is determined by a separate method. The next CL would use network quality estimator to determine the proxy connection timeout. BUG=704339 ==========
Description was changed from ========== Expose NQE to context and to http_proxy_client_socket_pool Also, refactor http_proxy_client_socket_pool.cc so that proxy connection timeout is determined by a separate method. The next CL would use network quality estimator to determine the proxy connection timeout. BUG=704339 ========== to ========== Plumb NQE to context and to http_proxy_client_socket_pool Also, refactor http_proxy_client_socket_pool.cc so that proxy connection timeout is determined by a separate method. The next CL would use network quality estimator to determine the proxy connection timeout. BUG=704339 ==========
Description was changed from ========== Plumb NQE to context and to http_proxy_client_socket_pool Also, refactor http_proxy_client_socket_pool.cc so that proxy connection timeout is determined by a separate method. The next CL would use network quality estimator to determine the proxy connection timeout. BUG=704339 ========== to ========== Plumb NQE to context and to http_proxy_client_socket_pool Also, refactor http_proxy_client_socket_pool.cc so that proxy connection timeout is determined by a separate method. The next CL would use network quality estimator to determine the proxy connection timeout in http_proxy_client_socket_pool. BUG=704339 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:60001) has been deleted
Description was changed from ========== Plumb NQE to context and to http_proxy_client_socket_pool Also, refactor http_proxy_client_socket_pool.cc so that proxy connection timeout is determined by a separate method. The next CL would use network quality estimator to determine the proxy connection timeout in http_proxy_client_socket_pool. BUG=704339 ========== to ========== Plumb NQE to context and to http_proxy_client_socket_pool Also, refactor http_proxy_client_socket_pool.cc so that proxy connection timeout is determined by a separate method. The next CL would use network quality estimator to determine the proxy connection timeout in http_proxy_client_socket_pool. This CL also enables NQE/SocketPerformanceWatcher for off the record profiles. BUG=704339 ==========
tbansal@chromium.org changed reviewers: + mmenke@chromium.org
mmenke: ptal. Thanks.
Quick cursory comments. I'll plan to take a closer look tomorrow. https://codereview.chromium.org/2899313006/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/2899313006/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.cc:1318: ->network_quality_estimator->GetSocketPerformanceWatcherFactory(); Is hooking this up in incognito mode a critical part of this CL? Whether it is or not, mind landing this change separately, and making sure you have a signoff from the privacy team? I don't see an issue, but I'll defer to them. https://codereview.chromium.org/2899313006/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.cc:1320: io_thread->globals()->network_quality_estimator.get(); Not needed, since it's plugged into SetHttpNetworkSessionComponents, a couple lines up. https://codereview.chromium.org/2899313006/diff/80001/ios/chrome/browser/brow... File ios/chrome/browser/browser_state/chrome_browser_state_io_data.cc (right): https://codereview.chromium.org/2899313006/diff/80001/ios/chrome/browser/brow... ios/chrome/browser/browser_state/chrome_browser_state_io_data.cc:495: io_thread->globals()->network_quality_estimator.get(); Again, SetHttpNetworkSessionComponents should do this. https://codereview.chromium.org/2899313006/diff/80001/net/http/http_network_s... File net/http/http_network_session.h (right): https://codereview.chromium.org/2899313006/diff/80001/net/http/http_network_s... net/http/http_network_session.h:192: NetworkQualityEstimator* network_quality_estimator; Having the bottom layer of the network stack depend on an API that depends on the top of the network stack (URLRequest) seems like bad API layering to me. I'm open to other way to avoid this, but one option is to make NQE a virtual interface, with only observer / get-network-info-type methods, and then make NQEImpl (Which could completely reasonably be part of URLRequestContext) depend on URLRequests. Another option is to make a separate class that NQE either subclasses or that depends on the NQE, that provides the socket pools with the API they need.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:120001) has been deleted
mmenke: ptal at *. I have rebased to the other network quality provider interface CL. https://codereview.chromium.org/2899313006/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/2899313006/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.cc:1318: ->network_quality_estimator->GetSocketPerformanceWatcherFactory(); On 2017/06/05 20:04:25, mmenke wrote: > Is hooking this up in incognito mode a critical part of this CL? Whether it is > or not, mind landing this change separately, and making sure you have a signoff > from the privacy team? I don't see an issue, but I'll defer to them. Done. https://codereview.chromium.org/2899313006/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_io_data.cc:1320: io_thread->globals()->network_quality_estimator.get(); On 2017/06/05 20:04:25, mmenke wrote: > Not needed, since it's plugged into SetHttpNetworkSessionComponents, a couple > lines up. Done. https://codereview.chromium.org/2899313006/diff/80001/ios/chrome/browser/brow... File ios/chrome/browser/browser_state/chrome_browser_state_io_data.cc (right): https://codereview.chromium.org/2899313006/diff/80001/ios/chrome/browser/brow... ios/chrome/browser/browser_state/chrome_browser_state_io_data.cc:495: io_thread->globals()->network_quality_estimator.get(); On 2017/06/05 20:04:26, mmenke wrote: > Again, SetHttpNetworkSessionComponents should do this. Done. https://codereview.chromium.org/2899313006/diff/80001/net/http/http_network_s... File net/http/http_network_session.h (right): https://codereview.chromium.org/2899313006/diff/80001/net/http/http_network_s... net/http/http_network_session.h:192: NetworkQualityEstimator* network_quality_estimator; On 2017/06/05 20:04:26, mmenke wrote: > Having the bottom layer of the network stack depend on an API that depends on > the top of the network stack (URLRequest) seems like bad API layering to me. > > I'm open to other way to avoid this, but one option is to make NQE a virtual > interface, with only observer / get-network-info-type methods, and then make > NQEImpl (Which could completely reasonably be part of URLRequestContext) depend > on URLRequests. > > Another option is to make a separate class that NQE either subclasses or that > depends on the NQE, that provides the socket pools with the API they need. Done.
Description was changed from ========== Plumb NQE to context and to http_proxy_client_socket_pool Also, refactor http_proxy_client_socket_pool.cc so that proxy connection timeout is determined by a separate method. The next CL would use network quality estimator to determine the proxy connection timeout in http_proxy_client_socket_pool. This CL also enables NQE/SocketPerformanceWatcher for off the record profiles. BUG=704339 ========== to ========== Plumb NQP to context and to http_proxy_client_socket_pool Also, refactor http_proxy_client_socket_pool.cc so that proxy connection timeout is determined by a separate method. The next CL would use network quality provider to determine the proxy connection timeout in http_proxy_client_socket_pool. BUG=704339 ==========
Description was changed from ========== Plumb NQP to context and to http_proxy_client_socket_pool Also, refactor http_proxy_client_socket_pool.cc so that proxy connection timeout is determined by a separate method. The next CL would use network quality provider to determine the proxy connection timeout in http_proxy_client_socket_pool. BUG=704339 ========== to ========== Plumb NQP to context and to http_proxy_client_socket_pool Also, refactor http_proxy_client_socket_pool.cc so that proxy connection timeout is determined by a separate method. The next CL would use Network Quality Provider (NQP) to determine the proxy connection timeout in http_proxy_client_socket_pool. BUG=704339 ==========
https://codereview.chromium.org/2899313006/diff/100001/chrome/browser/profile... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/2899313006/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_io_data.cc:1315: DCHECK(io_thread->globals()->network_quality_estimator); Does this DCHECK get us anything? https://codereview.chromium.org/2899313006/diff/100001/ios/chrome/browser/bro... File ios/chrome/browser/browser_state/chrome_browser_state_io_data.cc (right): https://codereview.chromium.org/2899313006/diff/100001/ios/chrome/browser/bro... ios/chrome/browser/browser_state/chrome_browser_state_io_data.cc:490: DCHECK(io_thread->globals()->network_quality_estimator); Again, not sure we really get anything from this. https://codereview.chromium.org/2899313006/diff/100001/net/http/http_proxy_cl... File net/http/http_proxy_client_socket_pool.cc (right): https://codereview.chromium.org/2899313006/diff/100001/net/http/http_proxy_cl... net/http/http_proxy_client_socket_pool.cc:38: base::TimeDelta GetConnectionTimeout( Why do we have two methods? https://codereview.chromium.org/2899313006/diff/100001/net/http/http_proxy_cl... net/http/http_proxy_client_socket_pool.cc:42: // determine the connection timeout. Should this logic be in the socket pools in in net/nqe? https://codereview.chromium.org/2899313006/diff/100001/net/http/http_proxy_cl... net/http/http_proxy_client_socket_pool.cc:52: #endif #else seems a bit more natural here (And I don't trust our tools not to start complaining about statements unreachable on some platforms) https://codereview.chromium.org/2899313006/diff/100001/net/http/http_proxy_cl... File net/http/http_proxy_client_socket_pool_unittest.cc (right): https://codereview.chromium.org/2899313006/diff/100001/net/http/http_proxy_cl... net/http/http_proxy_client_socket_pool_unittest.cc:729: TEST_P(HttpProxyClientSocketPoolTest, ProxyPoolTimeout) { Hrm...Do we get anything from this? We admittedly don't do a great job of testing timeouts, just not sure this gets us anything, though we should certainly test both new methods once they do stuff. https://codereview.chromium.org/2899313006/diff/100001/net/http/http_proxy_cl... net/http/http_proxy_client_socket_pool_unittest.cc:729: TEST_P(HttpProxyClientSocketPoolTest, ProxyPoolTimeout) { Not sure this really gets us anything. We should certainly test both methods once they actually do stuff, just not sure this test currently is useful. https://codereview.chromium.org/2899313006/diff/100001/net/url_request/url_re... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/2899313006/diff/100001/net/url_request/url_re... net/url_request/url_request_context_builder.cc:423: if (socket_performance_watcher_factory_) { Should we instead get rid of socket_performance_watcher_factory_, and instead have: if (network_quality_estimator_) { network_session_context.socket_performance_watcher_factory = network_quality_estimator_->GetSocketPerformanceWatcherFactory(); } Could even move that logic into SetHttpNetworkSessionComponents
Patchset #3 (id:140001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Patchset #3 (id:160001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #3 (id:180001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mmenke: ptal. Thanks. https://codereview.chromium.org/2899313006/diff/100001/chrome/browser/profile... File chrome/browser/profiles/profile_io_data.cc (right): https://codereview.chromium.org/2899313006/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_io_data.cc:1315: DCHECK(io_thread->globals()->network_quality_estimator); On 2017/06/07 21:31:25, mmenke wrote: > Does this DCHECK get us anything? Done. https://codereview.chromium.org/2899313006/diff/100001/ios/chrome/browser/bro... File ios/chrome/browser/browser_state/chrome_browser_state_io_data.cc (right): https://codereview.chromium.org/2899313006/diff/100001/ios/chrome/browser/bro... ios/chrome/browser/browser_state/chrome_browser_state_io_data.cc:490: DCHECK(io_thread->globals()->network_quality_estimator); On 2017/06/07 21:31:25, mmenke wrote: > Again, not sure we really get anything from this. Done. https://codereview.chromium.org/2899313006/diff/100001/net/http/http_proxy_cl... File net/http/http_proxy_client_socket_pool.cc (right): https://codereview.chromium.org/2899313006/diff/100001/net/http/http_proxy_cl... net/http/http_proxy_client_socket_pool.cc:38: base::TimeDelta GetConnectionTimeout( On 2017/06/07 21:31:25, mmenke wrote: > Why do we have two methods? Done. https://codereview.chromium.org/2899313006/diff/100001/net/http/http_proxy_cl... net/http/http_proxy_client_socket_pool.cc:42: // determine the connection timeout. On 2017/06/07 21:31:26, mmenke wrote: > Should this logic be in the socket pools in in net/nqe? The timeout is now dynamic i.e., it can change with time during the coarse of a single Chrome session. The timeout would also depend on whether the connection is secure or not. Given these two constraints, it seems this is the best place. IIUC, if I put timeout in socket pool, then neither of these two constraints would be satisfied. https://codereview.chromium.org/2899313006/diff/100001/net/http/http_proxy_cl... net/http/http_proxy_client_socket_pool.cc:52: #endif On 2017/06/07 21:31:26, mmenke wrote: > #else seems a bit more natural here (And I don't trust our tools not to start > complaining about statements unreachable on some platforms) Modified this. Hopefully the new one is better. https://codereview.chromium.org/2899313006/diff/100001/net/http/http_proxy_cl... File net/http/http_proxy_client_socket_pool_unittest.cc (right): https://codereview.chromium.org/2899313006/diff/100001/net/http/http_proxy_cl... net/http/http_proxy_client_socket_pool_unittest.cc:729: TEST_P(HttpProxyClientSocketPoolTest, ProxyPoolTimeout) { On 2017/06/07 21:31:26, mmenke wrote: > Not sure this really gets us anything. We should certainly test both methods > once they actually do stuff, just not sure this test currently is useful. Well, admittedly the test is very basic, but it checks that the CL is not changing status quo. I was planning to expand the tests once the new methods do more meaningful stuff. I can remove this test until then if you feel like so. https://codereview.chromium.org/2899313006/diff/100001/net/http/http_proxy_cl... net/http/http_proxy_client_socket_pool_unittest.cc:729: TEST_P(HttpProxyClientSocketPoolTest, ProxyPoolTimeout) { On 2017/06/07 21:31:26, mmenke wrote: > Not sure this really gets us anything. We should certainly test both methods > once they actually do stuff, just not sure this test currently is useful. Acknowledged. https://codereview.chromium.org/2899313006/diff/100001/net/url_request/url_re... File net/url_request/url_request_context_builder.cc (right): https://codereview.chromium.org/2899313006/diff/100001/net/url_request/url_re... net/url_request/url_request_context_builder.cc:423: if (socket_performance_watcher_factory_) { On 2017/06/07 21:31:26, mmenke wrote: > Should we instead get rid of socket_performance_watcher_factory_, and instead > have: > > if (network_quality_estimator_) { > network_session_context.socket_performance_watcher_factory = > network_quality_estimator_->GetSocketPerformanceWatcherFactory(); > } > > Could even move that logic into SetHttpNetworkSessionComponents Doing in a separate CL.
LGTM https://codereview.chromium.org/2899313006/diff/100001/net/http/http_proxy_cl... File net/http/http_proxy_client_socket_pool_unittest.cc (right): https://codereview.chromium.org/2899313006/diff/100001/net/http/http_proxy_cl... net/http/http_proxy_client_socket_pool_unittest.cc:729: TEST_P(HttpProxyClientSocketPoolTest, ProxyPoolTimeout) { On 2017/06/08 05:01:50, tbansal1 wrote: > On 2017/06/07 21:31:26, mmenke wrote: > > Not sure this really gets us anything. We should certainly test both methods > > once they actually do stuff, just not sure this test currently is useful. > > Well, admittedly the test is very basic, but it checks that the CL is not > changing status quo. I was planning to expand the tests once the new methods do > more meaningful stuff. I can remove this test until then if you feel like so. Still skeptical, but OK. https://codereview.chromium.org/2899313006/diff/200001/net/http/http_proxy_cl... File net/http/http_proxy_client_socket_pool.cc (right): https://codereview.chromium.org/2899313006/diff/200001/net/http/http_proxy_cl... net/http/http_proxy_client_socket_pool.cc:201: // Silence unused variable warning. Could we just pass it in to GetDefaultProxyConnectionTimeout instead, and rename GetDefaultProxyConnectionTimeout to GetProxyConnectionTimeout? I don't think we still need a default/ (I mean, we still need behavior when we don't have enough data to estimate the network performance, but seems like that logic should just be in the same method as everything else)
https://codereview.chromium.org/2899313006/diff/200001/net/http/http_proxy_cl... File net/http/http_proxy_client_socket_pool.cc (right): https://codereview.chromium.org/2899313006/diff/200001/net/http/http_proxy_cl... net/http/http_proxy_client_socket_pool.cc:201: // Silence unused variable warning. On 2017/06/08 17:53:02, mmenke wrote: > Could we just pass it in to GetDefaultProxyConnectionTimeout instead, and rename > GetDefaultProxyConnectionTimeout to GetProxyConnectionTimeout? I don't think we > still need a default/ (I mean, we still need behavior when we don't have enough > data to estimate the network performance, but seems like that logic should just > be in the same method as everything else) Ok, then I am keeping everything here, and removing the method from the anonymous namespace. It does not seem useful to have 2 methods where the class method is simply calling the anonymous method.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:220001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Still LGTM, but there's a macro for the void trick. https://codereview.chromium.org/2899313006/diff/200001/net/http/http_proxy_cl... File net/http/http_proxy_client_socket_pool.cc (right): https://codereview.chromium.org/2899313006/diff/200001/net/http/http_proxy_cl... net/http/http_proxy_client_socket_pool.cc:201: // Silence unused variable warning. On 2017/06/08 19:59:14, tbansal1 wrote: > On 2017/06/08 17:53:02, mmenke wrote: > > Could we just pass it in to GetDefaultProxyConnectionTimeout instead, and > rename > > GetDefaultProxyConnectionTimeout to GetProxyConnectionTimeout? I don't think > we > > still need a default/ (I mean, we still need behavior when we don't have > enough > > data to estimate the network performance, but seems like that logic should > just > > be in the same method as everything else) > > Ok, then I am keeping everything here, and removing the method from the > anonymous namespace. It does not seem useful to have 2 methods where the class > method is simply calling the anonymous method. SGTM.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/08 20:02:10, mmenke wrote: > Still LGTM, but there's a macro for the void trick. Cool, now using the macro in PS#5.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/08 20:12:26, tbansal1 wrote: > On 2017/06/08 20:02:10, mmenke wrote: > > Still LGTM, but there's a macro for the void trick. > Cool, now using the macro in PS#5. Sorry, I thought I said what the macro was. Didn't mean to send you on a chase for something that was somewhere hidden in the chrome code base!
On 2017/06/08 20:16:30, mmenke wrote: > On 2017/06/08 20:12:26, tbansal1 wrote: > > On 2017/06/08 20:02:10, mmenke wrote: > > > Still LGTM, but there's a macro for the void trick. > > Cool, now using the macro in PS#5. > > Sorry, I thought I said what the macro was. Didn't mean to send you on a chase > for something that was somewhere hidden in the chrome code base! It was easy to find. I just did not know it existed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2899313006/#ps280001 (title: "fix compile error")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 280001, "attempt_start_ts": 1496972639323330,
"parent_rev": "dfc69988626f677be76ad509a0dbe47d8a8ecc54", "commit_rev":
"16196a1e96e34678cbccaf3d0c0bea0dd2b4fb42"}
Message was sent while issue was closed.
Description was changed from ========== Plumb NQP to context and to http_proxy_client_socket_pool Also, refactor http_proxy_client_socket_pool.cc so that proxy connection timeout is determined by a separate method. The next CL would use Network Quality Provider (NQP) to determine the proxy connection timeout in http_proxy_client_socket_pool. BUG=704339 ========== to ========== Plumb NQP to context and to http_proxy_client_socket_pool Also, refactor http_proxy_client_socket_pool.cc so that proxy connection timeout is determined by a separate method. The next CL would use Network Quality Provider (NQP) to determine the proxy connection timeout in http_proxy_client_socket_pool. BUG=704339 Review-Url: https://codereview.chromium.org/2899313006 Cr-Commit-Position: refs/heads/master@{#478159} Committed: https://chromium.googlesource.com/chromium/src/+/16196a1e96e34678cbccaf3d0c0b... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:280001) as https://chromium.googlesource.com/chromium/src/+/16196a1e96e34678cbccaf3d0c0b... |
