If location services are turned off, have the BT chooser prompt the user to turn them on.

If location services are turned off, have the BT chooser prompt the user to turn them on.

BUG=545867

[Jeffrey Yasskin, 2016-06-03 01:40:44]

Description was changed from

==========
If location services are turned off, have the BT chooser prompt the user to turn
them on.

BUG=545867
R=newt@chromium.org, finnur@chromium.org
==========

to

==========
If location services are turned off, have the BT chooser prompt the user to turn
them on.

BUG=545867
R=newt@chromium.org, finnur@chromium.org
==========

[Jeffrey Yasskin, 2016-06-03 01:40:44]

jyasskin@chromium.org changed reviewers:
+ scheib@chromium.org, tedchoc@chromium.org
- newt@chromium.org

[Jeffrey Yasskin, 2016-06-03 01:52:48]

I have two questions for the Android experts below. If you like what I've done,
go ahead and review the rest of the change, but if you want me to refactor
LocationSettings, just say that and I'll ping you again once I've done that.

Thanks!

https://codereview.chromium.org/2032273002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java
(right):

https://codereview.chromium.org/2032273002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:194:
LocationSettings.getInstance().isSystemLocationSettingEnabled();
It's a little odd for this chooser to call into preferences.LocationSettings.
Would folks be happy to see me refactor that class into a more general location,
since the concept is used ~6 places in the codebase after this change? If so,
where would you like it? Is it a base/ kind of utility?

https://codereview.chromium.org/2032273002/diff/1/device/bluetooth/android/ja...
File
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothAdapter.java
(right):

https://codereview.chromium.org/2032273002/diff/1/device/bluetooth/android/ja...
device/bluetooth/android/java/src/org/chromium/device/bluetooth/ChromeBluetoothAdapter.java:200:
locationManager.isProviderEnabled(LocationManager.NETWORK_PROVIDER)
https://developer.android.com/reference/android/location/LocationManager.html...
recommends using Settings.Secure.getInt(LOCATION_MODE), but Chrome has 3 uses of
isProviderEnabled() vs 1 use of getInt(LOCATION_MODE). Which should we be using
in general?

[Jeffrey Yasskin, 2016-06-03 01:53:56]

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

[commit-bot: I haz the power, 2016-06-03 01:55:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2032273002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2032273002/1

[Finnur, 2016-06-03 02:25:04]

https://codereview.chromium.org/2032273002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java
(right):

https://codereview.chromium.org/2032273002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:194:
LocationSettings.getInstance().isSystemLocationSettingEnabled();
These questions are probably more appropriate for Ted. I don't have a strong
opinion, but don't mind seeing this be made more general.

[commit-bot: I haz the power, 2016-06-03 04:11:10]

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

[commit-bot: I haz the power, 2016-06-03 04:11:11]

Dry run: This issue passed the CQ dry run.

[scheib, 2016-06-03 20:48:29]

https://codereview.chromium.org/2032273002/diff/1/chrome/android/java/strings...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2032273002/diff/1/chrome/android/java/strings...
chrome/android/java/strings/android_chrome_strings.grd:1197: <message
name="IDS_BLUETOOTH_NEED_LOCATION_SERVICES_ON" desc="The message to show when
scanning cannot start because Android location services are off, but they must
be turned on for Android to return any Bluetooth devices from a scan.">
I left string thoughts on
https://bugs.chromium.org/p/chromium/issues/detail?id=545867

https://codereview.chromium.org/2032273002/diff/1/device/bluetooth/test/bluet...
File device/bluetooth/test/bluetooth_test_android.h (right):

https://codereview.chromium.org/2032273002/diff/1/device/bluetooth/test/bluet...
device/bluetooth/test/bluetooth_test_android.h:33: void
TurnOffLocationServices();
Not a Base override, move to just before ForceIllegalStateException.

I'd been using 'Simulate' as a prefix to make reading tests clear when we were
just simulating events etc. So I suggest SimulateLocationServicesOff().

[Jeffrey Yasskin, 2016-06-04 01:49:47]

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

[commit-bot: I haz the power, 2016-06-04 01:50:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2032273002/20001

[commit-bot: I haz the power, 2016-06-04 06:07:57]

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

[commit-bot: I haz the power, 2016-06-04 06:07:58]

Dry run: This issue passed the CQ dry run.

[Jeffrey Yasskin, 2016-06-06 15:56:20]

Description was changed from

==========
If location services are turned off, have the BT chooser prompt the user to turn
them on.

BUG=545867
R=newt@chromium.org, finnur@chromium.org
==========

to

==========
If location services are turned off, have the BT chooser prompt the user to turn
them on.

BUG=545867
==========

[Jeffrey Yasskin, 2016-06-06 16:16:56]

https://codereview.chromium.org/2032273002/diff/1/chrome/android/java/strings...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/2032273002/diff/1/chrome/android/java/strings...
chrome/android/java/strings/android_chrome_strings.grd:1197: <message
name="IDS_BLUETOOTH_NEED_LOCATION_SERVICES_ON" desc="The message to show when
scanning cannot start because Android location services are off, but they must
be turned on for Android to return any Bluetooth devices from a scan.">
On 2016/06/03 20:48:29, scheib wrote:
> I left string thoughts on
> https://bugs.chromium.org/p/chromium/issues/detail?id=545867

I've updated the strings to roughly Rebecca's suggestions, and will wait for her
to approve them on the bug.

https://codereview.chromium.org/2032273002/diff/1/device/bluetooth/test/bluet...
File device/bluetooth/test/bluetooth_test_android.h (right):

https://codereview.chromium.org/2032273002/diff/1/device/bluetooth/test/bluet...
device/bluetooth/test/bluetooth_test_android.h:33: void
TurnOffLocationServices();
On 2016/06/03 20:48:29, scheib wrote:
> Not a Base override, move to just before ForceIllegalStateException.
> 
> I'd been using 'Simulate' as a prefix to make reading tests clear when we were
> just simulating events etc. So I suggest SimulateLocationServicesOff().

This isn't really an event, but sure.

[scheib, 2016-06-06 17:22:58]

LGTM

[Ted C, 2016-06-07 00:30:14]

https://codereview.chromium.org/2032273002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java
(right):

https://codereview.chromium.org/2032273002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/BluetoothChooserDialog.java:194:
LocationSettings.getInstance().isSystemLocationSettingEnabled();
On 2016/06/03 02:25:04, Finnur wrote:
> These questions are probably more appropriate for Ted. I don't have a strong
> opinion, but don't mind seeing this be made more general.

It seems to only be used in preferences right now.  I think it's fine to move it
up somewhere out of preferences, but I'm not sure how much we need to move it to
base.

In particular, we have an internal version of the class and moving it to base
will make that more difficult.

The internal one is under org.chromium.chrome.browser.location so we could
conceivably move it there easier.

[Jeffrey Yasskin, 2016-06-16 16:10:43]

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

[commit-bot: I haz the power, 2016-06-16 16:11:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2032273002/60001

[commit-bot: I haz the power, 2016-06-16 18:48:52]

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

[commit-bot: I haz the power, 2016-06-16 18:48:54]

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