Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2416473004:
Add functionality for embedders to configure NQE (Closed)
DescriptionAdd functionality for embedders to configure NQE
Embedders can specify Network Quality Estimator's (NQE)
configuration params via experimental options JSON string.
These params are passed to NQE's constructor. This way
the embedder can configure NQE similar to how Chrome
currently does it using field trial params.
The CL also makes |experimental_options| member in the
UrlRequestContextContextConfig as a private member since
|effective_experimental_options| captures similar information,
and it is confusing to have 2 public members which capture similar
information.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet
BUG=638308
Review-Url: https://codereview.chromium.org/2416473004
Cr-Commit-Position: refs/heads/master@{#484120}
Committed: https://chromium.googlesource.com/chromium/src/+/e11aa36c4a842d64337c224824963527d5338847
Patch Set 1 : ps #Patch Set 2 : rebased #
Total comments: 5
Patch Set 3 : ps #
Total comments: 30
Patch Set 4 : rebased, mgersh comments #Patch Set 5 : rebased, mgersh comments #
Total comments: 8
Patch Set 6 : mgersh comments #Messages
Total messages: 161 (138 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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 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:1) has been deleted
Patchset #2 (id:30001) has been deleted
Patchset #2 (id:50001) 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...
Description was changed from ========== Add functionality for embedders to configure NQE BUG= ========== to ========== Add functionality for embedders to configure NQE Embedders can specify Network Quality Estimator's (NQE) configuration params via a map of key-value pairs. These params are passed to NQE's constructor. This way the embedder can configure NQE similar to how Chrome currently does it using field trial params. BUG=638308 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:20001) 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.
tbansal@chromium.org changed reviewers: + xunjieli@chromium.org
xunjieli: ptal. I had to update the include order in CronetUrlRequestContextTest.java because the rules for the include order changed recently (see https://codereview.chromium.org/2052823002 for details).
Patchset #1 (id:70001) has been deleted
xunjieli@chromium.org changed reviewers: + mgersh@chromium.org
+mgersh@. Miriam, could you take this? it might be good to get more familiarity with NQE APIs (since this probably falls into the same space of metrics measurements). I can do look over one more time and give OWNER stamp once you are done! https://codereview.chromium.org/2416473004/diff/90001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2416473004/diff/90001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:746: mNetworkQualityConfigurationParams = configurationParams; This retains a reference of |configurationParams|. Caller can still change the values afterwards. Do you want to do a copy instead?
This API seems like it's doing a similar thing to setExperimentalOptions() (https://cs.chromium.org/chromium/src/components/cronet/android/api/src/org/ch...), and it would be great for it to work a similar way for consistency, taking a String or a JSONObject (we're planning on adding that overload soon) instead of a Map. This would also make it easier to pass the options from Java to native. What do you think? https://codereview.chromium.org/2416473004/diff/90001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2416473004/diff/90001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:718: * the method below. How often do we expect users of NQE to want a non-default configuration? If people will want to use it without configuring it, we could keep this method. https://codereview.chromium.org/2416473004/diff/90001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:736: * @param configurationParams Map of configuration params to control Where are these params documented?
Description was changed from ========== Add functionality for embedders to configure NQE Embedders can specify Network Quality Estimator's (NQE) configuration params via a map of key-value pairs. These params are passed to NQE's constructor. This way the embedder can configure NQE similar to how Chrome currently does it using field trial params. BUG=638308 ========== to ========== Add functionality for embedders to configure NQE Embedders can specify Network Quality Estimator's (NQE) configuration params via a map of key-value pairs. These params are passed to NQE's constructor. This way the embedder can configure NQE similar to how Chrome currently does it using field trial params. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=638308 ==========
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...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
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.
Description was changed from ========== Add functionality for embedders to configure NQE Embedders can specify Network Quality Estimator's (NQE) configuration params via a map of key-value pairs. These params are passed to NQE's constructor. This way the embedder can configure NQE similar to how Chrome currently does it using field trial params. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=638308 ========== to ========== Add functionality for embedders to configure NQE Embedders can specify Network Quality Estimator's (NQE) configuration params via experimental options JSON string. These params are passed to NQE's constructor. This way the embedder can configure NQE similar to how Chrome currently does it using field trial params. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=638308 ==========
Patchset #2 (id:110001) has been deleted
Patchset #1 (id:90001) has been deleted
Description was changed from ========== Add functionality for embedders to configure NQE Embedders can specify Network Quality Estimator's (NQE) configuration params via experimental options JSON string. These params are passed to NQE's constructor. This way the embedder can configure NQE similar to how Chrome currently does it using field trial params. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=638308 ========== to ========== Add functionality for embedders to configure NQE Embedders can specify Network Quality Estimator's (NQE) configuration params via experimental options JSON string. These params are passed to NQE's constructor. This way the embedder can configure NQE similar to how Chrome currently does it using field trial params. The CL also makes |experimental_options| member in the UrlRequestContextContextConfig as a private member since |effective_experimental_options| captures similar information, and it is confusing to have 2 public members which capture similar information. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=638308 ==========
Patchset #1 (id:130001) has been deleted
xunjieli, mgersh: ptal a high level look. I will land this after the refactor in https://codereview.chromium.org/2850873002/.
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.
High-level idea looks good, ping when you want a full review.
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...
mgersh: ptal. Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2416473004/diff/170001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/2416473004/diff/170001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:688: kNetworkQualityEstimatorFieldTrialName, &nqe_args)) { Can we move the parsing of NQE options to url_request_context_config.cc? The reason is that we want to detect any unrecognized options at one place, so Cronet's NetLog dumps can have a full picture of what's in effect. https://codereview.chromium.org/2416473004/diff/170001/components/cronet/url_... File components/cronet/url_request_context_config.h (right): https://codereview.chromium.org/2416473004/diff/170001/components/cronet/url_... components/cronet/url_request_context_config.h:171: const std::string experimental_options; nice!
https://codereview.chromium.org/2416473004/diff/170001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/2416473004/diff/170001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:679: const char kNetworkQualityEstimatorFieldTrialName[] = Instead of duplicating this here, you could make it accessible from url_request_context_config.h.
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 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_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 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_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
Patchset #6 (id:250001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #5 (id:230001) 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 #4 (id:210001) has been deleted
Patchset #3 (id:190001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
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_cronet_tester on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
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:290001) has been deleted
Patchset #3 (id:270001) 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_cronet_tester on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
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 #3 (id:310001) has been deleted
Patchset #3 (id:330001) 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...
Patchset #4 (id:370001) 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_cronet_tester on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
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.
ptal. Thanks. Sorry for the delay. https://codereview.chromium.org/2416473004/diff/170001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/2416473004/diff/170001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:679: const char kNetworkQualityEstimatorFieldTrialName[] = On 2017/05/02 18:06:49, mgersh wrote: > Instead of duplicating this here, you could make it accessible from > url_request_context_config.h. Obsolete. https://codereview.chromium.org/2416473004/diff/170001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:688: kNetworkQualityEstimatorFieldTrialName, &nqe_args)) { On 2017/05/02 17:56:34, xunjieli wrote: > Can we move the parsing of NQE options to url_request_context_config.cc? The > reason is that we want to detect any unrecognized options at one place, so > Cronet's NetLog dumps can have a full picture of what's in effect. Done.
Thanks for the update! https://codereview.chromium.org/2416473004/diff/350001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/2416473004/diff/350001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:683: config->nqe_params->DetachFromThread(); This shouldn't be necessary, since experimental option parsing happens on the network sequence. https://codereview.chromium.org/2416473004/diff/350001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:686: std::move(config->nqe_params), g_net_log.Get().net_log()); Moving a pointer not owned by this object defeats the point of ownership, but I think that's more of a problem with the way the existing Cronet code is structured than with your CL. It might be better to store the individual options in the URLRequestContextConfig and create the Params object here. I don't feel strongly about that, though. What do you think? https://codereview.chromium.org/2416473004/diff/350001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:790: !config->storage_path.empty()) { This block logically goes together with HostCachePersistenceManager initialization and it would be nice to combine them. That's in a CL that I sent to the CQ right when you sent this, so you'll need to rebase again to pick it up. There might be some merge conflicts. Sorry about the race condition :) https://codereview.chromium.org/2416473004/diff/350001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/NQETest.java (right): https://codereview.chromium.org/2416473004/diff/350001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/NQETest.java:113: JSONObject experimentalOptions = new JSONObject(); These two lines shouldn't be necessary. https://codereview.chromium.org/2416473004/diff/350001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/NQETest.java:163: JSONObject experimentalOptions = new JSONObject(); Also shouldn't be necessary. https://codereview.chromium.org/2416473004/diff/350001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/NQETest.java:250: deletePrefsFile(); CronetTestBase() does this for you, so no need for it here. https://codereview.chromium.org/2416473004/diff/350001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/NQETest.java:314: while (true) { If you do this after cronetEngine.shutdown(), you won't need the while loop and you won't have to ignore a FileNotFoundException. You'll still have to wait for a prefs write before shutting down, but it can probably be a much shorter wait than 12 seconds without making the test flaky. https://codereview.chromium.org/2416473004/diff/350001/components/cronet/url_... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/2416473004/diff/350001/components/cronet/url_... components/cronet/url_request_context_config.cc:152: nqe_params.reset(new net::NetworkQualityEstimatorParams( base::MakeUnique is preferred over raw new. https://codereview.chromium.org/2416473004/diff/350001/components/cronet/url_... components/cronet/url_request_context_config.cc:346: if (nqe_args->GetString("persistent_cache_reading_enabled", This string can be moved to the top with the other string constants. https://codereview.chromium.org/2416473004/diff/350001/components/cronet/url_... components/cronet/url_request_context_config.cc:349: nqe_option == "true" ? true : false); This option could be a boolean instead of a string. https://codereview.chromium.org/2416473004/diff/350001/components/cronet/url_... components/cronet/url_request_context_config.cc:352: if (nqe_args->GetString("effective_connection_type_algorithm", This string too. https://codereview.chromium.org/2416473004/diff/350001/components/cronet/url_... components/cronet/url_request_context_config.cc:366: DCHECK(effective_connection_type_available); This depends on a user-provided value being correct, so it shouldn't be a DCHECK, which is for conditions that should be impossible. I'd check it and log a warning if it fails, and then only set the option in params if it succeeds. https://codereview.chromium.org/2416473004/diff/350001/net/nqe/network_qualit... File net/nqe/network_quality_estimator_params.h (right): https://codereview.chromium.org/2416473004/diff/350001/net/nqe/network_qualit... net/nqe/network_quality_estimator_params.h:140: void DetachFromThread() { thread_checker_.DetachFromThread(); } While you're here, can this ThreadChecker be changed to a SequenceChecker?
Patchset #4 (id:390001) has been deleted
mgersh: ptal. Thanks. https://codereview.chromium.org/2416473004/diff/350001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/2416473004/diff/350001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:683: config->nqe_params->DetachFromThread(); On 2017/06/28 18:26:50, mgersh wrote: > This shouldn't be necessary, since experimental option parsing happens on the > network sequence. Done. https://codereview.chromium.org/2416473004/diff/350001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:686: std::move(config->nqe_params), g_net_log.Get().net_log()); On 2017/06/28 18:26:50, mgersh wrote: > Moving a pointer not owned by this object defeats the point of ownership, but I > think that's more of a problem with the way the existing Cronet code is > structured than with your CL. It might be better to store the individual options > in the URLRequestContextConfig and create the Params object here. I don't feel > strongly about that, though. What do you think? Done. https://codereview.chromium.org/2416473004/diff/350001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:790: !config->storage_path.empty()) { On 2017/06/28 18:26:50, mgersh wrote: > This block logically goes together with HostCachePersistenceManager > initialization and it would be nice to combine them. That's in a CL that I sent > to the CQ right when you sent this, so you'll need to rebase again to pick it > up. There might be some merge conflicts. Sorry about the race condition :) This code block should run after |jcronet_url_request_context_| has been initialized. So, IIUC, it's not possible to move this up. https://codereview.chromium.org/2416473004/diff/350001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/NQETest.java (right): https://codereview.chromium.org/2416473004/diff/350001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/NQETest.java:113: JSONObject experimentalOptions = new JSONObject(); On 2017/06/28 18:26:50, mgersh wrote: > These two lines shouldn't be necessary. Done. https://codereview.chromium.org/2416473004/diff/350001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/NQETest.java:163: JSONObject experimentalOptions = new JSONObject(); On 2017/06/28 18:26:50, mgersh wrote: > Also shouldn't be necessary. Done. https://codereview.chromium.org/2416473004/diff/350001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/NQETest.java:250: deletePrefsFile(); On 2017/06/28 18:26:50, mgersh wrote: > CronetTestBase() does this for you, so no need for it here. Added comment. https://codereview.chromium.org/2416473004/diff/350001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/NQETest.java:314: while (true) { On 2017/06/28 18:26:50, mgersh wrote: > If you do this after cronetEngine.shutdown(), you won't need the while loop and > you won't have to ignore a FileNotFoundException. You'll still have to wait for > a prefs write before shutting down, but it can probably be a much shorter wait > than 12 seconds without making the test flaky. Cool. Done. https://codereview.chromium.org/2416473004/diff/350001/components/cronet/url_... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/2416473004/diff/350001/components/cronet/url_... components/cronet/url_request_context_config.cc:152: nqe_params.reset(new net::NetworkQualityEstimatorParams( On 2017/06/28 18:26:50, mgersh wrote: > base::MakeUnique is preferred over raw new. Done. https://codereview.chromium.org/2416473004/diff/350001/components/cronet/url_... components/cronet/url_request_context_config.cc:346: if (nqe_args->GetString("persistent_cache_reading_enabled", On 2017/06/28 18:26:50, mgersh wrote: > This string can be moved to the top with the other string constants. Done. https://codereview.chromium.org/2416473004/diff/350001/components/cronet/url_... components/cronet/url_request_context_config.cc:349: nqe_option == "true" ? true : false); On 2017/06/28 18:26:50, mgersh wrote: > This option could be a boolean instead of a string. Thanks, that's much better. https://codereview.chromium.org/2416473004/diff/350001/components/cronet/url_... components/cronet/url_request_context_config.cc:352: if (nqe_args->GetString("effective_connection_type_algorithm", On 2017/06/28 18:26:50, mgersh wrote: > This string too. Done. https://codereview.chromium.org/2416473004/diff/350001/components/cronet/url_... components/cronet/url_request_context_config.cc:366: DCHECK(effective_connection_type_available); On 2017/06/28 18:26:50, mgersh wrote: > This depends on a user-provided value being correct, so it shouldn't be a > DCHECK, which is for conditions that should be impossible. I'd check it and log > a warning if it fails, and then only set the option in params if it succeeds. Done. https://codereview.chromium.org/2416473004/diff/350001/net/nqe/network_qualit... File net/nqe/network_quality_estimator_params.h (right): https://codereview.chromium.org/2416473004/diff/350001/net/nqe/network_qualit... net/nqe/network_quality_estimator_params.h:140: void DetachFromThread() { thread_checker_.DetachFromThread(); } On 2017/06/28 18:26:50, mgersh wrote: > While you're here, can this ThreadChecker be changed to a SequenceChecker? Done.
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_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #4 (id:410001) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
xunjieli@chromium.org changed reviewers: - xunjieli@chromium.org
Removing myself and deferring to Miriam.
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...)
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.
mgersh: ptal. Thanks. The testbots are now all green.
mgersh: ptal. Thanks. The testbots are now all green.
Cool, almost there. https://codereview.chromium.org/2416473004/diff/350001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/2416473004/diff/350001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:790: !config->storage_path.empty()) { On 2017/06/29 01:17:58, tbansal1 wrote: > On 2017/06/28 18:26:50, mgersh wrote: > > This block logically goes together with HostCachePersistenceManager > > initialization and it would be nice to combine them. That's in a CL that I > sent > > to the CQ right when you sent this, so you'll need to rebase again to pick it > > up. There might be some merge conflicts. Sorry about the race condition :) > > This code block should run after |jcronet_url_request_context_| has been > initialized. So, IIUC, it's not possible to move this up. Ah, ok, I misread which object needs to be initialized and why. I think it would still be safe to do it earlier, even where it was before this CL, because all of these things run on the same task runner (prefs and network task runners are the same in Cronet) so the posted task won't run until this one finishes. https://codereview.chromium.org/2416473004/diff/350001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/NQETest.java (right): https://codereview.chromium.org/2416473004/diff/350001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/NQETest.java:250: deletePrefsFile(); On 2017/06/29 01:17:58, tbansal1 wrote: > On 2017/06/28 18:26:50, mgersh wrote: > > CronetTestBase() does this for you, so no need for it here. > > Added comment. Oh, I see what's going on. You're running into crbug.com/637972 and these tests are being run twice with the same configuration. They're testing functionality that relies on native code, so they should all have the @OnlyRunNativeCronet annotation. Then they'll only run once and you can remove this line. https://codereview.chromium.org/2416473004/diff/450001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/NQETest.java (right): https://codereview.chromium.org/2416473004/diff/450001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/NQETest.java:310: while (true) { What shutdown() does is destroy the native CronetURLRequestContextAdapter, which flushes the prefs to disk. So with the line moved here, there's no point to this while loop, the prefs write has either happened already or never will. https://codereview.chromium.org/2416473004/diff/450001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/NQETest.java:316: } catch (FileNotFoundException e) { This try/catch can be removed if you wait for a prefs write. https://codereview.chromium.org/2416473004/diff/450001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/NQETest.java:320: Thread.sleep(2000); In order to be useful, this sleep should happen before shutdown, if it's needed at all. https://codereview.chromium.org/2416473004/diff/450001/components/cronet/url_... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/2416473004/diff/450001/components/cronet/url_... components/cronet/url_request_context_config.cc:91: // Name of the boolean to enable reading of the persistemnt prefs in NQE. nit: typo in "persistent"
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_cronet_tester on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
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_cronet_tester on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
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.
mgersh: ptal. Thanks. https://codereview.chromium.org/2416473004/diff/350001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/2416473004/diff/350001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:790: !config->storage_path.empty()) { On 2017/06/29 19:02:31, mgersh wrote: > On 2017/06/29 01:17:58, tbansal1 wrote: > > On 2017/06/28 18:26:50, mgersh wrote: > > > This block logically goes together with HostCachePersistenceManager > > > initialization and it would be nice to combine them. That's in a CL that I > > sent > > > to the CQ right when you sent this, so you'll need to rebase again to pick > it > > > up. There might be some merge conflicts. Sorry about the race condition :) > > > > This code block should run after |jcronet_url_request_context_| has been > > initialized. So, IIUC, it's not possible to move this up. > > Ah, ok, I misread which object needs to be initialized and why. I think it would > still be safe to do it earlier, even where it was before this CL, because all of > these things run on the same task runner (prefs and network task runners are the > same in Cronet) so the posted task won't run until this one finishes. The problem is that it is not a posted task. I am using a post task now. Hopefully that's cleaner. https://codereview.chromium.org/2416473004/diff/350001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/NQETest.java (right): https://codereview.chromium.org/2416473004/diff/350001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/NQETest.java:250: deletePrefsFile(); On 2017/06/29 19:02:31, mgersh wrote: > On 2017/06/29 01:17:58, tbansal1 wrote: > > On 2017/06/28 18:26:50, mgersh wrote: > > > CronetTestBase() does this for you, so no need for it here. > > > > Added comment. > > Oh, I see what's going on. You're running into crbug.com/637972 and these tests > are being run twice with the same configuration. They're testing functionality > that relies on native code, so they should all have the @OnlyRunNativeCronet > annotation. Then they'll only run once and you can remove this line. Done. https://codereview.chromium.org/2416473004/diff/450001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/NQETest.java (right): https://codereview.chromium.org/2416473004/diff/450001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/NQETest.java:310: while (true) { On 2017/06/29 19:02:32, mgersh wrote: > What shutdown() does is destroy the native CronetURLRequestContextAdapter, which > flushes the prefs to disk. So with the line moved here, there's no point to this > while loop, the prefs write has either happened already or never will. Done. https://codereview.chromium.org/2416473004/diff/450001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/NQETest.java:316: } catch (FileNotFoundException e) { On 2017/06/29 19:02:32, mgersh wrote: > This try/catch can be removed if you wait for a prefs write. Done. https://codereview.chromium.org/2416473004/diff/450001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/NQETest.java:320: Thread.sleep(2000); On 2017/06/29 19:02:32, mgersh wrote: > In order to be useful, this sleep should happen before shutdown, if it's needed > at all. Done. https://codereview.chromium.org/2416473004/diff/450001/components/cronet/url_... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/2416473004/diff/450001/components/cronet/url_... components/cronet/url_request_context_config.cc:91: // Name of the boolean to enable reading of the persistemnt prefs in NQE. On 2017/06/29 19:02:32, mgersh wrote: > nit: typo in "persistent" Done.
lgtm, and thanks for doing this!
tbansal@chromium.org changed reviewers: + ryansturm@chromium.org
ryansturm: ptal at net/nqe/. Thanks.
net/nqe:lgtm
The CQ bit was checked by tbansal@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_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: This issue passed the CQ dry run.
The CQ bit was checked by tbansal@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": 470001, "attempt_start_ts": 1499183738155550,
"parent_rev": "6da2ebceadf127b3a7e9e52ef412deead53f12f3", "commit_rev":
"e11aa36c4a842d64337c224824963527d5338847"}
Message was sent while issue was closed.
Description was changed from ========== Add functionality for embedders to configure NQE Embedders can specify Network Quality Estimator's (NQE) configuration params via experimental options JSON string. These params are passed to NQE's constructor. This way the embedder can configure NQE similar to how Chrome currently does it using field trial params. The CL also makes |experimental_options| member in the UrlRequestContextContextConfig as a private member since |effective_experimental_options| captures similar information, and it is confusing to have 2 public members which capture similar information. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=638308 ========== to ========== Add functionality for embedders to configure NQE Embedders can specify Network Quality Estimator's (NQE) configuration params via experimental options JSON string. These params are passed to NQE's constructor. This way the embedder can configure NQE similar to how Chrome currently does it using field trial params. The CL also makes |experimental_options| member in the UrlRequestContextContextConfig as a private member since |effective_experimental_options| captures similar information, and it is confusing to have 2 public members which capture similar information. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=638308 Review-Url: https://codereview.chromium.org/2416473004 Cr-Commit-Position: refs/heads/master@{#484120} Committed: https://chromium.googlesource.com/chromium/src/+/e11aa36c4a842d64337c22482496... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:470001) as https://chromium.googlesource.com/chromium/src/+/e11aa36c4a842d64337c22482496... |
