Closed Bug 1207649 Opened 5 years ago Closed 5 years ago

Use |BluetoothAddress| throughout Bluetooth backend interface

Categories

(Firefox OS Graveyard :: Bluetooth, defect, P2)

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:2.5+, firefox44 fixed)

RESOLVED FIXED
FxOS-S8 (02Oct)
blocking-b2g 2.5+
Tracking Status
firefox44 --- fixed

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(8 files, 9 obsolete files)

11.62 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
3.71 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
51.70 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
12.20 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
55.47 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
2.68 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
47.39 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
9.04 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
We currently represent most Bluetooth addresses as strings. This has a number of drawbacks: such as memory and CPU overhead, fragile string parsing, and explicit conversions to binary data in the backend code.

This bug is about moving the string-based address representation from the backend into the Bluetooth mid-layer and profile managers. The long-term perspective is to move address strings into the DOM layer and convert them as early as possible to binary representation.
Comment on attachment 8664946 [details] [diff] [review]
[04] Bug 1207649: Convert Bluetooth Handsfree backend to |BluetoothAddress|

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

Thomas,

What's the reason to put string/address conversion in hfp manager instead of hfp daemon interface?

::: dom/bluetooth/bluedroid/BluetoothDaemonHandsfreeInterface.cpp
@@ +1002,3 @@
>       */
>  #if ANDROID_VERSION >= 21
> +    rv = UnpackPDU( pdu, aArg2);

nit: remove space after (
Set ni? per comment 9.
Flags: needinfo?(tzimmermann)
Hi Ben

> 
> What's the reason to put string/address conversion in hfp manager instead of
> hfp daemon interface?

I only briefly outlined this in the bug description. Let me give a more complete explanation.

Using strings for storing most of our data (addresses, UUIDs, pins, etc) is results from the early days of Bluetooth, when it was just a wrapper around BlueZ. BlueZ uses strings in it's interfaces, and the Bluetooth code was built around it.

Compared to binary data structures (|BluetoothAddress|, |BluetoothUuid|, etc), strings have a number of drawbacks, such as

  * memory overhead when storing the data, and
  * CPU overhead for comparing, copying, etc.

And there is also the additional problem of conversion between strings and raw representation. If the string is malformed, the conversion fails. This makes parsing fragile and can easily lead to bugs.

We can fix these problems by using binary data in our mid-layer code and profile managers. The strings that come from the DOM API should be converted as early as possible.

The patch set is not yet the full solution. The purpose of the patch set is to introduce |BluetoothAddress| in the Bluetooth mid-layer and profile managers. A later patch set will further change them to use binary data as well, and move the string conversion into the DOM front-end code. This will simplify many of the current conversion in the mid-layer.

My hope is that we might be able to handle all strings in the App process. When the calls a DOM interface, we'd convert the input strings in the App process. For the rest of the call, we'd use binary data. That would restrict the overhead of the strings to the App process; and if the string parsing/conversion fails, it fails in the (un-privilegued) App process.
Flags: needinfo?(tzimmermann)
Attachment #8664942 - Flags: feedback?(brsun)
Attachment #8664943 - Flags: feedback?(brsun)
Attachment #8664945 - Flags: feedback?(brsun)
Attachment #8664946 - Flags: feedback?(brsun)
Comment on attachment 8664942 [details] [diff] [review]
[01] Bug 1207649: Prepare |BluetoothAddress| for general use throughout Bluetooth code

Forward review to new bluetooth module peer Bruce since he's familiar with daemon related code.
Attachment #8664942 - Flags: review?(btian)
Attachment #8664942 - Flags: review?(brsun)
Attachment #8664942 - Flags: feedback?(brsun)
Attachment #8664943 - Flags: review?(btian)
Attachment #8664943 - Flags: review?(brsun)
Attachment #8664943 - Flags: feedback?(brsun)
Attachment #8664945 - Flags: review?(btian)
Attachment #8664945 - Flags: review?(brsun)
Attachment #8664945 - Flags: feedback?(brsun)
Attachment #8664946 - Flags: review?(btian)
Attachment #8664946 - Flags: review?(brsun)
Attachment #8664946 - Flags: feedback?(brsun)
Attachment #8664950 - Flags: review?(btian) → review?(brsun)
Comment on attachment 8664942 [details] [diff] [review]
[01] Bug 1207649: Prepare |BluetoothAddress| for general use throughout Bluetooth code

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

r=me if the following suggestions would be addressed.

Feel free to share your ideas if some of them don't make sense to you.

::: dom/bluetooth/common/BluetoothCommon.h
@@ +310,5 @@
> +struct BluetoothAddress {
> +
> +  static const BluetoothAddress ANY;
> +  static const BluetoothAddress ALL;
> +  static const BluetoothAddress LOCAL;

From the naming of these three static |BluetoothAddress|, it seems we could use them for bit-wise operation such as AND or OR or even XOR. It would be a very interesting use case to dig into the value of each segment of the Bluetooth address, but I am not sure how we might use them in real scenarios in Gecko. I think it is good to have these predefined constants. Just curious about the usage.

I've noticed that |BluetoothAddress::ANY| is used to reset some |BluetoothAddress| instances, would it be good to have another predefined constant named |BluetoothAddress::EMPTY| (or some other naming) to explicitly describe the cleanup usage of this constant? Although |BluetoothAddress::EMPTY| might have the same value of |BluetoothAddress::ANY| in the memory and seems as a duplicate, I would suggest not to use |BluetoothAddress::ANY| for reset/cleanup purpose. So that the code itself would be more self-explanatory. What do you think?

@@ +314,5 @@
> +  static const BluetoothAddress LOCAL;
> +
> +  uint8_t mAddr[6];
> +
> +  BluetoothAddress() = default;

How about initialize |mAddr| with an empty value or some value that represented as an invalid address? So that we can avoid handling |BluetoothAddress| instances with undefined values in any cases.

::: dom/bluetooth/common/BluetoothHashKeys.h
@@ +53,5 @@
> +                         (static_cast<uint64_t>(aKey->mAddr[1]) << 32) |
> +                         (static_cast<uint64_t>(aKey->mAddr[2]) << 24) |
> +                         (static_cast<uint64_t>(aKey->mAddr[3]) << 16) |
> +                         (static_cast<uint64_t>(aKey->mAddr[4]) << 8) |
> +                         (static_cast<uint64_t>(aKey->mAddr[5]) << 0));

By definition[1], |PLDHashNumber| is a 32-bit unsigned integer. During the conversion from 64-bit to 32-bit, information of higher 32-bit would be always dropped. Probably using |HashKnownLength|[2] or |HashBytes|[3] to hash all bytes would be better in this case.

[1] https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/PLDHashTable.h#26
[2] https://dxr.mozilla.org/mozilla-central/source/mfbt/HashFunctions.h?offset=1100#226
[3] https://dxr.mozilla.org/mozilla-central/source/mfbt/HashFunctions.h?offset=1700#313

::: dom/bluetooth/common/BluetoothUtils.cpp
@@ +51,5 @@
> +                   &aAddress.mAddr[2],
> +                   &aAddress.mAddr[3],
> +                   &aAddress.mAddr[4],
> +                   &aAddress.mAddr[5]);
> +  if (res < static_cast<ssize_t>(MOZ_ARRAY_LENGTH(aAddress.mAddr))) {

It is possible that |sscanf| returns |EOF| value. Although |EOF| might be the same as -1 in most cases, it would be good to design our logic without this assumption. Suggest explicitly compare the value of |res| with |EOF| in the condition.
Attachment #8664942 - Flags: review?(brsun) → review+
(In reply to Bruce Sun [:brsun] from comment #13)
> Comment on attachment 8664942 [details] [diff] [review]
> [01] Bug 1207649: Prepare |BluetoothAddress| for general use throughout
> Bluetooth code
> 
> Review of attachment 8664942 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me if the following suggestions would be addressed.
> 
> Feel free to share your ideas if some of them don't make sense to you.
> 
> ::: dom/bluetooth/common/BluetoothCommon.h
> @@ +310,5 @@
> > +struct BluetoothAddress {
> > +
> > +  static const BluetoothAddress ANY;
> > +  static const BluetoothAddress ALL;
> > +  static const BluetoothAddress LOCAL;
> 
> From the naming of these three static |BluetoothAddress|, it seems we could
> use them for bit-wise operation such as AND or OR or even XOR. It would be a
> very interesting use case to dig into the value of each segment of the
> Bluetooth address, but I am not sure how we might use them in real scenarios
> in Gecko. I think it is good to have these predefined constants. Just
> curious about the usage.

These constants are defined by BlueZ headers at [1]. We obviously use ANY and I also took the others as I was working on it anyway.

I tried to find the definition in a spec. Bluetooth 4.2 [2] referred to IEEE 802-2001, sec 9.2 [3]. So they are just Ethernet addresses, sort of. I'd welcome pointers to the exact definition of these constants.

> I've noticed that |BluetoothAddress::ANY| is used to reset some
> |BluetoothAddress| instances, would it be good to have another predefined
> constant named |BluetoothAddress::EMPTY| (or some other naming) to
> explicitly describe the cleanup usage of this constant? Although
> |BluetoothAddress::EMPTY| might have the same value of
> |BluetoothAddress::ANY| in the memory and seems as a duplicate, I would
> suggest not to use |BluetoothAddress::ANY| for reset/cleanup purpose. So
> that the code itself would be more self-explanatory. What do you think?

I'd say we should not introduce more constants that are not covered by any specification or related work.

I'm also skeptical about defining bit operators, because one usually cannot apply bit patterns onto network addresses and expect to get meaningful results.

But let's add some helper methods to |BluetoothAddress|. |Clear| could set the address to ANY. We can also add getters and setters for the LAP, UAP and NAP fields of the address. OK?

> 
> @@ +314,5 @@
> > +  static const BluetoothAddress LOCAL;
> > +
> > +  uint8_t mAddr[6];
> > +
> > +  BluetoothAddress() = default;
> 
> How about initialize |mAddr| with an empty value or some value that
> represented as an invalid address? So that we can avoid handling
> |BluetoothAddress| instances with undefined values in any cases.

Makes sense to me.

> ::: dom/bluetooth/common/BluetoothHashKeys.h
> @@ +53,5 @@
> > +                         (static_cast<uint64_t>(aKey->mAddr[1]) << 32) |
> > +                         (static_cast<uint64_t>(aKey->mAddr[2]) << 24) |
> > +                         (static_cast<uint64_t>(aKey->mAddr[3]) << 16) |
> > +                         (static_cast<uint64_t>(aKey->mAddr[4]) << 8) |
> > +                         (static_cast<uint64_t>(aKey->mAddr[5]) << 0));
> 
> By definition[1], |PLDHashNumber| is a 32-bit unsigned integer. During the
> conversion from 64-bit to 32-bit, information of higher 32-bit would be
> always dropped. Probably using |HashKnownLength|[2] or |HashBytes|[3] to
> hash all bytes would be better in this case.

Sure. I wasn't aware of that.

> ::: dom/bluetooth/common/BluetoothUtils.cpp
> @@ +51,5 @@
> > +                   &aAddress.mAddr[2],
> > +                   &aAddress.mAddr[3],
> > +                   &aAddress.mAddr[4],
> > +                   &aAddress.mAddr[5]);
> > +  if (res < static_cast<ssize_t>(MOZ_ARRAY_LENGTH(aAddress.mAddr))) {
> 
> It is possible that |sscanf| returns |EOF| value. Although |EOF| might be
> the same as -1 in most cases, it would be good to design our logic without
> this assumption. Suggest explicitly compare the value of |res| with |EOF| in
> the condition.

Makes sense to me.

[1] https://android.googlesource.com/platform/external/bluetooth/bluez/+/master/lib/bluetooth/bluetooth.h#134
[2] https://www.bluetooth.org/DocMan/handlers/DownloadDoc.ashx?doc_id=286439
[3] http://www.ieee802.org/secmail/pdfYD89wBpRqH.pdf
[4] https://msdn.microsoft.com/en-us/library/cc510863.aspx
> I'd say we should not introduce more constants that are not covered by any
> specification or related work.
> 
> I'm also skeptical about defining bit operators, because one usually cannot
> apply bit patterns onto network addresses and expect to get meaningful
> results.
> 
> But let's add some helper methods to |BluetoothAddress|. |Clear| could set
> the address to ANY. We can also add getters and setters for the LAP, UAP and
> NAP fields of the address. OK?

It sounds good.
Trying to describe more for comment 15.

> |Clear| could set the address to ANY.
This does match what was suggested. Thanks.

> We can also add getters and setters for the LAP, UAP and NAP fields of the address. OK?
It is good to have these implementation in this bug, but it's also OK for me if these are planed to be addressed on other bug.
Comment on attachment 8664943 [details] [diff] [review]
[02] Bug 1207649: Convert Bluetooth Core backend to |BluetoothAddress|

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

Suppose |BluetoothAddress| would be used entirely within Gecko in the near future, the overhead of |StringToAddress()| and |AddressToAddress()| and the corresponding error handling could be saved while all the refactor stuffs are done. But at this moment, some trivial error handling might still be needed due to the refactor stuffs are still in progress. Hope we can eliminate those overhead soon. :)

r=me if the following suggestions would be addressed.

::: dom/bluetooth/bluedroid/BluetoothServiceBluedroid.cpp
@@ +730,5 @@
> +
> +  BluetoothAddress address;
> +  nsresult rv = StringToAddress(addressString, address);
> +  if (NS_FAILED(rv)) {
> +    return rv;

Add |DispatchReplyError(aRunnable, ...)| before returning the function.

@@ +765,5 @@
> +
> +    BluetoothAddress address;
> +    nsresult rv = StringToAddress(aDeviceAddress[i], address);
> +    if (NS_FAILED(rv)) {
> +      return rv;

Add |DispatchReplyError(aRunnable, ...)| before returning the function.

@@ +833,5 @@
>  
> +  BluetoothAddress address;
> +  nsresult rv = StringToAddress(aDeviceAddress, address);
> +  if (NS_FAILED(rv)) {
> +    return rv;

Add |DispatchReplyError(aRunnable, ...)| before returning the function.

@@ +1095,5 @@
>  
> +  BluetoothAddress address;
> +  nsresult rv = StringToAddress(aDeviceAddress, address);
> +  if (NS_FAILED(rv)) {
> +    return rv;

Add |DispatchReplyError(aRunnable, ...)| before returning the function.

@@ +1116,5 @@
>  
> +  BluetoothAddress address;
> +  nsresult rv = StringToAddress(aDeviceAddress, address);
> +  if (NS_FAILED(rv)) {
> +    return rv;

Add |DispatchReplyError(aRunnable, ...)| before returning the function.

@@ +1160,5 @@
>  
> +  BluetoothAddress address;
> +  nsresult rv = StringToAddress(aDeviceAddress, address);
> +  if (NS_FAILED(rv)) {
> +    return;

Add |DispatchReplyError(aRunnable, ...)| before returning the function.

@@ +1217,5 @@
>  
> +  BluetoothAddress address;
> +  nsresult rv = StringToAddress(aDeviceAddress, address);
> +  if (NS_FAILED(rv)) {
> +    return;

Add |DispatchReplyError(aRunnable, ...)| before returning the function.

@@ +1701,4 @@
>      NS_ENSURE_TRUE_VOID(bs);
>  
>      // Cleanup static adapter properties and notify adapter.
> +    mBdAddress = BluetoothAddress::ANY;

As discussed on comment 14, assigning the value of |BluetoothAddress::ANY| to a |BluetoothAddress| instance seems not self-explanatory enough to the logic of this statement. Suppose this would be modified based on the refined version of |BluetoothAddress|.

::: dom/bluetooth/common/BluetoothCommon.h
@@ +383,2 @@
>    /* PROPERTY_BDNAME
>       PROPERTY_BDADDR

Suppose the corresponding value of |PROPERTY_BDADDR| would be contained in |mBdAddress|, so probably we could remove the wording (i.e. |PROPERTY_BDADDR|) from the comments of |mString|.
Attachment #8664943 - Flags: review?(brsun) → review+
Comment on attachment 8664945 [details] [diff] [review]
[03] Bug 1207649: Convert Bluetooth Socket backend to |BluetoothAddress|

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

r=me if the following questions has been clarified.

::: dom/bluetooth/bluedroid/BluetoothSocketMessageWatcher.cpp
@@ +142,2 @@
>    ReadBdAddress(OFF_BDADDRESS, bdAddress);
> +  return BluetoothAddress(static_cast<const BluetoothAddress&>(bdAddress));

Since we have copy constructor already for |BluetoothAddress|, could we simply return |bdAddress| here?
Attachment #8664945 - Flags: review?(brsun) → review+
[Blocking Requested - why for this release]:

nominate as 2.5 blocker since there's a 2.5+ dependent bug.
blocking-b2g: --- → 2.5?
Comment on attachment 8664946 [details] [diff] [review]
[04] Bug 1207649: Convert Bluetooth Handsfree backend to |BluetoothAddress|

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

r=me if the following question would be clarified.

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +1171,4 @@
>    }
>  
>    if (mAudioState != HFP_AUDIO_STATE_DISCONNECTED) {
> +    BluetoothAddress deviceAddress;

How about moving the conversion between string to |BluetoothAddress| in the beginning of this function? So that the same logic won't duplicate in both |mConnectionState != HFP_CONNECTION_STATE_DISCONNECTED| case and |mAudioState != HFP_AUDIO_STATE_DISCONNECTED| case.
Attachment #8664946 - Flags: review?(brsun) → review+
Comment on attachment 8664950 [details] [diff] [review]
[08] Bug 1207649: Remove obsolete string/address conversion from Bluetooth backend

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

Looks good to me. :)
Attachment #8664950 - Flags: review?(brsun) → review+
Comment on attachment 8664948 [details] [diff] [review]
[06] Bug 1207649: Convert Bluetooth AVRCP backend to |BluetoothAddress|

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

LGTM
Attachment #8664948 - Flags: review?(shuang) → review+
Changes since v1:

  - initialize addresses to ANY by default 
  - added getter/setter for address parts
  - added |BluetoothAddress::Clear|
  - test for EOF result in address conversion
  - use |HashBytes| for generating hash key
Attachment #8664942 - Attachment is obsolete: true
Attachment #8665968 - Flags: review+
Changes since v1:

  - dispatch error runnables when address conversion fails
  - use |Clear| to clear addresses
  - fix comment in |BluetoothProperty|
Attachment #8664943 - Attachment is obsolete: true
Attachment #8665970 - Flags: review+
Changes since v1:

  - return address from socket-setup message directly
Attachment #8664945 - Attachment is obsolete: true
Attachment #8665973 - Flags: review+
Change since v1:

  - only convert address once in |HandleBackendError|
Attachment #8664946 - Attachment is obsolete: true
Attachment #8665976 - Flags: review+
Changes since v1:

  - rebased onto latest m-c
Attachment #8664949 - Attachment is obsolete: true
Attachment #8664949 - Flags: review?(joliu)
Attachment #8665978 - Flags: review?(joliu)
Changes since v1:

  - rebased onto latest m-c
Attachment #8664950 - Attachment is obsolete: true
Attachment #8665979 - Flags: review+
The patches at v2 should fix everything that has been mentioned so far.
Comment on attachment 8665978 [details] [diff] [review]
[07] Bug 1207649: Convert Bluetooth GATT backend to |BluetoothAddress| (v2)

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

Hi Thomas,

I have few questions regarding to BluetoothGattManager.cpp.

1. Any reason that BluetoothGattClient::mDeviceAddr type is not replaced by BluetoothAddress? So as the key type of BluetoothGattServer::mConnectionMap.
2. For command functions(ex: Connect, Disconnect, ConnectPeripheral,...) in BluetoothGattManager.cpp, how about we call StringToAddress and check the result as early as possible, since it's checking the validity of the argument?

Haven't finish the review of GattMgr.cpp, I would like to know the answers of these questions to review the rest part in GattMgr.cpp.

Thanks,
Jocelyn

::: dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.cpp
@@ +1601,4 @@
>        return rv;
>      }
>      /* Read address */
> +    rv = UnpackPDU(pdu, aArg4);

We could use UnpackPDUInitOp directly in ClientNtf and ClientDisconnectNtf, right?

Same thing applies to ClientReadRemoteRssiInitOp, ServerRequestReadInitOp, ServerRequestExecuteWriteInitOp below.

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +819,5 @@
> +        BluetoothValue(false)); // Disconnected
> +
> +      // Reject the connect request
> +      DispatchReplyError(client->mConnectRunnable,
> +                         NS_LITERAL_STRING("Connect failed"));

I would suggest to revise as
|DispatchReplyError(client->mConnectRunnable, STATUS_PARM_INVALID);|
Applies to other |DispatchReplyError| calls below.

@@ +828,2 @@
>      sBluetoothGattInterface->Connect(client->mClientIf,
> +                                           deviceAddr,

Would you mind to help to fix the alignment here?

@@ +899,5 @@
> +  if (NS_FAILED(rv)) {
> +    BluetoothService* bs = BluetoothService::Get();
> +    NS_ENSURE_TRUE_VOID(bs);
> +
> +    // Notify BluetoothGatt for client disconnected

Notify BluetoothGatt that client remains connected

@@ +903,5 @@
> +    // Notify BluetoothGatt for client disconnected
> +    bs->DistributeSignal(
> +      NS_LITERAL_STRING(GATT_CONNECTION_STATE_CHANGED_ID),
> +      client->mAppUuid,
> +      BluetoothValue(true)); // Disconnected

the comment should be Connected

@@ +1031,5 @@
>  
> +  BluetoothAddress deviceAddr;
> +  nsresult rv = StringToAddress(aDeviceAddr, deviceAddr);
> +  if (NS_FAILED(rv)) {
> +    MOZ_ASSERT(client->mReadRemoteRssiRunnable);

redundant here.

@@ +1119,5 @@
>  
> +  BluetoothAddress deviceAddr;
> +  nsresult rv = StringToAddress(client->mDeviceAddr, deviceAddr);
> +  if (NS_FAILED(rv)) {
> +    MOZ_ASSERT(client->mRegisterNotificationsRunnable);

redundant

@@ +1205,5 @@
>  
> +  BluetoothAddress deviceAddr;
> +  nsresult rv = StringToAddress(client->mDeviceAddr, deviceAddr);
> +  if (NS_FAILED(rv)) {
> +    MOZ_ASSERT(client->mDeregisterNotificationsRunnable);

DITTO.

@@ +1671,5 @@
>    if (server->mServerIf > 0) {
> +    BluetoothAddress address;
> +    nsresult rv = StringToAddress(aAddress, address);
> +    if (NS_FAILED(rv)) {
> +      MOZ_ASSERT(server->mConnectPeripheralRunnable);

DITTO.
Hi

> 
> I have few questions regarding to BluetoothGattManager.cpp.
> 
> 1. Any reason that BluetoothGattClient::mDeviceAddr type is not replaced by
> BluetoothAddress? So as the key type of BluetoothGattServer::mConnectionMap.

Shall I add these changes?

I simply didn't do this because the patch set was already large and I focused on the Bluetooth backend interfaces.

> 2. For command functions(ex: Connect, Disconnect, ConnectPeripheral,...) in
> BluetoothGattManager.cpp, how about we call StringToAddress and check the
> result as early as possible, since it's checking the validity of the
> argument?

Makes sense. I simply tried to stay close to the original code.

> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.cpp
> @@ +1601,4 @@
> >        return rv;
> >      }
> >      /* Read address */
> > +    rv = UnpackPDU(pdu, aArg4);
> 
> We could use UnpackPDUInitOp directly in ClientNtf and ClientDisconnectNtf,
> right?

With all the explicit conversions gone, there are a lot more places that can use |UnpackPDUInitOp|. I wanted to do these changes in another bug.
Fixing this bug will close other issue too, bug 1200495. 

Blocks 2.5 with a priority P2.
blocking-b2g: 2.5? → 2.5+
Priority: -- → P2
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #31)
> Hi
> 
> > 
> > I have few questions regarding to BluetoothGattManager.cpp.
> > 
> > 1. Any reason that BluetoothGattClient::mDeviceAddr type is not replaced by
> > BluetoothAddress? So as the key type of BluetoothGattServer::mConnectionMap.
> 
> Shall I add these changes?
> 
> I simply didn't do this because the patch set was already large and I
> focused on the Bluetooth backend interfaces.
> 
I originally thought the scope here also covers profile managers, but found that's not true when revisiting Comment 1.
Please ignore my comment, let's do it when cleaning profile managers.
Sorry.

> > 2. For command functions(ex: Connect, Disconnect, ConnectPeripheral,...) in
> > BluetoothGattManager.cpp, how about we call StringToAddress and check the
> > result as early as possible, since it's checking the validity of the
> > argument?
> 
> Makes sense. I simply tried to stay close to the original code.
> 
> > 
> > ::: dom/bluetooth/bluedroid/BluetoothDaemonGattInterface.cpp
> > @@ +1601,4 @@
> > >        return rv;
> > >      }
> > >      /* Read address */
> > > +    rv = UnpackPDU(pdu, aArg4);
> > 
> > We could use UnpackPDUInitOp directly in ClientNtf and ClientDisconnectNtf,
> > right?
> 
> With all the explicit conversions gone, there are a lot more places that can
> use |UnpackPDUInitOp|. I wanted to do these changes in another bug.

Ok, got it.
Comment on attachment 8665978 [details] [diff] [review]
[07] Bug 1207649: Convert Bluetooth GATT backend to |BluetoothAddress| (v2)

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

Almost there, but I would like to double check the logic after moving argument check, thanks.

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +905,5 @@
> +      NS_LITERAL_STRING(GATT_CONNECTION_STATE_CHANGED_ID),
> +      client->mAppUuid,
> +      BluetoothValue(true)); // Disconnected
> +
> +    // Reject the connect request

disconnect request here and mDisconnectRunnable below.
Attachment #8665978 - Flags: review?(joliu)
Changes since v2:

  - convert address strings as early as possible
  - typo and indention fixes
Attachment #8665978 - Attachment is obsolete: true
Attachment #8667131 - Flags: review?(joliu)
Comment on attachment 8667131 [details] [diff] [review]
[07] Bug 1207649: Convert Bluetooth GATT backend to |BluetoothAddress| (v3)

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

r=me, thanks for the effort.

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +888,5 @@
> +  if (NS_FAILED(rv)) {
> +    BluetoothService* bs = BluetoothService::Get();
> +    NS_ENSURE_TRUE_VOID(bs);
> +
> +    // Notify BluetoothGatt for client connected

nit: Notify BluetoothGatt that client remains connected
Attachment #8667131 - Flags: review?(joliu) → review+
Changes since v3:

  - changed comment as advised in review
Attachment #8667131 - Attachment is obsolete: true
Attachment #8667229 - Flags: review+
sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=2889789&repo=b2g-inbound
Flags: needinfo?(tzimmermann)
Changes since v2:

  - removed move c'tor/assignment from |BluetoothAddress| to build on Windows

We don't need move here anyway.
Attachment #8665968 - Attachment is obsolete: true
Attachment #8667753 - Flags: review+
You need to log in before you can comment on or make changes to this bug.