Use Android callback API to obtain cellular signal strength

Use Android callback API to obtain cellular signal strength

Move cellular signal strength methods to a class. Also, use the 
PhoneStateListener to listen to the changes in the signal strength.

BUG=703740
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2763853002
Cr-Commit-Position: refs/heads/master@{#482981}
Committed: https://chromium.googlesource.com/chromium/src/+/de702d52aea0e1daae4a827d0eb3fe3d4d477b68

[tbansal1, 2017-03-21 01:20:38]

Description was changed from

==========
Use callback API to obtain signal strength

BUG=
==========

to

==========
Use callback API to obtain signal strength

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[tbansal1, 2017-03-21 01:22:14]

Description was changed from

==========
Use callback API to obtain signal strength

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Use callback API to obtain cellular signal strength

Move cellular signal strength methods to a class. Also, use the 
PhoneStateListener to listen to the changes in the signal strength.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[tbansal1, 2017-03-21 01:42:57]

Patchset #1 (id:1) has been deleted

[tbansal1, 2017-03-21 01:43:03]

Patchset #1 (id:20001) has been deleted

[tbansal1, 2017-03-21 01:47:06]

Patchset #1 (id:40001) has been deleted

[tbansal1, 2017-03-21 01:47:32]

Patchset #1 (id:60001) has been deleted

[tbansal1, 2017-03-21 01:47:39]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 01:48:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 03:16:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 03:16:26]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[tbansal1, 2017-03-21 05:35:39]

Patchset #2 (id:100001) has been deleted

[tbansal1, 2017-03-21 05:35:46]

Patchset #1 (id:80001) has been deleted

[tbansal1, 2017-03-21 05:35:52]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 05:36:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 06:51:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 06:51:04]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[tbansal1, 2017-03-21 06:57:54]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 06:58:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 07:57:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 07:57:38]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-03-21 16:50:03]

Patchset #1 (id:120001) has been deleted

[tbansal1, 2017-03-21 18:18:26]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 18:18:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbansal1, 2017-03-21 18:24:41]

Description was changed from

==========
Use callback API to obtain cellular signal strength

Move cellular signal strength methods to a class. Also, use the 
PhoneStateListener to listen to the changes in the signal strength.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Use callback API to obtain cellular signal strength

Move cellular signal strength methods to a class. Also, use the 
PhoneStateListener to listen to the changes in the signal strength.

BUG=703740
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[tbansal1, 2017-03-21 18:25:22]

Description was changed from

==========
Use callback API to obtain cellular signal strength

Move cellular signal strength methods to a class. Also, use the 
PhoneStateListener to listen to the changes in the signal strength.

BUG=703740
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Use Android callback API to obtain cellular signal strength

Move cellular signal strength methods to a class. Also, use the 
PhoneStateListener to listen to the changes in the signal strength.

BUG=703740
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[commit-bot: I haz the power, 2017-03-21 19:27:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 19:27:54]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-03-21 19:42:32]

Patchset #1 (id:140001) has been deleted

[tbansal1, 2017-03-21 19:43:00]

tbansal@chromium.org changed reviewers:
+ ryansturm@chromium.org

[tbansal1, 2017-03-21 19:43:01]

ryansturm: ptal at //net/nqe/* or everything. Thanks.

[RyanSturm, 2017-03-22 21:51:08]

Should NQE really own the android cellular object? Might be better to be owned
by IOThread Globals.

Aside from that:
lgtm % nits

https://codereview.chromium.org/2763853002/diff/160001/net/android/cellular_s...
File net/android/cellular_signal_strength.h (right):

https://codereview.chromium.org/2763853002/diff/160001/net/android/cellular_s...
net/android/cellular_signal_strength.h:31: bool GetSignalStrengthLevel(int32_t*
signal_strength_level) const
nit: Could be a base::optional.

https://codereview.chromium.org/2763853002/diff/160001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator.cc (right):

https://codereview.chromium.org/2763853002/diff/160001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.cc:882: signal_strength_ = INT32_MIN;
Is this the right bound (would 0 and 6 work for min and max now?)

https://codereview.chromium.org/2763853002/diff/160001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2763853002/diff/160001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:42271: +  <obsolete>
replace by NQE.CellularSignalStrength.LevelAvailable

https://codereview.chromium.org/2763853002/diff/160001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:42285: +    Obsoleted in March 2017.
Replaced by NQE.CellularSignalStrength.LevelDifference

[tbansal1, 2017-03-23 01:27:52]

Patchset #2 (id:180001) has been deleted

[tbansal1, 2017-03-23 01:43:14]

Patchset #2 (id:200001) has been deleted

[tbansal1, 2017-03-23 01:43:38]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 01:44:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 02:08:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 02:08:23]

Dry run: Try jobs failed on following builders:
  android_cronet_tester on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[tbansal1, 2017-03-23 02:09:03]

Patchset #2 (id:220001) has been deleted

[tbansal1, 2017-03-23 02:09:08]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 02:09:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 02:30:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 02:30:42]

Dry run: Try jobs failed on following builders:
  android_cronet_tester on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[tbansal1, 2017-03-23 05:14:55]

Patchset #2 (id:240001) has been deleted

[tbansal1, 2017-03-23 05:15:01]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 05:15:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 05:30:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 05:30:41]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[tbansal1, 2017-03-23 18:28:53]

tbansal@chromium.org changed reviewers:
+ pauljensen@chromium.org

[tbansal1, 2017-03-23 18:28:54]

pauljensen: ptal at //net/android. Thanks!!

https://codereview.chromium.org/2763853002/diff/160001/net/android/cellular_s...
File net/android/cellular_signal_strength.h (right):

https://codereview.chromium.org/2763853002/diff/160001/net/android/cellular_s...
net/android/cellular_signal_strength.h:31: bool GetSignalStrengthLevel(int32_t*
signal_strength_level) const
On 2017/03/22 21:51:08, Ryan Sturm wrote:
> nit: Could be a base::optional.

Done.

https://codereview.chromium.org/2763853002/diff/160001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator.cc (right):

https://codereview.chromium.org/2763853002/diff/160001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.cc:882: signal_strength_ = INT32_MIN;
On 2017/03/22 21:51:08, Ryan Sturm wrote:
> Is this the right bound (would 0 and 6 work for min and max now?)

I have changed these to base::Optional.

https://codereview.chromium.org/2763853002/diff/160001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2763853002/diff/160001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:42271: +  <obsolete>
On 2017/03/22 21:51:08, Ryan Sturm wrote:
> replace by NQE.CellularSignalStrength.LevelAvailable

Done.

https://codereview.chromium.org/2763853002/diff/160001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:42285: +    Obsoleted in March 2017.
On 2017/03/22 21:51:08, Ryan Sturm wrote:
> Replaced by NQE.CellularSignalStrength.LevelDifference

Done.

[tbansal1, 2017-03-28 20:24:20]

pauljensen: ping. Thanks.

[pauljensen, 2017-04-13 11:36:30]

I'm a little concerned that you're adding more code that requires to run on the
UI thread.  In Cronet we're removing all UI thread initialization:
https://codereview.chromium.org/2812963002/

https://codereview.chromium.org/2763853002/diff/260001/net/android/cellular_s...
File net/android/cellular_signal_strength.cc (right):

https://codereview.chromium.org/2763853002/diff/260001/net/android/cellular_s...
net/android/cellular_signal_strength.cc:34:
base::android::GetApplicationContext()));
does this need to be called on the UI thread? if so please add
DCHECK(base::MessageLoopForUI::IsCurrent());

https://codereview.chromium.org/2763853002/diff/260001/net/android/cellular_s...
File net/android/cellular_signal_strength.h (right):

https://codereview.chromium.org/2763853002/diff/260001/net/android/cellular_s...
net/android/cellular_signal_strength.h:21: class NET_EXPORT_PRIVATE
CellularSignalStrength {
needs comments

https://codereview.chromium.org/2763853002/diff/260001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java
(right):

https://codereview.chromium.org/2763853002/diff/260001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:47: if
(telephonyManager.getSimState() != TelephonyManager.SIM_STATE_READY) return;
Do these calls require any permissions?

https://codereview.chromium.org/2763853002/diff/260001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:97: *
{@link   android.telephony.SignalStrength#getLevel} is only available on API
Level
extra spaces after @link

[tbansal1, 2017-06-07 20:21:33]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 20:21:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbansal1, 2017-06-07 21:37:20]

Patchset #5 (id:320001) has been deleted

[tbansal1, 2017-06-07 21:39:20]

pauljensen: ptal. Thanks.

> I'm a little concerned that you're adding more code that requires to run on
the
UI thread.  In Cronet we're removing all UI thread initialization:
https://codereview.chromium.org/2812963002/

The CL runs only the initialization code on UI thread where an observer
registers to changes in the signal strength. Are there alternatives? I thought
that was imposed by Android. Thanks.

https://codereview.chromium.org/2763853002/diff/260001/net/android/cellular_s...
File net/android/cellular_signal_strength.cc (right):

https://codereview.chromium.org/2763853002/diff/260001/net/android/cellular_s...
net/android/cellular_signal_strength.cc:34:
base::android::GetApplicationContext()));
On 2017/04/13 11:36:30, pauljensen wrote:
> does this need to be called on the UI thread? if so please add
> DCHECK(base::MessageLoopForUI::IsCurrent());

No, it does not have to be called on UI thread.
AndroidCellularSignalStrength.java explicitly creates a listener on UI thread.
This allows the rest of the code to run on non-UI threads.

https://codereview.chromium.org/2763853002/diff/260001/net/android/cellular_s...
File net/android/cellular_signal_strength.h (right):

https://codereview.chromium.org/2763853002/diff/260001/net/android/cellular_s...
net/android/cellular_signal_strength.h:21: class NET_EXPORT_PRIVATE
CellularSignalStrength {
On 2017/04/13 11:36:30, pauljensen wrote:
> needs comments

Done.

https://codereview.chromium.org/2763853002/diff/260001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java
(right):

https://codereview.chromium.org/2763853002/diff/260001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:47: if
(telephonyManager.getSimState() != TelephonyManager.SIM_STATE_READY) return;
On 2017/04/13 11:36:30, pauljensen wrote:
> Do these calls require any permissions?

No permissions required.

https://codereview.chromium.org/2763853002/diff/260001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:97: *
{@link   android.telephony.SignalStrength#getLevel} is only available on API
Level
On 2017/04/13 11:36:30, pauljensen wrote:
> extra spaces after @link

Done.

[tbansal1, 2017-06-07 21:41:30]

Also, I checked that in NCNAutoDetect.java, register()  (Line 805) and
onReceive() (Line 975) are called on UI thread. So, I am not sure how I can
avoid UI thread completely. Suggestions? Thanks.

[tbansal1, 2017-06-08 00:07:11]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-08 00:07:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-08 01:49:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-08 01:49:58]

Dry run: This issue passed the CQ dry run.

[pauljensen, 2017-06-08 18:07:33]

My main concern with this CL is what are the power consequences of this?  Is the
Chrome process going to be awoken frequently?  Is this going to drain the
battery unnecessarily?

On 2017/06/07 21:41:30, tbansal1 wrote:
> Also, I checked that in NCNAutoDetect.java, register()  (Line 805) and
> onReceive() (Line 975) are called on UI thread. So, I am not sure how I can
> avoid UI thread completely. Suggestions? Thanks.

NCNAutoDetect.register() is not called on the UI thread in Cronet anymore.  The
NCN lives on another thread in Cronet.

https://codereview.chromium.org/2763853002/diff/300001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java
(right):

https://codereview.chromium.org/2763853002/diff/300001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:28:
private final Context mContext;
Don't keep pointers to Context, just use ContextUtils.getApplicationContext()

https://codereview.chromium.org/2763853002/diff/300001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:33:
private int mSignalLevel = CellularSignalStrengthError.ERROR_NOT_SUPPORTED;
I don't think this needs to be guarded by a lock.  Can you just make it
volatile?  "int"s are atomic in Java.

https://codereview.chromium.org/2763853002/diff/300001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:44:
ThreadUtils.assertOnUiThread();
Why does this need to be done on the UI thread?

https://codereview.chromium.org/2763853002/diff/300001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:103:
return Build.VERSION.SDK_INT >= Build.VERSION_CODES.M;
Are you aware that M+ is only ~40% of users:
https://d.android.com/about/dashboards

[tbansal1, 2017-06-08 23:20:11]

Patchset #5 (id:340001) has been deleted

[tbansal1, 2017-06-08 23:34:15]

On 2017/06/08 18:07:33, pauljensen wrote:
> My main concern with this CL is what are the power consequences of this?  Is
the
> Chrome process going to be awoken frequently?  Is this going to drain the
> battery unnecessarily?
Good point. Restricted it to when the application is in foreground.
> 
> On 2017/06/07 21:41:30, tbansal1 wrote:
> > Also, I checked that in NCNAutoDetect.java, register()  (Line 805) and
> > onReceive() (Line 975) are called on UI thread. So, I am not sure how I can
> > avoid UI thread completely. Suggestions? Thanks.
> 
> NCNAutoDetect.register() is not called on the UI thread in Cronet anymore. 
The
> NCN lives on another thread in Cronet.
OK, I checked on Chrome, and not on Cronet.
This CL does not affect Cronet since the creation of CellularSignalStrength
object
is guarded behind the field trial experiment. If the experiment shows favorable
results, one option is to change ownership of CellularSignalStrength to
NetworkChangeNotifier, and initialize CellularSignalStrength as part of
NCN.

[tbansal1, 2017-06-08 23:34:30]

pauljensen: ptal. Thanks.

https://codereview.chromium.org/2763853002/diff/300001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java
(right):

https://codereview.chromium.org/2763853002/diff/300001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:28:
private final Context mContext;
On 2017/06/08 18:07:33, pauljensen wrote:
> Don't keep pointers to Context, just use ContextUtils.getApplicationContext()

Done.

https://codereview.chromium.org/2763853002/diff/300001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:33:
private int mSignalLevel = CellularSignalStrengthError.ERROR_NOT_SUPPORTED;
On 2017/06/08 18:07:33, pauljensen wrote:
> I don't think this needs to be guarded by a lock.  Can you just make it
> volatile?  "int"s are atomic in Java.

Done.

https://codereview.chromium.org/2763853002/diff/300001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:44:
ThreadUtils.assertOnUiThread();
On 2017/06/08 18:07:33, pauljensen wrote:
> Why does this need to be done on the UI thread?

On IO thread, I get an error:
06-08 15:55:20.685 11863 11930 W System.err: java.lang.NullPointerException:
Attempt to read from field 'android.os.MessageQueue android.os.Looper.mQueue' on
a null object reference
06-08 15:55:20.685 11863 11930 W System.err: 	at
android.os.Handler.<init>(Handler.java:229)
06-08 15:55:20.685 11863 11930 W System.err: 	at
android.os.Handler.<init>(Handler.java:137)
06-08 15:55:20.685 11863 11930 W System.err: 	at
android.telephony.PhoneStateListener$1.<init>(PhoneStateListener.java:275)
06-08 15:55:20.685 11863 11930 W System.err: 	at
android.telephony.PhoneStateListener.<init>(PhoneStateListener.java:275)
06-08 15:55:20.686 11863 11930 W System.err: 	at
android.telephony.PhoneStateListener.<init>(PhoneStateListener.java:245)
06-08 15:55:20.686 11863 11930 W System.err: 	at
org.chromium.net.AndroidCellularSignalStrength$CellStateListener.<init>(AndroidCellularSignalStrength.java:47)
06-08 15:55:20.686 11863 11930 W System.err: 	at
org.chromium.net.AndroidCellularSignalStrength.<init>(AndroidCellularSignalStrength.java:103)
06-08 15:55:20.686 11863 11930 W System.err: 	at
org.chromium.net.AndroidCellularSignalStrength.create(AndroidCellularSignalStrength.java:89)

https://codereview.chromium.org/2763853002/diff/300001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:103:
return Build.VERSION.SDK_INT >= Build.VERSION_CODES.M;
On 2017/06/08 18:07:33, pauljensen wrote:
> Are you aware that M+ is only ~40% of users:
> https://d.android.com/about/dashboards

Yes. I think this is probably the best option for now.

[pauljensen, 2017-06-13 11:53:43]

https://codereview.chromium.org/2763853002/diff/300001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java
(right):

https://codereview.chromium.org/2763853002/diff/300001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:44:
ThreadUtils.assertOnUiThread();
I don't think it needs to be done on the UI thread.  The UI thread is probably
the most overbooked thread and we've gone great lengths to move as much as work
as possible off the UI thread.   It appears to need to be done on a thread that
has a Looper, which the IO thread does not apparently.  I have suspicions that
this kind of operation could take a while; Context.registerListener() takes a
while.  Can you measure how long this operation takes on an older device?  We
may want to move it to another thread if it takes a while.  I wonder if there is
a thread-pool containing Java threads.

[tbansal1, 2017-06-19 19:54:03]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-19 19:54:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-19 19:57:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-19 19:57:19]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[tbansal1, 2017-06-20 01:52:06]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 01:52:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-20 04:13:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-20 04:13:51]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-06-20 14:14:09]

Patchset #8 (id:420001) has been deleted

[tbansal1, 2017-06-20 14:14:36]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 14:14:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-20 15:37:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-20 15:37:38]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-06-20 15:48:51]

Patchset #7 (id:400001) has been deleted

[tbansal1, 2017-06-20 15:50:54]

ptal. Thanks.

https://codereview.chromium.org/2763853002/diff/300001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java
(right):

https://codereview.chromium.org/2763853002/diff/300001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:44:
ThreadUtils.assertOnUiThread();
On 2017/06/13 11:53:43, pauljensen wrote:
> I don't think it needs to be done on the UI thread.  The UI thread is probably
> the most overbooked thread and we've gone great lengths to move as much as
work
> as possible off the UI thread.   It appears to need to be done on a thread
that
> has a Looper, which the IO thread does not apparently.  I have suspicions that
> this kind of operation could take a while; Context.registerListener() takes a
> while.  Can you measure how long this operation takes on an older device?  We
> may want to move it to another thread if it takes a while.  I wonder if there
is
> a thread-pool containing Java threads.

It takes around 10 msec or so. I have moved it to a different thread based on
suggestion here: http://shortn/_jJ1xZ2wPwR

[pauljensen, 2017-06-20 18:18:42]

I'm not excited about adding another thread, but I don't see a good thread or
thread pool to share otherwise.

It looks like the thread is never killed.  Can we either:
1. Use org.chromium.base.JavaHandlerThread which supports stopping the thread on
newer Androids, or
2. Use a global static singleton in Java and get rid of the C++
CellularSignalStrength class.

[tbansal1, 2017-06-20 18:50:18]

On 2017/06/20 18:18:42, pauljensen wrote:
> I'm not excited about adding another thread, but I don't see a good thread or
> thread pool to share otherwise.
> 
> It looks like the thread is never killed.  Can we either:
> 1. Use org.chromium.base.JavaHandlerThread which supports stopping the thread
on
> newer Androids, or
> 2. Use a global static singleton in Java and get rid of the C++
> CellularSignalStrength class.

Is it enough to call quit() on the |mHandlerThread| in the finalize of
AndroidCellularSignalStrength,
or by calling Java code from the destructor of CellularSignalStrength class?

In org.chromium.base.JavaHandlerThread, stop() is called by the native. And,
it is not clear if we need org.chromium.base.JavaHandlerThread with the native
baggage here. WDYT?

[pauljensen, 2017-06-20 18:56:36]

On 2017/06/20 18:50:18, tbansal1 wrote:
> On 2017/06/20 18:18:42, pauljensen wrote:
> > I'm not excited about adding another thread, but I don't see a good thread
or
> > thread pool to share otherwise.
> > 
> > It looks like the thread is never killed.  Can we either:
> > 1. Use org.chromium.base.JavaHandlerThread which supports stopping the
thread
> on
> > newer Androids, or
> > 2. Use a global static singleton in Java and get rid of the C++
> > CellularSignalStrength class.
> 
> Is it enough to call quit() on the |mHandlerThread| in the finalize of
> AndroidCellularSignalStrength,
> or by calling Java code from the destructor of CellularSignalStrength class?

No, first off finalizers are forbidden:
https://source.android.com/source/code-style#dont-use-finalizers
Secondly I think your thread will have a pointer to the
AndroidCellularSignalStrength so it will keep it alive forever.

> 
> In org.chromium.base.JavaHandlerThread, stop() is called by the native. And,
> it is not clear if we need org.chromium.base.JavaHandlerThread with the native
> baggage here. WDYT?

Your call.  We don't really want many AndroidCellularSignalStrength instances
created do we?  each will have a thread and listen for redundant events. 
Perhaps a global singleton is the way to go?  You could integrate into the NCN
too, though it lives on the UI thread in Chrome.

[tbansal1, 2017-06-20 18:58:01]

On 2017/06/20 18:56:36, pauljensen wrote:
> On 2017/06/20 18:50:18, tbansal1 wrote:
> > On 2017/06/20 18:18:42, pauljensen wrote:
> > > I'm not excited about adding another thread, but I don't see a good thread
> or
> > > thread pool to share otherwise.
> > > 
> > > It looks like the thread is never killed.  Can we either:
> > > 1. Use org.chromium.base.JavaHandlerThread which supports stopping the
> thread
> > on
> > > newer Androids, or
> > > 2. Use a global static singleton in Java and get rid of the C++
> > > CellularSignalStrength class.
> > 
> > Is it enough to call quit() on the |mHandlerThread| in the finalize of
> > AndroidCellularSignalStrength,
> > or by calling Java code from the destructor of CellularSignalStrength class?
> 
> No, first off finalizers are forbidden:
> https://source.android.com/source/code-style#dont-use-finalizers
> Secondly I think your thread will have a pointer to the
> AndroidCellularSignalStrength so it will keep it alive forever.
> 
> > 
> > In org.chromium.base.JavaHandlerThread, stop() is called by the native. And,
> > it is not clear if we need org.chromium.base.JavaHandlerThread with the
native
> > baggage here. WDYT?
> 
> Your call.  We don't really want many AndroidCellularSignalStrength instances
> created do we?  each will have a thread and listen for redundant events. 
> Perhaps a global singleton is the way to go?  You could integrate into the NCN
> too, though it lives on the UI thread in Chrome.

Thanks, that makes a lot of sense. I will change to global singleton.

[tbansal1, 2017-06-20 22:41:10]

Patchset #8 (id:460001) has been deleted

[tbansal1, 2017-06-20 22:45:25]

Patchset #8 (id:480001) has been deleted

[tbansal1, 2017-06-20 22:47:51]

Patchset #8 (id:500001) has been deleted

[tbansal1, 2017-06-20 22:54:17]

Patchset #8 (id:520001) has been deleted

[tbansal1, 2017-06-20 22:55:58]

Patchset #8 (id:540001) has been deleted

[tbansal1, 2017-06-20 23:17:37]

Patchset #8 (id:560001) has been deleted

[tbansal1, 2017-06-20 23:21:46]

Patchset #8 (id:580001) has been deleted

[tbansal1, 2017-06-20 23:21:55]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 23:22:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-20 23:38:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-20 23:38:31]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[tbansal1, 2017-06-20 23:43:31]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 23:43:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 01:43:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-21 01:43:38]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[tbansal1, 2017-06-21 04:36:26]

Patchset #8 (id:600001) has been deleted

[tbansal1, 2017-06-21 04:36:34]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 04:36:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 06:05:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-21 06:05:48]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[tbansal1, 2017-06-21 06:53:12]

Patchset #8 (id:620001) has been deleted

[tbansal1, 2017-06-21 07:19:45]

Patchset #9 (id:650001) has been deleted

[tbansal1, 2017-06-21 07:19:57]

Patchset #8 (id:490012) has been deleted

[tbansal1, 2017-06-21 07:20:08]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 07:20:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 07:59:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-21 07:59:25]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[tbansal1, 2017-06-21 13:00:34]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-21 13:00:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 14:14:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-21 14:14:27]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-06-21 16:00:39]

pauljensen: ptal. Thanks.

[tbansal1, 2017-06-21 17:36:38]

On 2017/06/21 16:00:39, tbansal1 wrote:
> pauljensen: ptal. Thanks.

Another option that I considered was to not use singleton, but stop the
HandlerThread (and unregister as a listener) when Chrome goes in the background.

Similarly, start the HandlerThread (and register as a listener) when Chrome is
in foreground.
Does that sound better?

[pauljensen, 2017-06-22 13:32:15]

https://codereview.chromium.org/2763853002/diff/670001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java
(right):

https://codereview.chromium.org/2763853002/diff/670001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:39:
private HandlerThread mHandlerThread;
mHandlerThread should be a local not a member variable.

https://codereview.chromium.org/2763853002/diff/670001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:67:
private void register() {
I think you want to initialize mSignalLevel to ERROR_NOT_SUPPORTED here,
otherwise you'll get a prior value that may be incorrect.

https://codereview.chromium.org/2763853002/diff/670001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:126: }
This is racy and also I don't see any reason to restrict to single-threaded
access.  Why not just make sInstance a "static final"?  Then the race is gone,
sThreadCheck can be removed, getInstance can be removed.

[tbansal1, 2017-06-22 21:55:07]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 21:55:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbansal1, 2017-06-22 23:15:41]

Patchset #10 (id:710001) has been deleted

[tbansal1, 2017-06-22 23:17:52]

ptal. Thanks.

https://codereview.chromium.org/2763853002/diff/670001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java
(right):

https://codereview.chromium.org/2763853002/diff/670001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:39:
private HandlerThread mHandlerThread;
On 2017/06/22 13:32:14, pauljensen wrote:
> mHandlerThread should be a local not a member variable.

Done.

https://codereview.chromium.org/2763853002/diff/670001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:67:
private void register() {
On 2017/06/22 13:32:14, pauljensen wrote:
> I think you want to initialize mSignalLevel to ERROR_NOT_SUPPORTED here,
> otherwise you'll get a prior value that may be incorrect.

Done.

https://codereview.chromium.org/2763853002/diff/670001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:126: }
On 2017/06/22 13:32:14, pauljensen wrote:
> This is racy and also I don't see any reason to restrict to single-threaded
> access.  Why not just make sInstance a "static final"?  Then the race is gone,
> sThreadCheck can be removed, getInstance can be removed.

Done.

[tbansal1, 2017-06-22 23:17:56]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-22 23:18:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-23 01:50:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-23 01:50:05]

Dry run: This issue passed the CQ dry run.

[pauljensen, 2017-06-23 14:31:25]

https://codereview.chromium.org/2763853002/diff/730001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java
(right):

https://codereview.chromium.org/2763853002/diff/730001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:34:
private CellStateListener mCellStateListener;
unused, remove

https://codereview.chromium.org/2763853002/diff/730001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:37: new
AndroidCellularSignalStrength();
When is this created?  I think statics are initialized when the class is first
referenced.  Is this during JNI registration?  if so we may not want to always
do this so early, e.g. if getSignalStrengthLevel() is never called.  Perhaps we
should make this non-final and have a synchronized block in
getSignalStrengthLevel() that initializes sInstance if it's not yet initialized.

https://codereview.chromium.org/2763853002/diff/730001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:66:
mSignalLevel = CellularSignalStrengthError.ERROR_NOT_SUPPORTED;
This is racy.  An onSignalStrengthsChanged() event could already be queued up in
the Looper and could fire after unregister() sets mSignalLevel.  You may want to
add a "private boolean mRegistered" and check it in onSignalStrengthsChanged()

[tbansal1, 2017-06-26 16:08:00]

pauljensen: ptal. Thanks.

https://codereview.chromium.org/2763853002/diff/730001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java
(right):

https://codereview.chromium.org/2763853002/diff/730001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:34:
private CellStateListener mCellStateListener;
On 2017/06/23 14:31:25, pauljensen wrote:
> unused, remove

umm, this is in use.

https://codereview.chromium.org/2763853002/diff/730001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:37: new
AndroidCellularSignalStrength();
On 2017/06/23 14:31:25, pauljensen wrote:
> When is this created?  I think statics are initialized when the class is first
> referenced.  Is this during JNI registration?  if so we may not want to always
> do this so early, e.g. if getSignalStrengthLevel() is never called.  Perhaps
we
> should make this non-final and have a synchronized block in
> getSignalStrengthLevel() that initializes sInstance if it's not yet
initialized.

This is created when the static object is first referenced. That happens when
the signal strength is first queried from the native. If the signal strength is
never queried, the object is not created. I verified using Log statements. Also,
this matches with  https://stackoverflow.com/a/3499322.

https://codereview.chromium.org/2763853002/diff/730001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:66:
mSignalLevel = CellularSignalStrengthError.ERROR_NOT_SUPPORTED;
On 2017/06/23 14:31:25, pauljensen wrote:
> This is racy.  An onSignalStrengthsChanged() event could already be queued up
in
> the Looper and could fire after unregister() sets mSignalLevel.  You may want
to
> add a "private boolean mRegistered" and check it in onSignalStrengthsChanged()

Done.

[pauljensen, 2017-06-26 16:24:22]

https://codereview.chromium.org/2763853002/diff/730001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java
(right):

https://codereview.chromium.org/2763853002/diff/730001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:34:
private CellStateListener mCellStateListener;
On 2017/06/26 16:08:00, tbansal1 wrote:
> On 2017/06/23 14:31:25, pauljensen wrote:
> > unused, remove
> 
> umm, this is in use.

Where is the use?

[tbansal1, 2017-06-26 16:28:09]

https://codereview.chromium.org/2763853002/diff/730001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java
(right):

https://codereview.chromium.org/2763853002/diff/730001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:34:
private CellStateListener mCellStateListener;
On 2017/06/26 16:24:22, pauljensen wrote:
> On 2017/06/26 16:08:00, tbansal1 wrote:
> > On 2017/06/23 14:31:25, pauljensen wrote:
> > > unused, remove
> > 
> > umm, this is in use.
> 
> Where is the use?

mCellStateListener is used in Line 100 below in the ctor of
AndroidCellularSignalStrength.

[pauljensen, 2017-06-26 16:29:37]

https://codereview.chromium.org/2763853002/diff/730001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java
(right):

https://codereview.chromium.org/2763853002/diff/730001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:34:
private CellStateListener mCellStateListener;
On 2017/06/26 16:28:09, tbansal1 wrote:
> On 2017/06/26 16:24:22, pauljensen wrote:
> > On 2017/06/26 16:08:00, tbansal1 wrote:
> > > On 2017/06/23 14:31:25, pauljensen wrote:
> > > > unused, remove
> > > 
> > > umm, this is in use.
> > 
> > Where is the use?
> 
> mCellStateListener is used in Line 100 below in the ctor of
> AndroidCellularSignalStrength.

There is a dead write, but no use of it.

[tbansal1, 2017-06-26 16:49:28]

https://codereview.chromium.org/2763853002/diff/730001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java
(right):

https://codereview.chromium.org/2763853002/diff/730001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:34:
private CellStateListener mCellStateListener;
On 2017/06/26 16:29:37, pauljensen wrote:
> On 2017/06/26 16:28:09, tbansal1 wrote:
> > On 2017/06/26 16:24:22, pauljensen wrote:
> > > On 2017/06/26 16:08:00, tbansal1 wrote:
> > > > On 2017/06/23 14:31:25, pauljensen wrote:
> > > > > unused, remove
> > > > 
> > > > umm, this is in use.
> > > 
> > > Where is the use?
> > 
> > mCellStateListener is used in Line 100 below in the ctor of
> > AndroidCellularSignalStrength.
> 
> There is a dead write, but no use of it.

Thanks. Fixed.

[pauljensen, 2017-06-26 17:26:20]

net/android/ lgtm modulo comments

https://codereview.chromium.org/2763853002/diff/770001/net/android/cellular_s...
File net/android/cellular_signal_strength_unittest.cc (right):

https://codereview.chromium.org/2763853002/diff/770001/net/android/cellular_s...
net/android/cellular_signal_strength_unittest.cc:25:
ASSERT_FALSE(signal_strength);
ASSERT->EXPECT?

https://codereview.chromium.org/2763853002/diff/770001/net/android/cellular_s...
net/android/cellular_signal_strength_unittest.cc:29:
ASSERT_TRUE(signal_strength);
ASSERT->EXPECT?

https://codereview.chromium.org/2763853002/diff/770001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java
(right):

https://codereview.chromium.org/2763853002/diff/770001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:118:
private static boolean isAPIAvailable(Context context) {
|context| is unused, please remove.  Perhaps this one-liner with one call-site
should just be inlined.

[tbansal1, 2017-06-26 17:33:58]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[tbansal1, 2017-06-26 17:34:17]

tbansal@chromium.org changed reviewers:
+ rkaplow@chromium.org

[tbansal1, 2017-06-26 17:34:19]

rkaplow: ptal at histograms.xml. Thanks.

https://codereview.chromium.org/2763853002/diff/770001/net/android/cellular_s...
File net/android/cellular_signal_strength_unittest.cc (right):

https://codereview.chromium.org/2763853002/diff/770001/net/android/cellular_s...
net/android/cellular_signal_strength_unittest.cc:25:
ASSERT_FALSE(signal_strength);
On 2017/06/26 17:26:19, pauljensen wrote:
> ASSERT->EXPECT?

Done.

https://codereview.chromium.org/2763853002/diff/770001/net/android/cellular_s...
net/android/cellular_signal_strength_unittest.cc:29:
ASSERT_TRUE(signal_strength);
On 2017/06/26 17:26:19, pauljensen wrote:
> ASSERT->EXPECT?

Done.

https://codereview.chromium.org/2763853002/diff/770001/net/android/java/src/o...
File net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java
(right):

https://codereview.chromium.org/2763853002/diff/770001/net/android/java/src/o...
net/android/java/src/org/chromium/net/AndroidCellularSignalStrength.java:118:
private static boolean isAPIAvailable(Context context) {
On 2017/06/26 17:26:19, pauljensen wrote:
> |context| is unused, please remove.  Perhaps this one-liner with one call-site
> should just be inlined.

Done.

[commit-bot: I haz the power, 2017-06-26 17:34:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-26 19:06:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-26 19:06:06]

Dry run: This issue passed the CQ dry run.

[rkaplow, 2017-06-27 21:32:22]

lgtm

histograms lg

[tbansal1, 2017-06-27 21:33:57]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2017-06-27 21:33:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
ryansturm@chromium.org, pauljensen@chromium.org
Link to the patchset: https://codereview.chromium.org/2763853002/#ps790001
(title: "comments")

[commit-bot: I haz the power, 2017-06-27 21:34:22]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 22:53:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-27 22:53:26]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tbansal1, 2017-06-27 22:55:45]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-27 22:55:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 23:55:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-27 23:55:27]

Dry run: Try jobs failed on following builders:
  android_cronet_tester on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[tbansal1, 2017-06-28 00:16:28]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-28 00:16:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-28 01:55:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-28 01:55:55]

Dry run: Try jobs failed on following builders:
  android_cronet_tester on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[tbansal1, 2017-06-28 02:17:47]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[tbansal1, 2017-06-28 02:17:54]

The CQ bit was unchecked by tbansal@chromium.org

[tbansal1, 2017-06-28 02:17:58]

The CQ bit was unchecked by tbansal@chromium.org

[tbansal1, 2017-06-28 02:18:08]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-28 02:18:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-28 04:04:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-28 04:04:51]

Dry run: Try jobs failed on following builders:
  android_cronet_tester on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[tbansal1, 2017-06-28 13:46:30]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-28 13:46:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-28 14:08:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-28 14:08:13]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-06-28 14:19:03]

The CQ bit was checked by tbansal@chromium.org

[commit-bot: I haz the power, 2017-06-28 14:19:29]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-28 14:23:39]

CQ is committing da patch.

Bot data: {"patchset_id": 790001, "attempt_start_ts": 1498659543437040,
"parent_rev": "56ccf932aa2976881d7da78c38dd64b34f719e4b", "commit_rev":
"de702d52aea0e1daae4a827d0eb3fe3d4d477b68"}

[commit-bot: I haz the power, 2017-06-28 14:23:55]

Description was changed from

==========
Use Android callback API to obtain cellular signal strength

Move cellular signal strength methods to a class. Also, use the 
PhoneStateListener to listen to the changes in the signal strength.

BUG=703740
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Use Android callback API to obtain cellular signal strength

Move cellular signal strength methods to a class. Also, use the 
PhoneStateListener to listen to the changes in the signal strength.

BUG=703740
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2763853002
Cr-Commit-Position: refs/heads/master@{#482981}
Committed:
https://chromium.googlesource.com/chromium/src/+/de702d52aea0e1daae4a827d0eb3...
==========

[commit-bot: I haz the power, 2017-06-28 14:23:58]

Committed patchset #13 (id:790001) as
https://chromium.googlesource.com/chromium/src/+/de702d52aea0e1daae4a827d0eb3...