Closed Bug 1215525 Opened 9 years ago Closed 9 years ago

Use strong typing in Bluetooth mid-layer

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

Product:
Firefox OS Graveyard
Component:
Bluetooth
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox45 fixed)

Status:
RESOLVED FIXED
Milestone:
FxOS-S11 (13Nov)
Tracking Flags:
Tracking Status
firefox45 --- fixed

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(4 files, 11 obsolete files)

[03] Bug 1215525: Replace strings with Bluetooth addresses and UUIDs in GATT mid-layer (v3)
35.74 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
[02] Bug 1215525: Replace strings with Bluetooth addresses and UUIDs in Bluetooth mid-layer (v5)
103.87 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
[01] Bug 1215525: Update |BluetoothUuid| structure with c'tors and helper methods (v4)
11.29 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
[04] Bug 1215525: Use strong typing in Bluetooth GATT mid-layer interfaces (v3)
38.01 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review 
Bluetooth managers should not use string-based addresses and UUIDs internally. This is the next step after bug 1207649.
Bruce,

sorry for uploading such a huge patch. Changing the interfaces in BluetoothProfileManagerBase.h affects almost all manager classes, except GATT. I hope it's not too bad for reviewing.
Attachment #8674881 - Flags: review?(brsun)
The next step after this patch set would be to change the interfaces of the mid-layer from strings to strict typing.
Changes since v1:

  - rebased onto bug 1207245
Attachment #8674881 - Attachment is obsolete: true
Attachment #8674881 - Flags: review?(brsun)
Attachment #8675617 - Flags: review?(brsun)
Changes since v1:

  - rebased onto bug 1207245
Attachment #8674882 - Attachment is obsolete: true
Attachment #8674882 - Flags: review?(joliu)
Attachment #8675618 - Flags: review?(joliu)
Changes since v2:

  - fixed build breakers from last rebase in DBus service and HFP manager
Attachment #8675617 - Attachment is obsolete: true
Attachment #8675617 - Flags: review?(brsun)
Attachment #8675689 - Flags: review?(brsun)
Apparently, GATT is the only code with mid-layer interface changes. Let's land the respective patch here as well.
Attachment #8675691 - Flags: review?(joliu)
Comment on attachment 8674879 [details] [diff] [review]
[01] Bug 1215525: Update |BluetoothUuid| structure with c'tors and helper methods

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

Hi Thomas,

Sorry for the long response time. I'm still digesting these patches, and there are some quick questions in-line. Would you mind sharing your comments on them?

::: dom/bluetooth/bluez/BluetoothHfpManager.cpp
@@ +89,3 @@
>  
>    // Unknown service UUID (for SCO socket)
> +  static const BluetoothUuid kUnknownService(BluetoothUuid::BASE);

Not sure where |BluetoothUuid::BASE| is defined. Would you mind helping me to locate the definition or the declaration?

Suppose |UNKNOWN| enum of |BluetoothServiceClass| is designed just for this use case, how about using |UNKNOWN| here?

::: dom/bluetooth/common/BluetoothCommon.h
@@ +535,4 @@
>  struct BluetoothUuid {
> +
> +  static const BluetoothUuid ZERO;
> +  static const BluetoothUuid SDP_BASE;

Although the UUID definition and the base value are declared in SDP specification, the base value is named as "Bluetooth Base UUID"[1] or "BASE_UUID"[2]. Since UUID is used extensively in many other profiles as well (ex. for describing Attribute Type in Attribute Protocol), how about use a more generic naming for this base value (ex. BLUETOOTH_BASE)?

[1] "The first UUID in this pre-allocated range is known as the
Bluetooth Base UUID and has the value 00000000-0000-1000-8000-
00805F9B34FB, from the Bluetooth Assigned Numbers document." from Bluetooth Core Specification Version 4.2, Volume 3, Part B, Section 2.5.1.
[2] https://www.bluetooth.org/en-us/specification/assigned-numbers/service-discovery

@@ +593,5 @@
> +
> +  BluetoothUuid& operator=(const BluetoothUuid&) = default;
> +
> +  /**
> +   * |Clear| assigns an invalid value (i.e., ANY) to the address.

nit: "ANY" or "ZERO"?

@@ +620,5 @@
> +   */
> +
> +  void SetUuid32(uint32_t aUuid32)
> +  {
> +    mUuid[0] = aUuid32 >> 24;

I understand this statement runs perfectly correct as we want, but the intention of this statement is a little bit unclear by simply glancing on it without awaring the side effect of type conversion between |mUuid| and |aUuid32|. The value we want to assigned here is just the value on the last byte instead of the whole value of the shifted integer.

I would suggest:
 - to explicitly use bit-wise AND with 0xff on the shifted value of |aUuid32|, and
 - to explicitly cast the value with the desired type (i.e. |uint8_t|).

Suppose this function is not the performance bottleneck, it might be worth to make codes more expressive while trading off the overhead of one additional bit-wise AND operation.

If this makes sense to you, please apply changes to the rest statements of this function.
Hi

> Hi Thomas,
> 
> Sorry for the long response time.

NP

 I'm still digesting these patches, and
> there are some quick questions in-line. Would you mind sharing your comments
> on them?
> 
> ::: dom/bluetooth/bluez/BluetoothHfpManager.cpp
> @@ +89,3 @@
> >  
> >    // Unknown service UUID (for SCO socket)
> > +  static const BluetoothUuid kUnknownService(BluetoothUuid::BASE);
> 
> Not sure where |BluetoothUuid::BASE| is defined. Would you mind helping me
> to locate the definition or the declaration?

Originally, this was named 'SDP_BASE' was named 'BASE', but I renamed it and I later fixed this line in patch [02]. I didn't realize that it would belong into patch [01]. Some of the other UUIDs look similar and I though there might also be an OBEX base UUID. I couldn't find it in the OBEX spec. There was only a single UUID given, so OBEX_BASE didn't make it into the code.

Below, you mention that BASE would actually be the better name. I'll change SDP_BASE back to BASE. That should also address your comments below.

> 
> @@ +593,5 @@
> > +
> > +  BluetoothUuid& operator=(const BluetoothUuid&) = default;
> > +
> > +  /**
> > +   * |Clear| assigns an invalid value (i.e., ANY) to the address.
> 
> nit: "ANY" or "ZERO"?

ZERO, will be fixed.

> @@ +620,5 @@
> > +   */
> > +
> > +  void SetUuid32(uint32_t aUuid32)
> > +  {
> > +    mUuid[0] = aUuid32 >> 24;
> 
> I understand this statement runs perfectly correct as we want, but the
> intention of this statement is a little bit unclear by simply glancing on it
> without awaring the side effect of type conversion between |mUuid| and
> |aUuid32|. The value we want to assigned here is just the value on the last
> byte instead of the whole value of the shifted integer.
> 
> I would suggest:
>  - to explicitly use bit-wise AND with 0xff on the shifted value of
> |aUuid32|, and
>  - to explicitly cast the value with the desired type (i.e. |uint8_t|).
> 
> Suppose this function is not the performance bottleneck, it might be worth
> to make codes more expressive while trading off the overhead of one
> additional bit-wise AND operation.
> 
> If this makes sense to you, please apply changes to the rest statements of
> this function.

That's a good point. Will be fixed in the next version. I wouldn't expect the resulting binary to look much different.
Comment on attachment 8675689 [details] [diff] [review]
[02] Bug 1215525: Replace strings with Bluetooth addresses and UUIDs in Bluetooth mid-layer (v3)

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

There are some more quick questions here and below, would you mind sharing your comments on them?

|BluetoothAddress::ANY| is used extensively to compare directly with many |BluetoothAddress| variable to check the emptiness of the value. I still feel the naming of |BluetoothAddress::ANY| doesn't express the purpose of emptiness checking. Passing |BluetoothAddress::ANY| to |BluetoothUnixSocketConnector| constructor doesn't confuse me though. There are some discussions about the naming and the usage of it on bug 1207649 comment 13 and bug 1207649 comment 14 already. If adding |BluetoothAddress::EMPTY| (or |BluetoothAddress::ZERO| or something like that) is not an option, probably adding one function to check the emptiness of |BluetoothAddress| is more comfortable to you? Any other suggestions?

I suspect there would be potential bugs while using |BluetoothUuidHelper::GetString|. There might be characters in upper cases from 'A' to 'F'[1] mixing with characters in lower cases from 'a' to 'f'[2] inside the output result of |BluetoothUuidHelper::GetString|. Since there are no use cases to call this function anymore, suppose |BluetoothUuidHelper::GetString| could be removed as well. At least, we have |UuidToString| to convert from |BluetoothUuid| to |nsAString|. What do you think?

The usage of |BluetoothServiceClass| is a little bit weird. I mean, the logic of how we use it is clear to me, but the way of how we use it in codes is really interesting. From the declaration of |BluetoothServiceClass|, it is an unscoped enumeration[3]. But we use the individual enum value (ex. |BluetoothServiceClass::OBJECT_PUSH|) in a scoped enumeration[4] way. Maybe I misunderstand something about the usage of enumeration. Do you have any idea about it?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/BluetoothUuid.cpp#23-25
[2] https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsStringAPI.cpp#519
[3] http://en.cppreference.com/w/cpp/language/enum#Unscoped_enumeration
[4] http://en.cppreference.com/w/cpp/language/enum#Scoped_enumerations

::: dom/bluetooth/bluedroid/BluetoothOppManager.cpp
@@ +297,5 @@
> +  nsString uuidStr;
> +  BluetoothUuidHelper::GetString(BluetoothServiceClass::OBJECT_PUSH, uuidStr);
> +
> +  BluetoothUuid uuid;
> +  StringToUuid(uuidStr, uuid);

Suppose simply using |const BluetoothUuid uuid(OBJECT_PUSH);| could do the job, right?

@@ +1673,5 @@
> +  nsString uuidStr;
> +  BluetoothUuidHelper::GetString(BluetoothServiceClass::OBJECT_PUSH, uuidStr);
> +
> +  BluetoothUuid uuid;
> +  StringToUuid(uuidStr, uuid);

ditto

::: dom/bluetooth/bluez/BluetoothDBusService.cpp
@@ +2882,5 @@
> +
> +    nsAutoString addressStr;
> +    AddressToString(address, addressStr);
> +
> +    deviceAddresses.AppendElement(addressStr);

Why not let the type of |deviceAddresses| be an array of |BluetoothAddress| and let |BluetoothArrayOfDevicePropertiesReplyHandler| accepts an array of |BluetoothAddress| directly?

@@ +3549,5 @@
>      MOZ_ASSERT(aManager);
>  
> +    const nsString deviceAddressStr = GetAddressFromObjectPath(aObjectPath);
> +
> +    StringToAddress(deviceAddressStr, mDeviceAddress);

It should be able to call |GetAddressFromObjectPath(aObjectPath, mDeviceAddress)| directly, right?

::: dom/bluetooth/bluez/BluetoothOppManager.cpp
@@ +412,4 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  BluetoothAddress deviceAddress;

Why not pass |BluetoothAddress| into this function directly?

::: dom/bluetooth/bluez/BluetoothOppManager.h
@@ +99,1 @@
>    void AppendBlobToSend(const nsAString& aDeviceAddress, Blob* aBlob);

There are still some functions in |BluetoothOppManager| which use |nsAString| as the device address under this |bluez| folder, wouldn't these functions be refined as well in this patch?

::: dom/bluetooth/bluez/BluetoothSocket.cpp
@@ +718,3 @@
>    if (!mIO || GetConnectionStatus() != SOCKET_CONNECTED) {
>      NS_WARNING("No socket currently open!");
> +    aAddress.Clear();

Cleanup output parameters at the beginning of a function would be a good practice. Suggest to put this statement to the beginning of this function.

@@ +721,4 @@
>      return;
>    }
> +  nsAutoString addressStr;
> +  mIO->GetSocketAddr(addressStr);

Suggest to have |BluetoothSocket::BluetoothSocketIO::GetSocketAddr(...)| accept |BluetoothAddress&| as the function parameter directly, so that the rest of this function beyond this statement could be saved.

::: dom/bluetooth/bluez/BluetoothUnixSocketConnector.cpp
@@ +299,5 @@
> +BluetoothUnixSocketConnector::ConvertAddress(const BluetoothAddress& aAddress,
> +                                             bdaddr_t& aBdAddr)
> +{
> +  /* read source address from end backwards */
> +  auto src = aAddress.mAddr + MOZ_ARRAY_LENGTH(aBdAddr.b) - 1;

Although the length of |BluetoothAddress.mAddr|[1] and |bdaddr_t.b|[2] are equally the same as 6 from the definition, they are declared in different places (one in Gecko, another in BlueZ) on their own. Since there are no dependencies between these two declarations programmatically, additional checking might be needed before using the size of one array as the index/boundary of another array.

Suggest to add one assertion to make sure the size of one array is viable to be used to manipulate another array. For example, we have to make sure |MOZ_ARRAY_LENGTH(aAddress.mAddr)| is at least as big as |MOZ_ARRAY_LENGTH(aBdAddr.b)| before dereferencing |src|.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/BluetoothCommon.h?from=BluetoothAddress#410
[2] http://git.kernel.org/cgit/bluetooth/bluez.git/tree/lib/bluetooth.h#n301

@@ +301,5 @@
> +{
> +  /* read source address from end backwards */
> +  auto src = aAddress.mAddr + MOZ_ARRAY_LENGTH(aBdAddr.b) - 1;
> +
> +  for (auto i = 0ul; i < MOZ_ARRAY_LENGTH(aBdAddr.b); ++i) {

The type of the result of |MOZ_ARRAY_LENGTH(...)|[1] would be |size_t|[2]. Explicitly using |size_t| while declaring |i| would be more suitable than using |auto i = 0ul| to deduce the type |unsigned long|.

Suggest to explicitly use |size_t| as the type of |i| in this case.

[1] https://dxr.mozilla.org/mozilla-central/source/mfbt/ArrayUtils.h#183-192
[2] http://en.cppreference.com/w/cpp/language/sizeof

@@ +309,5 @@
> +    char* endp;
> +    *d-- = strtoul(aAddressString, &endp, 16);
> +    MOZ_ASSERT(!(*endp != ':' && i != 5));
> +    aAddressString = endp + 1;
> +    */

Probably these comments could be removed, right?

::: dom/bluetooth/bluez/BluetoothUnixSocketConnector.h
@@ +54,2 @@
>    static nsresult ConvertAddressString(const char* aAddressString,
>                                         bdaddr_t& aAddress);

Suppose |ConvertAddressString| can be removed if |ConvertAddress| is introduced, right?

::: dom/bluetooth/common/BluetoothProfileController.cpp
@@ -229,4 @@
>  BluetoothProfileController::StartSession()
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> -  MOZ_ASSERT(!mDeviceAddress.IsEmpty());

Suppose the corresponding checking are still needed, right?

@@ -284,4 @@
>  BluetoothProfileController::Next()
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> -  MOZ_ASSERT(!mDeviceAddress.IsEmpty());

ditto

::: dom/bluetooth/common/BluetoothService.h
@@ +225,4 @@
>     * Get corresponding service channel of specific service on remote device.
>     * It's usually the very first step of establishing an outbound connection.
>     *
> +   * @param aObjectPath Address of remote device

nit: "aDeviceAddress" instead of "aObjectPath"

@@ +232,5 @@
>     * @return NS_OK if the task begins, NS_ERROR_FAILURE otherwise
>     */
>    virtual nsresult
> +  GetServiceChannel(const BluetoothAddress& aDeviceAddress,
> +                    const BluetoothUuid& aServiceUuid,

I guess your intention of this bug is just to use |BluetoothAddress| and |BluetoothUuid| in the middle layer (ex. profile managers) and keep all interfaces of |BluetoothService| unchanged. But |BluetoothService::GetServiceChannel(...)| and |BluetoothService::UpdateSdpRecords(...)| are modified in this patch. I haven't had a clear idea about the reason why these two functions are affected but others are not. Would you mind sharing some hints?

::: dom/bluetooth/ipc/BluetoothServiceChildProcess.cpp
@@ +213,1 @@
>                                                  BluetoothProfileManagerBase* aManager)

nit: 80 characters per line

@@ +220,1 @@
>                                                 BluetoothProfileManagerBase* aManager)

nit: 80 characters per line
Hi!

I'll try to answer your questions as good as possible. I'll go though all of them when preparing the next patches.


> |BluetoothAddress::ANY| is used extensively to compare directly with many
> |BluetoothAddress| variable to check the emptiness of the value. I still
> feel the naming of |BluetoothAddress::ANY| doesn't express the purpose of
> emptiness checking. Passing |BluetoothAddress::ANY| to
> |BluetoothUnixSocketConnector| constructor doesn't confuse me though. There
> are some discussions about the naming and the usage of it on bug 1207649
> comment 13 and bug 1207649 comment 14 already. If adding
> |BluetoothAddress::EMPTY| (or |BluetoothAddress::ZERO| or something like
> that) is not an option, probably adding one function to check the emptiness
> of |BluetoothAddress| is more comfortable to you? Any other suggestions?

We're following the nomenclature of the BlueZ API and the socket-address naming in general. That seems to be the right thing to me.

I don't like ZERO because it tells what the address contains, which is irrelevant. A good name says what the address does. And ANY is closer to that. EMPTY appears wrong to me, because the address is not empty: it contains 6 bytes.

There is already |BluetoothAddress::Clear| and I suggest we add |BluetoothAddress::IsClear| for testing.


> I suspect there would be potential bugs while using
> |BluetoothUuidHelper::GetString|. There might be characters in upper cases
> from 'A' to 'F'[1] mixing with characters in lower cases from 'a' to 'f'[2]
> inside the output result of |BluetoothUuidHelper::GetString|. Since there
> are no use cases to call this function anymore, suppose
> |BluetoothUuidHelper::GetString| could be removed as well. At least, we have
> |UuidToString| to convert from |BluetoothUuid| to |nsAString|. What do you
> think?

I have not checked if |BluetoothUuidHelper| is still in use; if it's not, I'm happy to remove it. The whole UUID helper code seems a bit fragile to me anyway.


> The usage of |BluetoothServiceClass| is a little bit weird. I mean, the
> logic of how we use it is clear to me, but the way of how we use it in codes
> is really interesting. From the declaration of |BluetoothServiceClass|, it
> is an unscoped enumeration[3]. But we use the individual enum value (ex.
> |BluetoothServiceClass::OBJECT_PUSH|) in a scoped enumeration[4] way. Maybe
> I misunderstand something about the usage of enumeration. Do you have any
> idea about it?

Yeah, we're not really doing good here. Converting all the enums to scoped definitions and reducing the storage sizes is somewhere on my TODO list.


> ::: dom/bluetooth/bluedroid/BluetoothOppManager.cpp
> @@ +297,5 @@
> > +  nsString uuidStr;
> > +  BluetoothUuidHelper::GetString(BluetoothServiceClass::OBJECT_PUSH, uuidStr);
> > +
> > +  BluetoothUuid uuid;
> > +  StringToUuid(uuidStr, uuid);
> 
> Suppose simply using |const BluetoothUuid uuid(OBJECT_PUSH);| could do the
> job, right?

Yeah. After several dozens of these conversions it becomes routine and you just don't look at the code any more. ;) :D Here and below.

> ::: dom/bluetooth/bluez/BluetoothDBusService.cpp
> @@ +2882,5 @@
> > +
> > +    nsAutoString addressStr;
> > +    AddressToString(address, addressStr);
> > +
> > +    deviceAddresses.AppendElement(addressStr);
> 
> Why not let the type of |deviceAddresses| be an array of |BluetoothAddress|
> and let |BluetoothArrayOfDevicePropertiesReplyHandler| accepts an array of
> |BluetoothAddress| directly?


> ::: dom/bluetooth/bluez/BluetoothOppManager.cpp
> @@ +412,4 @@
> >  {
> >    MOZ_ASSERT(NS_IsMainThread());
> >  
> > +  BluetoothAddress deviceAddress;
> 
> Why not pass |BluetoothAddress| into this function directly?
> 
> ::: dom/bluetooth/bluez/BluetoothOppManager.h
> @@ +99,1 @@
> >    void AppendBlobToSend(const nsAString& aDeviceAddress, Blob* aBlob);
> 
> There are still some functions in |BluetoothOppManager| which use
> |nsAString| as the device address under this |bluez| folder, wouldn't these
> functions be refined as well in this patch?

Yes, maybe I forgot some.

Generally I preferred to add code into methods, instead of modifying interface. Because interfaces might require a lot of other changes elsewhere. I'll go through the code and see if there are other places where modifying interfaces makes sense.


> ::: dom/bluetooth/bluez/BluetoothSocket.cpp
> @@ +718,3 @@
> >    if (!mIO || GetConnectionStatus() != SOCKET_CONNECTED) {
> >      NS_WARNING("No socket currently open!");
> > +    aAddress.Clear();
> 
> Cleanup output parameters at the beginning of a function would be a good
> practice. Suggest to put this statement to the beginning of this function.

I don't like doing such cleanups for nothing, but I'll consider it.


> @@ +721,4 @@
> >      return;
> >    }
> > +  nsAutoString addressStr;
> > +  mIO->GetSocketAddr(addressStr);
> 
> Suggest to have |BluetoothSocket::BluetoothSocketIO::GetSocketAddr(...)|
> accept |BluetoothAddress&| as the function parameter directly, so that the
> rest of this function beyond this statement could be saved.


> ::: dom/bluetooth/bluez/BluetoothUnixSocketConnector.cpp
> @@ +299,5 @@
> > +BluetoothUnixSocketConnector::ConvertAddress(const BluetoothAddress& aAddress,
> > +                                             bdaddr_t& aBdAddr)
> > +{
> > +  /* read source address from end backwards */
> > +  auto src = aAddress.mAddr + MOZ_ARRAY_LENGTH(aBdAddr.b) - 1;
> 
> Although the length of |BluetoothAddress.mAddr|[1] and |bdaddr_t.b|[2] are
> equally the same as 6 from the definition, they are declared in different
> places (one in Gecko, another in BlueZ) on their own. Since there are no
> dependencies between these two declarations programmatically, additional
> checking might be needed before using the size of one array as the
> index/boundary of another array.

Good point.


> Suggest to add one assertion to make sure the size of one array is viable to
> be used to manipulate another array. For example, we have to make sure
> |MOZ_ARRAY_LENGTH(aAddress.mAddr)| is at least as big as
> |MOZ_ARRAY_LENGTH(aBdAddr.b)| before dereferencing |src|.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/
> BluetoothCommon.h?from=BluetoothAddress#410
> [2] http://git.kernel.org/cgit/bluetooth/bluez.git/tree/lib/bluetooth.h#n301
> 
> @@ +301,5 @@
> > +{
> > +  /* read source address from end backwards */
> > +  auto src = aAddress.mAddr + MOZ_ARRAY_LENGTH(aBdAddr.b) - 1;
> > +
> > +  for (auto i = 0ul; i < MOZ_ARRAY_LENGTH(aBdAddr.b); ++i) {
> 
> The type of the result of |MOZ_ARRAY_LENGTH(...)|[1] would be |size_t|[2].
> Explicitly using |size_t| while declaring |i| would be more suitable than
> using |auto i = 0ul| to deduce the type |unsigned long|.
> 
> Suggest to explicitly use |size_t| as the type of |i| in this case.
> 
> [1] https://dxr.mozilla.org/mozilla-central/source/mfbt/ArrayUtils.h#183-192
> [2] http://en.cppreference.com/w/cpp/language/sizeof
> 
> @@ +309,5 @@
> > +    char* endp;
> > +    *d-- = strtoul(aAddressString, &endp, 16);
> > +    MOZ_ASSERT(!(*endp != ':' && i != 5));
> > +    aAddressString = endp + 1;
> > +    */
> 
> Probably these comments could be removed, right?
> 
> ::: dom/bluetooth/bluez/BluetoothUnixSocketConnector.h
> @@ +54,2 @@
> >    static nsresult ConvertAddressString(const char* aAddressString,
> >                                         bdaddr_t& aAddress);
> 
> Suppose |ConvertAddressString| can be removed if |ConvertAddress| is
> introduced, right?

I have to check this.


> ::: dom/bluetooth/common/BluetoothProfileController.cpp
> @@ -229,4 @@
> >  BluetoothProfileController::StartSession()
> >  {
> >    MOZ_ASSERT(NS_IsMainThread());
> > -  MOZ_ASSERT(!mDeviceAddress.IsEmpty());
> 
> Suppose the corresponding checking are still needed, right?
> 
> @@ -284,4 @@
> >  BluetoothProfileController::Next()
> >  {
> >    MOZ_ASSERT(NS_IsMainThread());
> > -  MOZ_ASSERT(!mDeviceAddress.IsEmpty());
> 
> ditto
> 
> ::: dom/bluetooth/common/BluetoothService.h
> @@ +225,4 @@
> >     * Get corresponding service channel of specific service on remote device.
> >     * It's usually the very first step of establishing an outbound connection.
> >     *
> > +   * @param aObjectPath Address of remote device
> 
> nit: "aDeviceAddress" instead of "aObjectPath"
> 
> @@ +232,5 @@
> >     * @return NS_OK if the task begins, NS_ERROR_FAILURE otherwise
> >     */
> >    virtual nsresult
> > +  GetServiceChannel(const BluetoothAddress& aDeviceAddress,
> > +                    const BluetoothUuid& aServiceUuid,
> 
> I guess your intention of this bug is just to use |BluetoothAddress| and
> |BluetoothUuid| in the middle layer (ex. profile managers) and keep all
> interfaces of |BluetoothService| unchanged. But
> |BluetoothService::GetServiceChannel(...)| and
> |BluetoothService::UpdateSdpRecords(...)| are modified in this patch. I
> haven't had a clear idea about the reason why these two functions are
> affected but others are not. Would you mind sharing some hints?

I updated the code in BluetoothProfileManagerBase.h. That led to changes throughout the Bluetooth code. These methods are part of this and used by the |BluetoothService| and the profile managers. It's not overly consistent. But they are internal interfaces so this change should be OK.
> > |BluetoothAddress::ANY| is used extensively to compare directly with many
> > |BluetoothAddress| variable to check the emptiness of the value. I still
> > feel the naming of |BluetoothAddress::ANY| doesn't express the purpose of
> > emptiness checking. Passing |BluetoothAddress::ANY| to
> > |BluetoothUnixSocketConnector| constructor doesn't confuse me though. There
> > are some discussions about the naming and the usage of it on bug 1207649
> > comment 13 and bug 1207649 comment 14 already. If adding
> > |BluetoothAddress::EMPTY| (or |BluetoothAddress::ZERO| or something like
> > that) is not an option, probably adding one function to check the emptiness
> > of |BluetoothAddress| is more comfortable to you? Any other suggestions?
> 
> We're following the nomenclature of the BlueZ API and the socket-address
> naming in general. That seems to be the right thing to me.
> 
> I don't like ZERO because it tells what the address contains, which is
> irrelevant. A good name says what the address does. And ANY is closer to
> that. EMPTY appears wrong to me, because the address is not empty: it
> contains 6 bytes.
> 
> There is already |BluetoothAddress::Clear| and I suggest we add
> |BluetoothAddress::IsClear| for testing.
> 
> 

Suppose what we would like to check here is trying to see if the value of the address is a valid value to be used or not. Maybe |BluetoothAddress::Invalidate| and |BluetoothAddress::IsValid|?

> > The usage of |BluetoothServiceClass| is a little bit weird. I mean, the
> > logic of how we use it is clear to me, but the way of how we use it in codes
> > is really interesting. From the declaration of |BluetoothServiceClass|, it
> > is an unscoped enumeration[3]. But we use the individual enum value (ex.
> > |BluetoothServiceClass::OBJECT_PUSH|) in a scoped enumeration[4] way. Maybe
> > I misunderstand something about the usage of enumeration. Do you have any
> > idea about it?
> 
> Yeah, we're not really doing good here. Converting all the enums to scoped
> definitions and reducing the storage sizes is somewhere on my TODO list.
> 
> 

Converting unscoped enums to scoped enums would be great. I'm just a little bit confused by the mixing usage of these styles.
 - unscoped: declare |enum SOME_ENUM { ENUM_VALUE }|, and use |ENUM_VALUE| as the value
 - scoped: declare |enum class SOME_ENUM { ENUM_VALUE }|, and use |SOME_ENUM::ENUM_VALUE| as the value
 - mixed: declare |enum SOME_ENUM { ENUM_VALUE }|, and use |SOME_ENUM::ENUM_VALUE| as the value

> > ::: dom/bluetooth/bluez/BluetoothOppManager.h
> > @@ +99,1 @@
> > >    void AppendBlobToSend(const nsAString& aDeviceAddress, Blob* aBlob);
> > 
> > There are still some functions in |BluetoothOppManager| which use
> > |nsAString| as the device address under this |bluez| folder, wouldn't these
> > functions be refined as well in this patch?
> 
> Yes, maybe I forgot some.
> 
> Generally I preferred to add code into methods, instead of modifying
> interface. Because interfaces might require a lot of other changes
> elsewhere. I'll go through the code and see if there are other places where
> modifying interfaces makes sense.
> 
> 

Some functions under |bluedroid| folder are changed, but the corresponding functions under |bluez| folder are not affected, which led to my previous comments. But I think you've got my point already, thanks for the clarification.

> > @@ +232,5 @@
> > >     * @return NS_OK if the task begins, NS_ERROR_FAILURE otherwise
> > >     */
> > >    virtual nsresult
> > > +  GetServiceChannel(const BluetoothAddress& aDeviceAddress,
> > > +                    const BluetoothUuid& aServiceUuid,
> > 
> > I guess your intention of this bug is just to use |BluetoothAddress| and
> > |BluetoothUuid| in the middle layer (ex. profile managers) and keep all
> > interfaces of |BluetoothService| unchanged. But
> > |BluetoothService::GetServiceChannel(...)| and
> > |BluetoothService::UpdateSdpRecords(...)| are modified in this patch. I
> > haven't had a clear idea about the reason why these two functions are
> > affected but others are not. Would you mind sharing some hints?
> 
> I updated the code in BluetoothProfileManagerBase.h. That led to changes
> throughout the Bluetooth code. These methods are part of this and used by
> the |BluetoothService| and the profile managers. It's not overly consistent.
> But they are internal interfaces so this change should be OK.

There might be some way to keep these service interfaces unchanged by adding temporary converting logic. But as long as the string-style device address would be totally replace by specific-type-style device address within service interfaces very soon, the changes referred here are not a big problem to me.
Comment on attachment 8674879 [details] [diff] [review]
[01] Bug 1215525: Update |BluetoothUuid| structure with c'tors and helper methods

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

r=me if suggestions and decisions made on comment 9 and comment 10 are addressed.
Attachment #8674879 - Flags: review?(brsun) → review+
Comment on attachment 8675689 [details] [diff] [review]
[02] Bug 1215525: Replace strings with Bluetooth addresses and UUIDs in Bluetooth mid-layer (v3)

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

r=me if suggestions and decisions made on comment 11 and comment 12 are addressed.
Attachment #8675689 - Flags: review?(brsun) → review+
Comment on attachment 8675618 [details] [diff] [review]
[03] Bug 1215525: Replace strings with Bluetooth addresses and UUIDs in GATT mid-layer (v2)

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

LGTM.
Attachment #8675618 - Flags: review?(joliu) → review+
Comment on attachment 8675691 [details] [diff] [review]
[04] Bug 1215525: Use strong typing in Bluetooth GATT mid-layer interfaces

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

LGTM. Sorry for the delay here. :(
Attachment #8675691 - Flags: review?(joliu) → review+
Changes since v1:

  - renamed |SDP_BASE| to |BASE| and made private
  - replaced |BASE| with |BluetoothServiceClass::UNKNOWN| in HFP manager
  - added |BluetoothUUID::IsCleared|
  - clear casting and bit masks in UUID assignment
Attachment #8674879 - Attachment is obsolete: true
Attachment #8679546 - Flags: review+
Changes since v2:

  - remove |BASE|, as initialization order is undefined; copying |BASE| during static construction might result in zeros
  - set full UUID in |SetUuid32|
Attachment #8679546 - Attachment is obsolete: true
Attachment #8679997 - Flags: review+
Changes since v3:

  - convert internals of BlueZ OPP manager as well
  - use |kObexObjectPush| throughout Bluedroid OPP manager
  - replace tests for |ANY| and |ZERO| with |IsCleared|
  - verify address length in |ConvertAddress|
  - cleanups of unused code
Attachment #8675689 - Attachment is obsolete: true
Attachment #8679999 - Flags: review+
Changes since v2:

  - replace tests for |ZERO| with |IsCleared|
Attachment #8675618 - Attachment is obsolete: true
Attachment #8680000 - Flags: review+
Changes since v1:

  - replace tests for |ZERO| with |IsCleared|
Attachment #8675691 - Attachment is obsolete: true
Attachment #8680001 - Flags: review+
sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=3195785&repo=b2g-inbound
Flags: needinfo?(tzimmermann)
Changes since v3:

  - removed |BluetoothUuid::ZERO| to resolve linker error
Attachment #8679997 - Attachment is obsolete: true
Attachment #8681265 - Flags: review+
Changes since v2:

  - removed reference to |BluetoothUuid::ZERO|
Attachment #8680001 - Attachment is obsolete: true
Attachment #8681266 - Flags: review+
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
status-b2g-v2.5: fixed → ---
See Also: → 1130306
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: