Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2567093002:
[cronet] Add a Builder class for UrlRequestContextConfig (Closed)
|
Created:
4 years ago by lilyhoughton1 Modified:
3 years, 11 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[cronet] Add a Builder class for UrlRequestContextConfig
This adds a Builder class for UrlRequestContextConfig, so that the rather
large constructor can be avoided when many of the arguments should remain
as their defaults.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
Review-Url: https://codereview.chromium.org/2567093002
Cr-Commit-Position: refs/heads/master@{#442902}
Committed: https://chromium.googlesource.com/chromium/src/+/14e2a1f1d3e8789304849a70bf471ae136a08ffc
Patch Set 1 #
Total comments: 10
Patch Set 2 : per #4 #
Total comments: 6
Patch Set 3 : per #6 #
Total comments: 8
Patch Set 4 : per #9 (+ sync) #Patch Set 5 : try fixing build (per #19) #
Messages
Total messages: 26 (8 generated)
Description was changed from ========== [cronet] Add a Builder class for UrlRequestContextConfig This adds a Builder class for UrlRequestContextConfig, so that the rather large constructor can be avoided when many of the arguments should remain as their defaults. ========== to ========== [cronet] Add a Builder class for UrlRequestContextConfig This adds a Builder class for UrlRequestContextConfig, so that the rather large constructor can be avoided when many of the arguments should remain as their defaults. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
lilyhoughton@chromium.org changed reviewers: + lilyhoughton@chromium.org, mef@chromium.org
https://codereview.chromium.org/2567093002/diff/1/components/cronet/url_reque... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/2567093002/diff/1/components/cronet/url_reque... components/cronet/url_request_context_config.cc:400: std::unique_ptr<URLRequestContextConfig> config(new URLRequestContextConfig( nit: base::MakeUnique<T> is preferred: https://cs.chromium.org/chromium/src/base/memory/ptr_util.h?rcl=1482236610&l=55 https://codereview.chromium.org/2567093002/diff/1/components/cronet/url_reque... components/cronet/url_request_context_config.cc:405: data_reduction_secure_proxy_check_url, std::move(mock_cert_verifier), It seems that calling Build() more than once would produce valid equivalent configs, except for |mock_cert_verifier| which is going to be moved into the first one. I think this is ok, but we should document it. https://codereview.chromium.org/2567093002/diff/1/components/cronet/url_reque... File components/cronet/url_request_context_config.h (right): https://codereview.chromium.org/2567093002/diff/1/components/cronet/url_reque... components/cronet/url_request_context_config.h:179: // Intermediate state for URLRequestContextConfig Full sentence, better description to explain the usage? https://codereview.chromium.org/2567093002/diff/1/components/cronet/url_reque... components/cronet/url_request_context_config.h:184: std::unique_ptr<URLRequestContextConfig> Build(); Need comments for public methods. https://codereview.chromium.org/2567093002/diff/1/components/cronet/url_reque... components/cronet/url_request_context_config.h:228: // Data to populte CertVerifierCache. nit: populate (sp)
https://codereview.chromium.org/2567093002/diff/1/components/cronet/url_reque... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/2567093002/diff/1/components/cronet/url_reque... components/cronet/url_request_context_config.cc:400: std::unique_ptr<URLRequestContextConfig> config(new URLRequestContextConfig( On 2016/12/20 19:32:04, mef wrote: > nit: base::MakeUnique<T> is preferred: > https://cs.chromium.org/chromium/src/base/memory/ptr_util.h?rcl=1482236610&l=55 Done. https://codereview.chromium.org/2567093002/diff/1/components/cronet/url_reque... components/cronet/url_request_context_config.cc:405: data_reduction_secure_proxy_check_url, std::move(mock_cert_verifier), On 2016/12/20 19:32:04, mef wrote: > It seems that calling Build() more than once would produce valid equivalent > configs, except for |mock_cert_verifier| which is going to be moved into the > first one. > > I think this is ok, but we should document it. Done, documented in comment for |Build()| in .h https://codereview.chromium.org/2567093002/diff/1/components/cronet/url_reque... File components/cronet/url_request_context_config.h (right): https://codereview.chromium.org/2567093002/diff/1/components/cronet/url_reque... components/cronet/url_request_context_config.h:179: // Intermediate state for URLRequestContextConfig On 2016/12/20 19:32:04, mef wrote: > Full sentence, better description to explain the usage? Done. https://codereview.chromium.org/2567093002/diff/1/components/cronet/url_reque... components/cronet/url_request_context_config.h:184: std::unique_ptr<URLRequestContextConfig> Build(); On 2016/12/20 19:32:04, mef wrote: > Need comments for public methods. Done. https://codereview.chromium.org/2567093002/diff/1/components/cronet/url_reque... components/cronet/url_request_context_config.h:228: // Data to populte CertVerifierCache. On 2016/12/20 19:32:05, mef wrote: > nit: populate (sp) Done.
https://codereview.chromium.org/2567093002/diff/20001/components/cronet/url_r... File components/cronet/url_request_context_config.h (right): https://codereview.chromium.org/2567093002/diff/20001/components/cronet/url_r... components/cronet/url_request_context_config.h:180: // (mostly) sane defaults, nit: unexpected line break. https://codereview.chromium.org/2567093002/diff/20001/components/cronet/url_r... components/cronet/url_request_context_config.h:181: // then modify it with appropriate setters, and finalize with Build(). Um, there are no setters. https://codereview.chromium.org/2567093002/diff/20001/components/cronet/url_r... components/cronet/url_request_context_config.h:187: // as once nit: unexpected line break.
On 2016/12/22 20:29:19, mef wrote: > https://codereview.chromium.org/2567093002/diff/20001/components/cronet/url_r... > File components/cronet/url_request_context_config.h (right): > > https://codereview.chromium.org/2567093002/diff/20001/components/cronet/url_r... > components/cronet/url_request_context_config.h:180: // (mostly) sane defaults, > nit: unexpected line break. > > https://codereview.chromium.org/2567093002/diff/20001/components/cronet/url_r... > components/cronet/url_request_context_config.h:181: // then modify it with > appropriate setters, and finalize with Build(). > Um, there are no setters. > > https://codereview.chromium.org/2567093002/diff/20001/components/cronet/url_r... > components/cronet/url_request_context_config.h:187: // as once > nit: unexpected line break. ping
https://codereview.chromium.org/2567093002/diff/20001/components/cronet/url_r... File components/cronet/url_request_context_config.h (right): https://codereview.chromium.org/2567093002/diff/20001/components/cronet/url_r... components/cronet/url_request_context_config.h:180: // (mostly) sane defaults, On 2016/12/22 20:29:19, mef wrote: > nit: unexpected line break. Done. https://codereview.chromium.org/2567093002/diff/20001/components/cronet/url_r... components/cronet/url_request_context_config.h:181: // then modify it with appropriate setters, and finalize with Build(). On 2016/12/22 20:29:18, mef wrote: > Um, there are no setters. Done. Mis-commented https://codereview.chromium.org/2567093002/diff/20001/components/cronet/url_r... components/cronet/url_request_context_config.h:187: // as once On 2016/12/22 20:29:18, mef wrote: > nit: unexpected line break. Done.
Looks good, just a couple more nits. https://codereview.chromium.org/2567093002/diff/40001/components/cronet/ios/c... File components/cronet/ios/cronet_environment.mm (right): https://codereview.chromium.org/2567093002/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.mm:275: context_config_builder.enable_spdy = http2_enabled_; // Enable SPDY nit: Enable HTTP/2. (including period). https://codereview.chromium.org/2567093002/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.mm:277: cache_path.value(); // Storage path for http cache and cookie storage. I'd set http_cache = URLRequestContextConfig::DISK here explicitly. https://codereview.chromium.org/2567093002/diff/40001/components/cronet/url_r... File components/cronet/url_request_context_config.h (right): https://codereview.chromium.org/2567093002/diff/40001/components/cronet/url_r... components/cronet/url_request_context_config.h:200: URLRequestContextConfig::HttpCacheType http_cache = We should make this and other values match defaults used on Android, specifically have http_cache 'DISABLED': https://cs.chromium.org/chromium/src/components/cronet/android/java/src/org/c... enable_spdy (aka Http2) = true;
https://codereview.chromium.org/2567093002/diff/40001/components/cronet/ios/c... File components/cronet/ios/cronet_environment.mm (right): https://codereview.chromium.org/2567093002/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.mm:275: context_config_builder.enable_spdy = http2_enabled_; // Enable SPDY On 2017/01/10 17:36:18, mef wrote: > nit: Enable HTTP/2. (including period). Done. Should the member itself be changed as well? (Or perhaps an alias-member added?) https://codereview.chromium.org/2567093002/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.mm:277: cache_path.value(); // Storage path for http cache and cookie storage. On 2017/01/10 17:36:18, mef wrote: > I'd set http_cache = URLRequestContextConfig::DISK here explicitly. Done. https://codereview.chromium.org/2567093002/diff/40001/components/cronet/url_r... File components/cronet/url_request_context_config.h (right): https://codereview.chromium.org/2567093002/diff/40001/components/cronet/url_r... components/cronet/url_request_context_config.h:200: URLRequestContextConfig::HttpCacheType http_cache = On 2017/01/10 17:36:18, mef wrote: > We should make this and other values match defaults used on Android, > specifically have http_cache 'DISABLED': > https://cs.chromium.org/chromium/src/components/cronet/android/java/src/org/c... > > enable_spdy (aka Http2) = true; Done.
https://codereview.chromium.org/2567093002/diff/40001/components/cronet/ios/c... File components/cronet/ios/cronet_environment.mm (right): https://codereview.chromium.org/2567093002/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.mm:275: context_config_builder.enable_spdy = http2_enabled_; // Enable SPDY On 2017/01/10 17:36:18, mef wrote: > nit: Enable HTTP/2. (including period). Done. Should the member itself be changed as well? (Or perhaps an alias-member added?) https://codereview.chromium.org/2567093002/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.mm:277: cache_path.value(); // Storage path for http cache and cookie storage. On 2017/01/10 17:36:18, mef wrote: > I'd set http_cache = URLRequestContextConfig::DISK here explicitly. Done. https://codereview.chromium.org/2567093002/diff/40001/components/cronet/url_r... File components/cronet/url_request_context_config.h (right): https://codereview.chromium.org/2567093002/diff/40001/components/cronet/url_r... components/cronet/url_request_context_config.h:200: URLRequestContextConfig::HttpCacheType http_cache = On 2017/01/10 17:36:18, mef wrote: > We should make this and other values match defaults used on Android, > specifically have http_cache 'DISABLED': > https://cs.chromium.org/chromium/src/components/cronet/android/java/src/org/c... > > enable_spdy (aka Http2) = true; Done.
lgtm https://codereview.chromium.org/2567093002/diff/40001/components/cronet/ios/c... File components/cronet/ios/cronet_environment.mm (right): https://codereview.chromium.org/2567093002/diff/40001/components/cronet/ios/c... components/cronet/ios/cronet_environment.mm:275: context_config_builder.enable_spdy = http2_enabled_; // Enable SPDY On 2017/01/10 18:25:36, lilyhoughton wrote: > On 2017/01/10 17:36:18, mef wrote: > > nit: Enable HTTP/2. (including period). > > Done. Should the member itself be changed as well? (Or perhaps an alias-member > added?) If anything we should rename it as spdy is deprecated, but it is called spdy all over net stack, so we could leave it for now.
> If anything we should rename it as spdy is deprecated, but it is called spdy all > over net stack, so we could leave it for now. It may be a little overengineering, but would it be reasonable to make a reference to enable_spdy, something like > bool &enable_http2 = this->enable_spdy So that old code would remain unbroken, but newer code could use the clearer variable. But maybe this just makes things more confusing?
The CQ bit was checked by lilyhoughton@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/10 18:42:17, lilyhoughton wrote: > > If anything we should rename it as spdy is deprecated, but it is called spdy > all > > over net stack, so we could leave it for now. > > It may be a little overengineering, but would it be reasonable to make a > reference to enable_spdy, something like > > > bool &enable_http2 = this->enable_spdy > > So that old code would remain unbroken, but newer code could use the clearer > variable. But maybe this just makes things more confusing? Yeah, currently we prioritize clarity and simplicity over backwards compatibility. Especially considering that this is implementation class, so all callers are internal in chromium.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_cronet_tester on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
https://codereview.chromium.org/2567093002/diff/40001/components/cronet/url_r... File components/cronet/url_request_context_config.h (right): https://codereview.chromium.org/2567093002/diff/40001/components/cronet/url_r... components/cronet/url_request_context_config.h:225: std::unique_ptr<net::CertVerifier> mock_cert_verifier = nullptr; I think you may need to #include "net/cert/cert_verifier.h" for this one per https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...
The CQ bit was checked by lilyhoughton@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org Link to the patchset: https://codereview.chromium.org/2567093002/#ps80001 (title: "try fixing build (per #19)")
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": 80001, "attempt_start_ts": 1484144321637810,
"parent_rev": "7b9e6bc428d965901cec2999f5ed0933b81bdd29", "commit_rev":
"14e2a1f1d3e8789304849a70bf471ae136a08ffc"}
Message was sent while issue was closed.
Description was changed from ========== [cronet] Add a Builder class for UrlRequestContextConfig This adds a Builder class for UrlRequestContextConfig, so that the rather large constructor can be avoided when many of the arguments should remain as their defaults. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== [cronet] Add a Builder class for UrlRequestContextConfig This adds a Builder class for UrlRequestContextConfig, so that the rather large constructor can be avoided when many of the arguments should remain as their defaults. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2567093002 Cr-Commit-Position: refs/heads/master@{#442902} Committed: https://chromium.googlesource.com/chromium/src/+/14e2a1f1d3e8789304849a70bf47... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/14e2a1f1d3e8789304849a70bf47...
Message was sent while issue was closed.
On 2017/01/10 20:26:59, mef wrote: > https://codereview.chromium.org/2567093002/diff/40001/components/cronet/url_r... > File components/cronet/url_request_context_config.h (right): > > https://codereview.chromium.org/2567093002/diff/40001/components/cronet/url_r... > components/cronet/url_request_context_config.h:225: > std::unique_ptr<net::CertVerifier> mock_cert_verifier = nullptr; > I think you may need to #include "net/cert/cert_verifier.h" for this one per > https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron... Fixed, thanks! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
