906019 - [Bluetooth] Non-blocking GetDefaultAdapterPathInternal

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 906019: Implemented non-blocking GetDefaultAdapterPathInternal

Comment 2

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

Comment on attachment 792681 [details] [diff] [review]
[01] Bug 906019: Implemented non-blocking GetDefaultAdapterPathInternal

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

Please see my following comments. Would like to see the patch again after updating. Thanks. :)

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +1852,5 @@
> +      return false;
> +    }
> +
> +    mAdapterPath = value.get_nsString();
> +    NS_WARNING(NS_ConvertUTF16toUTF8(mAdapterPath).get());

We're going to put mAdapterPath in log, right?

@@ +1861,5 @@
> +    bool success = dbus_func_args_async(gThreadConnection->GetConnection(), 1000,
> +                                        DefaultAdapterPathReplyHandler::Callback,
> +                                        handler.get(),
> +                                        NS_ConvertUTF16toUTF8(mAdapterPath).get(),
> +                                        GetInterface().get(), "GetProperties",

Since this function is only used to get properties of BluetoothAdapter, I think we can just make it DBUS_ADAPTER_IFACE here and remove class GetPropertiesReplyHandler.

@@ +1913,5 @@
>    }
>  
> +  nsRefPtr<DefaultAdapterPathReplyHandler> handler =
> +    new DefaultAdapterPathReplyHandler(nsCString(DBUS_ADAPTER_IFACE), aRunnable);
> +  bool success = dbus_func_args_async(gThreadConnection->GetConnection(), 1000,

Should we use mConnection here?

Comment 3

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

Hi Gina

(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #2)
> Comment on attachment 792681 [details] [diff] [review]
> [01] Bug 906019: Implemented non-blocking GetDefaultAdapterPathInternal
> 
> Review of attachment 792681 [details] [diff] [review]:
> -----------------------------------------------------------------

Wow, that was quick.

> Please see my following comments. Would like to see the patch again after
> updating. Thanks. :)
> 
> ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> @@ +1852,5 @@
> > +      return false;
> > +    }
> > +
> > +    mAdapterPath = value.get_nsString();
> > +    NS_WARNING(NS_ConvertUTF16toUTF8(mAdapterPath).get());
> 
> We're going to put mAdapterPath in log, right?

Arrg, that was for debugging and I forgot to remove it. Thanks for catching this line.

> 
> @@ +1861,5 @@
> > +    bool success = dbus_func_args_async(gThreadConnection->GetConnection(), 1000,
> > +                                        DefaultAdapterPathReplyHandler::Callback,
> > +                                        handler.get(),
> > +                                        NS_ConvertUTF16toUTF8(mAdapterPath).get(),
> > +                                        GetInterface().get(), "GetProperties",
> 
> Since this function is only used to get properties of BluetoothAdapter, I
> think we can just make it DBUS_ADAPTER_IFACE here and remove class
> GetPropertiesReplyHandler.

It looks like I can't convince you of GetPropertiesReplyHandler. ;) I have a third patch which might use this class. But if I can re-implement this patch without much hassle, I'll remove GetPropertiesReplyHandler with the next update here.

> @@ +1913,5 @@
> >    }
> >  
> > +  nsRefPtr<DefaultAdapterPathReplyHandler> handler =
> > +    new DefaultAdapterPathReplyHandler(nsCString(DBUS_ADAPTER_IFACE), aRunnable);
> > +  bool success = dbus_func_args_async(gThreadConnection->GetConnection(), 1000,
> 
> Should we use mConnection here?

Yes, and I should have grabbed a ref to gThreadConnected in HandleDefaultAdapterPathReply.

Thanks for the quick review. I'll upload an update later.

Comment 4

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

Comment on attachment 792681 [details] [diff] [review]
[01] Bug 906019: Implemented non-blocking GetDefaultAdapterPathInternal

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

Hey Thomas,

Several places need to be revised. Thanks!

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +1820,4 @@
>    {
> +    MOZ_ASSERT(!NS_IsMainThread()); // DBus thread
> +
> +    if (!aReply || (dbus_message_get_type(aReply) == DBUS_MESSAGE_TYPE_ERROR)) {

We should handle mRunnable here, otherwise Gaia may not be able to receive the onerror/onsuccess event of DOMRequest.

@@ +1840,5 @@
> +
> +protected:
> +  bool HandleDefaultAdapterPathReply(DBusMessage* aReply,
> +                                     nsAutoString& aReplyError)
> +  {

Can we have the thread-check assertion for this function and HandleGetPropertiesReply() as well?

@@ +1863,5 @@
> +                                        handler.get(),
> +                                        NS_ConvertUTF16toUTF8(mAdapterPath).get(),
> +                                        GetInterface().get(), "GetProperties",
> +                                        DBUS_TYPE_INVALID);
> +    NS_ENSURE_TRUE(success, false);

If we return false here without setting an error string, the if-block at line 1836 will not be entered, which means mRunnable won't be handled.

@@ +1913,5 @@
>    }
>  
> +  nsRefPtr<DefaultAdapterPathReplyHandler> handler =
> +    new DefaultAdapterPathReplyHandler(nsCString(DBUS_ADAPTER_IFACE), aRunnable);
> +  bool success = dbus_func_args_async(gThreadConnection->GetConnection(), 1000,

Please use mConnection instead of gThreadConnection->GetConnection() on main thread. At least that's how we do for now. ;)

Comment 5

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

Hi Eric

(In reply to Eric Chou [:ericchou] [:echou] from comment #4)
> Comment on attachment 792681 [details] [diff] [review]
> [01] Bug 906019: Implemented non-blocking GetDefaultAdapterPathInternal
> 
> Review of attachment 792681 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hey Thomas,
> 
> Several places need to be revised. Thanks!

Today you guys review the patches faster than I can upload them. :D

> 
> ::: dom/bluetooth/linux/BluetoothDBusService.cpp
> @@ +1820,4 @@
> >    {
> > +    MOZ_ASSERT(!NS_IsMainThread()); // DBus thread
> > +
> > +    if (!aReply || (dbus_message_get_type(aReply) == DBUS_MESSAGE_TYPE_ERROR)) {
> 
> We should handle mRunnable here, otherwise Gaia may not be able to receive
> the onerror/onsuccess event of DOMRequest.

Oh, good point.

> @@ +1840,5 @@
> > +
> > +protected:
> > +  bool HandleDefaultAdapterPathReply(DBusMessage* aReply,
> > +                                     nsAutoString& aReplyError)
> > +  {
> 
> Can we have the thread-check assertion for this function and
> HandleGetPropertiesReply() as well?

Sure.

> @@ +1863,5 @@
> > +                                        handler.get(),
> > +                                        NS_ConvertUTF16toUTF8(mAdapterPath).get(),
> > +                                        GetInterface().get(), "GetProperties",
> > +                                        DBUS_TYPE_INVALID);
> > +    NS_ENSURE_TRUE(success, false);
> 
> If we return false here without setting an error string, the if-block at
> line 1836 will not be entered, which means mRunnable won't be handled.

Let's add an error string here, but also remove the error-string check around line 1836.

> @@ +1913,5 @@
> >    }
> >  
> > +  nsRefPtr<DefaultAdapterPathReplyHandler> handler =
> > +    new DefaultAdapterPathReplyHandler(nsCString(DBUS_ADAPTER_IFACE), aRunnable);
> > +  bool success = dbus_func_args_async(gThreadConnection->GetConnection(), 1000,
> 
> Please use mConnection instead of gThreadConnection->GetConnection() on main
> thread. At least that's how we do for now. ;)

Sure.

Comment 6

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

> Today you guys review the patches faster than I can upload them. :D

We are more efficient now! ;)

Comment 7

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

Attachment: [01] Bug 906019: Remove GetPropertiesReplyHandler

Comment 8

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

Attachment: [02] Bug 906019: Implemented non-blocking GetDefaultAdapterPathInternal (v2)

Comment 9

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

Hey Thomas, just let you know that Taipei office will be closed tomorrow because of typhoon. Gina and I will review your patch on Thursday. :)

Comment 10

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

Comment on attachment 792758 [details] [diff] [review]
[02] Bug 906019: Implemented non-blocking GetDefaultAdapterPathInternal (v2)

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

Thanks for your quick update. Please check my following comment.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +1923,5 @@
> +                                      handler.get(),
> +                                      "/",
> +                                      DBUS_MANAGER_IFACE, "DefaultAdapter",
> +                                      DBUS_TYPE_INVALID);
> +  NS_ENSURE_TRUE(success, false);

NS_ENSURE_TRUE(success, NS_ERROR_FAILURE);

Comment 11

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

Comment on attachment 792757 [details] [diff] [review]
[01] Bug 906019: Remove GetPropertiesReplyHandler

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

Thanks. :)

Comment 12

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

Comment on attachment 792758 [details] [diff] [review]
[02] Bug 906019: Implemented non-blocking GetDefaultAdapterPathInternal (v2)

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

r=me with nits addressed.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +1852,5 @@
> +
> +    nsRefPtr<RawDBusConnection> threadConnection = gThreadConnection;
> +
> +    if (!threadConnection.get()) {
> +      aReplyError = NS_ConvertUTF8toUTF16("DBus connection has been closed.");

To avoid converting string runtime, I suggest that we could change the type of 2nd parameter from nsAutoString& to nsAString. Then this line could become

  aReplyError = NS_LITERAL_STRING("DBus connection has been closed.");

Here and below.

Comment 13

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

Thanks! I fixed the remaining issues and pushed the patches to b2g-inbound.

https://hg.mozilla.org/integration/b2g-inbound/rev/00d4b6d77db4
https://hg.mozilla.org/integration/b2g-inbound/rev/0a41844a2ce3

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/00d4b6d77db4
https://hg.mozilla.org/mozilla-central/rev/0a41844a2ce3