Add Bluetooth Checks to Service

Add Bluetooth Checks to Service

This change adds two bluetooth checks. 1 is to determine if the hardware has bluetooth capabilities. 2 is to determine if bluetooth is on for the device. Both conditions must be met for the PhysicalWebSharing feature to be enabled.

BUG=685856
Review-Url: https://codereview.chromium.org/2737393003
Cr-Commit-Position: refs/heads/master@{#456216}
Committed: https://chromium.googlesource.com/chromium/src/+/f0cc0ad34d0e5491eefe7bafc5e9ed397427a6cb

[iankc, 2017-03-10 00:01:07]

Description was changed from

==========
Add Bluetooth Checks to Service

Experiment to add bluetooth checks to service. Found that Services
cannot start activities for results.

BUG=685856
==========

to

==========
Add Bluetooth Checks to Service

This change adds two bluetooth checks. 1 is to determine if the hardware has
bluetooth capabilities. 2 is to determine if bluetooth is on for the device.
Both conditions must be met for the PhysicalWebSharing feature to be enabled.



BUG=685856
==========

[iankc, 2017-03-10 00:01:07]

iankc@google.com changed reviewers:
+ cco3@chromium.org, mattreynolds@chromium.org

[cco3, 2017-03-10 00:21:38]

lgtm

[iankc, 2017-03-10 00:24:56]

iankc@google.com changed reviewers:
+ tedchoc@chromium.org

[iankc, 2017-03-10 00:25:32]

Hey Ted,

Would you mind reviewing this two-liner.

[Ted C, 2017-03-10 04:38:43]

https://codereview.chromium.org/2737393003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java
(right):

https://codereview.chromium.org/2737393003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:181:
return bluetoothManager.getAdapter().getBluetoothLeAdvertiser() != null;
you null check the adapter above but not here...why?  Seems better to be
consistent one way or the other.

https://codereview.chromium.org/2737393003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java
(right):

https://codereview.chromium.org/2737393003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:36:
return PhysicalWeb.hasBleAdvertiseCapability() &&
PhysicalWeb.bluetoothIsEnabled()
You need to check the SDK version first.  These two methods require L+, so this
would crash I believe.

I'd probably pull out the SDK check first to be even more explicit

if (Build.VERSION.SDK_INT < Build.VERSION_CODES.M) return false;
... other checks ...

Or you could make the two methods you're calling into SDK safe and then keep it
as is.  Moving the SDK checks into the other methods is probably "safer" because
API clients don't have to know, but I'll defer to you.

[iankc, 2017-03-10 22:09:42]

https://codereview.chromium.org/2737393003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java
(right):

https://codereview.chromium.org/2737393003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWeb.java:181:
return bluetoothManager.getAdapter().getBluetoothLeAdvertiser() != null;
On 2017/03/10 04:38:43, Ted C wrote:
> you null check the adapter above but not here...why?  Seems better to be
> consistent one way or the other.

Done.

https://codereview.chromium.org/2737393003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java
(right):

https://codereview.chromium.org/2737393003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/physicalweb/PhysicalWebShareActivity.java:36:
return PhysicalWeb.hasBleAdvertiseCapability() &&
PhysicalWeb.bluetoothIsEnabled()
On 2017/03/10 04:38:43, Ted C wrote:
> You need to check the SDK version first.  These two methods require L+, so
this
> would crash I believe.
> 
> I'd probably pull out the SDK check first to be even more explicit
> 
> if (Build.VERSION.SDK_INT < Build.VERSION_CODES.M) return false;
> ... other checks ...
> 
> Or you could make the two methods you're calling into SDK safe and then keep
it
> as is.  Moving the SDK checks into the other methods is probably "safer"
because
> API clients don't have to know, but I'll defer to you.

the former seems simpler.

[Ted C, 2017-03-10 22:55:39]

lgtm

[iankc, 2017-03-10 22:56:26]

The CQ bit was checked by iankc@google.com

[iankc, 2017-03-10 22:56:26]

The patchset sent to the CQ was uploaded after l-g-t-m from cco3@chromium.org
Link to the patchset: https://codereview.chromium.org/2737393003/#ps20001
(title: "fixing teds comments")

[commit-bot: I haz the power, 2017-03-10 22:56:55]

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-10 23:31:45]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489186586438320,
"parent_rev": "e4437fe7b0818475b020f6b3bf2b1577f49a3ec8", "commit_rev":
"f0cc0ad34d0e5491eefe7bafc5e9ed397427a6cb"}

[commit-bot: I haz the power, 2017-03-10 23:33:14]

Description was changed from

==========
Add Bluetooth Checks to Service

This change adds two bluetooth checks. 1 is to determine if the hardware has
bluetooth capabilities. 2 is to determine if bluetooth is on for the device.
Both conditions must be met for the PhysicalWebSharing feature to be enabled.



BUG=685856
==========

to

==========
Add Bluetooth Checks to Service

This change adds two bluetooth checks. 1 is to determine if the hardware has
bluetooth capabilities. 2 is to determine if bluetooth is on for the device.
Both conditions must be met for the PhysicalWebSharing feature to be enabled.

BUG=685856

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

[commit-bot: I haz the power, 2017-03-10 23:33:15]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/f0cc0ad34d0e5491eefe7bafc5e9...