Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2502683003:
[Cronet] change InitializeOnNetworkThread to use URLRequestContextBuilder (Closed)
|
Created:
4 years, 1 month ago by lilyhoughton1 Modified:
4 years ago CC:
chromium-reviews, cbentzel+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange InitializeOnNetworkThread to use URLRequestContextBuilder, instead of
building the URLRequestContext manually.
This change is entirely internal, so it does not yet actually take a config
object as an argument.
BUG=523858
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
Committed: https://crrev.com/4e23c36f914563404f3eb3657fe97888e2a8640c
Cr-Commit-Position: refs/heads/master@{#437601}
Patch Set 1 #
Total comments: 9
Patch Set 2 : address mmenke's comments #
Total comments: 30
Patch Set 3 : remove unused includes and methods #Patch Set 4 : replace indirectly used includes with actual dependencies #
Total comments: 4
Patch Set 5 : Sync #
Messages
Total messages: 31 (15 generated)
Description was changed from ========== change InitializeOnNetworkThread to use URLRequestContextBuilder this change is entirely internal, so it does not yet actually take a config object as an argument BUG=523858 ========== to ========== change InitializeOnNetworkThread to use URLRequestContextBuilder this change is entirely internal, so it does not yet actually take a config object as an argument BUG=523858 ==========
lilyhoughton@chromium.org changed reviewers: + mef@chromium.org, mmenke@chromium.org
lilyhoughton@chromium.org changed reviewers: + lilyhoughton@chromium.org
Initial comments, haven't dug in to carefully check for changed behavior. https://codereview.chromium.org/2502683003/diff/1/components/cronet/ios/crone... File components/cronet/ios/cronet_environment.cc (right): https://codereview.chromium.org/2502683003/diff/1/components/cronet/ios/crone... components/cronet/ios/cronet_environment.cc:283: cert_verifier_ = net::CertVerifier::CreateDefault(); Not needed: URLRequestContextConfig will do this by default. Maybe remove this line and rename cert_verifier_ to mock_cert_verifier_ (And rename the setter as well)? That's what URLRequestContextConfig calls it, and that's the only reason we need to keep code here to set it. https://codereview.chromium.org/2502683003/diff/1/components/cronet/ios/crone... components/cronet/ios/cronet_environment.cc:291: std::unique_ptr<URLRequestContextConfig> config(new URLRequestContextConfig( include components/cronet/url_request_context_config.h? https://codereview.chromium.org/2502683003/diff/1/components/cronet/ios/crone... components/cronet/ios/cronet_environment.cc:294: std::move(cert_verifier_), false, true, "")); Should have comments labeling all the inscrutable arguments here. i.e. true /* should_take_mondays_off */, "" /* answer_to_question_of_life_and_death */, "{}" /* etc */ https://codereview.chromium.org/2502683003/diff/1/components/cronet/ios/crone... components/cronet/ios/cronet_environment.cc:305: net::HostResolver::CreateDefaultResolver(nullptr))); nullptr -> net_log_.get() (That was a bug in the old code)
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2502683003/diff/1/components/cronet/ios/crone... File components/cronet/ios/cronet_environment.cc (right): https://codereview.chromium.org/2502683003/diff/1/components/cronet/ios/crone... components/cronet/ios/cronet_environment.cc:283: cert_verifier_ = net::CertVerifier::CreateDefault(); On 2016/11/16 16:41:20, mmenke wrote: > Not needed: URLRequestContextConfig will do this by default. Maybe remove this > line and rename cert_verifier_ to mock_cert_verifier_ (And rename the setter as > well)? That's what URLRequestContextConfig calls it, and that's the only reason > we need to keep code here to set it. Done. https://codereview.chromium.org/2502683003/diff/1/components/cronet/ios/crone... components/cronet/ios/cronet_environment.cc:291: std::unique_ptr<URLRequestContextConfig> config(new URLRequestContextConfig( On 2016/11/16 16:41:20, mmenke wrote: > include components/cronet/url_request_context_config.h? This was in cronet_environment.h. That file didn't use it, however, so I've moved it into this one. https://codereview.chromium.org/2502683003/diff/1/components/cronet/ios/crone... components/cronet/ios/cronet_environment.cc:294: std::move(cert_verifier_), false, true, "")); On 2016/11/16 16:41:20, mmenke wrote: > Should have comments labeling all the inscrutable arguments here. i.e. > > true /* should_take_mondays_off */, > "" /* answer_to_question_of_life_and_death */, > "{}" /* etc */ Done. https://codereview.chromium.org/2502683003/diff/1/components/cronet/ios/crone... components/cronet/ios/cronet_environment.cc:305: net::HostResolver::CreateDefaultResolver(nullptr))); On 2016/11/16 16:41:20, mmenke wrote: > nullptr -> net_log_.get() (That was a bug in the old code) Done. What's the bug / would it be nontrivial to add a test case for it?
https://codereview.chromium.org/2502683003/diff/1/components/cronet/ios/crone... File components/cronet/ios/cronet_environment.cc (right): https://codereview.chromium.org/2502683003/diff/1/components/cronet/ios/crone... components/cronet/ios/cronet_environment.cc:305: net::HostResolver::CreateDefaultResolver(nullptr))); On 2016/11/22 19:44:01, lilyhoughton wrote: > On 2016/11/16 16:41:20, mmenke wrote: > > nullptr -> net_log_.get() (That was a bug in the old code) > > Done. > > What's the bug / would it be nontrivial to add a test case for it? It means we wouldn't have been emitting DNS lookup events to the NetLog. I suspect it's not worth testing that all individual classes are hooked up to logging infrastructure, given that no one has noticed this in how many ever years this has been broken, and it doesn't result in user-visible failures.
https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... File components/cronet/ios/cronet_environment.cc (left): https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.cc:364: // TODO(ellyjones): support persisting ChannelID. Keep this TODO? https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... File components/cronet/ios/cronet_environment.cc (right): https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.cc:31: #include "net/base/net_errors.h" Not used? https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.cc:38: #include "net/cert/multi_log_ct_verifier.h" None of the 5 lines above this are needed. https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.cc:43: #include "net/http/http_response_headers.h" I don't think any of the three lines above are needed? https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.cc:55: #include "net/url_request/static_http_user_agent_settings.h" None of the 4 includes above this line are needed. https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.cc:262: network_io_thread_->task_runner(), nullptr); This is no longer needed. https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.cc:283: // cert_verifier_ = net::CertVerifier::CreateDefault(); Remove this. https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... File components/cronet/ios/cronet_environment.h (right): https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.h:19: #include "net/url_request/url_request.h" Not used https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.h:30: class HttpCache; Not used. https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.h:33: class ProxyConfigService; Not used https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.h:137: scoped_refptr<JsonPrefStore> net_pref_store_; Not used (And wasn't used before this CL, either) https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.h:139: std::unique_ptr<net::ProxyConfigService> proxy_config_service_; No longer used. https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.h:140: std::unique_ptr<net::HttpServerProperties> http_server_properties_; No longer used.
https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... File components/cronet/ios/cronet_environment.cc (left): https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.cc:364: // TODO(ellyjones): support persisting ChannelID. On 2016/11/23 18:41:33, mmenke wrote: > Keep this TODO? Where would the best place to put it be? https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... File components/cronet/ios/cronet_environment.cc (right): https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.cc:31: #include "net/base/net_errors.h" On 2016/11/23 18:41:33, mmenke wrote: > Not used? Done. https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.cc:38: #include "net/cert/multi_log_ct_verifier.h" On 2016/11/23 18:41:33, mmenke wrote: > None of the 5 lines above this are needed. Done. https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.cc:43: #include "net/http/http_response_headers.h" On 2016/11/23 18:41:32, mmenke wrote: > I don't think any of the three lines above are needed? Removing "net/http/http_cache.h" breaks. https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.cc:55: #include "net/url_request/static_http_user_agent_settings.h" On 2016/11/23 18:41:32, mmenke wrote: > None of the 4 includes above this line are needed. Can you clarify? I get compile errors if I try to remove either "net/url_request/static_http_user_agent_settings.h" or "net/socket/ssl_client_socket.h" https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.cc:262: network_io_thread_->task_runner(), nullptr); On 2016/11/23 18:41:33, mmenke wrote: > This is no longer needed. Done. https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.cc:283: // cert_verifier_ = net::CertVerifier::CreateDefault(); On 2016/11/23 18:41:32, mmenke wrote: > Remove this. Done. https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... File components/cronet/ios/cronet_environment.h (right): https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.h:19: #include "net/url_request/url_request.h" On 2016/11/23 18:41:33, mmenke wrote: > Not used Done. https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.h:30: class HttpCache; On 2016/11/23 18:41:33, mmenke wrote: > Not used. Done. https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.h:33: class ProxyConfigService; On 2016/11/23 18:41:33, mmenke wrote: > Not used Done. https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.h:137: scoped_refptr<JsonPrefStore> net_pref_store_; On 2016/11/23 18:41:33, mmenke wrote: > Not used (And wasn't used before this CL, either) Done. https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.h:139: std::unique_ptr<net::ProxyConfigService> proxy_config_service_; On 2016/11/23 18:41:33, mmenke wrote: > No longer used. Done. https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.h:140: std::unique_ptr<net::HttpServerProperties> http_server_properties_; On 2016/11/23 18:41:33, mmenke wrote: > No longer used. Done.
https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... File components/cronet/ios/cronet_environment.cc (right): https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.cc:43: #include "net/http/http_response_headers.h" On 2016/11/28 16:42:21, lilyhoughton wrote: > On 2016/11/23 18:41:32, mmenke wrote: > > I don't think any of the three lines above are needed? > > Removing "net/http/http_cache.h" breaks. Why? It doesn't look like this class is depending on it anywhere. If it's some sort of transitive dependency, we should include the file this one actually depends on instead. https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.cc:55: #include "net/url_request/static_http_user_agent_settings.h" On 2016/11/28 16:42:21, lilyhoughton wrote: > On 2016/11/23 18:41:32, mmenke wrote: > > None of the 4 includes above this line are needed. > > Can you clarify? I get compile errors if I try to remove either > "net/url_request/static_http_user_agent_settings.h" or > "net/socket/ssl_client_socket.h" Looks like this file still has a dependency on user_agent_settings, which I missed, not static_user_agent_settings. ssl_client_socket.h is 5 lines above the comment, not 4.
https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... File components/cronet/ios/cronet_environment.cc (right): https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.cc:43: #include "net/http/http_response_headers.h" On 2016/11/28 17:35:42, mmenke wrote: > On 2016/11/28 16:42:21, lilyhoughton wrote: > > On 2016/11/23 18:41:32, mmenke wrote: > > > I don't think any of the three lines above are needed? > > > > Removing "net/http/http_cache.h" breaks. > > Why? It doesn't look like this class is depending on it anywhere. If it's some > sort of transitive dependency, we should include the file this one actually > depends on instead. The depended class appears to be HttpTransactionFactory, so replacing with "net/http/http_transaction_factory.h" https://codereview.chromium.org/2502683003/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.cc:55: #include "net/url_request/static_http_user_agent_settings.h" On 2016/11/28 17:35:42, mmenke wrote: > On 2016/11/28 16:42:21, lilyhoughton wrote: > > On 2016/11/23 18:41:32, mmenke wrote: > > > None of the 4 includes above this line are needed. > > > > Can you clarify? I get compile errors if I try to remove either > > "net/url_request/static_http_user_agent_settings.h" or > > "net/socket/ssl_client_socket.h" > > Looks like this file still has a dependency on user_agent_settings, which I > missed, not static_user_agent_settings. > > ssl_client_socket.h is 5 lines above the comment, not 4. Done.
LGTM. Sorry, forgot about this one.
looks good, could you change the CL subject to include [Cronet] prefix, and make description a little more verbose, according to https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list... https://codereview.chromium.org/2502683003/diff/80001/components/cronet/ios/c... File components/cronet/ios/cronet_environment.cc (right): https://codereview.chromium.org/2502683003/diff/80001/components/cronet/ios/c... components/cronet/ios/cronet_environment.cc:283: cache_path.value(), // Storage path for http cache and cookie storage. nit: should these comments be aligned with others?
https://codereview.chromium.org/2502683003/diff/80001/components/cronet/ios/c... File components/cronet/ios/cronet_environment.cc (right): https://codereview.chromium.org/2502683003/diff/80001/components/cronet/ios/c... components/cronet/ios/cronet_environment.cc:277: "", // QUIC User Agent ID. After this lands we need to make it non-empty, see https://bugs.chromium.org/p/chromium/issues/detail?id=670686
Description was changed from ========== change InitializeOnNetworkThread to use URLRequestContextBuilder this change is entirely internal, so it does not yet actually take a config object as an argument BUG=523858 ========== to ========== Change InitializeOnNetworkThread to use URLRequestContextBuilder. This change is entirely internal, so it does not yet actually take a config object as an argument. BUG=523858 ==========
Description was changed from ========== Change InitializeOnNetworkThread to use URLRequestContextBuilder. This change is entirely internal, so it does not yet actually take a config object as an argument. BUG=523858 ========== to ========== Change InitializeOnNetworkThread to use URLRequestContextBuilder, instead of building the URLRequestContext manually. This change is entirely internal, so it does not yet actually take a config object as an argument. BUG=523858 ==========
Description was changed from ========== Change InitializeOnNetworkThread to use URLRequestContextBuilder, instead of building the URLRequestContext manually. This change is entirely internal, so it does not yet actually take a config object as an argument. BUG=523858 ========== to ========== Change InitializeOnNetworkThread to use URLRequestContextBuilder, instead of building the URLRequestContext manually. This change is entirely internal, so it does not yet actually take a config object as an argument. BUG=523858 ==========
Description was changed from ========== Change InitializeOnNetworkThread to use URLRequestContextBuilder, instead of building the URLRequestContext manually. This change is entirely internal, so it does not yet actually take a config object as an argument. BUG=523858 ========== to ========== Change InitializeOnNetworkThread to use URLRequestContextBuilder, instead of building the URLRequestContext manually. This change is entirely internal, so it does not yet actually take a config object as an argument. BUG=523858 ==========
https://codereview.chromium.org/2502683003/diff/80001/components/cronet/ios/c... File components/cronet/ios/cronet_environment.cc (right): https://codereview.chromium.org/2502683003/diff/80001/components/cronet/ios/c... components/cronet/ios/cronet_environment.cc:277: "", // QUIC User Agent ID. On 2016/12/02 15:26:42, mef wrote: > After this lands we need to make it non-empty, see > https://bugs.chromium.org/p/chromium/issues/detail?id=670686 Acknowledged. https://codereview.chromium.org/2502683003/diff/80001/components/cronet/ios/c... components/cronet/ios/cronet_environment.cc:283: cache_path.value(), // Storage path for http cache and cookie storage. On 2016/12/01 20:57:24, mef wrote: > nit: should these comments be aligned with others? This is the output of cl format. Realigning them makes the lines too long.
Description was changed from ========== Change InitializeOnNetworkThread to use URLRequestContextBuilder, instead of building the URLRequestContext manually. This change is entirely internal, so it does not yet actually take a config object as an argument. BUG=523858 ========== to ========== Change InitializeOnNetworkThread to use URLRequestContextBuilder, instead of building the URLRequestContext manually. This change is entirely internal, so it does not yet actually take a config object as an argument. BUG=523858 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
lgtm
The CQ bit was checked by lilyhoughton@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/2502683003/#ps100001 (title: "Sync")
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": 100001, "attempt_start_ts": 1481308312503420,
"parent_rev": "be9fc52273ede95e0348b136ba8cd565d2296d32", "commit_rev":
"1549b002f24d1dc69fef63b97bc62b7a3437fa80"}
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481308312503420,
"parent_rev": "eb3bbe0f8cd9e9c221ca4a094d391634677c8870", "commit_rev":
"31ed9e4f2dd8460389b9f1bb063e18d781073525"}
Message was sent while issue was closed.
Description was changed from ========== Change InitializeOnNetworkThread to use URLRequestContextBuilder, instead of building the URLRequestContext manually. This change is entirely internal, so it does not yet actually take a config object as an argument. BUG=523858 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== Change InitializeOnNetworkThread to use URLRequestContextBuilder, instead of building the URLRequestContext manually. This change is entirely internal, so it does not yet actually take a config object as an argument. BUG=523858 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2502683003 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Change InitializeOnNetworkThread to use URLRequestContextBuilder, instead of building the URLRequestContext manually. This change is entirely internal, so it does not yet actually take a config object as an argument. BUG=523858 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2502683003 ========== to ========== Change InitializeOnNetworkThread to use URLRequestContextBuilder, instead of building the URLRequestContext manually. This change is entirely internal, so it does not yet actually take a config object as an argument. BUG=523858 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Committed: https://crrev.com/4e23c36f914563404f3eb3657fe97888e2a8640c Cr-Commit-Position: refs/heads/master@{#437601} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/4e23c36f914563404f3eb3657fe97888e2a8640c Cr-Commit-Position: refs/heads/master@{#437601} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
