[Bluetooth] Non-blocking BluetoothArrayOfDeviceProperties

RESOLVED FIXED

Status

Firefox OS
Bluetooth
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: tzimmermann, Assigned: tzimmermann)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Created attachment 795739 [details] [diff] [review]
[01] Bug 906020: Implement non-blocking BluetoothArrayOfDevicePropertiesRunnable

Originally, I wanted to send all GetProperties messages at once and handle the replies when they come in. It turned out that it's currently almost impossible to correlate messages, replies and device addresses. So we still send the GetProperties messages one by one, but now in a non-blocking way.
Attachment #795739 - Flags: review?(gyeh)
Attachment #795739 - Flags: review?(echou)
Created attachment 795742 [details] [diff] [review]
[02] Bug 906020: Remove BluetoothArrayOfDevicePropertiesRunnable
Attachment #795742 - Flags: review?(gyeh)
Attachment #795742 - Flags: review?(echou)
Comment on attachment 795742 [details] [diff] [review]
[02] Bug 906020: Remove BluetoothArrayOfDevicePropertiesRunnable

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

This is exactly what I'm looking for in the first patch ;)
Attachment #795742 - Flags: review?(gyeh) → review+
Comment on attachment 795739 [details] [diff] [review]
[01] Bug 906020: Implement non-blocking BluetoothArrayOfDevicePropertiesRunnable

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

Please take care of the following nits. Thanks.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +2026,5 @@
>  
> +class BluetoothArrayOfDevicePropertiesReplyHandler : public DBusReplyHandler
> +{
> +public:
> +  BluetoothArrayOfDevicePropertiesReplyHandler(const nsTArray<nsString>& aDeviceAddresses,

nit: 80 chars per line. Here and below.

@@ +2044,5 @@
> +    MOZ_ASSERT(!NS_IsMainThread()); // DBus thread
> +    MOZ_ASSERT(!mObjectPath.IsEmpty());
> +    MOZ_ASSERT(mProcessedDeviceAddresses < mDeviceAddresses.Length());
> +
> +    const nsTArray<nsString>::index_type i = mProcessedDeviceAddresses++;

nit: Variable |i| isn't used. Can we remove it?

@@ +2061,5 @@
> +
> +    bool success = UnpackPropertiesMessage(aReply, &err, deviceProperties,
> +                                           DBUS_DEVICE_IFACE);
> +    if (!success) {
> +      NS_WARNING("Failed to get device properties");

nit: BT_WARNING

@@ +2079,5 @@
> +    // Icon as audio-card. This is for PTS test TC_AG_COD_BV_02_I.
> +    // As HFP specification defined that
> +    // service class is "Audio" can be considered as HFP AG.
> +    if (!ContainsIcon(devicePropertiesArray)) {
> +

nit: blank line.

@@ +2107,5 @@
> +  void ProcessRemainingDeviceAddresses()
> +  {
> +    if (mProcessedDeviceAddresses < mDeviceAddresses.Length()) {
> +      if (!SendNextGetProperties()) {
> +        DispatchBluetoothReply(mRunnable, BluetoothValue(true),

nit: We usually return an error with BluetoothValue().

@@ +2108,5 @@
> +  {
> +    if (mProcessedDeviceAddresses < mDeviceAddresses.Length()) {
> +      if (!SendNextGetProperties()) {
> +        DispatchBluetoothReply(mRunnable, BluetoothValue(true),
> +            NS_LITERAL_STRING("SendNextGetProperties failed"));

nit: Please make it align to |mRunnable|.

@@ +2112,5 @@
> +            NS_LITERAL_STRING("SendNextGetProperties failed"));
> +      }
> +    } else {
> +      // Send resulting device properties
> +      DispatchBluetoothReply(mRunnable, mValues, nsAutoString());

nit: You can use EmptyString().

@@ +2118,5 @@
> +  }
> +
> +protected:
> +  bool SendNextGetProperties()
> +  {

Can we add the following assertion here?

MOZ_ASSERT(!mDeviceAddresses[mProcessedDeviceAddresses].IsEmpty());

@@ +2150,5 @@
> +  nsString mObjectPath;
> +  const nsTArray<nsString> mDeviceAddresses;
> +  nsTArray<nsString>::size_type mProcessedDeviceAddresses;
> +  const FilterFunc mFilterFunc;
> +  BluetoothReplyRunnable* mRunnable;

Please use nsRefPtr<BluetoothReplyRunnable>.
Hi,

Sorry, the quality of that patch could have been better. I was writing it during a meeting while having a jetlag of 9 hours. :) I cleaned up the patch according to your comments, except the one i mention below.

> @@ +2044,5 @@
> > +    MOZ_ASSERT(!NS_IsMainThread()); // DBus thread
> > +    MOZ_ASSERT(!mObjectPath.IsEmpty());
> > +    MOZ_ASSERT(mProcessedDeviceAddresses < mDeviceAddresses.Length());
> > +
> > +    const nsTArray<nsString>::index_type i = mProcessedDeviceAddresses++;
> 
> nit: Variable |i| isn't used. Can we remove it?

It's used on line 2100, within the mFilterFunc conditional. I just set it here because we need to increment mProcessedDeviceAddresses before calling ProcessRemainingDeviceAddresses.

Best regards
Thomas
Created attachment 796137 [details] [diff] [review]
[01] Bug 906020: Implement non-blocking BluetoothArrayOfDevicePropertiesRunnable (v2)
Attachment #795739 - Attachment is obsolete: true
Attachment #795739 - Flags: review?(gyeh)
Attachment #795739 - Flags: review?(echou)
Attachment #796137 - Flags: review?(gyeh)
Attachment #796137 - Flags: review?(echou)
Comment on attachment 796137 [details] [diff] [review]
[01] Bug 906020: Implement non-blocking BluetoothArrayOfDevicePropertiesRunnable (v2)

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

It looks much better. Thanks. :)

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +2048,5 @@
> +    const nsTArray<nsString>::index_type i = mProcessedDeviceAddresses++;
> +
> +    if (!aReply ||
> +        (dbus_message_get_type(aReply) == DBUS_MESSAGE_TYPE_ERROR)) {
> +      ProcessRemainingDeviceAddresses();

It would be great to add a warning message here.

@@ +2096,5 @@
> +    }
> +
> +    if (mFilterFunc(deviceProperties)) {
> +      mValues.get_ArrayOfBluetoothNamedValue().AppendElement(
> +          BluetoothNamedValue(mDeviceAddresses[i], deviceProperties));

strange indentation
Attachment #796137 - Flags: review?(gyeh) → review+
Thanks Gina for the review. I'll fix your remaining points together with what Eric mentions.
Comment on attachment 796137 [details] [diff] [review]
[01] Bug 906020: Implement non-blocking BluetoothArrayOfDevicePropertiesRunnable (v2)

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

Looks good, r=me with questions answered and nits addressed.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +2028,5 @@
> +{
> +public:
> +  BluetoothArrayOfDevicePropertiesReplyHandler(
> +      const nsTArray<nsString>& aDeviceAddresses,
> +      const FilterFunc aFilterFunc, BluetoothReplyRunnable* aRunnable)

super-nit: please use two spaces for indentation.

@@ +2123,5 @@
> +    MOZ_ASSERT(mProcessedDeviceAddresses < mDeviceAddresses.Length());
> +
> +    // cache object path for reply
> +    mObjectPath = GetObjectPathFromAddress(sAdapterPath,
> +        mDeviceAddresses[mProcessedDeviceAddresses]);

super-nit: please use two spaces for indentation.

@@ +2135,5 @@
> +
> +    nsRefPtr<BluetoothArrayOfDevicePropertiesReplyHandler> handler = this;
> +
> +    bool success = dbus_func_args_async(threadConnection->GetConnection(), 1000,
> +                                        BluetoothArrayOfDevicePropertiesReplyHandler::Callback,

nit: 80 characters per line.

@@ +2150,5 @@
> +
> +private:
> +  nsString mObjectPath;
> +  const nsTArray<nsString> mDeviceAddresses;
> +  nsTArray<nsString>::size_type mProcessedDeviceAddresses;

Question: Although I checked the code and realized that size_type and index_type are both uint_32, in this case, is there any reasons using different types to hold mProcessedDeviceAddresses and variable 'i' at line 2048?
Attachment #796137 - Flags: review?(echou) → review+
Attachment #795742 - Flags: review?(echou) → review+
Hi

> Question: Although I checked the code and realized that size_type and
> index_type are both uint_32, in this case, is there any reasons using
> different types to hold mProcessedDeviceAddresses and variable 'i' at line
> 2048?

'i' is of type index_type because it holds an index. At line 2046, there is a call to nsTArray::Length, which returns a size_type value that is compared to mProcessedDeviceAddresses. If mProcessedDeviceAddresses is of index_type, and index_type could also be  a signed integer, we'd get a warning from the compiler about comparing values with different signs. The code avoids this corner case by declaring mProcessedDeviceAddresses to be of type size_type. The increment and assignment statement at line 2048 does not generate a warning.

I don't know why nsTArray uses different types for sizes and indices in the first place. STL data structures use size_type everywhere.
https://hg.mozilla.org/mozilla-central/rev/c847f7095ca6
https://hg.mozilla.org/mozilla-central/rev/ab487e1139a5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.