Closed Bug 1152821 Opened 5 years ago Closed 5 years ago

Merge BluetoothCommon.h variants into single file

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

defect
Not set

Tracking

(firefox40 fixed)

RESOLVED FIXED
2.2 S10 (17apr)
Tracking Status
firefox40 --- fixed

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

Bluetooth v1 and v2 contain BluetoothCommon.h with minor differences. The file should be shared between both versions.
Attachment #8590323 - Attachment description: [01] Bug 1152821: Share BluetoothCommon.h between Bluetooth v1 and v2 → [02] Bug 1152821: Share BluetoothCommon.h between Bluetooth v1 and v2
This is the first merged file after bug 1146355 lands.
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?
Attachment #8590323 - Flags: review?(joliu) → review+
Attachment #8590321 - Flags: review?(joliu) → review+
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
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].
Attachment #8590323 - Attachment is obsolete: true
Attachment #8590713 - Flags: review?(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...
Attachment #8590713 - Flags: review?(joliu) → review+
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
It looks like the additional change was not picked up by the rename operation. :(
sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=1696585&repo=b2g-inbound
Flags: needinfo?(tzimmermann)
Flags: needinfo?(tzimmermann)
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.
Attachment #8590713 - Attachment is obsolete: true
Attachment #8590848 - Flags: review?(joliu)
Attachment #8590848 - Flags: review?(joliu) → review+
The patch set failed on non-gcc compilers. Let's try again.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c7c201e359c
(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
Depends on: 1154232
https://hg.mozilla.org/mozilla-central/rev/463cacc401a1
https://hg.mozilla.org/mozilla-central/rev/3c624d0fe037
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S10 (17apr)
You need to log in before you can comment on or make changes to this bug.