Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1414053008:
[Cronet] Replace setExperimentalQuicConnectionOptions with a more general API (Closed)
Description[Cronet] Replace setExperimentalQuicConnectionOptions with a more general API
This CL replaces CronetEngine.Builder#setExperimentalQuicConnectionOptions with
a more general API to set experimental options.
BUG=545118
Committed: https://crrev.com/61b1eaaab7d5f38c76a3fd41d23a737fe177d211
Cr-Commit-Position: refs/heads/master@{#360187}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address Misha's comments #
Total comments: 10
Patch Set 3 : Address Paul's comments #Patch Set 4 : Remove format from Javadoc #
Total comments: 18
Patch Set 5 : Address Paul's comments #
Total comments: 6
Patch Set 6 : Address Misha's comments #
Total comments: 2
Patch Set 7 : Address Misha's comment #
Total comments: 4
Patch Set 8 : Address Paul's comments #
Messages
Total messages: 25 (5 generated)
xunjieli@chromium.org changed reviewers: + mef@chromium.org, pauljensen@chromium.org
Paul & Misha, PTAL.
https://codereview.chromium.org/1414053008/diff/1/components/cronet/android/a... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1414053008/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:327: * ...}, ...} Maybe add an example of how existing quicConnectionOptions should convert into experimentalOptions? Do I understand correctly that it would be something like: "{\"QUIC\": {\"connection_options\": \"PACE,IW10\"}}"
Thanks for the review! PTAL https://codereview.chromium.org/1414053008/diff/1/components/cronet/android/a... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1414053008/diff/1/components/cronet/android/a... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:327: * ...}, ...} On 2015/10/30 17:15:41, mef wrote: > Maybe add an example of how existing quicConnectionOptions should convert into > experimentalOptions? > > Do I understand correctly that it would be something like: > > "{\"QUIC\": {\"connection_options\": \"PACE,IW10\"}}" Done. Yes, that is right.
I'm wondering if we want to go so far as to document the format the of the JSON. We may want to keep this pretty free-form. https://codereview.chromium.org/1414053008/diff/20001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1414053008/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:322: * experiment options as the value; the experiment options for an change semicolon to period? https://codereview.chromium.org/1414053008/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:323: * experiment is encoded as a JSON object with option name as the key is->are https://codereview.chromium.org/1414053008/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:323: * experiment is encoded as a JSON object with option name as the key remove "for an experiment" https://codereview.chromium.org/1414053008/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:330: * Here's an example to set QUIC connection options using this method: I don't think we want specific examples in our published API, as I imagine these options changing frequently. Could you just provide an example that perhaps sets {"experiment1": {"option1": "option_value1"} } https://codereview.chromium.org/1414053008/diff/20001/components/cronet/url_r... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/1414053008/diff/20001/components/cronet/url_r... components/cronet/url_request_context_config.cc:56: if (quic_args->GetString("connection_options", &quic_connection_options)) { if we're going to pull "QUIC" into a file-scoped constant, may as well pull "connection_options" out too.
Patchset #3 (id:40001) has been deleted
I think it is still nice to have a documentation of the string format. Maybe we can explicitly make a note to say that to experiment, talk to the Cronet team first? https://codereview.chromium.org/1414053008/diff/20001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1414053008/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:322: * experiment options as the value; the experiment options for an On 2015/10/30 18:05:23, pauljensen wrote: > change semicolon to period? Done. https://codereview.chromium.org/1414053008/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:323: * experiment is encoded as a JSON object with option name as the key On 2015/10/30 18:05:23, pauljensen wrote: > remove "for an experiment" Done. https://codereview.chromium.org/1414053008/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:323: * experiment is encoded as a JSON object with option name as the key On 2015/10/30 18:05:23, pauljensen wrote: > is->are Done. https://codereview.chromium.org/1414053008/diff/20001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:330: * Here's an example to set QUIC connection options using this method: On 2015/10/30 18:05:23, pauljensen wrote: > I don't think we want specific examples in our published API, as I imagine these > options changing frequently. Could you just provide an example that perhaps > sets {"experiment1": {"option1": "option_value1"} } Done. https://codereview.chromium.org/1414053008/diff/20001/components/cronet/url_r... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/1414053008/diff/20001/components/cronet/url_r... components/cronet/url_request_context_config.cc:56: if (quic_args->GetString("connection_options", &quic_connection_options)) { On 2015/10/30 18:05:23, pauljensen wrote: > if we're going to pull "QUIC" into a file-scoped constant, may as well pull > "connection_options" out too. Done.
On 2015/10/30 18:30:57, xunjieli wrote: > I think it is still nice to have a documentation of the string format. Maybe we > can explicitly make a note to say that to experiment, talk to the Cronet team > first? I'm not sure we want to invite people to try experimenting. I'm thinking this is more of a case where an embedder might want something and we recommend they try a particular experiment. I imagine Cronet experiments being like Chrome command line flags, where we don't want users getting accustomed to using them frequently/long-term. Experiments and command line flags should only be for prototyping and testing. I'm also not too keen to specify the format too exactly, I imagine various experiments having various different formats, for example "option_value1" could actually be a JSON map or some other JSON construct, rather than just a string.
On 2015/11/02 11:39:11, pauljensen wrote: > On 2015/10/30 18:30:57, xunjieli wrote: > > I think it is still nice to have a documentation of the string format. Maybe > we > > can explicitly make a note to say that to experiment, talk to the Cronet team > > first? > > I'm not sure we want to invite people to try experimenting. I'm thinking this > is more of a case where an embedder might want something and we recommend they > try a particular experiment. I imagine Cronet experiments being like Chrome > command line flags, where we don't want users getting accustomed to using them > frequently/long-term. Experiments and command line flags should only be for > prototyping and testing. > > I'm also not too keen to specify the format too exactly, I imagine various > experiments having various different formats, for example "option_value1" could > actually be a JSON map or some other JSON construct, rather than just a string. I see. If that's the case, I think we should probably not detail the format in the Javadoc. As you've said, we shouldn't make users accustomed to the use of the experimental options, or depend on the exact format of this API. I've adjusted the Javadoc. PTAL.
On 2015/11/02 16:18:23, xunjieli wrote: > On 2015/11/02 11:39:11, pauljensen wrote: > > On 2015/10/30 18:30:57, xunjieli wrote: > > > I think it is still nice to have a documentation of the string format. Maybe > > we > > > can explicitly make a note to say that to experiment, talk to the Cronet > team > > > first? > > > > I'm not sure we want to invite people to try experimenting. I'm thinking this > > is more of a case where an embedder might want something and we recommend they > > try a particular experiment. I imagine Cronet experiments being like Chrome > > command line flags, where we don't want users getting accustomed to using them > > frequently/long-term. Experiments and command line flags should only be for > > prototyping and testing. > > > > I'm also not too keen to specify the format too exactly, I imagine various > > experiments having various different formats, for example "option_value1" > could > > actually be a JSON map or some other JSON construct, rather than just a > string. > > I see. If that's the case, I think we should probably not detail the format in > the Javadoc. As you've said, we shouldn't make users accustomed to the use of > the experimental options, or depend on the exact format of this API. > I've adjusted the Javadoc. PTAL. I wonder whether we should somehow report experimental options that are used and/or parsing errors.
https://codereview.chromium.org/1414053008/diff/80001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1414053008/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:306: * @param experimentalOptions experimental options as a JSON formatted experimental options as a JSON formatted string->JSON formatted experimental options https://codereview.chromium.org/1414053008/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:309: public Builder setExperimentalOptions(String experimentalOptions) { nit: experimentalOptions->options https://codereview.chromium.org/1414053008/diff/80001/components/cronet/url_r... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/1414053008/diff/80001/components/cronet/url_r... components/cronet/url_request_context_config.cc:9: #include "base/json/json_reader.h" duplicate line https://codereview.chromium.org/1414053008/diff/80001/components/cronet/url_r... components/cronet/url_request_context_config.cc:14: #include "base/values.h" ditto https://codereview.chromium.org/1414053008/diff/80001/components/cronet/url_r... components/cronet/url_request_context_config.cc:54: const base::DictionaryValue* quic_args = nullptr; I don't think we need this initializer https://codereview.chromium.org/1414053008/diff/80001/components/cronet/url_r... components/cronet/url_request_context_config.cc:163: remove blank line to match style here? https://codereview.chromium.org/1414053008/diff/80001/components/cronet/url_r... File components/cronet/url_request_context_config.h (right): https://codereview.chromium.org/1414053008/diff/80001/components/cronet/url_r... components/cronet/url_request_context_config.h:78: remove blank line to match style here? https://codereview.chromium.org/1414053008/diff/80001/components/cronet/url_r... components/cronet/url_request_context_config.h:84: // as the value. An example: Can we remove the "The configuration options are..." sentence as I don't want to limit ourselves unnecessarily? https://codereview.chromium.org/1414053008/diff/80001/components/cronet/url_r... components/cronet/url_request_context_config.h:88: remove blank line to match style here?
Thanks! PTAL https://codereview.chromium.org/1414053008/diff/80001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1414053008/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:306: * @param experimentalOptions experimental options as a JSON formatted On 2015/11/04 15:03:26, pauljensen wrote: > experimental options as a JSON formatted string->JSON formatted experimental > options Done. https://codereview.chromium.org/1414053008/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:309: public Builder setExperimentalOptions(String experimentalOptions) { On 2015/11/04 15:03:26, pauljensen wrote: > nit: experimentalOptions->options Done. https://codereview.chromium.org/1414053008/diff/80001/components/cronet/url_r... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/1414053008/diff/80001/components/cronet/url_r... components/cronet/url_request_context_config.cc:9: #include "base/json/json_reader.h" On 2015/11/04 15:03:26, pauljensen wrote: > duplicate line Done. https://codereview.chromium.org/1414053008/diff/80001/components/cronet/url_r... components/cronet/url_request_context_config.cc:14: #include "base/values.h" On 2015/11/04 15:03:26, pauljensen wrote: > ditto Done. https://codereview.chromium.org/1414053008/diff/80001/components/cronet/url_r... components/cronet/url_request_context_config.cc:54: const base::DictionaryValue* quic_args = nullptr; On 2015/11/04 15:03:26, pauljensen wrote: > I don't think we need this initializer But I looked through code search. All the usage of DictionaryValue::GetDictionary has an initializer like this. An example: https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/t... https://codereview.chromium.org/1414053008/diff/80001/components/cronet/url_r... components/cronet/url_request_context_config.cc:163: On 2015/11/04 15:03:26, pauljensen wrote: > remove blank line to match style here? Done. https://codereview.chromium.org/1414053008/diff/80001/components/cronet/url_r... File components/cronet/url_request_context_config.h (right): https://codereview.chromium.org/1414053008/diff/80001/components/cronet/url_r... components/cronet/url_request_context_config.h:78: On 2015/11/04 15:03:26, pauljensen wrote: > remove blank line to match style here? Done. https://codereview.chromium.org/1414053008/diff/80001/components/cronet/url_r... components/cronet/url_request_context_config.h:84: // as the value. An example: On 2015/11/04 15:03:26, pauljensen wrote: > Can we remove the "The configuration options are..." sentence as I don't want to > limit ourselves unnecessarily? Done. https://codereview.chromium.org/1414053008/diff/80001/components/cronet/url_r... components/cronet/url_request_context_config.h:88: On 2015/11/04 15:03:26, pauljensen wrote: > remove blank line to match style here? Done.
https://codereview.chromium.org/1414053008/diff/100001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1414053008/diff/100001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:306: * @param options JSON formatted experimental options nits: in this file we end param@ comments with period. Also may want to restore the @return comment. https://codereview.chromium.org/1414053008/diff/100001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:309: return putString(CronetEngineBuilderList.EXPERIMENTAL_OPTIONS, options); Given that engine building is slow and rare I think it might be worth it to add a check that options is valid JSON string by trying to load it into JSONObject. https://codereview.chromium.org/1414053008/diff/100001/components/cronet/url_... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/1414053008/diff/100001/components/cronet/url_... components/cronet/url_request_context_config.cc:24: const char kQuicConnectionOptions[] = "connection_options"; I don't have strong opinion, but constants in io_thread.cc seem to use JavaCapitalization. OTOH, the following, argh: static const char kInitialMaxConcurrentStreams[] = "init-max-streams";
Patchset #6 (id:120001) has been deleted
PTAL. thanks! https://codereview.chromium.org/1414053008/diff/100001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1414053008/diff/100001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:306: * @param options JSON formatted experimental options On 2015/11/06 18:52:39, mef wrote: > nits: in this file we end param@ comments with period. > Also may want to restore the @return comment. Done. https://codereview.chromium.org/1414053008/diff/100001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:309: return putString(CronetEngineBuilderList.EXPERIMENTAL_OPTIONS, options); On 2015/11/06 18:52:39, mef wrote: > Given that engine building is slow and rare I think it might be worth it to add > a check that options is valid JSON string by trying to load it into JSONObject. Do we add a "throws JSONException" to the method signature? or should we catch the checked exception and throw a runtime exception? I am not sure if that necessary. We do have a dcheck in url_request_context_config.cc to make sure it is a json. https://codereview.chromium.org/1414053008/diff/100001/components/cronet/url_... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/1414053008/diff/100001/components/cronet/url_... components/cronet/url_request_context_config.cc:24: const char kQuicConnectionOptions[] = "connection_options"; On 2015/11/06 18:52:39, mef wrote: > I don't have strong opinion, but constants in io_thread.cc seem to use > JavaCapitalization. > > OTOH, the following, argh: > > static const char kInitialMaxConcurrentStreams[] = "init-max-streams"; "connection_options" is the string hardcoded in https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/io_..., so I am afraid that we can't change it.
Raman is waiting on this CL. Please take a look when you get time.
lgtm mod one comment. https://codereview.chromium.org/1414053008/diff/140001/components/cronet/url_... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/1414053008/diff/140001/components/cronet/url_... components/cronet/url_request_context_config.cc:55: DCHECK(dict); The dict will be null if experimental_options is valid json, but not the dictionary (e.g. list): https://code.google.com/p/chromium/codesearch#chromium/src/base/values.cc&l=355 I would add the check similar to line 47.
Thanks! Paul, PTAL. https://codereview.chromium.org/1414053008/diff/140001/components/cronet/url_... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/1414053008/diff/140001/components/cronet/url_... components/cronet/url_request_context_config.cc:55: DCHECK(dict); On 2015/11/16 18:19:22, mef wrote: > The dict will be null if experimental_options is valid json, but not the > dictionary (e.g. list): > https://code.google.com/p/chromium/codesearch#chromium/src/base/values.cc&l=355 > > I would add the check similar to line 47. Done.
lgtm modulo the comment in url_request_context_config.cc https://codereview.chromium.org/1414053008/diff/160001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (left): https://codereview.chromium.org/1414053008/diff/160001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:313: public Builder setExperimentalQuicConnectionOptions(String quicConnectionOptions) { Normally we don't get rid of old APIs, we @deprecate them for a milestone. I think it's okay in this case as there are so few callers. https://codereview.chromium.org/1414053008/diff/160001/components/cronet/url_... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/1414053008/diff/160001/components/cronet/url_... components/cronet/url_request_context_config.cc:57: DCHECK(false) << "Parsing experimental options failed: " differentiate this error message from the one above so we know which one might have failed.
https://codereview.chromium.org/1414053008/diff/160001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (left): https://codereview.chromium.org/1414053008/diff/160001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:313: public Builder setExperimentalQuicConnectionOptions(String quicConnectionOptions) { On 2015/11/17 18:31:10, pauljensen wrote: > Normally we don't get rid of old APIs, we @deprecate them for a milestone. I > think it's okay in this case as there are so few callers. Acknowledged. https://codereview.chromium.org/1414053008/diff/160001/components/cronet/url_... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/1414053008/diff/160001/components/cronet/url_... components/cronet/url_request_context_config.cc:57: DCHECK(false) << "Parsing experimental options failed: " On 2015/11/17 18:31:10, pauljensen wrote: > differentiate this error message from the one above so we know which one might > have failed. Done.
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org, pauljensen@chromium.org Link to the patchset: https://codereview.chromium.org/1414053008/#ps180001 (title: "Address Paul's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414053008/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414053008/180001
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/61b1eaaab7d5f38c76a3fd41d23a737fe177d211 Cr-Commit-Position: refs/heads/master@{#360187} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
