933195 - [B2G] [Bluetooth] Object path should be replaced with device address before broadcasting to BluetoothAdpater

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Gina Yeh [:gyeh] [:ginayeh]]

As Shawn noticed, we send an array of object path to BluetoothAdatper and replace them with device address one by one in BluetoothAdapter.

I think we should do these operations right after receiving signals from BlueZ rather than in the child process.

Comment 1

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v1): Replace object path with device address

It's quite short. Should be an easy one for you, Eric. ;)

Comment 2

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 825828 [details] [diff] [review]
Patch 1(v1): Replace object path with device address

Review of attachment 825828 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits addressed.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +621,5 @@
>     */
>    bool convert = false;
>    if (propertyName.EqualsLiteral("Connected") &&
>        receivedType == DBUS_TYPE_ARRAY) {
> +    MOZ_ASSERT(aPropertyTypes == sDeviceProperties);

Not sure if we need to add this assertion. Interface AudioSink, Control and Input have "Connected" property, too. It seems to be possible that the type of "Connected" property of these interface may change from boolean to array as well (I know it's really a disaster, but it could happen). :(

@@ +690,5 @@
>      MOZ_ASSERT(propertyValue.type() == BluetoothValue::TArrayOfuint8_t);
>  
>      bool b = propertyValue.get_ArrayOfuint8_t()[0];
>      propertyValue = BluetoothValue(b);
> +  } else if (propertyName.EqualsLiteral("Devices")){

super-nit: please insert an extra blank between ')' and '{'. Furthermore, how about leaving a few comments to describe why we did this (translate from object path to address)?

@@ +695,5 @@
> +    MOZ_ASSERT(aPropertyTypes == sAdapterProperties);
> +    MOZ_ASSERT(propertyValue.type() == BluetoothValue::TArrayOfnsString);
> +
> +    uint32_t length = propertyValue.get_ArrayOfnsString().Length();
> +    for (int i = 0; i < length; i++) {

nit: please make sure that 'i' has the same type with 'length' to avoid warnings.

Comment 3

[Gina Yeh [:gyeh] [:ginayeh]]

Thanks for your quick review.

(In reply to Eric Chou [:ericchou] [:echou] from comment #2)
> ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> @@ +621,5 @@
> >     */
> >    bool convert = false;
> >    if (propertyName.EqualsLiteral("Connected") &&
> >        receivedType == DBUS_TYPE_ARRAY) {
> > +    MOZ_ASSERT(aPropertyTypes == sDeviceProperties);
> 
> Not sure if we need to add this assertion. Interface AudioSink, Control and
> Input have "Connected" property, too. It seems to be possible that the type
> of "Connected" property of these interface may change from boolean to array
> as well (I know it's really a disaster, but it could happen). :(

If it happens again, I think that this assertion helps us to observe the change in debug build. For release build, there should be no difference. So, I'd like to keep it.

Comment 4

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: [Final] Replace object path with device address, r=echou

https://tbpl.mozilla.org/?tree=Try&rev=3c472526062d
https://tbpl.mozilla.org/?tree=Try&rev=2347cdfa57e9

Comment 5

[Gina Yeh [:gyeh] [:ginayeh]]

https://tbpl.mozilla.org/?tree=Try&3c472526062d

Comment 6

[Gina Yeh [:gyeh] [:ginayeh]]

Please ignore the above links. Sorry for bothering.

Try server links:
https://tbpl.mozilla.org/?tree=Try&rev=9f1b81f23108
https://tbpl.mozilla.org/?tree=Try&rev=0ad566f884dc

Comment 7

[Gina Yeh [:gyeh] [:ginayeh]]

https://hg.mozilla.org/integration/b2g-inbound/rev/ff8eecaf65e4

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ff8eecaf65e4

Comment 9

[Joe Cheng [:jcheng] (please needinfo)]

already in master v1.3. clear flag