1149511 - [Bluetooth] Prefer C++ branches over preprocessor conditionals

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

In Bluetooth's backend code, we often have preprocessor conditionals that depend on the Android version.

  #if ANDROID_VERSION >= 21
    ...
  #elif ANDROID_VERSION > 19
    ...
  #else
    ...
  #endif

We should replace them by C++ branches if possible

  if (ANDROID_VERSION >= 21) {
    ...
  } else if (ANDROID_VERSION >= 19) {
    ...
  } else {
    ...
  }

This change will make the C++ compiler process all code and it's less likely that patches breaks for some versions of Android.

This is a good bug for beginners.

Comment 1

[dattaz]

Attachment: bluetooth_bug_1149511.patch

compilation ok. There are tests to run?

Comment 2

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

Comment on attachment 8720357 [details] [diff] [review]
bluetooth_bug_1149511.patch

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

Sorry, this won't compile.

Comment 3

[Mantaroh Yoshinaga[:mantaroh]]

Modified whiteboard.('good-first-bug' to 'good first bug').
For detail, see https://wiki.mozilla.org/Good_first_bug

Comment 4

[Akshay Jain]

Can I work on this bug? Is it too difficult to resolve?

Comment 5

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

(In reply to Akshay Jain from comment #4)
> Can I work on this bug? Is it too difficult to resolve?

Sure, and it shouldn't be too difficult. But I cannot guarantee to be around for reviewing after the end of May.

Comment 6

[Akshay Jain]

Hello Thomas, Could you help me get started?
I am newbie here so I would appreciate it. Thanks

Comment 7

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

Hi

First of all, you have to be able to build and test Bluetooth for B2G. You'll have to know C++.

All code for this bug should be under [1]. There are preprocessor branches like [2]. They should be turned into C++ branches, in the style of 'if (ANDROID_VERSION ... ) {}. These branches implement different variants of an internal protocol, [3] which changes among Android releases.

There are several hard cases, like [4] and [5]. Here the preprocessor affects data structures, and thus cannot be converted directly.

In the case of [4], you'd have to compute the enum values at runtime. I suggest to use the 'ANDROID_VERSION >=19' branch and subtract 1 at runtime if the version is less. A macro could do this:

  #define AVRCP_NFC(_name) \
    ((_name) - (ANDROID_VERSION < 19))

This would map all enum names to the correct enum values, and map OPCODE_REMOTE_FEATURES_NTF to 0x80 if ANDROID_VERSION < 19. 0x80 is always invalid, so it shouldn't interfere with valid opcodes.

That macro might also be useful for resolving [5].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid
[2] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonHandsfreeInterface.cpp#156
[3] https://git.kernel.org/cgit/bluetooth/bluez.git/tree/android/hal-ipc-api.txt
[4] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonAvrcpInterface.h#39
[5] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonAvrcpInterface.cpp#761

Comment 8

[Devansh D]

Hi guys, Is the bug still open? I want to work on this one.

Comment 9

[Ben Tian (inactive)]

Devansh,

The bug is not required now since bug 1310020 removed bluetooth related code from Gecko.

Mark this bug as WONTFIX.