906020 - [Bluetooth] Non-blocking BluetoothArrayOfDeviceProperties

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

Part of bug 902396

Comment 1

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

Attachment: [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.

Comment 2

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

Attachment: [02] Bug 906020: Remove BluetoothArrayOfDevicePropertiesRunnable

Comment 3

[Gina Yeh [:gyeh] [:ginayeh]]

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 ;)

Comment 4

[Gina Yeh [:gyeh] [:ginayeh]]

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>.

Comment 5

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

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

Comment 6

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

Attachment: [01] Bug 906020: Implement non-blocking BluetoothArrayOfDevicePropertiesRunnable (v2)

Comment 7

[Gina Yeh [:gyeh] [:ginayeh]]

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

Comment 8

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

Thanks Gina for the review. I'll fix your remaining points together with what Eric mentions.

Comment 9

[Eric Chou [:ericchou] [:echou]]

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?

Comment 10

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

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.

Comment 11

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

Thanks for the reviews. I fixed the remaining nits and pushed the patch set to b2g-inbound

https://hg.mozilla.org/integration/b2g-inbound/rev/c847f7095ca6
https://hg.mozilla.org/integration/b2g-inbound/rev/ab487e1139a5

https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=ab487e1139a5

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c847f7095ca6
https://hg.mozilla.org/mozilla-central/rev/ab487e1139a5