Closed Bug 903422 Opened 11 years ago Closed 11 years ago

[Bluetooth] Non-blocking AppendDeviceNameRunnable

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(3 files, 5 obsolete files)

OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Attachment #788132 - Flags: review?(gyeh)
Attachment #788132 - Flags: review?(echou)
Eric, Gina,

AppendDeviceNameRunnable gets dispatched from within the DBus thread. The only reason why AppendDeviceNameRunnable still exists in the patch is because it calls BluetoothService::Get(), which needs to run from within the main thread. But the result of this call is never used. Do you think we can delete BluetoothService::Get(), and replace the runnable it by a simple function?

Best regards
Thomas
Attachment #788143 - Flags: review?(gyeh)
Attachment #788143 - Flags: review?(echou)
Comment on attachment 788143 [details] [diff] [review]
[02] Bug 903422: Implement non-blocking AppendDeviceNameRunnable

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

The overall is good. One question to be answered and a few nits to be fixed.

Note that |NS_WARNING| is used in many places and we'd like to replace them with |BT_WARNING|.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +842,5 @@
> +  }
> +
> +private:
> +  nsCString mIface;
> +};

Question: Can we remove class GetPropertiesReplyHandler and maintain the data member mIface in AppendDeviceNameReplyHandler directly?

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

nit: Please use BT_WARNING as well.

@@ +880,5 @@
> +      return;
> +    }
> +
> +    // Replace object path with device address and...
> +

nit: The comment here seems incomplete.

@@ +909,4 @@
>    }
>  
>  private:
> +  nsString        mDevicePath;

nit: Please remove multiple white-spaces here. we usually don't do that.

@@ +929,5 @@
>  
>      BluetoothValue v = mSignal.value();
>      if (v.type() != BluetoothValue::TArrayOfBluetoothNamedValue ||
>          v.get_ArrayOfBluetoothNamedValue().Length() == 0) {
> +      NS_WARNING("Invalid argument type for AppendDeviceNameRunnable");

nit: Please use BT_WARNING as well.

@@ +938,5 @@
>  
>      // Device object path should be put in the first element
>      if (!arr[0].name().EqualsLiteral("path") ||
>          arr[0].value().type() != BluetoothValue::TnsString) {
> +      NS_WARNING("Invalid object path for AppendDeviceNameRunnable");

nit: Please use BT_WARNING as well.

@@ +946,5 @@
> +    nsRefPtr<RawDBusConnection> threadConnection = gThreadConnection;
> +
> +    if (!threadConnection.get()) {
> +      BT_WARNING("%s: DBus connection has been closed.", __FUNCTION__);
> +      return;

return NS_ERROR_FAILURE;
Thanks for the review. See below for my comments.

(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #3)
> Comment on attachment 788143 [details] [diff] [review]
> [02] Bug 903422: Implement non-blocking AppendDeviceNameRunnable
> 
> Review of attachment 788143 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The overall is good. One question to be answered and a few nits to be fixed.
> 
> Note that |NS_WARNING| is used in many places and we'd like to replace them
> with |BT_WARNING|.

Oh, I didn't know. I'll update the patch.

> 
> ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> @@ +842,5 @@
> > +  }
> > +
> > +private:
> > +  nsCString mIface;
> > +};
> 
> Question: Can we remove class GetPropertiesReplyHandler and maintain the
> data member mIface in AppendDeviceNameReplyHandler directly?

We can, but I have patches for other operations that use 'GetProperties'. Introducing this base class here will make sense when the other patches arrive. Are you OK with that?

> @@ +875,5 @@
> > +
> > +    bool success = UnpackPropertiesMessage(aReply, &err, deviceProperties,
> > +                                           GetInterface().get());
> > +    if (!success) {
> > +      NS_WARNING("Failed to get device properties");
> 
> nit: Please use BT_WARNING as well.

Ok.

> @@ +880,5 @@
> > +      return;
> > +    }
> > +
> > +    // Replace object path with device address and...
> > +
> 
> nit: The comment here seems incomplete.

It continues in the next comment below. I'll update the commenting here to make it more readable.

> @@ +909,4 @@
> >    }
> >  
> >  private:
> > +  nsString        mDevicePath;
> 
> nit: Please remove multiple white-spaces here. we usually don't do that.

Ok.

> @@ +929,5 @@
> >  
> >      BluetoothValue v = mSignal.value();
> >      if (v.type() != BluetoothValue::TArrayOfBluetoothNamedValue ||
> >          v.get_ArrayOfBluetoothNamedValue().Length() == 0) {
> > +      NS_WARNING("Invalid argument type for AppendDeviceNameRunnable");
> 
> nit: Please use BT_WARNING as well.

Ok.

> @@ +938,5 @@
> >  
> >      // Device object path should be put in the first element
> >      if (!arr[0].name().EqualsLiteral("path") ||
> >          arr[0].value().type() != BluetoothValue::TnsString) {
> > +      NS_WARNING("Invalid object path for AppendDeviceNameRunnable");
> 
> nit: Please use BT_WARNING as well.

Ok.

> @@ +946,5 @@
> > +    nsRefPtr<RawDBusConnection> threadConnection = gThreadConnection;
> > +
> > +    if (!threadConnection.get()) {
> > +      BT_WARNING("%s: DBus connection has been closed.", __FUNCTION__);
> > +      return;
> 
> return NS_ERROR_FAILURE;

Strange. I don't remember getting a compiler warning here. I must have missed this.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #4)
> > 
> > The overall is good. One question to be answered and a few nits to be fixed.
> > 
> > Note that |NS_WARNING| is used in many places and we'd like to replace them
> > with |BT_WARNING|.
> 
> Oh, I didn't know. I'll update the patch.

Thanks for your help. :)

> > ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> > @@ +842,5 @@
> > > +  }
> > > +
> > > +private:
> > > +  nsCString mIface;
> > > +};
> > 
> > Question: Can we remove class GetPropertiesReplyHandler and maintain the
> > data member mIface in AppendDeviceNameReplyHandler directly?
> 
> We can, but I have patches for other operations that use 'GetProperties'.
> Introducing this base class here will make sense when the other patches
> arrive. Are you OK with that?

Got it. Then it would be fine for me.
Updated according to review.
Attachment #788143 - Attachment is obsolete: true
Attachment #788143 - Flags: review?(gyeh)
Attachment #788143 - Flags: review?(echou)
Attachment #788972 - Flags: review?(gyeh)
Attachment #788972 - Flags: review?(echou)
Comment on attachment 788972 [details] [diff] [review]
[02] Bug 903422: Implement non-blocking AppendDeviceNameRunnable (v2)

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

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +971,5 @@
> +                                        handler.get(),
> +                                        NS_ConvertUTF16toUTF8(devicePath).get(),
> +                                        DBUS_DEVICE_IFACE, "GetProperties",
> +                                        DBUS_TYPE_INVALID);
> +    NS_ENSURE_TRUE(success, false);

NS_ENSURE_TRUE(success, NS_ERROR_FAILURE);
Comment on attachment 788132 [details] [diff] [review]
[01] Bug 903422: Cleanup GetPropertiesInternal

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

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +736,5 @@
> +
> +  return replyError.IsEmpty();
> +}
> +
> +

super-nit: extra newline
Attachment #788132 - Flags: review?(echou) → review+
Hi Thomas,

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #2)
> Created attachment 788143 [details] [diff] [review]
> [02] Bug 903422: Implement non-blocking AppendDeviceNameRunnable
> 
> Eric, Gina,
> 
> AppendDeviceNameRunnable gets dispatched from within the DBus thread. The
> only reason why AppendDeviceNameRunnable still exists in the patch is
> because it calls BluetoothService::Get(), which needs to run from within the
> main thread. But the result of this call is never used. Do you think we can
> delete BluetoothService::Get(), and replace the runnable it by a simple
> function?
> 

Yes, you're right. We no longer need to get BluetoothService instance in this case. Please update the patch and I would be happy to review once it's done.
Fixed the extra newline and converted to BT_WARNING.
Attachment #788132 - Attachment is obsolete: true
Attachment #788132 - Flags: review?(gyeh)
Attachment #789478 - Flags: review?(gyeh)
Removed BluetoothService::Get and converted runnable to simple function call.
Attachment #788972 - Attachment is obsolete: true
Attachment #788972 - Flags: review?(gyeh)
Attachment #788972 - Flags: review?(echou)
Attachment #789479 - Flags: review?(gyeh)
Attachment #789479 - Flags: review?(echou)
Attachment #789479 - Attachment is obsolete: true
Attachment #789479 - Flags: review?(gyeh)
Attachment #789479 - Flags: review?(echou)
Comment on attachment 789481 [details] [diff] [review]
[02] Bug 903422: Implement non-blocking AppendDeviceNameRunnable (v3)

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

Overall looks good. The only reason I r- is that, after these two patches being merged, the call flow related to function UnpackPropertiesMessage() would become a little complicated since there would be 2 UnpackPropertiesMessage(). For example, in AppendDeviceNameReplyHandler::Handle(), it calls UnpackPropertiesMessage() to unpack the DBusMessage, then Unpack[Device|Adapter|Manager]PropertiesMessage would be called depending on the variable aIface, and then the other UnpackPropertiesMessage() would be called to really parse the message.

Thomas, I think we could simplify it by integrating UnpackPropertiesMessage() and Unpack[Device|Adapter|Manager]PropertiesMessage() into one function. Either modifying the second patch or adding another interdiff as the third patch would be fine.

Thanks!
Attachment #789481 - Flags: review?(echou) → review-
Maybe this was a bit hard to describe on irc. Separating the selection of the properties array from the actual parsing gives more readable code IMHO. I also added a boolean return value to UnpackPropertiesMessage to clearly signal when the function failed. The whole handling of error strings is still a bit sloppy, but I didn't want to introduce any extra changes here.
Attachment #790708 - Flags: review?(echou)
Comment on attachment 790708 [details] [diff] [review]
[03] Bug 903422: Cleanup UnpackPropertiesMessage

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

Still thinking that we could merge UnpackPropertiesMessageFromInterface and UnpackPropertiesMessage, not want to be stuck on this, though. ;)

It's much cleaner than before, r=me with the nit addressed. Thanks!

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +758,5 @@
>    } else {
>      return false;
>    }
>  
> +  nsAutoString errorStr;

nit: let's save a variable and just print warnings inside UnpackPropertiesMessage().
Attachment #790708 - Flags: review?(echou) → review+
> Maybe this was a bit hard to describe on irc. Separating the selection of
> the properties array from the actual parsing gives more readable code IMHO.
> I also added a boolean return value to UnpackPropertiesMessage to clearly
> signal when the function failed.

Actually I did know what you were trying to say on IRC, just not sure which way (integrate or not) would be better.
Comment on attachment 789481 [details] [diff] [review]
[02] Bug 903422: Implement non-blocking AppendDeviceNameRunnable (v3)

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

Change to r+ since we've got patch 3!
Attachment #789481 - Flags: review- → review+
Eric, 

I merged the two functions. We cannot remove errorStr, because it's needed by IsDBusMessageError and ParseProperties.
Attachment #790708 - Attachment is obsolete: true
Attachment #790731 - Flags: review+
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #18)
> Created attachment 790731 [details] [diff] [review]
> [03] Bug 903422: Cleanup UnpackPropertiesMessage (v2)
> 
> Eric, 
> 
> I merged the two functions. We cannot remove errorStr, because it's needed
> by IsDBusMessageError and ParseProperties.

Thanks!
Gina,

Don't forget about these patches here. :) Thanks!
Comment on attachment 789478 [details] [diff] [review]
[01] Bug 903422: Cleanup GetPropertiesInternal (v2)

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

Since this patch has been updated as v3, I think the review request should be cancelled, right?
Attachment #789478 - Flags: review?(gyeh)
There is no v3 of this patch. I just updated the patch once and thought you might want to take another look at it.
Comment on attachment 789481 [details] [diff] [review]
[02] Bug 903422: Implement non-blocking AppendDeviceNameRunnable (v3)

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

Please see my following comments. Thanks.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +1154,5 @@
>      // Increase ref count here because we need this message later.
>      // It'll be unrefed when set*Internal() is called.
>      dbus_message_ref(msg);
>  
> +    AppendDeviceName(signal);

We probably should handle the return value of function AppendDeviceName. Otherwise, we can just make the return value to void.

@@ +1159,3 @@
>    } else {
>      handler = new DistributeBluetoothSignalTask(signal);
> +    NS_DispatchToMainThread(handler);

We don't need variable |handler|. Please remove it and use the following line:

NS_DispatchToMainThread(new DistributeBluetoothSignalTask(signal));
Attachment #789481 - Flags: review?(gyeh) → review+
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #22)
> There is no v3 of this patch. I just updated the patch once and thought you
> might want to take another look at it.

I'm a bit confused :| Are we going to apply patch 2 AND patch 3?

Since some functions are removed in patch 3 (e.g. UnpackAdapterPropertiesMessage), I think the build would be broken, right?
I'm going to apply all 3 patches. Patch 3 merges all Unpack*PropertiesMessages into one. There should be just one user of UnpackAdapterPropertiesMessage et al. I'll check twice before committing this, but building and testing works for me.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #25)
> I'm going to apply all 3 patches. Patch 3 merges all
> Unpack*PropertiesMessages into one. There should be just one user of
> UnpackAdapterPropertiesMessage et al. I'll check twice before committing
> this, but building and testing works for me.

Great! Then let's ship it.
Blocks: 906019
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: