Closed Bug 1061126 Opened 5 years ago Closed 5 years ago

[Bluetooth] Remove remaining Bluedroid artifacts from Gecko

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S4 (12sep)

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(4 files, 3 obsolete files)

After the removal of Bluedroid callbacks, there might be remaining references to Bluedroid in the source code of Gecko. These artifacts should be removed and the Gecko source code should be cleaned up.
Summary: [Bluetooth] Removed remaining Bluedroid artifacts from Gecko → [Bluetooth] Remove remaining Bluedroid artifacts from Gecko
Hi Shawn,

After all these complicated internals, here are some simple patches for cleaning up Bluedroid from the Gecko source code. :)
Changes since v1:

  - don't handle btrc_interface_t in constructor of |BluetoothAvrcpInterface| if ANDROID_VERSION < 18

This fixes the patch for Flatfish.
Attachment #8482228 - Attachment is obsolete: true
Attachment #8482228 - Flags: review?(shuang)
Attachment #8482274 - Flags: review?(shuang)
Depends on: 1061219
Changes since v1:

  - rebased onto bug 1061219
Attachment #8482229 - Attachment is obsolete: true
Attachment #8482229 - Flags: review?(shuang)
Attachment #8482296 - Flags: review?(shuang)
Attachment #8482274 - Attachment description: [03] [03] Bug 1061126: Make Bluetooth AVRCP interface generally available (v2) → [03] Bug 1061126: Make Bluetooth AVRCP interface generally available (v2)
Comment on attachment 8482226 [details] [diff] [review]
[01] Bug 1061126: Add AVRCP_UID_SIZE to Bluetooth

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

::: dom/bluetooth/BluetoothCommon.h
@@ +364,5 @@
>  
> +enum {
> +  AVRCP_UID_SIZE = 8
> +};
> +

Is there any special reason to use enum hack instead of const?
Attachment #8482226 - Flags: review?(shuang) → review+
Comment on attachment 8482227 [details] [diff] [review]
[02] Bug 1061126: Fix constants in Bluedroid HFP manager

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

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +1434,5 @@
>  BluetoothHfpManager::CnumNotification()
>  {
> +  static const uint8_t sAddressType[] {
> +    [HFP_CALL_ADDRESS_TYPE_UNKNOWN] = 0x81,
> +    [HFP_CALL_ADDRESS_TYPE_INTERNATIONAL] = 0x91 // for completeness

This seems to break flatfish build again?
Attachment #8482227 - Flags: review?(shuang)
Hi Shawn

> > +enum {
> > +  AVRCP_UID_SIZE = 8
> > +};
> > +
> 
> Is there any special reason to use enum hack instead of const?

I wouldn't call it a 'hack', as it's usual an commonly used style. There's no actual reason to use this instead of a constant. A constant would have the disadvantage of requiring memory in the binaries data section, while this is built directly into the code.

I'm OK with either. Shall I change it?
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #10)
> Hi Shawn
> 
> > > +enum {
> > > +  AVRCP_UID_SIZE = 8
> > > +};
> > > +
> > 
> > Is there any special reason to use enum hack instead of const?
> 
> I wouldn't call it a 'hack', as it's usual an commonly used style. There's
> no actual reason to use this instead of a constant. A constant would have
> the disadvantage of requiring memory in the binaries data section, while
> this is built directly into the code.
> 
> I'm OK with either. Shall I change it?
No. It's ok for me just out of curiosity. Apparently it's not a problem.
(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #9)
> Comment on attachment 8482227 [details] [diff] [review]
> [02] Bug 1061126: Fix constants in Bluedroid HFP manager
> 
> Review of attachment 8482227 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
> @@ +1434,5 @@
> >  BluetoothHfpManager::CnumNotification()
> >  {
> > +  static const uint8_t sAddressType[] {
> > +    [HFP_CALL_ADDRESS_TYPE_UNKNOWN] = 0x81,
> > +    [HFP_CALL_ADDRESS_TYPE_INTERNATIONAL] = 0x91 // for completeness
> 
> This seems to break flatfish build again?

I just tried again, but it worked. Looks like designated initializers are supported for basic primitive types, such as 'uint8_t'. However I found that [03] breaks Flatfish, despite my attempts to prevent this. I'll upload another patch in a moment.
Comment on attachment 8482227 [details] [diff] [review]
[02] Bug 1061126: Fix constants in Bluedroid HFP manager

Tested and builds on Flatfish. Adding this patch for review again.
Attachment #8482227 - Flags: review?(shuang)
Changes since v2:

  - fix template specialization |CreateProfileInterface<BluetoothAvrcpInterface>|
  - remove ANDROID_VERSION around |DispatchBluetoothAvrcpResult|

More Flatfish fixes. It built for me now.
Attachment #8482274 - Attachment is obsolete: true
Attachment #8482593 - Flags: review?(shuang)
https://hg.mozilla.org/mozilla-central/rev/518ab7491783
https://hg.mozilla.org/mozilla-central/rev/ad902f1fd574
https://hg.mozilla.org/mozilla-central/rev/bae6b0b0a4b5
https://hg.mozilla.org/mozilla-central/rev/92f2ed449adb

Note that this was merged post-uplift, i.e. B2G v2.2.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S4 (12sep)
You need to log in before you can comment on or make changes to this bug.