1152821 - Merge BluetoothCommon.h variants into single file

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Bluetooth v1 and v2 contain BluetoothCommon.h with minor differences. The file should be shared between both versions.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1152821: Move dom/bluetooth/bluetooth2/BluetoothCommon.h to dom/bluetooth

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1152821: Share BluetoothCommon.h between Bluetooth v1 and v2

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

This is the first merged file after bug 1146355 lands.

Comment 4

[Jocelyn Liu [:jocelyn] [:joliu]]

Comment on attachment 8590323 [details] [diff] [review]
[02] Bug 1152821: Share BluetoothCommon.h between Bluetooth v1 and v2

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

Hi Thomas,

Please see my comments below and r=me with the fix in Bug 1144089 applied.

Thanks!

::: dom/bluetooth/BluetoothCommon.h
@@ +185,5 @@
> + * event.
> + *
> + * FIXME: maybe bluetooth1 only
> + */
> +#define PAIRED_STATUS_CHANGED_ID             "pairedstatuschanged"

Yes, it is bluetooth1 only.
Bluetooth2 use devicepaired/deviceunpaired.

@@ +192,5 @@
> + * This event would be fired when discovery procedure starts or stops.
> + *
> + * FIXME: maybe bluetooth1 only
> + */
> +#define DISCOVERY_STATE_CHANGED_ID           "discoverystatechanged"

Yes, it is bluetooth1 only.
Bluetooth2 use "attributechanged", and this string is used in Manager, Adapter, Device, but haven't been defined here yet.

@@ +529,1 @@
>  

Bug 1144089 fix AVRCP attribute ID mapping in BluetoothAvrcpMediaAttribute, I think the fix is missing here?

Comment 5

[Jocelyn Liu [:jocelyn] [:joliu]]

Just a note, Bug 1152702 has landed[1] for bluetooth2 today.
You might need to rebase upon this, thanks!

[1] https://hg.mozilla.org/integration/b2g-inbound/rev/013338161940

Comment 6

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1152821: Share BluetoothCommon.h between Bluetooth v1 and v2 (v1)

Changes since v1:

  - incorporate missing changes from bug 1144089
  - update CLOBBER, just in case

I checked the diff again and found only media attributes to be missing. The changes of bug 1152702 should be picked up automatically by the 'hg rename' operation in patch [01].

Comment 7

[Jocelyn Liu [:jocelyn] [:joliu]]

Comment on attachment 8590713 [details] [diff] [review]
[02] Bug 1152821: Share BluetoothCommon.h between Bluetooth v1 and v2 (v1)

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

Overall looks good to me, just a nit I didn't catch it last time.

::: dom/bluetooth/BluetoothCommon.h
@@ +4,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#ifndef mozilla_dom_bluetooth_bluetoothcommon_h
> +#define mozilla_dom_bluetooth_bluetoothcommon_h

nit:
Let's open a bug to revise all our include guards to follow the coding style later?
I'm OK if you revise only this one here first since there's already inconsistency in our code base...

Comment 8

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks, Jocelyn. I don't really mind having a separate bug for the guard cleanup. But since it's such a trivial change, I'm also OK with fixing them in the merge patches.

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

https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=eb14bd0103b6

Comment 9

[Thomas Zimmermann [:tzimmermann] [:tdz]]

It looks like the additional change was not picked up by the rename operation. :(

Comment 10

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=1696585&repo=b2g-inbound

Comment 11

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1152821: Share BluetoothCommon.h between Bluetooth v1 and v2 (v3)

Changes since v2:

  - replaced TYPE_INVALID by NUM_TYPE

So changes got picked up from b2g-inbound, but the v1 IPC code wasn't updated yet. I didn't see this error during testing because bug 1152702 is still in b2g-inbound. I fixed the respective code in v1. This should work now.

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

The missing patch has landed in m-c and the following changes built locally for. Trying again...

https://hg.mozilla.org/integration/b2g-inbound/rev/3939c36b0864
https://hg.mozilla.org/integration/b2g-inbound/rev/81c108144278

https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=81c108144278

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

The patch set failed on non-gcc compilers. Let's try again.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c7c201e359c

Comment 14

[Carsten Book [:Tomcat]]

caued out again in https://treeherder.mozilla.org/logviewer.html#?job_id=1705943&repo=b2g-inbound

Comment 15

[Carsten Book [:Tomcat]]

(In reply to Carsten Book [:Tomcat] from comment #14)
> caued out again in
> https://treeherder.mozilla.org/logviewer.html#?job_id=1705943&repo=b2g-
> inbound

in/for

Comment 16

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Try looks good...

https://hg.mozilla.org/integration/b2g-inbound/rev/463cacc401a1
https://hg.mozilla.org/integration/b2g-inbound/rev/3c624d0fe037

https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=3c624d0fe037

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/463cacc401a1
https://hg.mozilla.org/mozilla-central/rev/3c624d0fe037