Closed Bug 880168 Opened 6 years ago Closed 6 years ago

[Bluetooth] Replace RemoveDeviceTask by non-blocking implementation

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

(Whiteboard: [fixed-in-birch])

Attachments

(1 file, 5 obsolete files)

We need to replace RemoveDeviceTask by a non-blocking implementation in order to fix bug 853550.
Fixed the same problems as in bug 880167.
Attachment #761311 - Attachment is obsolete: true
Attachment #761311 - Flags: review?(gyeh)
Attachment #761311 - Flags: review?(echou)
Attachment #761992 - Flags: review?(gyeh)
Attachment #761992 - Flags: review?(echou)
Updated from review for bug 881067.
Attachment #761992 - Attachment is obsolete: true
Attachment #761992 - Flags: review?(gyeh)
Attachment #761992 - Flags: review?(echou)
Attachment #762519 - Flags: review?(gyeh)
Attachment #762519 - Flags: review?(echou)
Updated according to reviews for bug 880167.
Attachment #762519 - Attachment is obsolete: true
Attachment #762519 - Flags: review?(gyeh)
Attachment #762519 - Flags: review?(echou)
Attachment #762567 - Flags: review?(gyeh)
Attachment #762567 - Flags: review?(echou)
Attachment #762567 - Attachment is obsolete: true
Attachment #762567 - Flags: review?(gyeh)
Attachment #762567 - Flags: review?(echou)
Attachment #763460 - Flags: review?(gyeh)
Attachment #763460 - Flags: review?(echou)
Comment on attachment 763460 [details] [diff] [review]
[01] Bug 880168: Replace RemoveDeviceTask by non-blocking implementation (v5)

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

Almost there. Please have me review again after revision. Thanks.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +2219,5 @@
>                                                     aDeviceAddress));
>  
> +  nsRefPtr<BluetoothReplyRunnable> runnable(aRunnable);
> +
> +  bool success = dbus_func_args_async(mConnection, 0,

To be consistent, please use -1(default timeout) instead of 0.

@@ +2224,5 @@
> +                                      OnRemoveDeviceReply,
> +                                      static_cast<void*>(runnable.get()),
> +                                      NS_ConvertUTF16toUTF8(sAdapterPath).get(),
> +                                      DBUS_ADAPTER_IFACE, "RemoveDevice",
> +                                      DBUS_TYPE_OBJECT_PATH, &tempDeviceObjectPath,

In current implementation, |&tempDeviceObjectPath| represents address of the nsCString object. Since this parameter should be const char**, I think it may be better using |const char*| to declare tempDeviceObjectPath instead of using nsCString. Make sense?
Attachment #763460 - Attachment is obsolete: true
Attachment #763460 - Flags: review?(gyeh)
Attachment #763460 - Flags: review?(echou)
Attachment #764632 - Flags: review?(gyeh)
Attachment #764632 - Flags: review?(echou)
Comment on attachment 764632 [details] [diff] [review]
[01] Bug 880168: Replace RemoveDeviceTask by non-blocking implementation (v6)

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

Looks good. Thanks!
Attachment #764632 - Flags: review?(echou) → review+
Hi Eric,

I see you already r+'ed this patch. Here are just some remarks that came into my mind.

> > +  nsRefPtr<BluetoothReplyRunnable> runnable(aRunnable);
> > +
> > +  bool success = dbus_func_args_async(mConnection, 0,
> 
> To be consistent, please use -1(default timeout) instead of 0.

Ok, but the original code did not use a timeout. It's a change in the behavior of that function, not just a change in the implementation.
 
> @@ +2224,5 @@
> > +                                      OnRemoveDeviceReply,
> > +                                      static_cast<void*>(runnable.get()),
> > +                                      NS_ConvertUTF16toUTF8(sAdapterPath).get(),
> > +                                      DBUS_ADAPTER_IFACE, "RemoveDevice",
> > +                                      DBUS_TYPE_OBJECT_PATH, &tempDeviceObjectPath,
> 
> In current implementation, |&tempDeviceObjectPath| represents address of the
> nsCString object. Since this parameter should be const char**, I think it
> may be better using |const char*| to declare tempDeviceObjectPath instead of
> using nsCString. Make sense?

Yes. I just read throw the docs of nsCString and couldn't find an overloaded operator &, which makes me wonder why this code worked when I tested it.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #10)
> Hi Eric,
> 
> I see you already r+'ed this patch. Here are just some remarks that came
> into my mind.
> 

Thanks! Reply inline.

> > > +  nsRefPtr<BluetoothReplyRunnable> runnable(aRunnable);
> > > +
> > > +  bool success = dbus_func_args_async(mConnection, 0,
> > 
> > To be consistent, please use -1(default timeout) instead of 0.
> 
> Ok, but the original code did not use a timeout. It's a change in the
> behavior of that function, not just a change in the implementation.
>  

Yeah, thanks for reminding. I think using -1 should be better just because 0 millisecond timeout seems too short and I don't know if callback function would be invoked too soon without successful operation.

> > @@ +2224,5 @@
> > > +                                      OnRemoveDeviceReply,
> > > +                                      static_cast<void*>(runnable.get()),
> > > +                                      NS_ConvertUTF16toUTF8(sAdapterPath).get(),
> > > +                                      DBUS_ADAPTER_IFACE, "RemoveDevice",
> > > +                                      DBUS_TYPE_OBJECT_PATH, &tempDeviceObjectPath,
> > 
> > In current implementation, |&tempDeviceObjectPath| represents address of the
> > nsCString object. Since this parameter should be const char**, I think it
> > may be better using |const char*| to declare tempDeviceObjectPath instead of
> > using nsCString. Make sense?
> 
> Yes. I just read throw the docs of nsCString and couldn't find an overloaded
> operator &, which makes me wonder why this code worked when I tested it.

I guess it's because of a coincidence that the field 'mData' locates at the head of class nsTSubstring. (https://mxr.mozilla.org/mozilla-central/source/xpcom/string/public/nsTSubstring.h#658)
> > Ok, but the original code did not use a timeout. It's a change in the
> > behavior of that function, not just a change in the implementation.
> >  
> 
> Yeah, thanks for reminding. I think using -1 should be better just because 0
> millisecond timeout seems too short and I don't know if callback function
> would be invoked too soon without successful operation.

I checked this again in the code and you were right in the first place. The original code is based on a function in DbusUtils.cpp, which applies a timeout of -1. So we now have the same behavior as it always has been.

> > Yes. I just read throw the docs of nsCString and couldn't find an overloaded
> > operator &, which makes me wonder why this code worked when I tested it.
> 
> I guess it's because of a coincidence that the field 'mData' locates at the
> head of class nsTSubstring.
> (https://mxr.mozilla.org/mozilla-central/source/xpcom/string/public/
> nsTSubstring.h#658)

I kind of expected something like that. :D
Gina, could you review the patch?
Flags: needinfo?(gyeh)
Comment on attachment 764632 [details] [diff] [review]
[01] Bug 880168: Replace RemoveDeviceTask by non-blocking implementation (v6)

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

Sorry for the late reply. Seems perfect to me. :)
Attachment #764632 - Flags: review?(gyeh) → review+
Flags: needinfo?(gyeh)
https://hg.mozilla.org/mozilla-central/rev/bb2ce8f64e6e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.