Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1235373006:
Upstream changes for NetworkQualityProvider (Closed)
DescriptionUpstream changes for NetworkQualityProvider
Upstream changes for NetworkQualityProvider so that the
native code can call the APIs provided by operating system.
Downstream changes will come later for Chrome on Android.
BUG=472685
Committed: https://crrev.com/54e25ff788efd16ccf7dbf01899d9a34d8083712
Cr-Commit-Position: refs/heads/master@{#342395}
Patch Set 1 : More comments #
Total comments: 9
Patch Set 2 : Addressed comments, lots of assertions added #Patch Set 3 : Removed context #
Total comments: 20
Patch Set 4 : Addressed bengr comments #
Total comments: 19
Patch Set 5 : Addressed bwill comments, Also fetch new update on network change #
Total comments: 3
Patch Set 6 : javadoc style comments #
Total comments: 1
Patch Set 7 : Rebased #Patch Set 8 : Removed helper class #
Total comments: 10
Patch Set 9 : Addressed comments #Patch Set 10 : Minor change #Patch Set 11 : One more minor fix #
Messages
Total messages: 56 (17 generated)
Patchset #1 (id:1) has been deleted
tbansal@chromium.org changed reviewers: + bengr@chromium.org
bengr@: PTAL before I send it to OWNERS?
tbansal@chromium.org changed reviewers: + nyquist@chromium.org
nyquist@chromium.org: Please review changes in chrome/android/* and chrome/browser/*
https://codereview.chromium.org/1235373006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java (right): https://codereview.chromium.org/1235373006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:16: public static NetworkQualityProvider getInstance() { The public methods of this class needs comments. https://codereview.chromium.org/1235373006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:17: if (sInstance == null) sInstance = new NetworkQualityProvider(); This doesn't seem threadsafe. Could you document the threading model? Also, possibly use ThreadUtils or something to add asserts? https://codereview.chromium.org/1235373006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java (right): https://codereview.chromium.org/1235373006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java:22: public static void setContext(Context context) { The public methods of this class needs comments. https://codereview.chromium.org/1235373006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java:29: public static NetworkQualityProviderHelper create() { Could this just take in the context with base::android::GetApplicationContext() instead of having it as a static?
ptal. Thanks :) https://codereview.chromium.org/1235373006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java (right): https://codereview.chromium.org/1235373006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:16: public static NetworkQualityProvider getInstance() { On 2015/07/16 22:06:37, nyquist wrote: > The public methods of this class needs comments. Done. https://codereview.chromium.org/1235373006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:17: if (sInstance == null) sInstance = new NetworkQualityProvider(); On 2015/07/16 22:06:37, nyquist wrote: > This doesn't seem threadsafe. Could you document the threading model? > Also, possibly use ThreadUtils or something to add asserts? Done. https://codereview.chromium.org/1235373006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java (right): https://codereview.chromium.org/1235373006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java:22: public static void setContext(Context context) { On 2015/07/16 22:06:37, nyquist wrote: > The public methods of this class needs comments. Done. https://codereview.chromium.org/1235373006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java:29: public static NetworkQualityProviderHelper create() { On 2015/07/16 22:06:37, nyquist wrote: > Could this just take in the context with base::android::GetApplicationContext() > instead of having it as a static? Not sure why but that crashes Chrome (when I include my downstream changes that calls Google services APIs). I will send out the downstream changes for review later, may be we can discuss then?
https://codereview.chromium.org/1235373006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java (right): https://codereview.chromium.org/1235373006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java:29: public static NetworkQualityProviderHelper create() { On 2015/07/17 01:42:02, tbansal1 wrote: > On 2015/07/16 22:06:37, nyquist wrote: > > Could this just take in the context with > base::android::GetApplicationContext() > > instead of having it as a static? > > Not sure why but that crashes Chrome (when I include my downstream > changes that calls Google services APIs). I will send out the downstream > changes for review later, may be we can discuss then? Sure.
On 2015/07/17 03:53:46, nyquist wrote: > https://codereview.chromium.org/1235373006/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java > (right): > > https://codereview.chromium.org/1235373006/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java:29: > public static NetworkQualityProviderHelper create() { > On 2015/07/17 01:42:02, tbansal1 wrote: > > On 2015/07/16 22:06:37, nyquist wrote: > > > Could this just take in the context with > > base::android::GetApplicationContext() > > > instead of having it as a static? > > > > Not sure why but that crashes Chrome (when I include my downstream > > changes that calls Google services APIs). I will send out the downstream > > changes for review later, may be we can discuss then? > > Sure. After looking at the downstream codebase I'm still not sure why you can't just use the application context.
nyquist@: ptal. The extra setContext() is now gone :)
https://codereview.chromium.org/1235373006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java (right): https://codereview.chromium.org/1235373006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:590: * @return NetworkQualityProvider. How about: @return A provider of network quality. https://codereview.chromium.org/1235373006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java (right): https://codereview.chromium.org/1235373006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:23: if (sThreadCheck == null) sThreadCheck = new NonThreadSafe(); Add braces https://codereview.chromium.org/1235373006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:25: if (sInstance == null) sInstance = new NetworkQualityProvider(); Add braces https://codereview.chromium.org/1235373006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:29: /** The comment indentation is busted on all of your comments. It should look like this: /** * Creates... */ https://codereview.chromium.org/1235373006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java (right): https://codereview.chromium.org/1235373006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java:36: if (sContext == null) return false; Braces, on every if statement. https://codereview.chromium.org/1235373006/diff/60001/chrome/browser/android/... File chrome/browser/android/net/network_quality_provider.cc (right): https://codereview.chromium.org/1235373006/diff/60001/chrome/browser/android/... chrome/browser/android/net/network_quality_provider.cc:3: // found in the LICENSE file. Add a blank line. https://codereview.chromium.org/1235373006/diff/60001/chrome/browser/android/... File chrome/browser/android/net/network_quality_provider.h (right): https://codereview.chromium.org/1235373006/diff/60001/chrome/browser/android/... chrome/browser/android/net/network_quality_provider.h:3: // found in the LICENSE file. Add a blank line. https://codereview.chromium.org/1235373006/diff/60001/chrome/browser/android/... chrome/browser/android/net/network_quality_provider.h:32: // Returns 0 if the estimate is unavailable. Why not return -1. 0 is a valid estimate, no? https://codereview.chromium.org/1235373006/diff/60001/chrome/browser/android/... chrome/browser/android/net/network_quality_provider.h:36: // Returns 0 if the estimate is unavailable. Same here. https://codereview.chromium.org/1235373006/diff/60001/chrome/browser/android/... chrome/browser/android/net/network_quality_provider.h:40: // Return value should be discarded if the estimate is unavailable. You could assert that IsEstimateAvailable is called, or you could do: bool GetTimeSinceLastUpdate(base::TimeDelta* time_delta); and return false if unavailable. Same for all the other methods.
bengr@, nyquist@: ptal. Thank you. https://codereview.chromium.org/1235373006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java (right): https://codereview.chromium.org/1235373006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:590: * @return NetworkQualityProvider. On 2015/07/21 00:06:47, bengr wrote: > How about: > @return A provider of network quality. Done. https://codereview.chromium.org/1235373006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java (right): https://codereview.chromium.org/1235373006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:23: if (sThreadCheck == null) sThreadCheck = new NonThreadSafe(); On 2015/07/21 00:06:48, bengr wrote: > Add braces Umm, I may be incorrect but without braces shows 697 matches: https://code.google.com/p/chromium/codesearch#search/&q=f:java$%20if%5C%20.*%... With braces shows 9 matches (all of which seem to be third_party): https://code.google.com/p/chromium/codesearch#search/&q=f:java$%20if%5C%20%5C... Also, AOSP open source java guidelines (which Chromium also follows) are here: http://source.android.com/source/code-style.html#use-standard-brace-style It says: "We require braces around the statements for a conditional. Except, if the entire conditional (the condition and the body) fit on one line, you may (but are not obligated to) put it all on one line.". Seems like both formats are OK. So, may be go with one that is more popular :) wdyt? https://codereview.chromium.org/1235373006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:25: if (sInstance == null) sInstance = new NetworkQualityProvider(); On 2015/07/21 00:06:48, bengr wrote: > Add braces Please see above. https://codereview.chromium.org/1235373006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:29: /** On 2015/07/21 00:06:48, bengr wrote: > The comment indentation is busted on all of your comments. It should look like > this: > > /** > * Creates... > */ Done. https://codereview.chromium.org/1235373006/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java (right): https://codereview.chromium.org/1235373006/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java:36: if (sContext == null) return false; On 2015/07/21 00:06:48, bengr wrote: > Braces, on every if statement. Please see the reply on the other file. https://codereview.chromium.org/1235373006/diff/60001/chrome/browser/android/... File chrome/browser/android/net/network_quality_provider.cc (right): https://codereview.chromium.org/1235373006/diff/60001/chrome/browser/android/... chrome/browser/android/net/network_quality_provider.cc:3: // found in the LICENSE file. On 2015/07/21 00:06:48, bengr wrote: > Add a blank line. Done. https://codereview.chromium.org/1235373006/diff/60001/chrome/browser/android/... File chrome/browser/android/net/network_quality_provider.h (right): https://codereview.chromium.org/1235373006/diff/60001/chrome/browser/android/... chrome/browser/android/net/network_quality_provider.h:3: // found in the LICENSE file. On 2015/07/21 00:06:48, bengr wrote: > Add a blank line. Done. https://codereview.chromium.org/1235373006/diff/60001/chrome/browser/android/... chrome/browser/android/net/network_quality_provider.h:32: // Returns 0 if the estimate is unavailable. On 2015/07/21 00:06:48, bengr wrote: > Why not return -1. 0 is a valid estimate, no? good point, rewrote. https://codereview.chromium.org/1235373006/diff/60001/chrome/browser/android/... chrome/browser/android/net/network_quality_provider.h:36: // Returns 0 if the estimate is unavailable. On 2015/07/21 00:06:48, bengr wrote: > Same here. Done. https://codereview.chromium.org/1235373006/diff/60001/chrome/browser/android/... chrome/browser/android/net/network_quality_provider.h:40: // Return value should be discarded if the estimate is unavailable. On 2015/07/21 00:06:48, bengr wrote: > You could assert that IsEstimateAvailable is called, or you could do: > > bool GetTimeSinceLastUpdate(base::TimeDelta* time_delta); > > and return false if unavailable. Same for all the other methods. Done.
https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java (right): https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:439: getNetworkQualityProvider().initialize(ChromeApplication.this); Why |ChromeApplication.|? https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:593: return NetworkQualityProvider.getInstance(); Could this instead just create the object and pass in the constructor? If so, you probably want to rename this to createNetworkQualityProvider(). https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java (right): https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:44: public void initialize(Context context) { Why not pass this context in to the constructor instead? https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:91: private void assertCorrectness() { static? https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java (right): https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java:17: public class NetworkQualityProviderHelper { This class can not be instantiated externally, and all methods are private. So make class final? https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java:24: private static NetworkQualityProviderHelper create(Context context) { Why is this class here instead of just using the NetworkQualityProvider? Could you maybe move this method to the NetworkQualityProvider instead? You could pass in the context as an argument still, and use that context to create the instance? https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java:38: final NetworkQualityProvider networkQualityProvider = Why not store this reference in the constructor instead? https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java:48: final NetworkQualityProvider networkQualityProvider = Nit: Unnecessary final throughout this class. https://codereview.chromium.org/1235373006/diff/80001/chrome/browser/android/... File chrome/browser/android/net/network_quality_provider.h (right): https://codereview.chromium.org/1235373006/diff/80001/chrome/browser/android/... chrome/browser/android/net/network_quality_provider.h:47: int32_t no_value_ = -1; Should this be initialized here? Seems like you do this in the constructor instead?
ptal. thanks. Got rid of some of the functions. https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java (right): https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:439: getNetworkQualityProvider().initialize(ChromeApplication.this); On 2015/07/22 14:46:59, nyquist wrote: > Why |ChromeApplication.|? I don't think it matters. I was just being consistent with previous line. Obsolete for now. https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeApplication.java:593: return NetworkQualityProvider.getInstance(); On 2015/07/22 14:46:59, nyquist wrote: > Could this instead just create the object and pass in the constructor? > > If so, you probably want to rename this to createNetworkQualityProvider(). Done. https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java (right): https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:44: public void initialize(Context context) { On 2015/07/22 14:46:59, nyquist wrote: > Why not pass this context in to the constructor instead? Done. https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:91: private void assertCorrectness() { On 2015/07/22 14:46:59, nyquist wrote: > static? Obsolete. https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java (right): https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java:17: public class NetworkQualityProviderHelper { On 2015/07/22 14:47:00, nyquist wrote: > This class can not be instantiated externally, and all methods are private. So > make class final? Done. https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java:24: private static NetworkQualityProviderHelper create(Context context) { On 2015/07/22 14:47:00, nyquist wrote: > Why is this class here instead of just using the NetworkQualityProvider? Could > you maybe move this method to the NetworkQualityProvider instead? > You could pass in the context as an argument still, and use that context to > create the instance? OK, seems like my understanding is incorrect. I thought this extra helper class is needed because it will automatically either call the NetworkQualityProvider.java or the overriding class from the downstream code (please see the other CL for Chrome on Android) by appropriately creating the right provider when createNetworkQualityProvider() is called. My understanding is that only if there was no need to override NetworkQualityProvider in the downstream code, then this class was redundant and could be removed. Is that not correct? If not, then who calls createNetworkQualityProvider()? https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java:38: final NetworkQualityProvider networkQualityProvider = On 2015/07/22 14:47:00, nyquist wrote: > Why not store this reference in the constructor instead? Good idea, Done. https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java:48: final NetworkQualityProvider networkQualityProvider = On 2015/07/22 14:47:00, nyquist wrote: > Nit: Unnecessary final throughout this class. Done. https://codereview.chromium.org/1235373006/diff/80001/chrome/browser/android/... File chrome/browser/android/net/network_quality_provider.h (right): https://codereview.chromium.org/1235373006/diff/80001/chrome/browser/android/... chrome/browser/android/net/network_quality_provider.h:47: int32_t no_value_ = -1; On 2015/07/22 14:47:00, nyquist wrote: > Should this be initialized here? Seems like you do this in the constructor > instead? I do it there because I need |env| to initialize it.
https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java (right): https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java:24: private static NetworkQualityProviderHelper create(Context context) { On 2015/07/22 21:11:30, tbansal1 wrote: > On 2015/07/22 14:47:00, nyquist wrote: > > Why is this class here instead of just using the NetworkQualityProvider? Could > > you maybe move this method to the NetworkQualityProvider instead? > > You could pass in the context as an argument still, and use that context to > > create the instance? > > OK, seems like my understanding is incorrect. I thought this extra helper class > is needed because it will automatically either call the > NetworkQualityProvider.java or the overriding class > from the downstream code (please see the other CL for Chrome on Android) > by appropriately creating the right provider when > createNetworkQualityProvider() is called. > > My understanding is that only if there was no need > to override NetworkQualityProvider in the downstream code, > then this class was redundant and could be removed. > > Is that not correct? If not, then who calls > createNetworkQualityProvider()? I don't know what your plan was for who should own this. Is there a natural owner for the instance? You could of course keep it as a singleton, and lazily create it with a getInstance() style method (and use getFoo...() in the context) whenever you need it. I do believe normal polymorphism works regardless of JNI, so that should be fine. I don't think you need this helper class.
On 2015/07/22 21:42:44, nyquist wrote: > https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java > (right): > > https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java:24: > private static NetworkQualityProviderHelper create(Context context) { > On 2015/07/22 21:11:30, tbansal1 wrote: > > On 2015/07/22 14:47:00, nyquist wrote: > > > Why is this class here instead of just using the NetworkQualityProvider? > Could > > > you maybe move this method to the NetworkQualityProvider instead? > > > You could pass in the context as an argument still, and use that context to > > > create the instance? > > > > OK, seems like my understanding is incorrect. I thought this extra helper > class > > is needed because it will automatically either call the > > NetworkQualityProvider.java or the overriding class > > from the downstream code (please see the other CL for Chrome on Android) > > by appropriately creating the right provider when > > createNetworkQualityProvider() is called. > > > > My understanding is that only if there was no need > > to override NetworkQualityProvider in the downstream code, > > then this class was redundant and could be removed. > > > > Is that not correct? If not, then who calls > > createNetworkQualityProvider()? > > I don't know what your plan was for who should own this. Is there a natural > owner for the instance? network_quality_provider.h is the owner of the Java ref to NetworkQualityProviderHelper. And, NetworkQualityProviderHelper is the owner of NetworkQualityProvider object as well as of the overriding downstream object. > > You could of course keep it as a singleton, and lazily create it with a > getInstance() style method (and use getFoo...() in the context) whenever you > need it. > > I do believe normal polymorphism works regardless of JNI, so that should be > fine. I don't think you need this helper class. Is there an example? I think I followed existing code and saw this pattern but may be I missed something. Thanks.
On 2015/07/22 21:54:05, tbansal1 wrote: > On 2015/07/22 21:42:44, nyquist wrote: > > > https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java > > (right): > > > > > https://codereview.chromium.org/1235373006/diff/80001/chrome/android/java/src... > > > chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java:24: > > private static NetworkQualityProviderHelper create(Context context) { > > On 2015/07/22 21:11:30, tbansal1 wrote: > > > On 2015/07/22 14:47:00, nyquist wrote: > > > > Why is this class here instead of just using the NetworkQualityProvider? > > Could > > > > you maybe move this method to the NetworkQualityProvider instead? > > > > You could pass in the context as an argument still, and use that context > to > > > > create the instance? > > > > > > OK, seems like my understanding is incorrect. I thought this extra helper > > class > > > is needed because it will automatically either call the > > > NetworkQualityProvider.java or the overriding class > > > from the downstream code (please see the other CL for Chrome on Android) > > > by appropriately creating the right provider when > > > createNetworkQualityProvider() is called. > > > > > > My understanding is that only if there was no need > > > to override NetworkQualityProvider in the downstream code, > > > then this class was redundant and could be removed. > > > > > > Is that not correct? If not, then who calls > > > createNetworkQualityProvider()? > > > > I don't know what your plan was for who should own this. Is there a natural > > owner for the instance? > > network_quality_provider.h is the owner of the Java ref to > NetworkQualityProviderHelper. > And, NetworkQualityProviderHelper is the owner of NetworkQualityProvider object > as well as of the overriding downstream object. > > > > > You could of course keep it as a singleton, and lazily create it with a > > getInstance() style method (and use getFoo...() in the context) whenever you > > need it. > > > > I do believe normal polymorphism works regardless of JNI, so that should be > > fine. I don't think you need this helper class. > Is there an example? I think I followed existing code and saw this pattern > but may be I missed something. Thanks. I am assuming that if I get rid of Helper.java, then I would need to do something different in the network_quality_provider.cc, so that it calls the right Provider from upstream or downstream version. I am not sure how I can do it from the native code.
bwill@google.com changed reviewers: + bwill@google.com
https://codereview.chromium.org/1235373006/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java (right): https://codereview.chromium.org/1235373006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:41: * Returns 0 if the estimate is unavailable. Comments say you'll get 0 if unavailable, but the methods return NO_VALUE (-1).
https://codereview.chromium.org/1235373006/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java (right): https://codereview.chromium.org/1235373006/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:49: * Returns the expected downstream throughput in Kbps. Using the term "bps" has brought me nothing but pain; I personally have sworn to use KBytesSec/KBitsSec for the rest of my days. If you're not inclined to change the method name, maybe expand the term in the accompanying javadoc?
Patchset #5 (id:100001) has been deleted
PTAL. Addressed bwill comments. Also, added changes to make NetworkQualityProvider an observer to network change events. New Estimates are automatically requested when a network change event is detected. (Sorry one intermediate patchset got deleted).
https://codereview.chromium.org/1235373006/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java (right): https://codereview.chromium.org/1235373006/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:21: protected static final int NO_VALUE = -1; One other thought - NO_VALUE is part of the external interface, right? So it seems this should be public.
https://codereview.chromium.org/1235373006/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java (right): https://codereview.chromium.org/1235373006/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:21: protected static final int NO_VALUE = -1; On 2015/07/24 23:36:56, bwill wrote: > One other thought - NO_VALUE is part of the external interface, right? So it > seems this should be public. The only public interface will be exposed by chrome/browser/android/net/network_quality_provider.h In that class, I am using function definitions of form: bool GetMetric(Metric* metric) where GetMetric returns true if the value is available (and updates metric), otherwise it returns false. IIUC, this is the standard way for Chromium native code. In chrome/browser/android/net/network_quality_provider.cc in the implementation of GetMetric(...), Java code is called and return value is compared with |no_value|. If they are equal, false is returned.
https://codereview.chromium.org/1235373006/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java (right): https://codereview.chromium.org/1235373006/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:21: protected static final int NO_VALUE = -1; On 2015/07/24 23:43:16, tbansal1 wrote: > On 2015/07/24 23:36:56, bwill wrote: > > One other thought - NO_VALUE is part of the external interface, right? So it > > seems this should be public. > > The only public interface will be exposed by > chrome/browser/android/net/network_quality_provider.h > In that class, I am using function definitions of form: > bool GetMetric(Metric* metric) > where GetMetric returns true if the value is available (and updates metric), > otherwise it returns false. > IIUC, this is the standard way for Chromium native code. > > In chrome/browser/android/net/network_quality_provider.cc in the implementation > of GetMetric(...), Java code > is called and return value is compared with |no_value|. > If they are equal, false is returned. Done.
lgtm
On 2015/07/24 23:43:32, tbansal1 wrote: > https://codereview.chromium.org/1235373006/diff/120001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java > (right): > > https://codereview.chromium.org/1235373006/diff/120001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:21: > protected static final int NO_VALUE = -1; > On 2015/07/24 23:43:16, tbansal1 wrote: > > On 2015/07/24 23:36:56, bwill wrote: > > > One other thought - NO_VALUE is part of the external interface, right? So > it > > > seems this should be public. > > > > The only public interface will be exposed by > > chrome/browser/android/net/network_quality_provider.h > > In that class, I am using function definitions of form: > > bool GetMetric(Metric* metric) > > where GetMetric returns true if the value is available (and updates metric), > > otherwise it returns false. > > IIUC, this is the standard way for Chromium native code. > > > > In chrome/browser/android/net/network_quality_provider.cc in the > implementation > > of GetMetric(...), Java code > > is called and return value is compared with |no_value|. > > If they are equal, false is returned. > > Done. Re: public interface - understand and sorry for the trouble :-) LGTM, too.
nyquist@: PTAL. Thanks.
nyquist: Friendly ping :)
On 2015/07/31 16:50:23, tbansal1 wrote: > nyquist: Friendly ping :) nyquist@: Ping. PTAL. Thanks
Feel free to ping me on chat tomorrow if you want to discuss my proposal with a quick turnaround time :-) https://codereview.chromium.org/1235373006/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java (right): https://codereview.chromium.org/1235373006/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProviderHelper.java:25: private static NetworkQualityProviderHelper create(Context context) { Could this instead live in NetworkQualityProvider? You could have a create-method or get-method there that would use the application context to create it. You can even have a static member if you want it to be a singleton. I'm still not sure why you would want to have this helper class just to call a method on the other object. If you override the context downstream, you can ensure that downstream creates a different instance of the NetworkQualityProvider (the downstream class that just extends NetworkQualityProvider).
Patchset #8 (id:180001) has been deleted
Removed the helper class! nyquist@: PTAL. Thanks.
https://codereview.chromium.org/1235373006/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java (right): https://codereview.chromium.org/1235373006/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:25: private static final Object sLock = new Object(); Nit: Could you put this final static member above the non-final one? It helps clarifying what can be changed and not. In fact, could you reorder the sThreadCheck as well? so: ... static final .. ... static ... In practice something like this ordering: NO_VALUE, sLock, sThreadCheck, sNetworkQualityProvider And on the super-duper-nitty-and-optional-side (we are not consistent in chromium codebase), feel free to rename sLock to LOCK, since it's something in between a singleton and a constant :-p But that's seriously up to you. I feel stronger about the ordering. https://codereview.chromium.org/1235373006/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:44: public NetworkQualityProvider(Context context) { I'm assuming the downstream code needs the context, which is why it's passed here, but not used? Or is it just something old? If it's only necessary downstream, you could in fact just make it part of the constructor of the subclass instead. So all-in-all, remove |context| parameter? https://codereview.chromium.org/1235373006/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:49: * Returns true if the network quality estimate is available. How do you feel about using "@return ..." form for these comments? By the way, if you do that, I don't think you also have to have a "method-level comment" in addition to the @return line. I don't feel strongly though. https://codereview.chromium.org/1235373006/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:107: return NetworkQualityProvider.NO_VALUE; Just return NO_VALUE; https://codereview.chromium.org/1235373006/diff/200001/chrome/browser/android... File chrome/browser/android/net/network_quality_provider.cc (right): https://codereview.chromium.org/1235373006/diff/200001/chrome/browser/android... chrome/browser/android/net/network_quality_provider.cc:12: NetworkQualityProvider::NetworkQualityProvider() { Does this also need to be called on a valid thread?
Patchset #9 (id:220001) has been deleted
PTAL. https://codereview.chromium.org/1235373006/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java (right): https://codereview.chromium.org/1235373006/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:25: private static final Object sLock = new Object(); On 2015/08/06 23:26:28, nyquist wrote: > Nit: Could you put this final static member above the non-final one? It helps > clarifying what can be changed and not. > > In fact, could you reorder the sThreadCheck as well? > > so: > ... static final .. > ... static ... > > In practice something like this ordering: > NO_VALUE, sLock, sThreadCheck, sNetworkQualityProvider Done. > > And on the super-duper-nitty-and-optional-side (we are not consistent in > chromium codebase), feel free to rename sLock to LOCK, since it's something in > between a singleton and a constant :-p But that's seriously up to you. I feel > stronger about the ordering. Done. Going with the majority: sVarName is used 31 times. https://code.google.com/p/chromium/codesearch#search/&q=f:java$%20static%5C%2... VAR_NAME is used 53 times. https://code.google.com/p/chromium/codesearch#search/&q=f:java$%20static%5C%2... https://codereview.chromium.org/1235373006/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:44: public NetworkQualityProvider(Context context) { On 2015/08/06 23:26:28, nyquist wrote: > I'm assuming the downstream code needs the context, which is why it's passed > here, but not used? Or is it just something old? > > If it's only necessary downstream, you could in fact just make it part of the > constructor of the subclass instead. > > So all-in-all, remove |context| parameter? It is needed downstream. Done. https://codereview.chromium.org/1235373006/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:49: * Returns true if the network quality estimate is available. On 2015/08/06 23:26:28, nyquist wrote: > How do you feel about using "@return ..." form for these comments? By the way, > if you do that, I don't think you also have to have a "method-level comment" in > addition to the @return line. > > I don't feel strongly though. Done. https://codereview.chromium.org/1235373006/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/net/qualityprovider/NetworkQualityProvider.java:107: return NetworkQualityProvider.NO_VALUE; On 2015/08/06 23:26:27, nyquist wrote: > Just return NO_VALUE; Done. https://codereview.chromium.org/1235373006/diff/200001/chrome/browser/android... File chrome/browser/android/net/network_quality_provider.cc (right): https://codereview.chromium.org/1235373006/diff/200001/chrome/browser/android... chrome/browser/android/net/network_quality_provider.cc:12: NetworkQualityProvider::NetworkQualityProvider() { On 2015/08/06 23:26:28, nyquist wrote: > Does this also need to be called on a valid thread? No, |thread_checker_| is constructed in the constructor here. During constructor, it records the thread on which it was constructed. In other methods below, using CalledOnValidThread(), we are essentially checking if we are on the same thread as the constructor was called on. Doing DCHECK(..CalledOnValidThread()) in the constructor won't be useful because it would be checking if we are on the same thread as the one that called the constructor. This is always going to be true.
lgtm
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org, bwill@google.com Link to the patchset: https://codereview.chromium.org/1235373006/#ps240001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1235373006/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1235373006/240001
The CQ bit was unchecked by tbansal@chromium.org
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org, bengr@chromium.org, bwill@google.com Link to the patchset: https://codereview.chromium.org/1235373006/#ps260001 (title: "Minor change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1235373006/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1235373006/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org, bengr@chromium.org, bwill@google.com Link to the patchset: https://codereview.chromium.org/1235373006/#ps280001 (title: "One more minor fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1235373006/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1235373006/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by tbansal@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1235373006/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1235373006/280001
Message was sent while issue was closed.
Committed patchset #11 (id:280001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/54e25ff788efd16ccf7dbf01899d9a34d8083712 Cr-Commit-Position: refs/heads/master@{#342395} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
