The interface of |BluetoothInterface| is still specific to Bluedroid. We need to change all methods' arguments to Gecko types and do the conversion internally.

Shawn, Here are the patches for making the Bluetooth interface methods backend-agnostic. The conversion functions heavily depend on overloading and templates. It is handy here to let the compiler select the correct methods, and it simplifies the interface methods to a good deal. Later this implementation will become extremely useful for supporting notifications (from Bluedroid to Gecko). I have preliminary patches for that where all the conversion details are handled by the compiler.

Ben, This is a large patchset. I hope you can give some feedback on the patches before they reach bluedroid2/.

Changes since v1: - fixes variable type in |Convert(const ConvertArray<Tin>& aIn, nsAutoArrayPtr<Tout>& aOut)|

Changes since v1: - rebased on [05] (v2)

Comment on attachment 8460814 [details] [diff] [review] [01] Bug 1038591: Convert Bluetooth data types in |BluetoothInterface| Review of attachment 8460814 [details] [diff] [review]: ----------------------------------------------------------------- f=me with comment addressed. Thanks. ::: dom/bluetooth/bluedroid/BluetoothInterface.cpp @@ +43,5 @@ > +Convert(bool aIn, bt_scan_mode_t& aOut) > +{ > + static const bt_scan_mode_t sScanMode[] = { > + [false] = BT_SCAN_MODE_CONNECTABLE_DISCOVERABLE, > + [true] = BT_SCAN_MODE_CONNECTABLE Exchange the two values. Since BT_SCAN_MODE_CONNECTABLE_DISCOVERABLE stands for discoverable=true and BT_SCAN_MODE_CONNECTABLE is false. @@ +143,5 @@ > + > +static nsresult > +Convert(const uint8_t aIn[16], bt_uuid_t& aOut) > +{ > + if (sizeof(aOut.uu) == 16) { Should be != 16. @@ +172,5 @@ > + > + // Clear remaining bytes in aOut > + size_t ntrailing = > + (MOZ_ARRAY_LENGTH(aOut.pin) - aIn.Length()) * sizeof(aOut.pin[0]); > + memset(aOut.pin + i, 0, ntrailing); It's clearer to replace |i| with |aIn.Length()|. @@ +1493,5 @@ > + int status; > + ConvertNamedValue convertProperty(aProperty); > + bt_property_t property; > + > + if (!NS_FAILED(Convert(convertProperty, property))) { Use NS_SUCCEEDED instead. @@ +1771,5 @@ > + int status; > + uint8_t enable; > + > + if (!NS_FAILED(Convert(aEnable, enable))) { > + status = mInterface->dut_mode_configure(aEnable); Should be |enable|. ::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp @@ +1205,5 @@ > > for (int i = 0; i < requestedDeviceCount; i++) { > // Retrieve all properties of devices > bt_bdaddr_t addressType; > StringToBdAddressType(aDeviceAddress[i], &addressType); Remove this conversion.

Comment on attachment 8460815 [details] [diff] [review] [02] Bug 1038591: Convert Bluetooth Socket data types in |BluetoothSocketInterface| Review of attachment 8460815 [details] [diff] [review]: ----------------------------------------------------------------- f=me with comment addressed. ::: dom/bluetooth/bluedroid/BluetoothInterface.cpp @@ +180,5 @@ > > +static nsresult > +Convert(const bt_bdaddr_t& aIn, nsAString& aOut) > +{ > + char str[18]; Use defined constant: char str[BLUETOOTH_ADDRESS_LENGTH + 1];

Comment on attachment 8460816 [details] [diff] [review] [03] Bug 1038591: Convert Bluetooth Handsfree data types in |BluetoothHandsfreeInterface| Review of attachment 8460816 [details] [diff] [review]: ----------------------------------------------------------------- f=me with comment addressed. Thanks. ::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp @@ +1313,5 @@ > return nsITelephonyService::CALL_STATE_DISCONNECTED; > } > > +BluetoothHandsfreeCallState > +BluetoothHfpManager::ConvertToBluetoothHandsfreeCallState(int aCallState) const We should integrate this function into |BluetoothInterface::Convert| and replace |BluetoothHandsfreeCallState| with telephony call state. Please open a follow-up bug for this.

Comment on attachment 8460817 [details] [diff] [review] [04] Bug 1038591: Convert Bluetooth A2DP data types in |BluetoothA2dpInterface| Review of attachment 8460817 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.

Comment on attachment 8460842 [details] [diff] [review] [05] Bug 1038591: Convert Bluetooth AVRCP data types in |BluetoothAvrcpInterface| (v2) Review of attachment 8460842 [details] [diff] [review]: ----------------------------------------------------------------- f=me with comment addressed. Thanks. ::: dom/bluetooth/bluedroid/BluetoothInterface.cpp @@ +453,5 @@ > + const T* mData; > + unsigned long mLength; > +}; > + > +/* This implementation of |Convert| converts the elements of and typo: an array

Comment on attachment 8460848 [details] [diff] [review] [06] Bug 1038591: Convert Bluedroid status codes and error handlers (v2) Review of attachment 8460848 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.

Comment on attachment 8460814 [details] [diff] [review] [01] Bug 1038591: Convert Bluetooth data types in |BluetoothInterface| Review of attachment 8460814 [details] [diff] [review]: ----------------------------------------------------------------- Overall LGTM, but some parts need to be fixed. ::: dom/bluetooth/bluedroid/BluetoothInterface.cpp @@ +41,5 @@ > + > +static nsresult > +Convert(bool aIn, bt_scan_mode_t& aOut) > +{ > + static const bt_scan_mode_t sScanMode[] = { bt_scan_mode_t actually maps (1). Non-connectable and non-discoverable (BT_SCAN_MODE_NONE) (2). Connectable and non-discoverable (BT_SCAN_MODE_CONNECTABLE) (3). Connectable and discoverable (BT_SCAN_MODE_CONNECTABLE_DISCOVERABLE) It shall be reversed. @@ +137,5 @@ > +static nsresult > +Convert(const bool& aIn, uint8_t& aOut) > +{ > + aOut = static_cast<uint8_t>(aIn); > + return NS_OK; Please add one line comment to describe this function is trying to convert. From the argument, hard to identify the purpose.

Comment on attachment 8460815 [details] [diff] [review] [02] Bug 1038591: Convert Bluetooth Socket data types in |BluetoothSocketInterface| Review of attachment 8460815 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/bluedroid/BluetoothInterface.cpp @@ +211,5 @@ > + [0] = static_cast<btsock_type_t>(0), // invalid, [0] required by gcc > + [BluetoothSocketType::RFCOMM] = BTSOCK_RFCOMM, > + [BluetoothSocketType::SCO] = BTSOCK_SCO, > + [BluetoothSocketType::L2CAP] = BTSOCK_L2CAP, > + [BluetoothSocketType::EL2CAP] = BTSOCK_L2CAP bluedroid currently doesn't support EL2CAP but bluez does, but we probably add FIXME here, because L2CAP, EL2CAP are different.

> (2). Connectable and non-discoverable (BT_SCAN_MODE_CONNECTABLE) > (3). Connectable and discoverable (BT_SCAN_MODE_CONNECTABLE_DISCOVERABLE) I remember that I was surprised that the values were inverted when I first wrote the code, but it seemed correct at that time. I checked this again when Ben mentioned it and now I have now idea why I got it wrong in the first place. :)

> > ::: dom/bluetooth/bluedroid/BluetoothInterface.cpp > @@ +211,5 @@ > > + [0] = static_cast<btsock_type_t>(0), // invalid, [0] required by gcc > > + [BluetoothSocketType::RFCOMM] = BTSOCK_RFCOMM, > > + [BluetoothSocketType::SCO] = BTSOCK_SCO, > > + [BluetoothSocketType::L2CAP] = BTSOCK_L2CAP, > > + [BluetoothSocketType::EL2CAP] = BTSOCK_L2CAP > > bluedroid currently doesn't support EL2CAP but bluez does, but we probably > add FIXME here, because L2CAP, EL2CAP are different. There's no BTSOCK_EL2CAP. :( Do you know about possible support in the future? Returning 'invalid parameter' should probably be the way to handle this for now.

Comment on attachment 8460817 [details] [diff] [review] [04] Bug 1038591: Convert Bluetooth A2DP data types in |BluetoothA2dpInterface| Review of attachment 8460817 [details] [diff] [review]: ----------------------------------------------------------------- Super!

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #20) > > > > ::: dom/bluetooth/bluedroid/BluetoothInterface.cpp > > @@ +211,5 @@ > > > + [0] = static_cast<btsock_type_t>(0), // invalid, [0] required by gcc > > > + [BluetoothSocketType::RFCOMM] = BTSOCK_RFCOMM, > > > + [BluetoothSocketType::SCO] = BTSOCK_SCO, > > > + [BluetoothSocketType::L2CAP] = BTSOCK_L2CAP, > > > + [BluetoothSocketType::EL2CAP] = BTSOCK_L2CAP > > > > bluedroid currently doesn't support EL2CAP but bluez does, but we probably > > add FIXME here, because L2CAP, EL2CAP are different. > > There's no BTSOCK_EL2CAP. :( Do you know about possible support in the > future? Returning 'invalid parameter' should probably be the way to handle > this for now. I'm not sure when Android Bluetooth HAL will add EL2CAP packet type (actually even L2CAP currently is not exposed to application) but we can return unsupported parameter/feature here maybe.

Comment on attachment 8460814 [details] [diff] [review] [01] Bug 1038591: Convert Bluetooth data types in |BluetoothInterface| Review of attachment 8460814 [details] [diff] [review]: ----------------------------------------------------------------- f=me with nits addressed. ::: dom/bluetooth/bluedroid/BluetoothInterface.cpp @@ +155,5 @@ > + > +static nsresult > +Convert(const nsAString& aIn, bt_pin_code_t& aOut) > +{ > + if (aIn.Length() >= MOZ_ARRAY_LENGTH(aOut.pin)) { |aOut.pin| is a 16-byte array, which is the MAX size of a valid PIN code. |aIn| can be less or equal than 16 bytes, hence '>' should be used instead of '>='. @@ +169,5 @@ > + for (i = 0; i < aIn.Length(); ++i, ++str) { > + aOut.pin[i] = static_cast<uint8_t>(*str); > + } > + > + // Clear remaining bytes in aOut Suggestion: how about doing this way - (1) memset the whole aOut.pin to 0 (2) memcpy from str to aOut.pin with size aIn.Length() This is based on the length check at the beginning of this function, so that we can do memcpy safely.

Comment on attachment 8460815 [details] [diff] [review] [02] Bug 1038591: Convert Bluetooth Socket data types in |BluetoothSocketInterface| Review of attachment 8460815 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.

Comment on attachment 8460842 [details] [diff] [review] [05] Bug 1038591: Convert Bluetooth AVRCP data types in |BluetoothAvrcpInterface| (v2) Review of attachment 8460842 [details] [diff] [review]: ----------------------------------------------------------------- |GetElementAttrRsp| seems to be incomplete, please confirm again. ::: dom/bluetooth/bluedroid/BluetoothInterface.cpp @@ +1735,5 @@ > { > + bt_status_t status; > + btrc_element_attr_val_t* pAttrs; > + > + if (false /* TODO: we don't support any attributes currently */) { We have to support this function. This function is not part of getter/setter for Player App settings. Instead, it returns element attribute that the remote device requested. @@ +1736,5 @@ > + bt_status_t status; > + btrc_element_attr_val_t* pAttrs; > + > + if (false /* TODO: we don't support any attributes currently */) { > + status = mInterface->get_element_attr_rsp(aNumAttr, pAttrs); |pAttrs| seems did nothing here. I expect you would convert aIds and aTexts to |btrc_element_attr_val_t|.

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #25) > Comment on attachment 8460842 [details] [diff] [review] > [05] Bug 1038591: Convert Bluetooth AVRCP data types in > |BluetoothAvrcpInterface| (v2) > > Review of attachment 8460842 [details] [diff] [review]: > ----------------------------------------------------------------- > > |GetElementAttrRsp| seems to be incomplete, please confirm again. Yes. If an interface is not used in our code and has non-trivial argument (structures, array pointers), I left it out. I though this would be better then converting arguments without any clue how they are actually used. Looking at the related methods in BluetoothInterface.cpp, I guess they should fail hard, so that future users are aware that something's missing.

Changes since v1: - fixed conversion of scan mode - fixed several length checks in conversion functions - use |aIn.Length()| for memcpy offset - replace '!NS_FAILED' by 'NS_SUCCEED' - fixed call to |dut_mode_configure| - removed left-over conversion in BluetoothServiceBluedroid.cpp| - fail hard in un-finished interface methods - comment on bool to uint8_t conversion

Changes since v1: - compute array length from BLUETOOTH_ADDRESS_LENGTH - fail for unsupported EL2CAP - replace '!NS_FAILED' by 'NS_SUCCEEDED'

Changes since v1: - replaced '!NS_FAILED' by 'NS_SUCCEEDED'

Changes since v1: - replaced '!NS_FAILED' by 'NS_SUCCEEDED'

Changes since v2: - replaced '!NS_FAILED' by 'NS_SUCCEEDED' - fail hard in incomplete interface methods - fixed a typo

Changes since v3: - rebased on [04] (v2)

Comment on attachment 8465440 [details] [diff] [review] [05] Bug 1038591: Convert Bluetooth AVRCP data types in |BluetoothAvrcpInterface| (v4) Review of attachment 8465440 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/bluedroid/BluetoothInterface.cpp @@ +1748,5 @@ > + btrc_element_attr_val_t* pAttrs; > + > + /* FIXME: you need to implement the missing conversion functions */ > + NS_NOTREACHED("Conversion function missing"); > + Hmm, if we don't implement this function, the remote avrcp 1.3 meta data function would fail to work. That means we need to have a follow-up to fix this. Unlike Avrcp 1.3 Get/Set Player App settings, GetElementAttrRsp is mandatory function for AVRCP 1.3. How about we open another bug to fix this?

Comment on attachment 8465427 [details] [diff] [review] [01] Bug 1038591: Convert Bluetooth data types in |BluetoothInterface| (v2) Review of attachment 8465427 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, and I also tested the basic function.

Changes since v2: - rebased onto latest trunk rev

Changes since v4: - implemented support for GetElementAttrRsp

Changes since v2: - rebased onto latest trunk rev

Comment on attachment 8467099 [details] [diff] [review] [05] Bug 1038591: Convert Bluetooth AVRCP data types in |BluetoothAvrcpInterface| (v5) Review of attachment 8467099 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for your patience. It looks good to me. I also tested with all AVRCP attribute ids and meta data sending correctly, including multiple-bytes meta data title. ::: dom/bluetooth/bluedroid/BluetoothInterface.cpp @@ +496,5 @@ > +static nsresult > +Convert(const ConvertArray<Tin>& aIn, nsAutoArrayPtr<Tout>& aOut) > +{ > + aOut = new Tout[aIn.mLength]; > + nit:extra space

Changes since v5: - removed empty line

Changes since v3: - rebased onto [05] (v6)

https://hg.mozilla.org/integration/b2g-inbound/rev/79174501335c https://hg.mozilla.org/integration/b2g-inbound/rev/04194231399e https://hg.mozilla.org/integration/b2g-inbound/rev/32deb2e3031c https://hg.mozilla.org/integration/b2g-inbound/rev/cf3bcd7c1082 https://hg.mozilla.org/integration/b2g-inbound/rev/d55ed75e12a0 https://hg.mozilla.org/integration/b2g-inbound/rev/2b8c20e476c2 https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=2b8c20e476c2

https://hg.mozilla.org/mozilla-central/rev/79174501335c https://hg.mozilla.org/mozilla-central/rev/04194231399e https://hg.mozilla.org/mozilla-central/rev/32deb2e3031c https://hg.mozilla.org/mozilla-central/rev/cf3bcd7c1082 https://hg.mozilla.org/mozilla-central/rev/d55ed75e12a0 https://hg.mozilla.org/mozilla-central/rev/2b8c20e476c2

../../../gecko/dom/bluetooth/bluedroid/BluetoothInterface.cpp: In lambda function: ../../../gecko/dom/bluetooth/bluedroid/BluetoothInterface.cpp:30:25: error: expected '{' before '=' token ../../../gecko/dom/bluetooth/bluedroid/BluetoothInterface.cpp: In function 'nsresult mozilla::dom::bluetooth::Convert(bt_status_t, mozilla::dom::bluetooth::BluetoothStatus&)': ../../../gecko/dom/bluetooth/bluedroid/BluetoothInterface.cpp:30:27: error: no match for 'operator=' in '{(bt_status_t)0u} = (mozilla::dom::bluetooth::BluetoothStatus)0u' ../../../gecko/dom/bluetooth/bluedroid/BluetoothInterface.cpp:30:27: note: candidate is: ../../../gecko/dom/bluetooth/bluedroid/BluetoothInterface.cpp:30:23: note: mozilla::dom::bluetooth::Convert(bt_status_t, mozilla::dom::bluetooth::BluetoothStatus&)::<lambda()>& mozilla::dom::bluetooth::Convert(bt_status_t, mozilla::dom::bluetooth::BluetoothStatus&)::<lambda()>::operator=(const mozilla::dom::bluetooth::Convert(bt_status_t, mozilla::dom::bluetooth::BluetoothStatus&)::<lambda()>&) <deleted> ../../../gecko/dom/bluetooth/bluedroid/BluetoothInterface.cpp:30:23: note: no known conversion for argument 1 from 'mozilla::dom::bluetooth::BluetoothStatus' to 'const mozilla::dom::bluetooth::Convert(bt_status_t, mozilla::dom::bluetooth::BluetoothStatus&)::<lambda()>&'