Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1273173002:
Added Network Quality Estimator Real-time interface to Cronet (Closed)
|
Created:
5 years, 4 months ago by bengr Modified:
5 years, 2 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, Randy Smith (Not in Mondays), allanwoj, miloslav Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded Network Quality Estimator Real-time interface to Cronet
BUG=502423
Committed: https://crrev.com/f1b6738e27e2e31e5f8da99f3367ccb3d3c2911f
Cr-Commit-Position: refs/heads/master@{#352732}
Patch Set 1 : #
Total comments: 34
Patch Set 2 : Comments addressed #Patch Set 3 : Fixed test #Patch Set 4 : Rebase #Patch Set 5 : nits #
Total comments: 16
Patch Set 6 : Addressed comments #
Total comments: 32
Patch Set 7 : Fixed tests #Patch Set 8 : Removed static cast #Patch Set 9 : Addressed comments on patch set 6 #
Total comments: 8
Patch Set 10 : Addressed comments from tbansal #
Total comments: 17
Patch Set 11 : Added Executor #Patch Set 12 : Addressed comments from Paul #
Total comments: 20
Patch Set 13 : Addressed remaining comments #Patch Set 14 : Accidentally dropped a file #
Total comments: 11
Patch Set 15 : Addressed comments from tbansal #
Total comments: 29
Patch Set 16 : Addressed more from Paul #
Total comments: 6
Patch Set 17 : Fixed enum parser #
Total comments: 1
Patch Set 18 : Added enum script tests for comments #Patch Set 19 : Addressed nit from mef #
Total comments: 6
Patch Set 20 : Removed enum script #Patch Set 21 : Addressed comments from mef #Patch Set 22 : deprecated API to hide it #
Total comments: 22
Patch Set 23 : Addressed comments from Paul #Patch Set 24 : Removed VisibleForTesting #
Total comments: 5
Patch Set 25 : Fixed tests #
Total comments: 4
Patch Set 26 : nits #Patch Set 27 : nit #Messages
Total messages: 73 (15 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
bengr@chromium.org changed reviewers: + mef@chromium.org
mef: PTAL
https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:251: std::map<std::string, std::string> network_quality_estimator_params; Should this happen only if ConfigureNetworkQualityEstimator (maybe rename into EnableNetworkQualityEstimator()) is called? I imagine that many embedders don't care much about NQE. https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:315: jcronet_url_request_context_.Reset(env, jcronet_url_request_context.obj()); Does this add a reference to jcronet_url_request_context? https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.h (right): https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.h:87: void ConfigureNetworkQualityEstimator(JNIEnv* env, jobject jcaller, need comment, don't need nl above. https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.h:93: void ProvideRTTObservations(JNIEnv* env, jobject jcaller, bool should); Would it make sens to combine these 3 methods into one? https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.h:159: scoped_ptr<net::NetworkQualityEstimator> network_quality_estimator_; does it need special shutdown handling? https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/BandwidthObserver.java (right): https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/BandwidthObserver.java:11: public void onBandwidthObservation(int value, int when, need javadoc comment for public methods. https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:46: private ObserverList<RTTObserver> mRTTObserverList = final https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:159: boolean useLocalHostRequests, boolean useSmallerResponses) { synchronized (mLock) { checkHaveAdapter(); ... here and below. https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:262: for (RTTObserver observer : mRTTObserverList) { protect the list? https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/RTTObserver.java (right): https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/RTTObserver.java:10: public interface RTTObserver { Need moar comments. Also, should it have a name like |NetworkQualityRTTObserver|? https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:91: public abstract void configureNetworkQualityEstimator( need javadoc comments for all of these. Does it make sense to extract them into separate interface? https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:183: mActivity.mUrlRequestContext.configureNetworkQualityEstimator(true, What happens if you add observer without configuring NQE? https://codereview.chromium.org/1273173002/diff/40001/net/base/network_qualit... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1273173002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.cc:710: kbps_observations_.AddObservation(bandwidth_observation); I think it will be a little cleaner if bandwidth_observation lines were separate from rtt_observation lines. https://codereview.chromium.org/1273173002/diff/40001/net/base/network_qualit... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1273173002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:53: class NET_EXPORT RTTObserver { Why is this class NET_EXPORT but parent is NET_EXPORT_PRIVATE? https://codereview.chromium.org/1273173002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:56: virtual void OnRTTObservation(int32_t value, Need to specify units of |value|. Also below. https://codereview.chromium.org/1273173002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:68: class NET_EXPORT BandwidthObserver { add class comment?
Comments addressed. The test needs to be fixed still. https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:251: std::map<std::string, std::string> network_quality_estimator_params; On 2015/08/11 17:16:26, mef wrote: > Should this happen only if ConfigureNetworkQualityEstimator (maybe rename into > EnableNetworkQualityEstimator()) is called? > > I imagine that many embedders don't care much about NQE. Done. https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:315: jcronet_url_request_context_.Reset(env, jcronet_url_request_context.obj()); On 2015/08/11 17:16:26, mef wrote: > Does this add a reference to jcronet_url_request_context? We didn't store the reference before. Is there a better way? https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.h (right): https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.h:87: void ConfigureNetworkQualityEstimator(JNIEnv* env, jobject jcaller, On 2015/08/11 17:16:26, mef wrote: > need comment, don't need nl above. Done. https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.h:93: void ProvideRTTObservations(JNIEnv* env, jobject jcaller, bool should); On 2015/08/11 17:16:26, mef wrote: > Would it make sens to combine these 3 methods into one? Not really. These two are optimizations so that NQE doesn't pass observations over jni when there are no Java consumers. They are used in conjunction with their respective observer lists. https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.h:159: scoped_ptr<net::NetworkQualityEstimator> network_quality_estimator_; On 2015/08/11 17:16:26, mef wrote: > does it need special shutdown handling? Nope. https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/BandwidthObserver.java (right): https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/BandwidthObserver.java:11: public void onBandwidthObservation(int value, int when, On 2015/08/11 17:16:26, mef wrote: > need javadoc comment for public methods. Done. https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:46: private ObserverList<RTTObserver> mRTTObserverList = On 2015/08/11 17:16:26, mef wrote: > final Done. https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:159: boolean useLocalHostRequests, boolean useSmallerResponses) { On 2015/08/11 17:16:26, mef wrote: > synchronized (mLock) { checkHaveAdapter(); ... > > here and below. > Done. https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:262: for (RTTObserver observer : mRTTObserverList) { On 2015/08/11 17:16:26, mef wrote: > protect the list? I don't understand. https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/RTTObserver.java (right): https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/RTTObserver.java:10: public interface RTTObserver { On 2015/08/11 17:16:26, mef wrote: > Need moar comments. Also, should it have a name like > |NetworkQualityRTTObserver|? Done. https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:91: public abstract void configureNetworkQualityEstimator( On 2015/08/11 17:16:27, mef wrote: > need javadoc comments for all of these. > Does it make sense to extract them into separate interface? Done. I don't think a separate interface is needed yet. If we expand NQE support in Cronet, we may consider it. https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:183: mActivity.mUrlRequestContext.configureNetworkQualityEstimator(true, On 2015/08/11 17:16:27, mef wrote: > What happens if you add observer without configuring NQE? Removed the configure method. https://codereview.chromium.org/1273173002/diff/40001/net/base/network_qualit... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1273173002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.cc:710: kbps_observations_.AddObservation(bandwidth_observation); On 2015/08/11 17:16:27, mef wrote: > I think it will be a little cleaner if bandwidth_observation lines were separate > from rtt_observation lines. Done. https://codereview.chromium.org/1273173002/diff/40001/net/base/network_qualit... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1273173002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:53: class NET_EXPORT RTTObserver { On 2015/08/11 17:16:27, mef wrote: > Why is this class NET_EXPORT but parent is NET_EXPORT_PRIVATE? Done. https://codereview.chromium.org/1273173002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:56: virtual void OnRTTObservation(int32_t value, On 2015/08/11 17:16:27, mef wrote: > Need to specify units of |value|. Also below. Done. https://codereview.chromium.org/1273173002/diff/40001/net/base/network_qualit... net/base/network_quality_estimator.h:68: class NET_EXPORT BandwidthObserver { On 2015/08/11 17:16:27, mef wrote: > add class comment? Done.
few comments so far. https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:315: jcronet_url_request_context_.Reset(env, jcronet_url_request_context.obj()); On 2015/08/25 23:43:34, bengr wrote: > On 2015/08/11 17:16:26, mef wrote: > > Does this add a reference to jcronet_url_request_context? > > We didn't store the reference before. Is there a better way? It seems that we do the same in CronetUrlRequestAdapter. Not sure whether we should call it |owner_| for consistency. https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.h (right): https://codereview.chromium.org/1273173002/diff/40001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.h:93: void ProvideRTTObservations(JNIEnv* env, jobject jcaller, bool should); On 2015/08/25 23:43:34, bengr wrote: > On 2015/08/11 17:16:26, mef wrote: > > Would it make sens to combine these 3 methods into one? > > Not really. These two are optimizations so that NQE doesn't pass observations > over jni when there are no Java consumers. They are used in conjunction with > their respective observer lists. Acknowledged. https://codereview.chromium.org/1273173002/diff/120001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1273173002/diff/120001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:143: network_quality_estimator_->RemoveRTTObserver(this); what if network_quality_estimator_ is null? https://codereview.chromium.org/1273173002/diff/120001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:166: scoped_ptr<net::ExternalEstimateProvider> external_estimate_provider; uninitialized? https://codereview.chromium.org/1273173002/diff/120001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:167: std::map<std::string, std::string> network_quality_estimator_params; #include <map> https://codereview.chromium.org/1273173002/diff/120001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:190: network_quality_estimator_->AddRTTObserver(this); is there any danger in doing it multiple times? https://codereview.chromium.org/1273173002/diff/120001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/NetworkQualityThroughputObserver.java (right): https://codereview.chromium.org/1273173002/diff/120001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityThroughputObserver.java:8: * Interface to watch for observations of bandwidth. throughput? https://codereview.chromium.org/1273173002/diff/120001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityThroughputObserver.java:12: * Reports a new bandwidth observations. throughput?
https://codereview.chromium.org/1273173002/diff/120001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1273173002/diff/120001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:166: scoped_ptr<net::ExternalEstimateProvider> external_estimate_provider; include header for net::ExternalEstimateProvider? https://codereview.chromium.org/1273173002/diff/120001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1273173002/diff/120001/net/base/network_quali... net/base/network_quality_estimator.h:274: // the observation was made. add comment about source?
https://codereview.chromium.org/1273173002/diff/120001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1273173002/diff/120001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:143: network_quality_estimator_->RemoveRTTObserver(this); On 2015/08/26 16:58:59, mef wrote: > what if network_quality_estimator_ is null? Uggh. I wrote all of this when that wasn't possible. Fixed. https://codereview.chromium.org/1273173002/diff/120001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:166: scoped_ptr<net::ExternalEstimateProvider> external_estimate_provider; On 2015/08/26 17:28:40, mef wrote: > include header for net::ExternalEstimateProvider? Done. https://codereview.chromium.org/1273173002/diff/120001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:166: scoped_ptr<net::ExternalEstimateProvider> external_estimate_provider; On 2015/08/26 16:58:59, mef wrote: > uninitialized? Done. https://codereview.chromium.org/1273173002/diff/120001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:167: std::map<std::string, std::string> network_quality_estimator_params; On 2015/08/26 16:58:59, mef wrote: > #include <map> Done. https://codereview.chromium.org/1273173002/diff/120001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:190: network_quality_estimator_->AddRTTObserver(this); On 2015/08/26 16:58:59, mef wrote: > is there any danger in doing it multiple times? Nope. No issue. The observer will only be added once and removed once. https://codereview.chromium.org/1273173002/diff/120001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/NetworkQualityThroughputObserver.java (right): https://codereview.chromium.org/1273173002/diff/120001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityThroughputObserver.java:8: * Interface to watch for observations of bandwidth. On 2015/08/26 16:58:59, mef wrote: > throughput? Done. https://codereview.chromium.org/1273173002/diff/120001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityThroughputObserver.java:12: * Reports a new bandwidth observations. On 2015/08/26 16:58:59, mef wrote: > throughput? Done. https://codereview.chromium.org/1273173002/diff/120001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1273173002/diff/120001/net/base/network_quali... net/base/network_quality_estimator.h:274: // the observation was made. On 2015/08/26 17:28:40, mef wrote: > add comment about source? Done.
Patchset #8 (id:180001) has been deleted
https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:30: #include "net/base/network_quality_estimator.h" included into header? https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:168: EnableNetworkQualityEstimatorOnNetworkThread(bool use_local_host_requests, So, what happens if this is called twice? https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:193: bool should) { DCHECK(GetNetworkTaskRunner()->BelongsToCurrentThread()); https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:194: DCHECK(network_quality_estimator_); what if EnableNetworkQualityEstimator was never called? https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:212: DCHECK(network_quality_estimator_); what if EnableNetworkQualityEstimator was never called? https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.h (right): https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.h:20: #include "base/time/time.h" unused? https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.h:27: class Time; unused? https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.h:32: class NetworkQualityEstimator; do you need this if you have include? https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.h:134: const base::TimeTicks& timestamp, class TimeTicks on top? https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.h:177: base::android::ScopedJavaGlobalRef<jobject> jcronet_url_request_context_; add comment that this is java-side owner? https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:202: mThroughputObserverList.removeObserver(observer); this happens on arbitrary thread. https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:275: for (NetworkQualityRTTObserver observer : mRTTObserverList) { this happens on network thread. If anything, the iteration should be done under lock, but I wonder whether we should actually post observations to some executor, so if the app does anything interesting, like write observation to disk, it would not block the network thread. https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/NetworkQualityRTTObserver.java (right): https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityRTTObserver.java:12: * Reports a new round trip time observation. Maybe add comment that this is called on network thread and should NOT be doing anything lengthy? https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/NetworkQualityThroughputObserver.java (right): https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityThroughputObserver.java:15: * @param source The observation source from {@link NetworkQualityObservationSource}. NetworkQualityObservationSource is generated from .h file, but I'm not sure whether it is public. I other places we have explicitly defined constants on java side (with corresponding javadoc, etc), and provided mapping from internal values to public ones. https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:183: mActivity.mUrlRequestContext.addRTTObserver(networkQualityObserver); add tests for removeXYZObserver and for doing those things before enabling NQE. https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:191: assertTrue(networkQualityObserver.throughputObservationCount() > 0); add mActivity.mUrlRequestContext.shutdown() to make sure that it works properly.
https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:30: #include "net/base/network_quality_estimator.h" On 2015/08/28 20:53:15, mef wrote: > included into header? Oy. Sorry for the sloppiness. I should have checked these. https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:168: EnableNetworkQualityEstimatorOnNetworkThread(bool use_local_host_requests, On 2015/08/28 20:53:15, mef wrote: > So, what happens if this is called twice? Nothing bad if this registers as an observer twice too. If not, this won't get observations. https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:193: bool should) { On 2015/08/28 20:53:15, mef wrote: > DCHECK(GetNetworkTaskRunner()->BelongsToCurrentThread()); Done. https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:194: DCHECK(network_quality_estimator_); On 2015/08/28 20:53:15, mef wrote: > what if EnableNetworkQualityEstimator was never called? I changed these to silently fail, so tests wouldn't crash. https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:212: DCHECK(network_quality_estimator_); On 2015/08/28 20:53:15, mef wrote: > what if EnableNetworkQualityEstimator was never called? See above comment. https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.h (right): https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.h:20: #include "base/time/time.h" On 2015/08/28 20:53:16, mef wrote: > unused? Done. https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.h:27: class Time; On 2015/08/28 20:53:16, mef wrote: > unused? Done. https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.h:32: class NetworkQualityEstimator; On 2015/08/28 20:53:15, mef wrote: > do you need this if you have include? Done. https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.h:134: const base::TimeTicks& timestamp, On 2015/08/28 20:53:16, mef wrote: > class TimeTicks on top? Done. https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.h:177: base::android::ScopedJavaGlobalRef<jobject> jcronet_url_request_context_; On 2015/08/28 20:53:16, mef wrote: > add comment that this is java-side owner? Done. https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:202: mThroughputObserverList.removeObserver(observer); On 2015/08/28 20:53:16, mef wrote: > this happens on arbitrary thread. Are you saying that mThroughputObserverList isn't thread safe? If so, do you have any recommendations? Do you enforce use on a particular thread or something? https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:275: for (NetworkQualityRTTObserver observer : mRTTObserverList) { On 2015/08/28 20:53:16, mef wrote: > this happens on network thread. > If anything, the iteration should be done under lock, but I wonder whether we > should actually post observations to some executor, so if the app does anything > interesting, like write observation to disk, it would not block the network > thread. Good idea. Do you have examples of where you use an executor? I can do that in a follow-up CL. https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/NetworkQualityRTTObserver.java (right): https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityRTTObserver.java:12: * Reports a new round trip time observation. On 2015/08/28 20:53:16, mef wrote: > Maybe add comment that this is called on network thread and should NOT be doing > anything lengthy? Done. https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/NetworkQualityThroughputObserver.java (right): https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityThroughputObserver.java:15: * @param source The observation source from {@link NetworkQualityObservationSource}. On 2015/08/28 20:53:16, mef wrote: > NetworkQualityObservationSource is generated from .h file, but I'm not sure > whether it is public. I other places we have explicitly defined constants on > java side (with corresponding javadoc, etc), and provided mapping from internal > values to public ones. For now, this is just documentation. The caller can look up the code by reading the code. In a later CL we can expose this publicly in code. https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:183: mActivity.mUrlRequestContext.addRTTObserver(networkQualityObserver); On 2015/08/28 20:53:16, mef wrote: > add tests for removeXYZObserver and for doing those things before enabling NQE. Done. https://codereview.chromium.org/1273173002/diff/140001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:191: assertTrue(networkQualityObserver.throughputObservationCount() > 0); On 2015/08/28 20:53:16, mef wrote: > add mActivity.mUrlRequestContext.shutdown() to make sure that it works properly. Done.
tbansal@chromium.org changed reviewers: + tbansal@chromium.org
Drive-by review. :) https://codereview.chromium.org/1273173002/diff/220001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/220001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:274: private void onRTTObservation(int value, int when, int source) { |when| should be long. ditto below for throughput. https://codereview.chromium.org/1273173002/diff/220001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/NetworkQualityRTTObserver.java (right): https://codereview.chromium.org/1273173002/diff/220001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityRTTObserver.java:17: */ I believe |whenMs| should be long. Java prefers unix epoch time in milliseconds as long. http://docs.oracle.com/javase/6/docs/api/java/lang/System.html#currentTimeMil... https://codereview.chromium.org/1273173002/diff/220001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/NetworkQualityThroughputObserver.java (right): https://codereview.chromium.org/1273173002/diff/220001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityThroughputObserver.java:18: public void onThroughputObservation(int throughputKbps, int whenMs, int source); |whenMs| should be long. https://codereview.chromium.org/1273173002/diff/220001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1273173002/diff/220001/net/base/network_quali... net/base/network_quality_estimator.h:41: enum ObservationType { DOWNLINK_THROUGHPUT, RTT }; ObservationType seems unused.
https://codereview.chromium.org/1273173002/diff/220001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/220001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:274: private void onRTTObservation(int value, int when, int source) { On 2015/08/31 15:52:45, tbansal1 wrote: > |when| should be long. ditto below for throughput. Done. https://codereview.chromium.org/1273173002/diff/220001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/NetworkQualityRTTObserver.java (right): https://codereview.chromium.org/1273173002/diff/220001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityRTTObserver.java:17: */ On 2015/08/31 15:52:45, tbansal1 wrote: > I believe |whenMs| should be long. Java prefers unix epoch time in milliseconds > as long. > > http://docs.oracle.com/javase/6/docs/api/java/lang/System.html#currentTimeMil... Done. https://codereview.chromium.org/1273173002/diff/220001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/NetworkQualityThroughputObserver.java (right): https://codereview.chromium.org/1273173002/diff/220001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityThroughputObserver.java:18: public void onThroughputObservation(int throughputKbps, int whenMs, int source); On 2015/08/31 15:52:45, tbansal1 wrote: > |whenMs| should be long. Done. https://codereview.chromium.org/1273173002/diff/220001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1273173002/diff/220001/net/base/network_quali... net/base/network_quality_estimator.h:41: enum ObservationType { DOWNLINK_THROUGHPUT, RTT }; On 2015/08/31 15:52:45, tbansal1 wrote: > ObservationType seems unused. Thanks. Was left over. Done.
pauljensen@chromium.org changed reviewers: + pauljensen@chromium.org
https://codereview.chromium.org/1273173002/diff/240001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/240001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:97: * Enables the network quality estimator. This must be called before RTT You abbreviate the first occurrence of RTT but then write out "round trip time" later. Please write it out the first time. https://codereview.chromium.org/1273173002/diff/240001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:101: * @param useSmallerResponses Include small responses in throughput estimates. AFAIK these parameters are only for testing. Perhaps we should have a separate API to enable for testing; it could be either package-private or @hide. https://codereview.chromium.org/1273173002/diff/240001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:107: * Adds an observer of round trip time observations. Please make this more verbose. When and under what circumstances does this get called? Ditto for the other functions. https://codereview.chromium.org/1273173002/diff/240001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1273173002/diff/240001/net/base/network_quali... net/base/network_quality_estimator.h:128: void AddRTTObserver(RTTObserver* rtt_observer); These functions need comments, esp with regard to threading. https://codereview.chromium.org/1273173002/diff/240001/net/base/network_quali... net/base/network_quality_estimator.h:290: ObservationSource source; const https://codereview.chromium.org/1273173002/diff/240001/net/base/network_quali... net/base/network_quality_estimator.h:461: bool allow_localhost_requests_; why are you removing the const and adding Configure()? https://codereview.chromium.org/1273173002/diff/240001/net/base/network_quali... net/base/network_quality_estimator.h:505: scoped_refptr<base::ObserverListThreadSafe<RTTObserver>> rtt_observer_list_; const https://codereview.chromium.org/1273173002/diff/240001/net/base/network_quali... net/base/network_quality_estimator.h:505: scoped_refptr<base::ObserverListThreadSafe<RTTObserver>> rtt_observer_list_; why do these need to be ThreadSafe? can we instead guarantee they're only accessed on the network thread?
https://codereview.chromium.org/1273173002/diff/240001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/240001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:97: * Enables the network quality estimator. This must be called before RTT On 2015/09/01 10:59:10, pauljensen wrote: > You abbreviate the first occurrence of RTT but then write out "round trip time" > later. Please write it out the first time. Done. https://codereview.chromium.org/1273173002/diff/240001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:101: * @param useSmallerResponses Include small responses in throughput estimates. On 2015/09/01 10:59:10, pauljensen wrote: > AFAIK these parameters are only for testing. Perhaps we should have a separate > API to enable for testing; it could be either package-private or @hide. Done. https://codereview.chromium.org/1273173002/diff/240001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:107: * Adds an observer of round trip time observations. On 2015/09/01 10:59:10, pauljensen wrote: > Please make this more verbose. When and under what circumstances does this get > called? Ditto for the other functions. Done. https://codereview.chromium.org/1273173002/diff/240001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1273173002/diff/240001/net/base/network_quali... net/base/network_quality_estimator.h:128: void AddRTTObserver(RTTObserver* rtt_observer); On 2015/09/01 10:59:10, pauljensen wrote: > These functions need comments, esp with regard to threading. Done. https://codereview.chromium.org/1273173002/diff/240001/net/base/network_quali... net/base/network_quality_estimator.h:290: ObservationSource source; On 2015/09/01 10:59:10, pauljensen wrote: > const Done. https://codereview.chromium.org/1273173002/diff/240001/net/base/network_quali... net/base/network_quality_estimator.h:461: bool allow_localhost_requests_; On 2015/09/01 10:59:10, pauljensen wrote: > why are you removing the const and adding Configure()? For tests. https://codereview.chromium.org/1273173002/diff/240001/net/base/network_quali... net/base/network_quality_estimator.h:505: scoped_refptr<base::ObserverListThreadSafe<RTTObserver>> rtt_observer_list_; On 2015/09/01 10:59:10, pauljensen wrote: > why do these need to be ThreadSafe? can we instead guarantee they're only > accessed on the network thread? Funny. I fixed this before I saw your comment. Glad we're on the same page. https://codereview.chromium.org/1273173002/diff/240001/net/base/network_quali... net/base/network_quality_estimator.h:505: scoped_refptr<base::ObserverListThreadSafe<RTTObserver>> rtt_observer_list_; On 2015/09/01 10:59:10, pauljensen wrote: > const Acknowledged.
https://codereview.chromium.org/1273173002/diff/240001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1273173002/diff/240001/net/base/network_quali... net/base/network_quality_estimator.h:461: bool allow_localhost_requests_; On 2015/09/01 23:07:39, bengr wrote: > On 2015/09/01 10:59:10, pauljensen wrote: > > why are you removing the const and adding Configure()? > > For tests. Configure() seems unused. Can we go back to const and no Configure()? https://codereview.chromium.org/1273173002/diff/280001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/280001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:48: private final Object mNetworkQualityLock = new Object(); Please add a comment about what this is locking. https://codereview.chromium.org/1273173002/diff/280001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:50: private final ObserverList<NetworkQualityRTTObserver> mRTTObserverList = Can we use @GuardedBy? https://codereview.chromium.org/1273173002/diff/280001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:179: if (mRTTObserverList.isEmpty()) { You lock mRTTObserverList on line 186 but not here? Ditto for other functions and mThroughputObserverList. https://codereview.chromium.org/1273173002/diff/280001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/NetworkQualityRTTObserver.java (right): https://codereview.chromium.org/1273173002/diff/280001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityRTTObserver.java:16: * @param source The observation source from {@link NetworkQualityObservationSource}. So I do love GENERATED_JAVA_ENUM_PACKAGE...but I'm not sure if it translates C++ comments to Java comments. If it doesn't that's kinda a deal-breaker here as we need high quality javadocs for Cronet's APIs. https://codereview.chromium.org/1273173002/diff/280001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/280001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:111: abstract void enableNetworkQualityEstimator( @VisibleForTesting? https://codereview.chromium.org/1273173002/diff/280001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:116: * after the network quality estimator is enabled. Can you add an @link to enableNetworkQualityEstimator or replace "the network quality estimator is enabled" with the @link. Ditto for addThroughputObserver. https://codereview.chromium.org/1273173002/diff/280001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1273173002/diff/280001/net/base/network_quali... net/base/network_quality_estimator.h:50: OBSERVATION_SOURCE_DEFAULT_FROM_PLATFORM, Each of these needs a comment explaining it. https://codereview.chromium.org/1273173002/diff/280001/net/base/network_quali... net/base/network_quality_estimator.h:58: // times is specified in milliseconds. times->time https://codereview.chromium.org/1273173002/diff/280001/net/base/network_quali... net/base/network_quality_estimator.h:60: const base::TimeTicks& timestamp, You might want to explain what |timestamp| is a timestamp of...I'd imagine OnRTTObservation() getting called immediately after an observation is taken, so a timestamp of when the observation was taken seems redundant with TimeTicks::Now().
https://codereview.chromium.org/1273173002/diff/280001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/NetworkQualityRTTObserver.java (right): https://codereview.chromium.org/1273173002/diff/280001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityRTTObserver.java:10: public interface NetworkQualityRTTObserver { The rest of Cronet uses "Acronyms as words": https://source.android.com/source/code-style.html#treat-acronyms-as-words. It is also in the google3 style guide, so "Rtt" instead of "RTT" would be better (also in a number of other places).
https://codereview.chromium.org/1273173002/diff/280001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/280001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:48: private final Object mNetworkQualityLock = new Object(); On 2015/09/02 16:54:48, pauljensen wrote: > Please add a comment about what this is locking. Done. https://codereview.chromium.org/1273173002/diff/280001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:50: private final ObserverList<NetworkQualityRTTObserver> mRTTObserverList = On 2015/09/02 16:54:48, pauljensen wrote: > Can we use @GuardedBy? Done. https://codereview.chromium.org/1273173002/diff/280001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:179: if (mRTTObserverList.isEmpty()) { On 2015/09/02 16:54:48, pauljensen wrote: > You lock mRTTObserverList on line 186 but not here? Ditto for other functions > and mThroughputObserverList. Good catch. Thanks. https://codereview.chromium.org/1273173002/diff/280001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/NetworkQualityRTTObserver.java (right): https://codereview.chromium.org/1273173002/diff/280001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityRTTObserver.java:10: public interface NetworkQualityRTTObserver { On 2015/09/05 18:52:40, miloslav wrote: > The rest of Cronet uses "Acronyms as words": > https://source.android.com/source/code-style.html#treat-acronyms-as-words. It is > also in the google3 style guide, so "Rtt" instead of "RTT" would be better (also > in a number of other places). Done. https://codereview.chromium.org/1273173002/diff/280001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityRTTObserver.java:16: * @param source The observation source from {@link NetworkQualityObservationSource}. On 2015/09/02 16:54:48, pauljensen wrote: > So I do love GENERATED_JAVA_ENUM_PACKAGE...but I'm not sure if it translates C++ > comments to Java comments. If it doesn't that's kinda a deal-breaker here as we > need high quality javadocs for Cronet's APIs. Let's leave it as is here, until we're more confident that this is the right way to expose the source, and then I'll stop using GENERATED_JAVA_ENUM_PACKAGE if needed. https://codereview.chromium.org/1273173002/diff/280001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/280001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:111: abstract void enableNetworkQualityEstimator( On 2015/09/02 16:54:48, pauljensen wrote: > @VisibleForTesting? Added to CronetUrlRequestContext. https://codereview.chromium.org/1273173002/diff/280001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:116: * after the network quality estimator is enabled. On 2015/09/02 16:54:48, pauljensen wrote: > Can you add an @link to enableNetworkQualityEstimator or replace "the network > quality estimator is enabled" with the @link. Ditto for addThroughputObserver. Done. https://codereview.chromium.org/1273173002/diff/280001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1273173002/diff/280001/net/base/network_quali... net/base/network_quality_estimator.h:50: OBSERVATION_SOURCE_DEFAULT_FROM_PLATFORM, On 2015/09/02 16:54:48, pauljensen wrote: > Each of these needs a comment explaining it. Done. https://codereview.chromium.org/1273173002/diff/280001/net/base/network_quali... net/base/network_quality_estimator.h:58: // times is specified in milliseconds. On 2015/09/02 16:54:48, pauljensen wrote: > times->time Done. https://codereview.chromium.org/1273173002/diff/280001/net/base/network_quali... net/base/network_quality_estimator.h:60: const base::TimeTicks& timestamp, On 2015/09/02 16:54:48, pauljensen wrote: > You might want to explain what |timestamp| is a timestamp of...I'd imagine > OnRTTObservation() getting called immediately after an observation is taken, so > a timestamp of when the observation was taken seems redundant with > TimeTicks::Now(). Right. I wanted to decouple this so that there wouldn't be any confusion about when the observation was taken and to save the observer the trouble of calling Now(). I'd imagine that observers might queue observations, so it's also a convenience to have the timestamp. Moreover, I wasn't completely sure that observations would always be reported immediately after being made.
https://codereview.chromium.org/1273173002/diff/320001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1273173002/diff/320001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:26: #include "net/base/external_estimate_provider.h" nit: This is already included in cronet_url_request_context_adapter.h https://codereview.chromium.org/1273173002/diff/320001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/320001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:51: // Locks operations on network quality observers, because observer addition and removal may May be javadoc style comments? /** * */ https://codereview.chromium.org/1273173002/diff/320001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:299: @SuppressWarnings("unused") Why does it need SuppressWarnings. Looks like it is called by cronet_url_request_context_adapter.cc. https://codereview.chromium.org/1273173002/diff/320001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:301: private void onRttObservation(final int value, final long when, final int source) { Why not whenMs here and below? https://codereview.chromium.org/1273173002/diff/320001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1273173002/diff/320001/net/base/network_quali... net/base/network_quality_estimator.cc:409: allow_localhost_requests_ = allow_local_host_requests, nit: DCHECK(thread_checker...)
https://codereview.chromium.org/1273173002/diff/320001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1273173002/diff/320001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:26: #include "net/base/external_estimate_provider.h" On 2015/09/15 17:10:24, tbansal1 wrote: > nit: This is already included in cronet_url_request_context_adapter.h No it isn't. https://codereview.chromium.org/1273173002/diff/320001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/320001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:51: // Locks operations on network quality observers, because observer addition and removal may On 2015/09/15 17:10:24, tbansal1 wrote: > May be javadoc style comments? > /** > * > */ Done. https://codereview.chromium.org/1273173002/diff/320001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:299: @SuppressWarnings("unused") On 2015/09/15 17:10:24, tbansal1 wrote: > Why does it need SuppressWarnings. Looks like it is called by > cronet_url_request_context_adapter.cc. Following the pattern for other methods that are called by native. https://codereview.chromium.org/1273173002/diff/320001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:301: private void onRttObservation(final int value, final long when, final int source) { On 2015/09/15 17:10:24, tbansal1 wrote: > Why not whenMs here and below? Done. https://codereview.chromium.org/1273173002/diff/320001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1273173002/diff/320001/net/base/network_quali... net/base/network_quality_estimator.cc:409: allow_localhost_requests_ = allow_local_host_requests, On 2015/09/15 17:10:24, tbansal1 wrote: > nit: DCHECK(thread_checker...) Done.
lgtm https://codereview.chromium.org/1273173002/diff/320001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1273173002/diff/320001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:26: #include "net/base/external_estimate_provider.h" On 2015/09/16 15:32:38, bengr wrote: > On 2015/09/15 17:10:24, tbansal1 wrote: > > nit: This is already included in cronet_url_request_context_adapter.h > > No it isn't. Acknowledged.
https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/NetworkQualityRttObserver.java (right): https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityRttObserver.java:8: * Interface to watch for observations of round trip times. need to clarify which round trip times you're talking about. https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityRttObserver.java:12: * Reports a new round trip time observation. This is called on the network thread, so the I don't think customers have an idea what the "network thread" is; is this comment out of data? is it called back on the executor? https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityRttObserver.java:16: * @param source The observation source from {@link NetworkQualityObservationSource}. Two big problems with NetworkQualityObservationSource: 1. No enums allowed in public APIs. This has been endlessly discussed, but the decision is final. go/android-api-guidelines 2. I don't think generated enums have any comments and we need lots of comments for public APIs. https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:97: * Enables the network quality estimator. This must be called before round Please provide a verbose description of what the NQE is and does, and what it measures. Including what the downsides of enabling it are (otherwise why shouldn't it be enabled always?) https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:98: * trip time and throughput observers are added. include @link's to specific methods to add observers. https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:99: * @param executor An executor on which all observers will be called. "all observers" is a too broad, please clarify. https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:107: * @param useLocalHostRequests Include requests to localhost in estimates. all your param descriptions are capitalized. This goes against the javadoc recommendations: http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html which are linked from the Android coding standard: https://source.android.com/source/code-style.html#use-javadoc-standard-comments https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:109: * @param executor An executor on which all observers will be called. @link Executor https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:115: * Adds an observer of round trip time observations. This must be called This first sentence is mostly redundant with the name of the method which is frowned on by our coding standards. Perhaps something along the lines of "Registers an observer that gets called back whenever the network quality estimator witnesses a sample round trip time." https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:115: * Adds an observer of round trip time observations. This must be called we need to clarify what round trip times we're talking about. https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:117: * @param observer The observer of round trip times. need to mention the Executor that this is called back on https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:130: * Adds an observer of throughout. This must be called after need to clarify what "throughput" we're talking about. https://codereview.chromium.org/1273173002/diff/340001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1273173002/diff/340001/net/base/network_quali... net/base/network_quality_estimator.h:52: OBSERVATION_SOURCE_URL_REQUEST, why are these prefixed with "OBSERVATION_SOURCE_"? that seems redundant with the enum name.
https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/NetworkQualityRttObserver.java (right): https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityRttObserver.java:8: * Interface to watch for observations of round trip times. On 2015/09/18 16:07:51, pauljensen wrote: > need to clarify which round trip times you're talking about. Done. https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityRttObserver.java:12: * Reports a new round trip time observation. This is called on the network thread, so the On 2015/09/18 16:07:52, pauljensen wrote: > I don't think customers have an idea what the "network thread" is; is this > comment out of data? is it called back on the executor? Yeah, it is. Fixed. https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityRttObserver.java:16: * @param source The observation source from {@link NetworkQualityObservationSource}. On 2015/09/18 16:07:52, pauljensen wrote: > Two big problems with NetworkQualityObservationSource: > 1. No enums allowed in public APIs. This has been endlessly discussed, but the > decision is final. go/android-api-guidelines > 2. I don't think generated enums have any comments and we need lots of comments > for public APIs. What other option do I have? https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:97: * Enables the network quality estimator. This must be called before round On 2015/09/18 16:07:52, pauljensen wrote: > Please provide a verbose description of what the NQE is and does, and what it > measures. Including what the downsides of enabling it are (otherwise why > shouldn't it be enabled always?) Done. https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:98: * trip time and throughput observers are added. On 2015/09/18 16:07:52, pauljensen wrote: > include @link's to specific methods to add observers. Done. https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:99: * @param executor An executor on which all observers will be called. On 2015/09/18 16:07:52, pauljensen wrote: > "all observers" is a too broad, please clarify. Done. https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:107: * @param useLocalHostRequests Include requests to localhost in estimates. On 2015/09/18 16:07:52, pauljensen wrote: > all your param descriptions are capitalized. This goes against the javadoc > recommendations: > http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html > which are linked from the Android coding standard: > https://source.android.com/source/code-style.html#use-javadoc-standard-comments Ah. Thanks. Done. https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:109: * @param executor An executor on which all observers will be called. On 2015/09/18 16:07:52, pauljensen wrote: > @link Executor Done. https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:115: * Adds an observer of round trip time observations. This must be called On 2015/09/18 16:07:52, pauljensen wrote: > we need to clarify what round trip times we're talking about. Done. https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:115: * Adds an observer of round trip time observations. This must be called On 2015/09/18 16:07:52, pauljensen wrote: > This first sentence is mostly redundant with the name of the method which is > frowned on by our coding standards. Perhaps something along the lines of > "Registers an observer that gets called back whenever the network quality > estimator witnesses a sample round trip time." Done. https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:117: * @param observer The observer of round trip times. On 2015/09/18 16:07:52, pauljensen wrote: > need to mention the Executor that this is called back on Done. https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:130: * Adds an observer of throughout. This must be called after On 2015/09/18 16:07:52, pauljensen wrote: > need to clarify what "throughput" we're talking about. Done. https://codereview.chromium.org/1273173002/diff/340001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1273173002/diff/340001/net/base/network_quali... net/base/network_quality_estimator.h:52: OBSERVATION_SOURCE_URL_REQUEST, On 2015/09/18 16:07:52, pauljensen wrote: > why are these prefixed with "OBSERVATION_SOURCE_"? that seems redundant with > the enum name. This is the convention in c++ Chromium afaik, e.g., https://code.google.com/p/chromium/codesearch#chromium/src/net/base/auth.h&l=85
https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/NetworkQualityRttObserver.java (right): https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityRttObserver.java:16: * @param source The observation source from {@link NetworkQualityObservationSource}. On 2015/09/18 23:13:42, bengr wrote: > On 2015/09/18 16:07:52, pauljensen wrote: > > Two big problems with NetworkQualityObservationSource: > > 1. No enums allowed in public APIs. This has been endlessly discussed, but > the > > decision is final. go/android-api-guidelines > > 2. I don't think generated enums have any comments and we need lots of > comments > > for public APIs. > > What other option do I have? You could fix up the generator (build/android/gyp/java_cpp_enum.py) to simply copy /* */ comments through verbatim. The other option is to write some int's by hand in Java and add some assertions that they match the generated ones. Note that enum's are forbayed in Android APIs so they need to be "public final static int"'s. https://codereview.chromium.org/1273173002/diff/340001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1273173002/diff/340001/net/base/network_quali... net/base/network_quality_estimator.h:52: OBSERVATION_SOURCE_URL_REQUEST, On 2015/09/18 23:13:42, bengr wrote: > On 2015/09/18 16:07:52, pauljensen wrote: > > why are these prefixed with "OBSERVATION_SOURCE_"? that seems redundant with > > the enum name. > > This is the convention in c++ Chromium afaik, e.g., > https://code.google.com/p/chromium/codesearch#chromium/src/net/base/auth.h&l=85 Let's not be redundant in C++. In Java they'll have to be int's so you'll want the prefix.
Sorry for the delay. We are going through formal API reviews, and couple of things mismatch the guidelines. https://codereview.chromium.org/1273173002/diff/360001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/360001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:331: synchronized (mNetworkQualityLock) { BUG? I think you need to protect the loop on mThroughputObserverList inside of the task instead of during post. https://codereview.chromium.org/1273173002/diff/360001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/NetworkQualityRttObserver.java (right): https://codereview.chromium.org/1273173002/diff/360001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityRttObserver.java:14: public interface NetworkQualityRttObserver { According to Android API Guildelines this here should be NetworkQualityRttListener. https://codereview.chromium.org/1273173002/diff/360001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/360001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:108: public abstract void enableNetworkQualityEstimator(Executor executor); I wonder whether this must be a runtime method, or should it be a parameter on UrlRequestContextConfig?
bengr@chromium.org changed reviewers: + pasko@chromium.org
pasko: java_enum* https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/NetworkQualityRttObserver.java (right): https://codereview.chromium.org/1273173002/diff/340001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityRttObserver.java:16: * @param source The observation source from {@link NetworkQualityObservationSource}. On 2015/09/22 16:03:03, pauljensen wrote: > On 2015/09/18 23:13:42, bengr wrote: > > On 2015/09/18 16:07:52, pauljensen wrote: > > > Two big problems with NetworkQualityObservationSource: > > > 1. No enums allowed in public APIs. This has been endlessly discussed, but > > the > > > decision is final. go/android-api-guidelines > > > 2. I don't think generated enums have any comments and we need lots of > > comments > > > for public APIs. > > > > What other option do I have? > > You could fix up the generator (build/android/gyp/java_cpp_enum.py) to simply > copy /* */ comments through verbatim. > The other option is to write some int's by hand in Java and add some assertions > that they match the generated ones. Note that enum's are forbayed in Android > APIs so they need to be "public final static int"'s. Done. https://codereview.chromium.org/1273173002/diff/360001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/360001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:331: synchronized (mNetworkQualityLock) { On 2015/09/25 19:06:42, mef wrote: > BUG? I think you need to protect the loop on mThroughputObserverList inside of > the task instead of during post. Done. https://codereview.chromium.org/1273173002/diff/360001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/NetworkQualityRttObserver.java (right): https://codereview.chromium.org/1273173002/diff/360001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityRttObserver.java:14: public interface NetworkQualityRttObserver { On 2015/09/25 19:06:42, mef wrote: > According to Android API Guildelines this here should be > NetworkQualityRttListener. Done. https://codereview.chromium.org/1273173002/diff/360001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/360001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:108: public abstract void enableNetworkQualityEstimator(Executor executor); On 2015/09/25 19:06:42, mef wrote: > I wonder whether this must be a runtime method, or should it be a parameter on > UrlRequestContextConfig? Since this is experimental, let's leave it as a separate runtime method.
pasko@chromium.org changed reviewers: + mnaganov@chromium.org
On 2015/09/29 23:22:51, bengr wrote: > pasko: java_enum* lgtm if mnaganov agrees (he touched it last and knows infinitely more) It seems generation of extra comments is not critical for this network quality estimator functionality, so it can be separated out if latency of this review is critical. Also, are these extra comments worth complexity?
On 2015/09/30 10:00:17, pasko wrote: > On 2015/09/29 23:22:51, bengr wrote: > > pasko: java_enum* > > lgtm if mnaganov agrees (he touched it last and knows infinitely more) > > It seems generation of extra comments is not critical for this network quality > estimator functionality, so it can be separated out if latency of this review is > critical. Also, are these extra comments worth complexity? The extra comments are a requirement for Cronet, so, yes, they're worth the complexity. I thought about separating it into a different CL, but we have once consumer that is very eager to start using this API, so I'd rather not delay.
On 2015/09/30 15:27:15, bengr wrote: > On 2015/09/30 10:00:17, pasko wrote: > > On 2015/09/29 23:22:51, bengr wrote: > > > pasko: java_enum* > > > > lgtm if mnaganov agrees (he touched it last and knows infinitely more) > > > > It seems generation of extra comments is not critical for this network quality > > estimator functionality, so it can be separated out if latency of this review > is > > critical. Also, are these extra comments worth complexity? > > The extra comments are a requirement for Cronet, so, yes, they're worth the > complexity. I thought about separating it into a different CL, but we have once > consumer that is very eager to start using this API, so I'd rather not delay. "One" consumer, I mean.
On 2015/09/30 15:28:02, bengr wrote: > On 2015/09/30 15:27:15, bengr wrote: > > On 2015/09/30 10:00:17, pasko wrote: > > > On 2015/09/29 23:22:51, bengr wrote: > > > > pasko: java_enum* > > > > > > lgtm if mnaganov agrees (he touched it last and knows infinitely more) > > > > > > It seems generation of extra comments is not critical for this network > quality > > > estimator functionality, so it can be separated out if latency of this > review > > is > > > critical. Also, are these extra comments worth complexity? > > > > The extra comments are a requirement for Cronet, so, yes, they're worth the > > complexity. I thought about separating it into a different CL, but we have > once > > consumer that is very eager to start using this API, so I'd rather not delay. > > "One" consumer, I mean. Would it make sense to explicitly define ObservationSource public constants on the java side, with javadoc and everything, and have a mapping from internal values generated by java_enum into those public constants?
On 2015/09/30 15:32:54, mef wrote: > On 2015/09/30 15:28:02, bengr wrote: > > On 2015/09/30 15:27:15, bengr wrote: > > > On 2015/09/30 10:00:17, pasko wrote: > > > > On 2015/09/29 23:22:51, bengr wrote: > > > > > pasko: java_enum* > > > > > > > > lgtm if mnaganov agrees (he touched it last and knows infinitely more) > > > > > > > > It seems generation of extra comments is not critical for this network > > quality > > > > estimator functionality, so it can be separated out if latency of this > > review > > > is > > > > critical. Also, are these extra comments worth complexity? > > > > > > The extra comments are a requirement for Cronet, so, yes, they're worth the > > > complexity. I thought about separating it into a different CL, but we have > > once > > > consumer that is very eager to start using this API, so I'd rather not > delay. > > > > "One" consumer, I mean. > > Would it make sense to explicitly define ObservationSource public constants on > the java side, with javadoc and everything, > and have a mapping from internal values generated by java_enum into those public > constants? That's one of two approaches that Paul suggested. I'm cool either way, so as the Cronet owner, I leave it up to you to decide. If we go that route, we'll need a separate file for the constants because they're shared by two listeners. We will also need asserts somewhere to verify that the constants in java keep in sync with the constants in C++. The easiest way to do that is to put these assertions in a test, and to check against java constants that are generated using that enum script. Otherwise, we need to set up a new jni path from UrlRequestContext down to network_quality_estimator.cc for this purpose. Wdyt?
I'm fine with extending the enum generator if you add tests that cover parsing of the comments -- this will tremendously help anyone who will be dealing with the parser in future.
On 2015/09/30 15:46:36, bengr wrote:
> On 2015/09/30 15:32:54, mef wrote:
> > On 2015/09/30 15:28:02, bengr wrote:
> > > On 2015/09/30 15:27:15, bengr wrote:
> > > > On 2015/09/30 10:00:17, pasko wrote:
> > > > > On 2015/09/29 23:22:51, bengr wrote:
> > > > > > pasko: java_enum*
> > > > >
> > > > > lgtm if mnaganov agrees (he touched it last and knows infinitely more)
> > > > >
> > > > > It seems generation of extra comments is not critical for this network
> > > quality
> > > > > estimator functionality, so it can be separated out if latency of this
> > > review
> > > > is
> > > > > critical. Also, are these extra comments worth complexity?
> > > >
> > > > The extra comments are a requirement for Cronet, so, yes, they're worth
> the
> > > > complexity. I thought about separating it into a different CL, but we
have
> > > once
> > > > consumer that is very eager to start using this API, so I'd rather not
> > delay.
> > >
> > > "One" consumer, I mean.
> >
> > Would it make sense to explicitly define ObservationSource public constants
on
> > the java side, with javadoc and everything,
> > and have a mapping from internal values generated by java_enum into those
> public
> > constants?
>
> That's one of two approaches that Paul suggested. I'm cool either way, so as
the
> Cronet owner, I leave it up to you to decide. If we go that route, we'll need
a
> separate file for the constants because they're shared by two listeners.
I imagine that there could be something like this:
public abstract class NetworkQualityEstimator {
// public constants go here with javadocs.
...
// Listener interfaces go here with java docs.
public interface RttListener {
}
public interface ThroughputListener {
}
// NQE-specific methods from UrlRequestContext go here.
AddRttListener
RemoveRttListener
...
}
> We will
> also need asserts somewhere to verify that the constants in java keep in sync
> with the constants in C++. The easiest way to do that is to put these
assertions
> in a test, and to check against java constants that are generated using that
> enum script.
sgtm
> Otherwise, we need to set up a new jni path from UrlRequestContext
> down to network_quality_estimator.cc for this purpose.
I'm not sure that I understand.
On 2015/09/30 16:50:17, mef wrote:
> On 2015/09/30 15:46:36, bengr wrote:
> > On 2015/09/30 15:32:54, mef wrote:
> > > On 2015/09/30 15:28:02, bengr wrote:
> > > > On 2015/09/30 15:27:15, bengr wrote:
> > > > > On 2015/09/30 10:00:17, pasko wrote:
> > > > > > On 2015/09/29 23:22:51, bengr wrote:
> > > > > > > pasko: java_enum*
> > > > > >
> > > > > > lgtm if mnaganov agrees (he touched it last and knows infinitely
more)
> > > > > >
> > > > > > It seems generation of extra comments is not critical for this
network
> > > > quality
> > > > > > estimator functionality, so it can be separated out if latency of
this
> > > > review
> > > > > is
> > > > > > critical. Also, are these extra comments worth complexity?
> > > > >
> > > > > The extra comments are a requirement for Cronet, so, yes, they're
worth
> > the
> > > > > complexity. I thought about separating it into a different CL, but we
> have
> > > > once
> > > > > consumer that is very eager to start using this API, so I'd rather not
> > > delay.
> > > >
> > > > "One" consumer, I mean.
> > >
> > > Would it make sense to explicitly define ObservationSource public
constants
> on
> > > the java side, with javadoc and everything,
> > > and have a mapping from internal values generated by java_enum into those
> > public
> > > constants?
> >
> > That's one of two approaches that Paul suggested. I'm cool either way, so as
> the
> > Cronet owner, I leave it up to you to decide. If we go that route, we'll
need
> a
> > separate file for the constants because they're shared by two listeners.
>
> I imagine that there could be something like this:
>
> public abstract class NetworkQualityEstimator {
> // public constants go here with javadocs.
> ...
> // Listener interfaces go here with java docs.
> public interface RttListener {
> }
>
> public interface ThroughputListener {
> }
>
> // NQE-specific methods from UrlRequestContext go here.
> AddRttListener
> RemoveRttListener
>
> ...
>
>
>
> }
>
>
> > We will
> > also need asserts somewhere to verify that the constants in java keep in
sync
> > with the constants in C++. The easiest way to do that is to put these
> assertions
> > in a test, and to check against java constants that are generated using that
> > enum script.
>
> sgtm
>
> > Otherwise, we need to set up a new jni path from UrlRequestContext
> > down to network_quality_estimator.cc for this purpose.
>
> I'm not sure that I understand.
ok, I'll undo the change to the enum script and will add these checks.
On 2015/09/30 18:57:41, bengr wrote:
> On 2015/09/30 16:50:17, mef wrote:
> > On 2015/09/30 15:46:36, bengr wrote:
> > > On 2015/09/30 15:32:54, mef wrote:
> > > > On 2015/09/30 15:28:02, bengr wrote:
> > > > > On 2015/09/30 15:27:15, bengr wrote:
> > > > > > On 2015/09/30 10:00:17, pasko wrote:
> > > > > > > On 2015/09/29 23:22:51, bengr wrote:
> > > > > > > > pasko: java_enum*
> > > > > > >
> > > > > > > lgtm if mnaganov agrees (he touched it last and knows infinitely
> more)
> > > > > > >
> > > > > > > It seems generation of extra comments is not critical for this
> network
> > > > > quality
> > > > > > > estimator functionality, so it can be separated out if latency of
> this
> > > > > review
> > > > > > is
> > > > > > > critical. Also, are these extra comments worth complexity?
> > > > > >
> > > > > > The extra comments are a requirement for Cronet, so, yes, they're
> worth
> > > the
> > > > > > complexity. I thought about separating it into a different CL, but
we
> > have
> > > > > once
> > > > > > consumer that is very eager to start using this API, so I'd rather
not
> > > > delay.
> > > > >
> > > > > "One" consumer, I mean.
> > > >
> > > > Would it make sense to explicitly define ObservationSource public
> constants
> > on
> > > > the java side, with javadoc and everything,
> > > > and have a mapping from internal values generated by java_enum into
those
> > > public
> > > > constants?
> > >
> > > That's one of two approaches that Paul suggested. I'm cool either way, so
as
> > the
> > > Cronet owner, I leave it up to you to decide. If we go that route, we'll
> need
> > a
> > > separate file for the constants because they're shared by two listeners.
> >
> > I imagine that there could be something like this:
> >
> > public abstract class NetworkQualityEstimator {
> > // public constants go here with javadocs.
> > ...
> > // Listener interfaces go here with java docs.
> > public interface RttListener {
> > }
> >
> > public interface ThroughputListener {
> > }
> >
> > // NQE-specific methods from UrlRequestContext go here.
> > AddRttListener
> > RemoveRttListener
> >
> > ...
> >
> >
> >
> > }
> >
> >
> > > We will
> > > also need asserts somewhere to verify that the constants in java keep in
> sync
> > > with the constants in C++. The easiest way to do that is to put these
> > assertions
> > > in a test, and to check against java constants that are generated using
that
> > > enum script.
> >
> > sgtm
> >
> > > Otherwise, we need to set up a new jni path from UrlRequestContext
> > > down to network_quality_estimator.cc for this purpose.
> >
> > I'm not sure that I understand.
>
> ok, I'll undo the change to the enum script and will add these checks.
On second thought, it really doesn't make sense to have a generated
intermediate.
mnaganov: I added tests that cover the basics of converting comments, which I think is sufficient. PTAL.
Can't post as a comment on the CL due to the certificate issue, but here is my comment for the test: "What about comments that follow the value: VALUE_FOO, // Foo" On Wed, Sep 30, 2015 at 12:48 PM, <bengr@chromium.org> wrote: > mnaganov: I added tests that cover the basics of converting comments, > which I > think is sufficient. PTAL. > > https://codereview.chromium.org/1273173002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
So two things:
1) I think there is demand for this to land sooner than later, furthermore I'm
not sure we're going to get to a perfect API for this on the first try. I think
this is something we'll want to iterate on a bit potentially. How about we mark
the new APIs with @hide? This way they should be accessible to embedders (I
think) but just won't be included in our javadocs and any API review.
2) My potential solution to augment java_cpp_enum.py might not have been a great
idea. It has some problems:
a) I don't think generated files are included in our javadocs...so the
comments are lost.
b) I don't think we want to include all the generated files in our javadoc
generation, as all of them except the one from this CL are internal
implementation details, not exposed APIs.
c) I'm not sure the class layout will be exactly what we want. I imagine
java_cpp_enum.py generating a separate class for each enum, where in reality we
want the constants to be within another Cronet class.
d) All of Cronet's exposed API comes from Java, not C++. I think in this
case we really want a generator that goes in the opposite direction, generating
C++ from Java.
For now, how about we table the java_cpp_enum.py modifications and instead
follow my other suggestion and have another copy of the enum values in Java and
either translate from the Java ones to the C++ ones or use static asserts to
verify they're the same.
Per conversations with Ben and Paul this is currently a prototype interface to assess its usefulness. Observation Source values will be used for logging, so the constants may even be unnecessary on Java side. Also, currently generated Java classes are not included into Cronet Javadoc, so the approach with explicit definition of constants in public interface file is more appropriate, but could done in a separate CL. https://codereview.chromium.org/1273173002/diff/380001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1273173002/diff/380001/net/base/network_quali... net/base/network_quality_estimator.h:166: // the requirement of throughtput observations that response size be above nit: throughput
On 2015/09/30 20:30:25, mnaganov wrote: > Can't post as a comment on the CL due to the certificate issue, but here is > my comment for the test: > > "What about comments that follow the value: > > VALUE_FOO, // Foo" > > On Wed, Sep 30, 2015 at 12:48 PM, <mailto:bengr@chromium.org> wrote: > > > mnaganov: I added tests that cover the basics of converting comments, > > which I > > think is sufficient. PTAL. > > > > https://codereview.chromium.org/1273173002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. These are not supported yet.
I addressed the nit. As for the changes to the enum script, the current change supports single line comments above the constant, but not to the right of the constant. I can land as is or move the enum script code to a different CL and land this one without further changes. mef@ and mnaganov@, please let me know your preference. Thanks!
On 2015/10/01 16:23:16, bengr wrote: > I addressed the nit. As for the changes to the enum script, the current change > supports single line comments above the constant, but not to the right of the > constant. I can land as is or move the enum script code to a different CL and > land this one without further changes. mef@ and mnaganov@, please let me know > your preference. Thanks! I'm fine with those changes to the enum generator, if they can actually help you -- note that those comments will end up in generated Java files, which are usually temporary, and thus no one reads them. My main point is that there should be tests, as otherwise it's hard to make any further changes, but as you have added tests, you are good to go :)
https://codereview.chromium.org/1273173002/diff/420001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/420001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:179: mNetworkQualityExecutor = executor; This should check that executor != null and throw NullPointerException if it is. https://codereview.chromium.org/1273173002/diff/420001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:306: for (NetworkQualityRttListener listener : mRttListenerList) { BUG: need synchronized (mNetworkQualityLock) { https://codereview.chromium.org/1273173002/diff/420001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/420001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:143: * Registers an listener that gets called whenever the network quality I'm not native speaker, but shouldn't "an listener" be "a listener" here and elsewhere.
https://codereview.chromium.org/1273173002/diff/420001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/420001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:179: mNetworkQualityExecutor = executor; On 2015/10/01 19:02:28, mef wrote: > This should check that executor != null and throw NullPointerException if it is. Done. https://codereview.chromium.org/1273173002/diff/420001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:306: for (NetworkQualityRttListener listener : mRttListenerList) { On 2015/10/01 19:02:28, mef wrote: > BUG: need synchronized (mNetworkQualityLock) { Done. https://codereview.chromium.org/1273173002/diff/420001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/420001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:143: * Registers an listener that gets called whenever the network quality On 2015/10/01 19:02:28, mef wrote: > I'm not native speaker, but shouldn't "an listener" be "a listener" here and > elsewhere. Damn search and replace. Done.
lgtm
I thought we agreed the new APIs should be hidden for now? BTW @hide doesn't work (we'd need doclava) but @deprecated does work (because we build with nodeprecated="yes"), or you can just add <excluded> tags in src/components/cronet/android/java/build.xml
On 2015/10/02 15:04:44, pauljensen wrote: > I thought we agreed the new APIs should be hidden for now? > BTW @hide doesn't work (we'd need doclava) but @deprecated does work (because we > build with nodeprecated="yes"), or you can just add <excluded> tags in > src/components/cronet/android/java/build.xml Oh, right. Excluding it from javadoc and adding clear comment / annotation about it is good.
still lgtm. Paul, PTAL?
https://codereview.chromium.org/1273173002/diff/480001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.h (right): https://codereview.chromium.org/1273173002/diff/480001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.h:32: class NetworkQualityEstimator; this seems redundant with line 20 https://codereview.chromium.org/1273173002/diff/480001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/NetworkQualityThroughputListener.java (right): https://codereview.chromium.org/1273173002/diff/480001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityThroughputListener.java:14: * @param whenMs the time since the epoch in milliseconds. "the epoch" is a little ambiguos, perhaps "the Epoch (January 1st 1970, 00:00:00.000)" as Android does: http://developer.android.com/reference/java/sql/Time.html#Time(long) https://codereview.chromium.org/1273173002/diff/480001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/480001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:121: @Deprecated @VisibleForTesting https://codereview.chromium.org/1273173002/diff/480001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:122: abstract void enableNetworkQualityEstimator( nit: might be good to add "ForTesting" suffix https://codereview.chromium.org/1273173002/diff/480001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:128: * after {@link #enableNetworkQualityEstimator}. Round trip times may be might be good to mention what happens if not called after enableNQE() (does it throw?) https://codereview.chromium.org/1273173002/diff/480001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:139: * Removes a listener of round trip times if on the listener list. This "the listener list" is not a term a reader might be familiar with. How about changing to "if previously registered with {@link #addRttListener}" https://codereview.chromium.org/1273173002/diff/480001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:140: * should be called after a NetworkQualityRttListener is added in order to wrap NQERttListener in @link https://codereview.chromium.org/1273173002/diff/480001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:163: * NetworkQualityThroughputListener is added in order to stop receiving wrap NetworkQualityThroughputListener in @link https://codereview.chromium.org/1273173002/diff/480001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:163: * NetworkQualityThroughputListener is added in order to stop receiving "is added"->"is added with {@link #addThroughputListener}" https://codereview.chromium.org/1273173002/diff/480001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1273173002/diff/480001/net/base/network_quali... net/base/network_quality_estimator.h:169: bool allow_smaller_responses); Can we pass these in via the constructor? Then we can know that they aren't changed after some observations have been observed, so they don't have any crazy retro-active effects. https://codereview.chromium.org/1273173002/diff/480001/net/base/network_quali... File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1273173002/diff/480001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:959: } // namespace net Please add tests for the new NQE APIs.
https://codereview.chromium.org/1273173002/diff/480001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.h (right): https://codereview.chromium.org/1273173002/diff/480001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.h:32: class NetworkQualityEstimator; On 2015/10/05 14:49:40, pauljensen wrote: > this seems redundant with line 20 Done. https://codereview.chromium.org/1273173002/diff/480001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/NetworkQualityThroughputListener.java (right): https://codereview.chromium.org/1273173002/diff/480001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/NetworkQualityThroughputListener.java:14: * @param whenMs the time since the epoch in milliseconds. On 2015/10/05 14:49:40, pauljensen wrote: > "the epoch" is a little ambiguos, perhaps "the Epoch (January 1st 1970, > 00:00:00.000)" as Android does: > http://developer.android.com/reference/java/sql/Time.html#Time(long) Done. https://codereview.chromium.org/1273173002/diff/480001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/480001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:121: @Deprecated On 2015/10/05 14:49:40, pauljensen wrote: > @VisibleForTesting Done. https://codereview.chromium.org/1273173002/diff/480001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:122: abstract void enableNetworkQualityEstimator( On 2015/10/05 14:49:40, pauljensen wrote: > nit: might be good to add "ForTesting" suffix Done. https://codereview.chromium.org/1273173002/diff/480001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:128: * after {@link #enableNetworkQualityEstimator}. Round trip times may be On 2015/10/05 14:49:40, pauljensen wrote: > might be good to mention what happens if not called after enableNQE() (does it > throw?) Done. https://codereview.chromium.org/1273173002/diff/480001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:139: * Removes a listener of round trip times if on the listener list. This On 2015/10/05 14:49:40, pauljensen wrote: > "the listener list" is not a term a reader might be familiar with. How about > changing to "if previously registered with {@link #addRttListener}" Done. https://codereview.chromium.org/1273173002/diff/480001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:140: * should be called after a NetworkQualityRttListener is added in order to On 2015/10/05 14:49:40, pauljensen wrote: > wrap NQERttListener in @link Done. https://codereview.chromium.org/1273173002/diff/480001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:163: * NetworkQualityThroughputListener is added in order to stop receiving On 2015/10/05 14:49:40, pauljensen wrote: > "is added"->"is added with {@link #addThroughputListener}" Done. https://codereview.chromium.org/1273173002/diff/480001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:163: * NetworkQualityThroughputListener is added in order to stop receiving On 2015/10/05 14:49:40, pauljensen wrote: > wrap NetworkQualityThroughputListener in @link Done. https://codereview.chromium.org/1273173002/diff/480001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1273173002/diff/480001/net/base/network_quali... net/base/network_quality_estimator.h:169: bool allow_smaller_responses); On 2015/10/05 14:49:40, pauljensen wrote: > Can we pass these in via the constructor? Then we can know that they aren't > changed after some observations have been observed, so they don't have any crazy > retro-active effects. Done. https://codereview.chromium.org/1273173002/diff/480001/net/base/network_quali... File net/base/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/1273173002/diff/480001/net/base/network_quali... net/base/network_quality_estimator_unittest.cc:959: } // namespace net On 2015/10/05 14:49:40, pauljensen wrote: > Please add tests for the new NQE APIs. Done.
lgtm https://codereview.chromium.org/1273173002/diff/520001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/520001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:353: void postObservationTaskToExecutor(Runnable task) { Executor->NetworkQualityExecutor so as to differentiate from the same named function in CronetUrlRequest and to indicate which executor this is https://codereview.chromium.org/1273173002/diff/520001/net/base/network_quali... File net/base/network_quality_estimator.cc (right): https://codereview.chromium.org/1273173002/diff/520001/net/base/network_quali... net/base/network_quality_estimator.cc:862: NotifyObserversOfThroughput(througphput_observation); This is a little odd. We didn't really observe a new measurement, we just read the last estimate out of cache. I guess it's fine. https://codereview.chromium.org/1273173002/diff/520001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1273173002/diff/520001/net/base/network_quali... net/base/network_quality_estimator.h:47: // GENERATED_JAVA_PREFIX_TO_STRIP: I don't think you need to specify to strip nothing https://codereview.chromium.org/1273173002/diff/540001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1273173002/diff/540001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:203: if (should) nit: I think in net/ we generally use curly braces when else clause is present. Ditto for line 223. https://codereview.chromium.org/1273173002/diff/540001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1273173002/diff/540001/net/base/network_quali... net/base/network_quality_estimator.h:520: bool allow_localhost_requests_; I think you can add back const, ditto for line 525
https://codereview.chromium.org/1273173002/diff/540001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1273173002/diff/540001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:203: if (should) On 2015/10/06 19:10:29, pauljensen wrote: > nit: I think in net/ we generally use curly braces when else clause is present. > Ditto for line 223. Done, though I see it both ways in net/. Style indicates that curly braces are only needed when the a clause is more than one line. https://codereview.chromium.org/1273173002/diff/540001/net/base/network_quali... File net/base/network_quality_estimator.h (right): https://codereview.chromium.org/1273173002/diff/540001/net/base/network_quali... net/base/network_quality_estimator.h:520: bool allow_localhost_requests_; On 2015/10/06 19:10:29, pauljensen wrote: > I think you can add back const, ditto for line 525 Done.
The CQ bit was checked by bengr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org, pasko@chromium.org, mef@chromium.org, pauljensen@chromium.org Link to the patchset: https://codereview.chromium.org/1273173002/#ps560001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1273173002/560001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1273173002/560001
https://codereview.chromium.org/1273173002/diff/520001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/520001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:353: void postObservationTaskToExecutor(Runnable task) { On 2015/10/06 19:10:28, pauljensen wrote: > Executor->NetworkQualityExecutor > so as to differentiate from the same named function in CronetUrlRequest and to > indicate which executor this is You didn't address this.
The CQ bit was unchecked by bengr@chromium.org
https://codereview.chromium.org/1273173002/diff/520001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java (right): https://codereview.chromium.org/1273173002/diff/520001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:353: void postObservationTaskToExecutor(Runnable task) { On 2015/10/06 20:06:09, pauljensen wrote: > On 2015/10/06 19:10:28, pauljensen wrote: > > Executor->NetworkQualityExecutor > > so as to differentiate from the same named function in CronetUrlRequest and to > > indicate which executor this is > > You didn't address this. Done.
The CQ bit was checked by bengr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pauljensen@chromium.org, pasko@chromium.org, mef@chromium.org, tbansal@chromium.org Link to the patchset: https://codereview.chromium.org/1273173002/#ps580001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1273173002/580001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1273173002/580001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by bengr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1273173002/580001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1273173002/580001
Message was sent while issue was closed.
Committed patchset #27 (id:580001)
Message was sent while issue was closed.
Patchset 27 (id:??) landed as https://crrev.com/f1b6738e27e2e31e5f8da99f3367ccb3d3c2911f Cr-Commit-Position: refs/heads/master@{#352732} |
