Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2760073002:
[cronet] Expose API to set experimental options (Closed)
|
Created:
3 years, 9 months ago by lilyhoughton1 Modified:
3 years, 8 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[cronet] Expose API to set experimental options
Add CPP function CronetEnvironment::set_experimental_options(std::string)
and Obj-C function [Cronet setExperimentalOption:(NSString*)].
BUG=702789
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
Review-Url: https://codereview.chromium.org/2760073002
Cr-Original-Commit-Position: refs/heads/master@{#461511}
Committed: https://chromium.googlesource.com/chromium/src/+/afb25842158a4bad4c7e61dfc0b1c48223f9d351
Review-Url: https://codereview.chromium.org/2760073002
Cr-Commit-Position: refs/heads/master@{#467399}
Committed: https://chromium.googlesource.com/chromium/src/+/3c82bec19b9876265cc17d515f8c49f638dbaf00
Patch Set 1 #
Total comments: 6
Patch Set 2 : some nits #Patch Set 3 : add sslkeylogfile test #
Total comments: 1
Patch Set 4 : fix bugs per xunjieli #
Total comments: 1
Patch Set 5 : clean up string formatting #Patch Set 6 : sync #
Messages
Total messages: 39 (20 generated)
Description was changed from ========== [cronet] Expose API to set experimental options Add CPP function CronetEnvironment::set_experimental_options(std::string) and Obj-C function [Cronet setExperimentalOption:(NSString*)]. BUG= ========== to ========== [cronet] Expose API to set experimental options Add CPP function CronetEnvironment::set_experimental_options(std::string) and Obj-C function [Cronet setExperimentalOption:(NSString*)]. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
Description was changed from ========== [cronet] Expose API to set experimental options Add CPP function CronetEnvironment::set_experimental_options(std::string) and Obj-C function [Cronet setExperimentalOption:(NSString*)]. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== [cronet] Expose API to set experimental options Add CPP function CronetEnvironment::set_experimental_options(std::string) and Obj-C function [Cronet setExperimentalOption:(NSString*)]. BUG=702789 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
lilyhoughton@chromium.org changed reviewers: + mef@chromium.org
https://codereview.chromium.org/2760073002/diff/1/components/cronet/ios/crone... File components/cronet/ios/cronet_environment.h (right): https://codereview.chromium.org/2760073002/diff/1/components/cronet/ios/crone... components/cronet/ios/cronet_environment.h:94: void set_experimental_options(std::string experimental_options) { const std::string& https://codereview.chromium.org/2760073002/diff/1/components/cronet/ios/crone... File components/cronet/ios/cronet_environment.mm (right): https://codereview.chromium.org/2760073002/diff/1/components/cronet/ios/crone... components/cronet/ios/cronet_environment.mm:224: experimental_options_("{}"), nit: I don't think this is necessary as it is covered on objC side and empty string appears to be a valid default. https://codereview.chromium.org/2760073002/diff/1/components/cronet/ios/crone... components/cronet/ios/cronet_environment.mm:297: context_config_builder.experimental_options = I think we also need to do something similar to CronetURLRequestContextAdapter::GetNetLogInfo() in order to be able to log those as part of net log : https://cs.chromium.org/chromium/src/components/cronet/android/cronet_url_req... Check with xunjieli@, It should be ok to do as followup CL depending on how big are required changes.
lilyhoughton@chromium.org changed reviewers: + lilyhoughton@chromium.org
https://codereview.chromium.org/2760073002/diff/1/components/cronet/ios/crone... File components/cronet/ios/cronet_environment.mm (right): https://codereview.chromium.org/2760073002/diff/1/components/cronet/ios/crone... components/cronet/ios/cronet_environment.mm:224: experimental_options_("{}"), On 2017/03/21 20:44:57, mef wrote: > nit: I don't think this is necessary as it is covered on objC side and empty > string appears to be a valid default. Done. Should I have Obj-C default be an empty string as well? How much should we be thinking about what the use of C++ API without Obj-C will look like/having sane defaults for that? https://codereview.chromium.org/2760073002/diff/1/components/cronet/ios/crone... components/cronet/ios/cronet_environment.mm:297: context_config_builder.experimental_options = On 2017/03/21 20:44:58, mef wrote: > I think we also need to do something similar to > CronetURLRequestContextAdapter::GetNetLogInfo() in order to be able to log those > as part of net log : > https://cs.chromium.org/chromium/src/components/cronet/android/cronet_url_req... > > Check with xunjieli@, It should be ok to do as followup CL depending on how big > are required changes. Acknowledged.
We should add a test, for example something similar to testSetSSLKeyLogFile: https://cs.chromium.org/chromium/src/components/cronet/android/test/javatests... FWIW experimental options seem to include SslKeyLogFile and HostResolverRules, amd it would be cool if we can replace them in iOS API. https://codereview.chromium.org/2760073002/diff/1/components/cronet/ios/crone... File components/cronet/ios/cronet_environment.mm (right): https://codereview.chromium.org/2760073002/diff/1/components/cronet/ios/crone... components/cronet/ios/cronet_environment.mm:224: experimental_options_("{}"), On 2017/03/22 13:40:55, lilyhoughton wrote: > On 2017/03/21 20:44:57, mef wrote: > > nit: I don't think this is necessary as it is covered on objC side and empty > > string appears to be a valid default. > > Done. Should I have Obj-C default be an empty string as well? I think so. It seems that on Android they default to empty string: https://cs.chromium.org/chromium/src/components/cronet/android/java/src/org/c... > > How much should we be thinking about what the use of C++ API without Obj-C will > look like/having sane defaults for that? That would be nice to reduce confusion.
On 2017/03/22 19:18:26, mef wrote: > We should add a test, for example something similar to testSetSSLKeyLogFile: > https://cs.chromium.org/chromium/src/components/cronet/android/test/javatests... Done > FWIW experimental options seem to include SslKeyLogFile and HostResolverRules, > amd it would be cool if we can replace them in iOS API. Don't want to remove these immediately, since the new change is still fairly preliminary and it seems reasonable to wait for more thorough testing (probably blocking on having a ShutDown() interface) before getting rid of these API calls.
lgtm
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...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491245299762890,
"parent_rev": "0be71969c80ff018be92afb6bba2076bf3c1f55f", "commit_rev":
"afb25842158a4bad4c7e61dfc0b1c48223f9d351"}
Message was sent while issue was closed.
Description was changed from ========== [cronet] Expose API to set experimental options Add CPP function CronetEnvironment::set_experimental_options(std::string) and Obj-C function [Cronet setExperimentalOption:(NSString*)]. BUG=702789 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== [cronet] Expose API to set experimental options Add CPP function CronetEnvironment::set_experimental_options(std::string) and Obj-C function [Cronet setExperimentalOption:(NSString*)]. BUG=702789 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2760073002 Cr-Commit-Position: refs/heads/master@{#461511} Committed: https://chromium.googlesource.com/chromium/src/+/afb25842158a4bad4c7e61dfc0b1... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/afb25842158a4bad4c7e61dfc0b1...
Message was sent while issue was closed.
xunjieli@chromium.org changed reviewers: + xunjieli@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2760073002/diff/40001/components/cronet/ios/t... File components/cronet/ios/test/cronet_http_test.mm (right): https://codereview.chromium.org/2760073002/diff/40001/components/cronet/ios/t... components/cronet/ios/test/cronet_http_test.mm:166: TEST(SslTest, CreateFile) { I think this needs to be a TEST_F(HttpTest, CreateFile) in order for the SetUp()/StartCronetIfNecessary() to be run. This test seems to be broken on the ios-simulator-cronet trybot. https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.mac%2...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2803443002/ by lilyhoughton@chromium.org. The reason for reverting is: seems to break ios-simulator trybot.
Message was sent while issue was closed.
Description was changed from ========== [cronet] Expose API to set experimental options Add CPP function CronetEnvironment::set_experimental_options(std::string) and Obj-C function [Cronet setExperimentalOption:(NSString*)]. BUG=702789 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2760073002 Cr-Commit-Position: refs/heads/master@{#461511} Committed: https://chromium.googlesource.com/chromium/src/+/afb25842158a4bad4c7e61dfc0b1... ========== to ========== [cronet] Expose API to set experimental options Add CPP function CronetEnvironment::set_experimental_options(std::string) and Obj-C function [Cronet setExperimentalOption:(NSString*)]. BUG=702789 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2760073002 Cr-Commit-Position: refs/heads/master@{#461511} Committed: https://chromium.googlesource.com/chromium/src/+/afb25842158a4bad4c7e61dfc0b1... ==========
On 2017/04/04 16:36:39, xunjieli wrote: > https://codereview.chromium.org/2760073002/diff/40001/components/cronet/ios/t... > File components/cronet/ios/test/cronet_http_test.mm (right): > > https://codereview.chromium.org/2760073002/diff/40001/components/cronet/ios/t... > components/cronet/ios/test/cronet_http_test.mm:166: TEST(SslTest, CreateFile) { > I think this needs to be a TEST_F(HttpTest, CreateFile) in order for the > SetUp()/StartCronetIfNecessary() to be run. > > This test seems to be broken on the ios-simulator-cronet trybot. > > https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.mac%2... I think I fixed this issue; PTAL
xunjieli@chromium.org changed reviewers: - xunjieli@chromium.org
https://codereview.chromium.org/2760073002/diff/60001/components/cronet/ios/t... File components/cronet/ios/test/start_cronet.mm (right): https://codereview.chromium.org/2760073002/diff/60001/components/cronet/ios/t... components/cronet/ios/test/start_cronet.mm:27: stringWithFormat:@"%@%@%@", @"{\"ssl_key_log_file\":\"", Can this be simplified to have first and third strings (constants) as part of format?
On 2017/04/06 18:58:27, mef wrote: > https://codereview.chromium.org/2760073002/diff/60001/components/cronet/ios/t... > File components/cronet/ios/test/start_cronet.mm (right): > > https://codereview.chromium.org/2760073002/diff/60001/components/cronet/ios/t... > components/cronet/ios/test/start_cronet.mm:27: stringWithFormat:@"%@%@%@", > @"{\"ssl_key_log_file\":\"", > Can this be simplified to have first and third strings (constants) as part of > format? It can indeed
lgtm
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) 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...)
On 2017/04/07 14:07:28, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > ios-device on master.tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) > ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) > 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...) Lily, could you resync this and try it again?
The CQ bit was checked by lilyhoughton@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 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/2760073002/#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": 1493231531885620,
"parent_rev": "308b44e5bfb17c42ca3ea5b4fa5ed66c2698107b", "commit_rev":
"3c82bec19b9876265cc17d515f8c49f638dbaf00"}
Message was sent while issue was closed.
Description was changed from ========== [cronet] Expose API to set experimental options Add CPP function CronetEnvironment::set_experimental_options(std::string) and Obj-C function [Cronet setExperimentalOption:(NSString*)]. BUG=702789 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2760073002 Cr-Commit-Position: refs/heads/master@{#461511} Committed: https://chromium.googlesource.com/chromium/src/+/afb25842158a4bad4c7e61dfc0b1... ========== to ========== [cronet] Expose API to set experimental options Add CPP function CronetEnvironment::set_experimental_options(std::string) and Obj-C function [Cronet setExperimentalOption:(NSString*)]. BUG=702789 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2760073002 Cr-Original-Commit-Position: refs/heads/master@{#461511} Committed: https://chromium.googlesource.com/chromium/src/+/afb25842158a4bad4c7e61dfc0b1... Review-Url: https://codereview.chromium.org/2760073002 Cr-Commit-Position: refs/heads/master@{#467399} Committed: https://chromium.googlesource.com/chromium/src/+/3c82bec19b9876265cc17d515f8c... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/3c82bec19b9876265cc17d515f8c... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
