Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1879743002:
Expose cellular signal strength on Android to native (Closed)
DescriptionExpose cellular signal strength on Android to native
This will be later used by NetworkQualityEstimator when
computing estimated effective connection type.
BUG=513681
Committed: https://crrev.com/c04b7aa2dff9fb89c666b1f1063e6a184049895f
Cr-Commit-Position: refs/heads/master@{#403608}
Patch Set 1 : Rebased #
Total comments: 17
Patch Set 2 : Rebased #Patch Set 3 : Addressed pauljensen comments #
Total comments: 14
Patch Set 4 : Addressed Paul's comments #
Total comments: 43
Patch Set 5 : Rebased #Patch Set 6 : Addressed bengr, paul comments #Patch Set 7 : Rebased #Patch Set 8 : Return error if there are multiple SIMs #
Total comments: 4
Patch Set 9 : Addressed Paul's comments #
Total comments: 8
Patch Set 10 : Some rebasing, Addressed bengr, pauljensen comments #
Total comments: 4
Patch Set 11 : pauljensen's comments #
Total comments: 2
Patch Set 12 : Added missing param #
Total comments: 18
Patch Set 13 : Paul's comments #
Total comments: 4
Patch Set 14 : Rebased #Patch Set 15 : Addressed Paul's comments #Patch Set 16 : Rebased #Patch Set 17 : Test #
Messages
Total messages: 63 (27 generated)
Description was changed from ========== w w w w w BUG=513681 ========== to ========== Expose cellular signal strength on Android to native BUG=513681 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
tbansal@chromium.org changed reviewers: + pauljensen@chromium.org
pauljensen: PTAL at *. Thanks!!
Description was changed from ========== Expose cellular signal strength on Android to native BUG=513681 ========== to ========== Expose cellular signal strength on Android to native This will be later used by NetworkQualityEstimator when computing estimated effective connection type. BUG=513681 ==========
Patchset #3 (id:80001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:40001) has been deleted
pauljensen: ping. Thanks!
https://codereview.chromium.org/1879743002/diff/100001/net/android/cellular_s... File net/android/cellular_signal_strength.h (right): https://codereview.chromium.org/1879743002/diff/100001/net/android/cellular_s... net/android/cellular_signal_strength.h:22: NET_EXPORT bool GetRssiDbm(int64_t* rssi) WARN_UNUSED_RESULT; why int64_t? why not int32_t? https://codereview.chromium.org/1879743002/diff/100001/net/android/cellular_s... net/android/cellular_signal_strength.h:27: NET_EXPORT bool GetSignalLevel(int64_t* signal_level) WARN_UNUSED_RESULT; why not just int? https://codereview.chromium.org/1879743002/diff/100001/net/android/java/src/o... File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java (right): https://codereview.chromium.org/1879743002/diff/100001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:35: private static boolean sAPIAvailable = final https://codereview.chromium.org/1879743002/diff/100001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:48: if (!sAPIAvailable) { how about moving lines 48-60 into a function? it can return an empty list if !sAPIAvailable or telephonyManager == null or cellInfos == null. Then this function is just: for (CellInfo cellInfo : getCellInfos(context)) ... return invalidValue; https://codereview.chromium.org/1879743002/diff/100001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:77: private static long getSignalLevel(Context context, long invalidValue) { why not return an int? https://codereview.chromium.org/1879743002/diff/100001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:105: private static long getRssiDbm(CellInfo cellInfo, long invalidValue) { how about returning an int? and you could remove invalidValue and just return Integer.MAX_VALUE for invalid values. https://codereview.chromium.org/1879743002/diff/100001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:129: private static long getSignalLevel(CellInfo cellInfo, long invalidValue) { why not an int? I think all the getLevel functions return 0-4 https://codereview.chromium.org/1879743002/diff/100001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:129: private static long getSignalLevel(CellInfo cellInfo, long invalidValue) { instead of invalidValue, you could pre-define some invalid value. if you want the value shared between native and Java you could use GENERATED_JAVA_ENUM_PACKAGE
pauljensen: ptal. Thanks. https://codereview.chromium.org/1879743002/diff/100001/net/android/cellular_s... File net/android/cellular_signal_strength.h (right): https://codereview.chromium.org/1879743002/diff/100001/net/android/cellular_s... net/android/cellular_signal_strength.h:22: NET_EXPORT bool GetRssiDbm(int64_t* rssi) WARN_UNUSED_RESULT; On 2016/04/22 11:54:12, pauljensen wrote: > why int64_t? why not int32_t? Changed to int*. Done. https://codereview.chromium.org/1879743002/diff/100001/net/android/cellular_s... net/android/cellular_signal_strength.h:27: NET_EXPORT bool GetSignalLevel(int64_t* signal_level) WARN_UNUSED_RESULT; On 2016/04/22 11:54:12, pauljensen wrote: > why not just int? Done. https://codereview.chromium.org/1879743002/diff/100001/net/android/java/src/o... File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java (right): https://codereview.chromium.org/1879743002/diff/100001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:35: private static boolean sAPIAvailable = On 2016/04/22 11:54:13, pauljensen wrote: > final Done. https://codereview.chromium.org/1879743002/diff/100001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:48: if (!sAPIAvailable) { On 2016/04/22 11:54:12, pauljensen wrote: > how about moving lines 48-60 into a function? it can return an empty list if > !sAPIAvailable or telephonyManager == null or cellInfos == null. Then this > function is just: > > for (CellInfo cellInfo : getCellInfos(context)) > ... > return invalidValue; Done. https://codereview.chromium.org/1879743002/diff/100001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:77: private static long getSignalLevel(Context context, long invalidValue) { On 2016/04/22 11:54:12, pauljensen wrote: > why not return an int? Done. https://codereview.chromium.org/1879743002/diff/100001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:105: private static long getRssiDbm(CellInfo cellInfo, long invalidValue) { On 2016/04/22 11:54:12, pauljensen wrote: > how about returning an int? and you could remove invalidValue and just return > Integer.MAX_VALUE for invalid values. Done. https://codereview.chromium.org/1879743002/diff/100001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:129: private static long getSignalLevel(CellInfo cellInfo, long invalidValue) { On 2016/04/22 11:54:13, pauljensen wrote: > why not an int? I think all the getLevel functions return 0-4 Done. https://codereview.chromium.org/1879743002/diff/100001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:129: private static long getSignalLevel(CellInfo cellInfo, long invalidValue) { On 2016/04/22 11:54:13, pauljensen wrote: > instead of invalidValue, you could pre-define some invalid value. if you want > the value shared between native and Java you could use > GENERATED_JAVA_ENUM_PACKAGE Done.
pauljensen: Ping. Thanks.
does calling this API work on Marshmallow devices or does it need modification for run-time permissions? ala https://developer.android.com/training/permissions/requesting.html https://codereview.chromium.org/1879743002/diff/100001/net/android/cellular_s... File net/android/cellular_signal_strength.h (right): https://codereview.chromium.org/1879743002/diff/100001/net/android/cellular_s... net/android/cellular_signal_strength.h:22: NET_EXPORT bool GetRssiDbm(int64_t* rssi) WARN_UNUSED_RESULT; On 2016/04/28 20:12:02, tbansal1 wrote: > On 2016/04/22 11:54:12, pauljensen wrote: > > why int64_t? why not int32_t? > > Changed to int*. Done. can we change to int32_t so it's more defined? https://codereview.chromium.org/1879743002/diff/140001/net/android/cellular_s... File net/android/cellular_signal_strength.cc (right): https://codereview.chromium.org/1879743002/diff/140001/net/android/cellular_s... net/android/cellular_signal_strength.cc:22: ERROR_NOT_SUPPORTED = -2147483647 - 1, why the subtraction? https://codereview.chromium.org/1879743002/diff/140001/net/android/cellular_s... net/android/cellular_signal_strength.cc:28: "CellularSignalStrengthError.ERROR_NOT_SUPPORTED has unexpected value"); can we move this assert up a scope level to right below ERROR_NOT_SUPPORTED's definition? https://codereview.chromium.org/1879743002/diff/140001/net/android/cellular_s... File net/android/cellular_signal_strength.h (right): https://codereview.chromium.org/1879743002/diff/140001/net/android/cellular_s... net/android/cellular_signal_strength.h:20: // connection is available, and sets |rssi| to that value. |rssi|->|*rssi| https://codereview.chromium.org/1879743002/diff/140001/net/android/cellular_s... net/android/cellular_signal_strength.h:23: // Returns true if the signal level (between 0 and 4, both inclusive) of the mention which of 0 or 4 is the highest and lowest https://codereview.chromium.org/1879743002/diff/140001/net/android/cellular_s... net/android/cellular_signal_strength.h:25: // |signal_level| to that value. |signal_level|->|*signal_level| https://codereview.chromium.org/1879743002/diff/140001/net/android/cellular_s... File net/android/cellular_signal_strength_unittest.cc (right): https://codereview.chromium.org/1879743002/diff/140001/net/android/cellular_s... net/android/cellular_signal_strength_unittest.cc:17: TEST(CellularSignalStrengthAndroidTest, DISABLED_RssiTest) { why are these tests disabled? https://codereview.chromium.org/1879743002/diff/140001/net/android/net_jni_re... File net/android/net_jni_registrar.cc (right): https://codereview.chromium.org/1879743002/diff/140001/net/android/net_jni_re... net/android/net_jni_registrar.cc:31: {"AndroidCellularSignalStrength", cellular_signal_strength::Register}, this looks alphabetized, can we move this up so it remains so?
Patchset #4 (id:160001) has been deleted
Patchset #4 (id:180001) has been deleted
Patchset #5 (id:220001) has been deleted
Patchset #5 (id:240001) has been deleted
Patchset #4 (id:200001) has been deleted
pauljensen: ptal I had to made changes to nqe in the same CL because of compiler warning about unused functions. I also rebased (accidntally in the same patch) but the rebase only affects the .gyp and .gn files. So, hopefully it does not make it too difficult to review. Thanks. https://codereview.chromium.org/1879743002/diff/140001/net/android/cellular_s... File net/android/cellular_signal_strength.cc (right): https://codereview.chromium.org/1879743002/diff/140001/net/android/cellular_s... net/android/cellular_signal_strength.cc:22: ERROR_NOT_SUPPORTED = -2147483647 - 1, On 2016/05/23 12:25:10, pauljensen wrote: > why the subtraction? This is INT32_MIN. I just set it directly to -2147483648. https://codereview.chromium.org/1879743002/diff/140001/net/android/cellular_s... net/android/cellular_signal_strength.cc:28: "CellularSignalStrengthError.ERROR_NOT_SUPPORTED has unexpected value"); On 2016/05/23 12:25:10, pauljensen wrote: > can we move this assert up a scope level to right below ERROR_NOT_SUPPORTED's > definition? Done. https://codereview.chromium.org/1879743002/diff/140001/net/android/cellular_s... File net/android/cellular_signal_strength.h (right): https://codereview.chromium.org/1879743002/diff/140001/net/android/cellular_s... net/android/cellular_signal_strength.h:20: // connection is available, and sets |rssi| to that value. On 2016/05/23 12:25:10, pauljensen wrote: > |rssi|->|*rssi| Done. https://codereview.chromium.org/1879743002/diff/140001/net/android/cellular_s... net/android/cellular_signal_strength.h:23: // Returns true if the signal level (between 0 and 4, both inclusive) of the On 2016/05/23 12:25:10, pauljensen wrote: > mention which of 0 or 4 is the highest and lowest Done. https://codereview.chromium.org/1879743002/diff/140001/net/android/cellular_s... net/android/cellular_signal_strength.h:25: // |signal_level| to that value. On 2016/05/23 12:25:10, pauljensen wrote: > |signal_level|->|*signal_level| Done. https://codereview.chromium.org/1879743002/diff/140001/net/android/cellular_s... File net/android/cellular_signal_strength_unittest.cc (right): https://codereview.chromium.org/1879743002/diff/140001/net/android/cellular_s... net/android/cellular_signal_strength_unittest.cc:17: TEST(CellularSignalStrengthAndroidTest, DISABLED_RssiTest) { On 2016/05/23 12:25:10, pauljensen wrote: > why are these tests disabled? The test device needs to have an active cellular connection for these tests to run. https://codereview.chromium.org/1879743002/diff/140001/net/android/net_jni_re... File net/android/net_jni_registrar.cc (right): https://codereview.chromium.org/1879743002/diff/140001/net/android/net_jni_re... net/android/net_jni_registrar.cc:31: {"AndroidCellularSignalStrength", cellular_signal_strength::Register}, On 2016/05/23 12:25:10, pauljensen wrote: > this looks alphabetized, can we move this up so it remains so? Done.
Patchset #4 (id:260001) has been deleted
tbansal@chromium.org changed reviewers: + bengr@chromium.org
bengr: ptal. Thanks.
https://codereview.chromium.org/1879743002/diff/280001/net/android/cellular_s... File net/android/cellular_signal_strength_unittest.cc (right): https://codereview.chromium.org/1879743002/diff/280001/net/android/cellular_s... net/android/cellular_signal_strength_unittest.cc:21: return; if the device isn't using cellular, can we at least call your api to make sure it doesn't crash? https://codereview.chromium.org/1879743002/diff/280001/net/android/cellular_s... net/android/cellular_signal_strength_unittest.cc:35: return; ditto https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java (right): https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:39: * permission from the user when querying for cellular signal strength. You could probably get this info from TelephonyManager.getNeighboringCellInfo() in earlier versions of Android if you wanted to cover those versions. https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:44: || ContextUtils.getApplicationContext().checkSelfPermission( I don't know a whole lot about Java static initializer order, but how do you know the context has been set this early? https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:122: return signalStrength.getDbm(); can we get rid of these signalStrength variables? they appear to only be used once. https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:123: } else if (cellInfo instanceof CellInfoGsm) { all these "else"'s aren't needed because you return from each one. https://codereview.chromium.org/1879743002/diff/280001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1879743002/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:34720: + right before a connection change event. Recorded only on cellular "Recorded only on cellular connections" is misleading as it is recorded for devices on WiFi that happen to also have cellular connections, how about "Recorded only on Android devices with cellular connections"
https://codereview.chromium.org/1879743002/diff/280001/net/android/cellular_s... File net/android/cellular_signal_strength.cc (right): https://codereview.chromium.org/1879743002/diff/280001/net/android/cellular_s... net/android/cellular_signal_strength.cc:20: ERROR_NOT_SUPPORTED = -2147483648, Why not just use INT32_MIN? Add a comment that doing so is incompatible with the Java generation magic. You can add that comment just before the static_assert. https://codereview.chromium.org/1879743002/diff/280001/net/android/cellular_s... net/android/cellular_signal_strength.cc:28: int32_t rssi_tmp = Java_AndroidCellularSignalStrength_getRssiDbm( It might be better to avoid using the term RSSI and instead use SignalStrengthDbm. https://codereview.chromium.org/1879743002/diff/280001/net/android/cellular_s... net/android/cellular_signal_strength.cc:34: *rssi = rssi_tmp; I don't like _tmp, but can't think of anything better. Maybe: bool GetSignalStrengthDbm(int43_t* rssi_to_return) { int32_t rssi = whatever(); ... *rssi_to_return = rssi; return true; } https://codereview.chromium.org/1879743002/diff/280001/net/android/cellular_s... File net/android/cellular_signal_strength.h (right): https://codereview.chromium.org/1879743002/diff/280001/net/android/cellular_s... net/android/cellular_signal_strength.h:22: NET_EXPORT bool GetRssiDbm(int32_t* rssi) WARN_UNUSED_RESULT; Does this return false when in airplane mode or when there's no SIM? https://codereview.chromium.org/1879743002/diff/280001/net/android/cellular_s... File net/android/cellular_signal_strength_unittest.cc (right): https://codereview.chromium.org/1879743002/diff/280001/net/android/cellular_s... net/android/cellular_signal_strength_unittest.cc:23: int rssi = INT32_MIN; Shouldn't this be set to CellularSignalStrengthError.ERROR_NOT_SUPPORTED? https://codereview.chromium.org/1879743002/diff/280001/net/android/cellular_s... net/android/cellular_signal_strength_unittest.cc:27: EXPECT_GE(0, rssi); If you change rssi to signalStrengthDbm elsewhere, you should do it here to, and everywhere. https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java (right): https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:30: * This class interacts with the cellular signal strength API provided by Android. You mean specifically the SignalStrength API, right? Refer to that. https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:48: private AndroidCellularSignalStrength() {} Are all methods private because this should only be instantiated and used from native? Consider commenting on this somewhere. https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:57: private static int getRssiDbm(Context context) { Maybe rename this as getSignalStrengthDbm https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:59: if (cellInfos == null) { If getCellInfo returned an empty list instead of null, you wouldn't need this check. Alternatively, you could just create a function called getRegisteredCellInfo() that returns one CellInfo or null. https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:65: return getRssiDbm(cellInfo); I don't understand. Is at most one cellInfo registered? https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:80: private static int getSignalLevel(Context context) { Naming suggestion: getSignalStrengthLevel https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:82: if (cellInfos == null) { See comment on line 59.
Patchset #5 (id:300001) has been deleted
Patchset #6 (id:340001) has been deleted
Patchset #6 (id:360001) has been deleted
Patchset #6 (id:380001) has been deleted
ptal. Thanks. https://codereview.chromium.org/1879743002/diff/280001/net/android/cellular_s... File net/android/cellular_signal_strength.cc (right): https://codereview.chromium.org/1879743002/diff/280001/net/android/cellular_s... net/android/cellular_signal_strength.cc:20: ERROR_NOT_SUPPORTED = -2147483648, On 2016/06/10 20:57:37, bengr wrote: > Why not just use INT32_MIN? Add a comment that doing so is incompatible with the > Java generation magic. You can add that comment just before the static_assert. Done. https://codereview.chromium.org/1879743002/diff/280001/net/android/cellular_s... net/android/cellular_signal_strength.cc:28: int32_t rssi_tmp = Java_AndroidCellularSignalStrength_getRssiDbm( On 2016/06/10 20:57:37, bengr wrote: > It might be better to avoid using the term RSSI and instead use > SignalStrengthDbm. Done. https://codereview.chromium.org/1879743002/diff/280001/net/android/cellular_s... net/android/cellular_signal_strength.cc:34: *rssi = rssi_tmp; On 2016/06/10 20:57:37, bengr wrote: > I don't like _tmp, but can't think of anything better. Maybe: > > bool GetSignalStrengthDbm(int43_t* rssi_to_return) { > int32_t rssi = whatever(); > ... > *rssi_to_return = rssi; > return true; > } I would prefer not to put |rssi_to_return| in the function header which appears in the .h and .cc file. https://codereview.chromium.org/1879743002/diff/280001/net/android/cellular_s... File net/android/cellular_signal_strength.h (right): https://codereview.chromium.org/1879743002/diff/280001/net/android/cellular_s... net/android/cellular_signal_strength.h:22: NET_EXPORT bool GetRssiDbm(int32_t* rssi) WARN_UNUSED_RESULT; On 2016/06/10 20:57:37, bengr wrote: > Does this return false when in airplane mode or when there's no SIM? It returns false in both cases. https://codereview.chromium.org/1879743002/diff/280001/net/android/cellular_s... File net/android/cellular_signal_strength_unittest.cc (right): https://codereview.chromium.org/1879743002/diff/280001/net/android/cellular_s... net/android/cellular_signal_strength_unittest.cc:21: return; On 2016/06/10 13:52:15, pauljensen wrote: > if the device isn't using cellular, can we at least call your api to make sure > it doesn't crash? Done. https://codereview.chromium.org/1879743002/diff/280001/net/android/cellular_s... net/android/cellular_signal_strength_unittest.cc:23: int rssi = INT32_MIN; On 2016/06/10 20:57:37, bengr wrote: > Shouldn't this be set to CellularSignalStrengthError.ERROR_NOT_SUPPORTED? Done. https://codereview.chromium.org/1879743002/diff/280001/net/android/cellular_s... net/android/cellular_signal_strength_unittest.cc:27: EXPECT_GE(0, rssi); On 2016/06/10 20:57:37, bengr wrote: > If you change rssi to signalStrengthDbm elsewhere, you should do it here to, and > everywhere. Done. https://codereview.chromium.org/1879743002/diff/280001/net/android/cellular_s... net/android/cellular_signal_strength_unittest.cc:35: return; On 2016/06/10 13:52:15, pauljensen wrote: > ditto Done. https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java (right): https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:30: * This class interacts with the cellular signal strength API provided by Android. On 2016/06/10 20:57:37, bengr wrote: > You mean specifically the SignalStrength API, right? Refer to that. Done. https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:39: * permission from the user when querying for cellular signal strength. On 2016/06/10 13:52:15, pauljensen wrote: > You could probably get this info from TelephonyManager.getNeighboringCellInfo() > in earlier versions of Android if you wanted to cover those versions. Added a TODO. https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:44: || ContextUtils.getApplicationContext().checkSelfPermission( On 2016/06/10 13:52:15, pauljensen wrote: > I don't know a whole lot about Java static initializer order, but how do you > know the context has been set this early? Java static code is initialized in the order of when it is accessed. ApplicationContext should be initialized much before anyone access this class. https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... I also added a null check to avoid crash. https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:48: private AndroidCellularSignalStrength() {} On 2016/06/10 20:57:37, bengr wrote: > Are all methods private because this should only be instantiated and used from > native? Consider commenting on this somewhere. Changed two of them to public since it is okay to access them from Java code. https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:57: private static int getRssiDbm(Context context) { On 2016/06/10 20:57:37, bengr wrote: > Maybe rename this as getSignalStrengthDbm Done. https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:59: if (cellInfos == null) { On 2016/06/10 20:57:37, bengr wrote: > If getCellInfo returned an empty list instead of null, you wouldn't need this > check. Alternatively, you could just create a function called > getRegisteredCellInfo() that returns one CellInfo or null. Done. https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:65: return getRssiDbm(cellInfo); On 2016/06/10 20:57:37, bengr wrote: > I don't understand. Is at most one cellInfo registered? There could be multiple radios, and in that case there would be multiple cellInfos. Added comment. https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:80: private static int getSignalLevel(Context context) { On 2016/06/10 20:57:37, bengr wrote: > Naming suggestion: getSignalStrengthLevel Done. https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:82: if (cellInfos == null) { On 2016/06/10 20:57:37, bengr wrote: > See comment on line 59. Done. https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:122: return signalStrength.getDbm(); On 2016/06/10 13:52:15, pauljensen wrote: > can we get rid of these signalStrength variables? they appear to only be used > once. Done. https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:123: } else if (cellInfo instanceof CellInfoGsm) { On 2016/06/10 13:52:15, pauljensen wrote: > all these "else"'s aren't needed because you return from each one. Done. https://codereview.chromium.org/1879743002/diff/280001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1879743002/diff/280001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:34720: + right before a connection change event. Recorded only on cellular On 2016/06/10 13:52:15, pauljensen wrote: > "Recorded only on cellular connections" is misleading as it is recorded for > devices on WiFi that happen to also have cellular connections, how about > "Recorded only on Android devices with cellular connections" Right now, it is recorded only on cellular connection. In nqe.cc which records this UMA, there is a check if (NetworkChangeNotifier::IsConnectionCellular(current_network_id_.type)) {
https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java (right): https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:65: return getRssiDbm(cellInfo); On 2016/06/16 00:58:09, tbansal1 wrote: > On 2016/06/10 20:57:37, bengr wrote: > > I don't understand. Is at most one cellInfo registered? > > There could be multiple radios, and in that case there would be multiple > cellInfos. Added comment. If there are multiple radios, which cellinfo do you return?
https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java (right): https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:65: return getRssiDbm(cellInfo); On 2016/06/20 20:07:26, bengr wrote: > On 2016/06/16 00:58:09, tbansal1 wrote: > > On 2016/06/10 20:57:37, bengr wrote: > > > I don't understand. Is at most one cellInfo registered? > > > > There could be multiple radios, and in that case there would be multiple > > cellInfos. Added comment. > > If there are multiple radios, which cellinfo do you return? I return the first one, which depends on what Android returns. The Android API does not specify the order in which it will return cellInfos. So, Chromium API can't guarantee anything stronger. The other option could be to return ERROR when there are multiple radios. I did add the comment "May return incorrect result if there are multiple cellular radios on the device" on the API.
https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java (right): https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:65: return getRssiDbm(cellInfo); On 2016/06/20 21:32:03, tbansal1 wrote: > On 2016/06/20 20:07:26, bengr wrote: > > On 2016/06/16 00:58:09, tbansal1 wrote: > > > On 2016/06/10 20:57:37, bengr wrote: > > > > I don't understand. Is at most one cellInfo registered? > > > > > > There could be multiple radios, and in that case there would be multiple > > > cellInfos. Added comment. > > > > If there are multiple radios, which cellinfo do you return? > > I return the first one, which depends on what Android returns. The Android API > does not specify the order in which it will return cellInfos. So, Chromium API > can't guarantee anything stronger. > > The other option could be to return ERROR when there are multiple radios. I did > add the comment "May return incorrect result if there are multiple cellular > radios on the device" on the API. I don't like this interface because the rssi (or signal level in the next method) might have nothing to do with the connection used. Returning an error seems safer, but I don't know how much coverage we lose. Another possibility is to get the rssi/signal level for a particular connection. That makes more sense to me but probably introduces a lot more plumbing.
On 2016/06/22 16:54:07, bengr wrote: > https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... > File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java > (right): > > https://codereview.chromium.org/1879743002/diff/280001/net/android/java/src/o... > net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:65: > return getRssiDbm(cellInfo); > On 2016/06/20 21:32:03, tbansal1 wrote: > > On 2016/06/20 20:07:26, bengr wrote: > > > On 2016/06/16 00:58:09, tbansal1 wrote: > > > > On 2016/06/10 20:57:37, bengr wrote: > > > > > I don't understand. Is at most one cellInfo registered? > > > > > > > > There could be multiple radios, and in that case there would be multiple > > > > cellInfos. Added comment. > > > > > > If there are multiple radios, which cellinfo do you return? > > > > I return the first one, which depends on what Android returns. The Android API > > does not specify the order in which it will return cellInfos. So, Chromium API > > can't guarantee anything stronger. > > > > The other option could be to return ERROR when there are multiple radios. I > did > > add the comment "May return incorrect result if there are multiple cellular > > radios on the device" on the API. > > I don't like this interface because the rssi (or signal level in the next > method) might have nothing to do with the connection used. Returning an error > seems safer, but I don't know how much coverage we lose. Another possibility is > to get the rssi/signal level for a particular connection. That makes more sense > to me but probably introduces a lot more plumbing. I will change it to return error in that case. I do not anticipate significant loss in coverage.
Patchset #8 (id:440001) has been deleted
Patchset #8 (id:460001) has been deleted
bengr, pauljensen: Ptal. Thanks.
https://codereview.chromium.org/1879743002/diff/480001/net/android/java/src/o... File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java (right): https://codereview.chromium.org/1879743002/diff/480001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:84: return new ArrayList<CellInfo>(); in cases where the API is unavailable, can we return something that doesn't require an allocation? like null perhaps. I'd prefer to not run the garbage collector needlessly. https://codereview.chromium.org/1879743002/diff/480001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:91: List<CellInfo> cellInfos = telephonyManager.getAllCellInfo(); This requires ACCESS_COARSE_LOCATION permission. I assume WebView and potentially other Cronet embedders don't have this permission. So is this going to break Cronet embedders?
ptal. Thanks. https://codereview.chromium.org/1879743002/diff/480001/net/android/java/src/o... File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java (right): https://codereview.chromium.org/1879743002/diff/480001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:84: return new ArrayList<CellInfo>(); On 2016/06/23 13:03:40, pauljensen wrote: > in cases where the API is unavailable, can we return something that doesn't > require an allocation? like null perhaps. I'd prefer to not run the garbage > collector needlessly. Done. https://codereview.chromium.org/1879743002/diff/480001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:91: List<CellInfo> cellInfos = telephonyManager.getAllCellInfo(); On 2016/06/23 13:03:40, pauljensen wrote: > This requires ACCESS_COARSE_LOCATION permission. I assume WebView and > potentially other Cronet embedders don't have this permission. So is this going > to break Cronet embedders? Updated sAPIAvailable above. The updated check should work for embedders too. Thanks for pointing this out.
https://codereview.chromium.org/1879743002/diff/500001/net/android/java/src/o... File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java (right): https://codereview.chromium.org/1879743002/diff/500001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:43: && ContextUtils.getApplicationContext() != null The null-check here indicates a silent failure condition which isn't good. Considering all the public APIs here include a Context, why not have a lazily initialized variable instead that uses the passed in Context to calculate whether the permissions are available? https://codereview.chromium.org/1879743002/diff/500001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:91: if (telephonyManager == null) return null; I think Chrome style is not to put if-statement clauses on the same line as the if-condition. Please make this consistent with line 86.
lgtm with a few nits. https://codereview.chromium.org/1879743002/diff/500001/net/android/cellular_s... File net/android/cellular_signal_strength_unittest.cc (right): https://codereview.chromium.org/1879743002/diff/500001/net/android/cellular_s... net/android/cellular_signal_strength_unittest.cc:40: // Signal strength is likely unavailable if the device does not have an Why "likely?" Is the statement true if you remove the work "likely?" https://codereview.chromium.org/1879743002/diff/500001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/1879743002/diff/500001/net/nqe/network_qualit... net/nqe/network_quality_estimator.h:469: // Obtains the current cellular RSSI value and updates I would avoid using the term RSSI.
pauljensen: ptal. Thanks. https://codereview.chromium.org/1879743002/diff/500001/net/android/cellular_s... File net/android/cellular_signal_strength_unittest.cc (right): https://codereview.chromium.org/1879743002/diff/500001/net/android/cellular_s... net/android/cellular_signal_strength_unittest.cc:40: // Signal strength is likely unavailable if the device does not have an On 2016/06/27 01:36:22, bengr wrote: > Why "likely?" Is the statement true if you remove the work "likely?" I removed it. I am not sure what is the documented behavior of Android when the data connection is not active. https://codereview.chromium.org/1879743002/diff/500001/net/android/java/src/o... File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java (right): https://codereview.chromium.org/1879743002/diff/500001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:43: && ContextUtils.getApplicationContext() != null On 2016/06/24 11:53:14, pauljensen wrote: > The null-check here indicates a silent failure condition which isn't good. > Considering all the public APIs here include a Context, why not have a lazily > initialized variable instead that uses the passed in Context to calculate > whether the permissions are available? Done. https://codereview.chromium.org/1879743002/diff/500001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:91: if (telephonyManager == null) return null; On 2016/06/24 11:53:14, pauljensen wrote: > I think Chrome style is not to put if-statement clauses on the same line as the > if-condition. Please make this consistent with line 86. Done. https://codereview.chromium.org/1879743002/diff/500001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/1879743002/diff/500001/net/nqe/network_qualit... net/nqe/network_quality_estimator.h:469: // Obtains the current cellular RSSI value and updates On 2016/06/27 01:36:22, bengr wrote: > I would avoid using the term RSSI. Done.
Please upload separate rebase/sync patch sets and patch sets that address comments, otherwise it's very hard to see the diff for the comments alone. https://codereview.chromium.org/1879743002/diff/520001/net/android/java/src/o... File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java (right): https://codereview.chromium.org/1879743002/diff/520001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:34: UNKNOWN, UNKNOWN->UNSET https://codereview.chromium.org/1879743002/diff/520001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:98: if (sAPIAvailable == ApiAvailable.UNKNOWN) { can you move this if-statement into updateAPIAvailability()? and also rename that function to getApiAvailable() and make it return an ApiAvailable? and add a comment to sAPIAvailable that says don't use it directly instead call getApiAvailable()?
https://codereview.chromium.org/1879743002/diff/520001/net/android/java/src/o... File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java (right): https://codereview.chromium.org/1879743002/diff/520001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:34: UNKNOWN, On 2016/06/28 12:30:14, pauljensen wrote: > UNKNOWN->UNSET Obsolete. https://codereview.chromium.org/1879743002/diff/520001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:98: if (sAPIAvailable == ApiAvailable.UNKNOWN) { On 2016/06/28 12:30:14, pauljensen wrote: > can you move this if-statement into updateAPIAvailability()? and also rename > that function to getApiAvailable() and make it return an ApiAvailable? and add a > comment to sAPIAvailable that says don't use it directly instead call > getApiAvailable()? I just changed it to recompute api availability each time. I think that is needed to keep this class thread safe. Other option is to use final static boolean variable (as I was doing before).
ptal. thanks.
https://codereview.chromium.org/1879743002/diff/540001/net/android/java/src/o... File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java (right): https://codereview.chromium.org/1879743002/diff/540001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:84: if (!isAPIAvailable()) { I don't think this builds.
pauljensen: ptal. Thanks. https://codereview.chromium.org/1879743002/diff/540001/net/android/java/src/o... File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java (right): https://codereview.chromium.org/1879743002/diff/540001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:84: if (!isAPIAvailable()) { On 2016/06/29 11:35:57, pauljensen wrote: > I don't think this builds. Done.
https://codereview.chromium.org/1879743002/diff/560001/net/android/java/src/o... File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java (right): https://codereview.chromium.org/1879743002/diff/560001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:33: * {@link CellularSignalStrengthError.ERROR_NOT_SUPPORTED} if the signal strength is change "." to "#", ditto for line 47 https://codereview.chromium.org/1879743002/diff/560001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:38: public static int getSignalStrengthDbm(Context context) { I'd make this private, ditto for line 54 https://codereview.chromium.org/1879743002/diff/560001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:49: * device. might want to mention higher is stronger https://codereview.chromium.org/1879743002/diff/560001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:63: * getAllCellInfo is only available on API Level JELLY_BEAN_MR1 and higher. getAllCellInfo->{@link TelephonyManager#getAllCellInfo} https://codereview.chromium.org/1879743002/diff/560001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:63: * getAllCellInfo is only available on API Level JELLY_BEAN_MR1 and higher. @link JELLY_BEAN_MR1 https://codereview.chromium.org/1879743002/diff/560001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:66: * permission from the user when querying for cellular signal strength. are there circumstances where other parts of Chrome would ask the user for this permission at run-time? or are we never going to have this permission on Marshmallow? https://codereview.chromium.org/1879743002/diff/560001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:80: * mobile network. May return null. null->{@code null} https://codereview.chromium.org/1879743002/diff/560001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:102: registeredCellInfos.add(cellInfo); rather than creating a new list, why don't you just remove from the original list? https://codereview.chromium.org/1879743002/diff/560001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:112: @SuppressLint("NewApi") Rather than using SuppressLint, I think it's preferred to use @TargetApi(Build.VERSION_CODES.JELLY_BEAN_MR1). Ditto for other functions in this file.
pauljensen: ptal. Thanks. https://codereview.chromium.org/1879743002/diff/560001/net/android/java/src/o... File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java (right): https://codereview.chromium.org/1879743002/diff/560001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:33: * {@link CellularSignalStrengthError.ERROR_NOT_SUPPORTED} if the signal strength is On 2016/06/30 17:54:20, pauljensen wrote: > change "." to "#", ditto for line 47 Done. https://codereview.chromium.org/1879743002/diff/560001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:38: public static int getSignalStrengthDbm(Context context) { On 2016/06/30 17:54:20, pauljensen wrote: > I'd make this private, ditto for line 54 So, making this private would make it inaccessible to other classes in Java. Is there a need to add that restriction (this is thread safe, so presumably we do not care who calls it)? https://codereview.chromium.org/1879743002/diff/560001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:49: * device. On 2016/06/30 17:54:20, pauljensen wrote: > might want to mention higher is stronger Done. https://codereview.chromium.org/1879743002/diff/560001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:63: * getAllCellInfo is only available on API Level JELLY_BEAN_MR1 and higher. On 2016/06/30 17:54:20, pauljensen wrote: > @link JELLY_BEAN_MR1 Done. https://codereview.chromium.org/1879743002/diff/560001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:63: * getAllCellInfo is only available on API Level JELLY_BEAN_MR1 and higher. On 2016/06/30 17:54:21, pauljensen wrote: > getAllCellInfo->{@link TelephonyManager#getAllCellInfo} Done. https://codereview.chromium.org/1879743002/diff/560001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:66: * permission from the user when querying for cellular signal strength. On 2016/06/30 17:54:20, pauljensen wrote: > are there circumstances where other parts of Chrome would ask the user for this > permission at run-time? or are we never going to have this permission on > Marshmallow? I am not sure of other parts of Chrome, but a lot of websites (e.g., google.com, and other store websites when searching for nearest store location) ask for location information, and if the user approves on any website, Chromium (obviously) gets this permission too. https://codereview.chromium.org/1879743002/diff/560001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:80: * mobile network. May return null. On 2016/06/30 17:54:21, pauljensen wrote: > null->{@code null} Done. https://codereview.chromium.org/1879743002/diff/560001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:102: registeredCellInfos.add(cellInfo); On 2016/06/30 17:54:21, pauljensen wrote: > rather than creating a new list, why don't you just remove from the original > list? Done. https://codereview.chromium.org/1879743002/diff/560001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:112: @SuppressLint("NewApi") On 2016/06/30 17:54:21, pauljensen wrote: > Rather than using SuppressLint, I think it's preferred to use > @TargetApi(Build.VERSION_CODES.JELLY_BEAN_MR1). Ditto for other functions in > this file. Done.
lgtm https://codereview.chromium.org/1879743002/diff/570001/net/android/java/src/o... File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java (right): https://codereview.chromium.org/1879743002/diff/570001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:63: * {@link JELLY_BEAN_MR1} and higher. Also verifies that appropriate permissions are already JELLY_BEAN_MR1->Build.VERSION_CODES#JELLY_BEAN_MR1 https://codereview.chromium.org/1879743002/diff/570001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:64: * available. This ensures that on Android M and higher, Chromium would not request run-time would->will
https://codereview.chromium.org/1879743002/diff/570001/net/android/java/src/o... File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java (right): https://codereview.chromium.org/1879743002/diff/570001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:63: * {@link JELLY_BEAN_MR1} and higher. Also verifies that appropriate permissions are already On 2016/07/01 18:50:43, pauljensen wrote: > JELLY_BEAN_MR1->Build.VERSION_CODES#JELLY_BEAN_MR1 Done. https://codereview.chromium.org/1879743002/diff/570001/net/android/java/src/o... net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:64: * available. This ensures that on Android M and higher, Chromium would not request run-time On 2016/07/01 18:50:43, pauljensen wrote: > would->will Done.
tbansal@chromium.org changed reviewers: + isherman@chromium.org
isherman: PTAL at histograms.xml. Thanks.
metrics lgtm
Patchset #17 (id:650001) has been deleted
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, pauljensen@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1879743002/#ps670001 (title: "Test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Expose cellular signal strength on Android to native This will be later used by NetworkQualityEstimator when computing estimated effective connection type. BUG=513681 ========== to ========== Expose cellular signal strength on Android to native This will be later used by NetworkQualityEstimator when computing estimated effective connection type. BUG=513681 ==========
Message was sent while issue was closed.
Committed patchset #17 (id:670001)
Message was sent while issue was closed.
Description was changed from ========== Expose cellular signal strength on Android to native This will be later used by NetworkQualityEstimator when computing estimated effective connection type. BUG=513681 ========== to ========== Expose cellular signal strength on Android to native This will be later used by NetworkQualityEstimator when computing estimated effective connection type. BUG=513681 Committed: https://crrev.com/c04b7aa2dff9fb89c666b1f1063e6a184049895f Cr-Commit-Position: refs/heads/master@{#403608} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/c04b7aa2dff9fb89c666b1f1063e6a184049895f Cr-Commit-Position: refs/heads/master@{#403608} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
