880168 - [Bluetooth] Replace RemoveDeviceTask by non-blocking implementation

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

We need to replace RemoveDeviceTask by a non-blocking implementation in order to fix bug 853550.

Comment 1

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

Attachment: [01] Bug 880168: Replace RemoveDeviceTask by non-blocking implementation

Comment 2

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

It's for bug 853550.

Comment 3

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

Attachment: [01] Bug 880168: Replace RemoveDeviceTask by non-blocking implementation (v2)

Fixed the same problems as in bug 880167.

Comment 4

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

Attachment: [01] Bug 880168: Replace RemoveDeviceTask by non-blocking implementation (v3)

Updated from review for bug 881067.

Comment 5

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

Attachment: [01] Bug 880168: Replace RemoveDeviceTask by non-blocking implementation (v4)

Updated according to reviews for bug 880167.

Comment 6

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

Attachment: [01] Bug 880168: Replace RemoveDeviceTask by non-blocking implementation (v5)

Comment 7

[Eric Chou [:ericchou] [: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?

Comment 8

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

Attachment: [01] Bug 880168: Replace RemoveDeviceTask by non-blocking implementation (v6)

Comment 9

[Eric Chou [:ericchou] [: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!

Comment 10

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

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.

Comment 11

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

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

Comment 12

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

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

Comment 13

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

Gina, could you review the patch?

Comment 14

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

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

Comment 15

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

Thanks :)

https://hg.mozilla.org/projects/birch/rev/bb2ce8f64e6e

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bb2ce8f64e6e