1061126 - [Bluetooth] Remove remaining Bluedroid artifacts from Gecko

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

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.

Comment 1

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

Attachment: [01] Bug 1061126: Add AVRCP_UID_SIZE to Bluetooth

Comment 2

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

Attachment: [02] Bug 1061126: Fix constants in Bluedroid HFP manager

Comment 3

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

Attachment: [03] Bug 1061126: Make Bluetooth AVRCP interface generally available

Comment 4

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

Attachment: [04] Bug 1061126: Cleanup ANDROID_VERSION from BluetoothA2DPManager

Comment 5

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

Hi Shawn,

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

Comment 6

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

Attachment: [03] Bug 1061126: Make Bluetooth AVRCP interface generally available (v2)

Changes since v1:

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

This fixes the patch for Flatfish.

Comment 7

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

Attachment: [04] Bug 1061126: Cleanup ANDROID_VERSION from BluetoothA2DPManager (v2)

Changes since v1:

  - rebased onto bug 1061219

Comment 8

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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?

Comment 9

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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?

Comment 10

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

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?

Comment 11

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 12

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

(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 13

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

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.

Comment 14

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

Attachment: [03] Bug 1061126: Make Bluetooth AVRCP interface generally available (v3)

Changes since v2:

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

More Flatfish fixes. It built for me now.

Comment 15

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

Thanks a lot!

https://hg.mozilla.org/integration/b2g-inbound/rev/518ab7491783
https://hg.mozilla.org/integration/b2g-inbound/rev/ad902f1fd574
https://hg.mozilla.org/integration/b2g-inbound/rev/bae6b0b0a4b5
https://hg.mozilla.org/integration/b2g-inbound/rev/92f2ed449adb

https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=92f2ed449adb

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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.