arc: bluetooth: Use UUID 16 bits in advertising data

arc: bluetooth: Use UUID 16 bits in advertising data

For device advertising data received from low energy scan,
Android framework supports UUID 128 bits in services data
field. But, Android Bluetooth package supports only UUID
16 bits.

This patch makes Chrome send UUID 16 bits to Android
instead of UUID 128 bits.

BUG=636478
TEST=Build

Committed: https://crrev.com/31fcf11302a5dc9670458fe666f86c086191089d
Cr-Commit-Position: refs/heads/master@{#412417}

[puthik_chromium, 2016-08-04 02:44:56]

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

[commit-bot: I haz the power, 2016-08-04 02:45: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, 2016-08-04 03:16:16]

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

[commit-bot: I haz the power, 2016-08-04 03:16:17]

Dry run: This issue passed the CQ dry run.

[puthik_chromium, 2016-08-04 17:58:10]

Description was changed from

==========
arc: bluetooth: Use UUID 16 bits in advertising data

For device advertising data received from low energy scan,
Android framework support UUID 128 bits in services data field.
However, Bluetooth package support only UUID 16 bits.
This cause array index out of bound sometimes when scanning.

This patch max Chrome send UUID 16 bits to Android instead of
UUID 128 bits.

BUG=634209
TEST=No array index out of bound error in Minnie
==========

to

==========
arc: bluetooth: Use UUID 16 bits in advertising data

For device advertising data received from low energy scan,
Android framework supports UUID 128 bits in services data
field. But, Android Bluetooth package supports only UUID
16 bits. This causes array index out of bound sometimes
when doing the low energy scan.

This patch makes Chrome send UUID 16 bits to Android
instead of UUID 128 bits.

BUG=634209
TEST=No array index out of bound error in Minnie
==========

[puthik_chromium, 2016-08-04 17:58:52]

puthik@chromium.org changed reviewers:
+ ejcaruso@chromium.org, lhchavez@chromium.org, rkc@chromium.org

[rkc, 2016-08-04 20:01:57]

Can we add a test for this?

https://codereview.chromium.org/2204263003/diff/1/components/arc/bluetooth/ar...
File components/arc/bluetooth/arc_bluetooth_bridge.cc (right):

https://codereview.chromium.org/2204263003/diff/1/components/arc/bluetooth/ar...
components/arc/bluetooth/arc_bluetooth_bridge.cc:176: uint16_t GetUuid16(const
BluetoothUUID& uuid) {
GetUUID16 to keep it consistent with the casing of UUID other places.

[Luis Héctor Chávez, 2016-08-08 14:41:37]

+1 on the test, which would have caught the prepended zeroes.

https://codereview.chromium.org/2204263003/diff/1/components/arc/bluetooth/ar...
File components/arc/bluetooth/arc_bluetooth_bridge.cc (right):

https://codereview.chromium.org/2204263003/diff/1/components/arc/bluetooth/ar...
components/arc/bluetooth/arc_bluetooth_bridge.cc:1535:
mojo::Array<uint16_t>::New(uuid_list.size());
This would cause |uuid_list.size()| zeroes to be added. L1537 would then add the
data.

You can either use the default constructor and fill the array in the next loop,
or keep this and replace each added zero with its real value.

[puthik_chromium, 2016-08-10 19:02:41]

Description was changed from

==========
arc: bluetooth: Use UUID 16 bits in advertising data

For device advertising data received from low energy scan,
Android framework supports UUID 128 bits in services data
field. But, Android Bluetooth package supports only UUID
16 bits. This causes array index out of bound sometimes
when doing the low energy scan.

This patch makes Chrome send UUID 16 bits to Android
instead of UUID 128 bits.

BUG=634209
TEST=No array index out of bound error in Minnie
==========

to

==========
arc: bluetooth: Use UUID 16 bits in advertising data

For device advertising data received from low energy scan,
Android framework supports UUID 128 bits in services data
field. But, Android Bluetooth package supports only UUID
16 bits.

This patch makes Chrome send UUID 16 bits to Android
instead of UUID 128 bits.

BUG=636478
TEST=Build
==========

[puthik_chromium, 2016-08-10 19:07:27]

Look like this does not related to the ArrayIndexOutOfBound error I see before.
That is fixed in http://ag/1307747 I opened a new bug for this.

Also it's look like BlueZ does not support ServiceData in AdvertiseData [1] only
have mac version use it. rkc@ any comment?

[1] https://cs.chromium.org/search/?q=SetServiceData

https://codereview.chromium.org/2204263003/diff/1/components/arc/bluetooth/ar...
File components/arc/bluetooth/arc_bluetooth_bridge.cc (right):

https://codereview.chromium.org/2204263003/diff/1/components/arc/bluetooth/ar...
components/arc/bluetooth/arc_bluetooth_bridge.cc:176: uint16_t GetUuid16(const
BluetoothUUID& uuid) {
On 2016/08/04 20:01:57, rkc wrote:
> GetUUID16 to keep it consistent with the casing of UUID other places.

Done.

https://codereview.chromium.org/2204263003/diff/1/components/arc/bluetooth/ar...
components/arc/bluetooth/arc_bluetooth_bridge.cc:1535:
mojo::Array<uint16_t>::New(uuid_list.size());
On 2016/08/08 14:41:37, Luis Héctor Chávez wrote:
> This would cause |uuid_list.size()| zeroes to be added. L1537 would then add
the
> data.
> 
> You can either use the default constructor and fill the array in the next
loop,
> or keep this and replace each added zero with its real value.

Done.

[rkc, 2016-08-10 19:30:41]

We definitely have the service data field in BluetoothAdvertisement and we do
pass it all the way down to BlueZ and as far as I can tell, BlueZ does use that
service data.

I just tested this from the Chrome API and service data shows up just fine.

Where are you having an issue with this exactly?

[puthik_chromium, 2016-08-10 19:41:12]

On 2016/08/10 19:30:41, rkc wrote:
> We definitely have the service data field in BluetoothAdvertisement and we do
> pass it all the way down to BlueZ and as far as I can tell, BlueZ does use
that
> service data.
> 
> I just tested this from the Chrome API and service data shows up just fine.
> 
> Where are you having an issue with this exactly?

I think what you describe is BluetoothAdvertisement for local device. What I
can't find is Advertise data received from remote device.

[rkc, 2016-08-10 19:49:08]

On 2016/08/10 19:41:12, puthik_chromium wrote:
> On 2016/08/10 19:30:41, rkc wrote:
> > We definitely have the service data field in BluetoothAdvertisement and we
do
> > pass it all the way down to BlueZ and as far as I can tell, BlueZ does use
> that
> > service data.
> > 
> > I just tested this from the Chrome API and service data shows up just fine.
> > 
> > Where are you having an issue with this exactly?
> 
> I think what you describe is BluetoothAdvertisement for local device. What I
> can't find is Advertise data received from remote device.

Make sure your advertising packet is under 31 bytes. This means not using 128
bit UUIDs. If you advertise just two 128 bit UUIDs, that is 32 bytes right away,
and hence your advertisement will be rejected. If you set the service data field
correctly before you register the advertisement (see:
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/bluetooth_...),
things should work.

[Luis Héctor Chávez, 2016-08-15 15:30:20]

On 2016/08/08 14:41:37, Luis Héctor Chávez wrote:
> +1 on the test, which would have caught the prepended zeroes.

ping on the tests?

> 
>
https://codereview.chromium.org/2204263003/diff/1/components/arc/bluetooth/ar...
> File components/arc/bluetooth/arc_bluetooth_bridge.cc (right):
> 
>
https://codereview.chromium.org/2204263003/diff/1/components/arc/bluetooth/ar...
> components/arc/bluetooth/arc_bluetooth_bridge.cc:1535:
> mojo::Array<uint16_t>::New(uuid_list.size());
> This would cause |uuid_list.size()| zeroes to be added. L1537 would then add
the
> data.
> 
> You can either use the default constructor and fill the array in the next
loop,
> or keep this and replace each added zero with its real value.

[puthik_chromium, 2016-08-15 21:37:26]

On 2016/08/15 15:30:20, Luis Héctor Chávez wrote:
> On 2016/08/08 14:41:37, Luis Héctor Chávez wrote:
> > +1 on the test, which would have caught the prepended zeroes.
> 
> ping on the tests?
> 
> > 
> >
>
https://codereview.chromium.org/2204263003/diff/1/components/arc/bluetooth/ar...
> > File components/arc/bluetooth/arc_bluetooth_bridge.cc (right):
> > 
> >
>
https://codereview.chromium.org/2204263003/diff/1/components/arc/bluetooth/ar...
> > components/arc/bluetooth/arc_bluetooth_bridge.cc:1535:
> > mojo::Array<uint16_t>::New(uuid_list.size());
> > This would cause |uuid_list.size()| zeroes to be added. L1537 would then add
> the
> > data.
> > 
> > You can either use the default constructor and fill the array in the next
> loop,
> > or keep this and replace each added zero with its real value.

Test is done. This is also rebase to be on top of http://crrev.com/2244703005
because it make more sense to just modify the test from that patch.

[puthik_chromium, 2016-08-15 21:37:54]

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

[commit-bot: I haz the power, 2016-08-15 21:38:20]

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

[puthik_chromium, 2016-08-15 21:39:12]

cc: ortuno

Just fyi since rkc is already review this cl.

[commit-bot: I haz the power, 2016-08-15 22:11:06]

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

[commit-bot: I haz the power, 2016-08-15 22:11:06]

Dry run: This issue passed the CQ dry run.

[rkc, 2016-08-15 22:41:32]

bt usage lgtm

[Luis Héctor Chávez, 2016-08-15 22:47:13]

lgtm with one nit

https://codereview.chromium.org/2204263003/diff/40001/components/arc/bluetoot...
File components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc (right):

https://codereview.chromium.org/2204263003/diff/40001/components/arc/bluetoot...
components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:148:
snprintf(uuid16_str, sizeof(uuid16_str), "%04x",
nit: prefer base::StringPrintf("%04x", ...) over snprintf.

[puthik_chromium, 2016-08-15 23:19:19]

puthik@chromium.org changed reviewers:
+ rickyz@chromium.org

[puthik_chromium, 2016-08-15 23:19:20]

+rickyz for security

https://codereview.chromium.org/2204263003/diff/40001/components/arc/bluetoot...
File components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc (right):

https://codereview.chromium.org/2204263003/diff/40001/components/arc/bluetoot...
components/arc/bluetooth/arc_bluetooth_bridge_unittest.cc:148:
snprintf(uuid16_str, sizeof(uuid16_str), "%04x",
On 2016/08/15 22:47:13, Luis Héctor Chávez wrote:
> nit: prefer base::StringPrintf("%04x", ...) over snprintf.

Done.

[rickyz (no longer on Chrome), 2016-08-17 00:36:00]

lgtm

https://codereview.chromium.org/2204263003/diff/60001/components/arc/bluetoot...
File components/arc/bluetooth/arc_bluetooth_bridge.cc (right):

https://codereview.chromium.org/2204263003/diff/60001/components/arc/bluetoot...
components/arc/bluetooth/arc_bluetooth_bridge.cc:176: uint16_t GetUUID16(const
BluetoothUUID& uuid) {
Let's add a comment explaining why we send a uuid16. May also be worth filing
the bug with Android.

https://codereview.chromium.org/2204263003/diff/60001/components/arc/bluetoot...
components/arc/bluetooth/arc_bluetooth_bridge.cc:1536: // So we will only expose
local_name, service_uuids and service_data.
Should this be updated to service_uuids_16?

[puthik_chromium, 2016-08-17 00:45:50]

https://codereview.chromium.org/2204263003/diff/60001/components/arc/bluetoot...
File components/arc/bluetooth/arc_bluetooth_bridge.cc (right):

https://codereview.chromium.org/2204263003/diff/60001/components/arc/bluetoot...
components/arc/bluetooth/arc_bluetooth_bridge.cc:176: uint16_t GetUUID16(const
BluetoothUUID& uuid) {
On 2016/08/17 00:35:59, rickyz wrote:
> Let's add a comment explaining why we send a uuid16. May also be worth filing
> the bug with Android.

Comment added.

https://codereview.chromium.org/2204263003/diff/60001/components/arc/bluetoot...
components/arc/bluetooth/arc_bluetooth_bridge.cc:1536: // So we will only expose
local_name, service_uuids and service_data.
On 2016/08/17 00:35:59, rickyz wrote:
> Should this be updated to service_uuids_16?

Added comment that UUIDs are 16 bits.

[puthik_chromium, 2016-08-17 00:45:55]

The CQ bit was checked by puthik@chromium.org

[puthik_chromium, 2016-08-17 00:45:55]

The patchset sent to the CQ was uploaded after l-g-t-m from rkc@chromium.org,
lhchavez@chromium.org, rickyz@chromium.org
Link to the patchset: https://codereview.chromium.org/2204263003/#ps80001
(title: "Add comment")

[commit-bot: I haz the power, 2016-08-17 00:46:15]

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, 2016-08-17 01:44:41]

Description was changed from

==========
arc: bluetooth: Use UUID 16 bits in advertising data

For device advertising data received from low energy scan,
Android framework supports UUID 128 bits in services data
field. But, Android Bluetooth package supports only UUID
16 bits.

This patch makes Chrome send UUID 16 bits to Android
instead of UUID 128 bits.

BUG=636478
TEST=Build
==========

to

==========
arc: bluetooth: Use UUID 16 bits in advertising data

For device advertising data received from low energy scan,
Android framework supports UUID 128 bits in services data
field. But, Android Bluetooth package supports only UUID
16 bits.

This patch makes Chrome send UUID 16 bits to Android
instead of UUID 128 bits.

BUG=636478
TEST=Build
==========

[commit-bot: I haz the power, 2016-08-17 01:44:45]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-17 01:46:00]

Description was changed from

==========
arc: bluetooth: Use UUID 16 bits in advertising data

For device advertising data received from low energy scan,
Android framework supports UUID 128 bits in services data
field. But, Android Bluetooth package supports only UUID
16 bits.

This patch makes Chrome send UUID 16 bits to Android
instead of UUID 128 bits.

BUG=636478
TEST=Build
==========

to

==========
arc: bluetooth: Use UUID 16 bits in advertising data

For device advertising data received from low energy scan,
Android framework supports UUID 128 bits in services data
field. But, Android Bluetooth package supports only UUID
16 bits.

This patch makes Chrome send UUID 16 bits to Android
instead of UUID 128 bits.

BUG=636478
TEST=Build

Committed: https://crrev.com/31fcf11302a5dc9670458fe666f86c086191089d
Cr-Commit-Position: refs/heads/master@{#412417}
==========

[commit-bot: I haz the power, 2016-08-17 01:46:01]

Patchset 5 (id:??) landed as
https://crrev.com/31fcf11302a5dc9670458fe666f86c086191089d
Cr-Commit-Position: refs/heads/master@{#412417}