Closed Bug 969314 Opened 7 years ago Closed 7 years ago

[Bluetooth] Merge BluetoothGonkService into BluetoothDBusService

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S1 (14feb)

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(3 files, 5 obsolete files)

6.78 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
15.06 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
2.47 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
BluetoothGonkService is BluetoothDBusService plus some code for loading firmware. Merging the former into the latter will simplify the code base.
Only moves code around.
Attachment #8372249 - Flags: review?(echou)
Depends on: 964817
Comment on attachment 8372249 [details] [diff] [review]
[01] Bug 969314: Merge BluetoothGonkService into BluetoothDBusService

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

Looks good.
Attachment #8372249 - Flags: review?(echou) → review+
Comment on attachment 8372250 [details] [diff] [review]
[02] Bug 969314: Cleanup handling of libbluedroid.so

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

r=me with nits picked.

::: dom/bluetooth/bluez/linux/BluetoothDBusService.cpp
@@ +122,5 @@
> +    MOZ_ASSERT(!NS_IsMainThread()); // BT thread
> +
> +    if (!mHandle && !Init()) {
> +      return false;
> +    }

In Disable(), it checks the case 'request to disable when it's already disabled'. We can do the same thing in Enable().

@@ +1854,5 @@
>      return NS_OK;
>    }
>    if (aShouldEnable) {
> +    result = sBluedroid.Enable();
> +    if (!result) {

nit: we can remove this if-check since 'result' will be checked below.
Attachment #8372250 - Flags: review?(echou) → review+
Comment on attachment 8372251 [details] [diff] [review]
[03] Bug 969314: Cleanup Gonk start/stop code in BluetoothDBusService

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

Hey Thomas,

BluetoothGonkService.* should be removed in patch 1. I think you might attach the wrong patch (you could tell by the summary of this patch, "Bug 957103: Implement StartStopBluetooth in BluetoothGonkService").
Attachment #8372251 - Flags: review?(echou) → review-
Oops, this should be the correct patch. ;)
Attachment #8372251 - Attachment is obsolete: true
Attachment #8373901 - Flags: review?(echou)
Comment on attachment 8373901 [details] [diff] [review]
[03] Bug 969314: Cleanup Gonk start/stop code in BluetoothDBusService (v2)

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

Hey Thomas,

Thanks for cleaning this up. Please see my comments below.

::: dom/bluetooth/bluez/linux/BluetoothDBusService.cpp
@@ +1844,5 @@
>    // This could block. It should never be run on the main thread.
>    MOZ_ASSERT(!NS_IsMainThread());
>  
>  #ifdef MOZ_WIDGET_GONK
> +  if (!sBluedroid.IsEnabled() && !sBluedroid.Enable()) {

Assume that Bluetooth is enabled and somehow StartInternal() is still called, then:

1. sBluedroid.Enable() won't be called
2. StartDBus() will be called, and it will return false since the event loop has been started.

So, my first concern is that it means that we implicitly depend on StartDBus() to stop function StartInternal(). Moreover, NS_ERROR_FAILURE will be returned under this circumstance (because StartDBus() will return false). The result for Enable() is not the same as the Disable() case since sBluedroid.Disable() will return true while Bluetooth was disabled. It seems to be a little bit inconsistent to me.
Hi Eric

(In reply to Eric Chou [:ericchou] [:echou] from comment #8)
> Comment on attachment 8373901 [details] [diff] [review]
> [03] Bug 969314: Cleanup Gonk start/stop code in BluetoothDBusService (v2)
> 
> Review of attachment 8373901 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hey Thomas,
> 
> Thanks for cleaning this up. Please see my comments below.
> 
> ::: dom/bluetooth/bluez/linux/BluetoothDBusService.cpp
> @@ +1844,5 @@
> >    // This could block. It should never be run on the main thread.
> >    MOZ_ASSERT(!NS_IsMainThread());
> >  
> >  #ifdef MOZ_WIDGET_GONK
> > +  if (!sBluedroid.IsEnabled() && !sBluedroid.Enable()) {

Can you elaborate a bit on your example? I'm not sure if I really understand.

> Assume that Bluetooth is enabled and somehow StartInternal() is still
> called, then:
> 
> 1. sBluedroid.Enable() won't be called

I think sBluedroid should really just handle the state of the Bluedroid lib, but not the internal state of our Bluetooth code. So when IsEnabled() or Enabled() returns true, the bluedroid library has been loaded and initialized; nothing else. With the changes you requested for patch [02] we can even call Enable unconditionally.

> 2. StartDBus() will be called, and it will return false since the event loop
> has been started.

This behavior has been changed quite a while ago. Whenever you call StartDBus(), it'll assert that no connection has been opened. Or if assertions are disabled, it'll open a new DBus connection at worst. But having multiple connection is not a problem per se.

And actually, I though about removing StartDBus et al. completely. The details of connection handling have been moved to RawDBusConnection.{cpp,h}, and what's left in DBusThread.{cpp,h} should be handled by Bluetooth directly IMHO.

> 
> So, my first concern is that it means that we implicitly depend on
> StartDBus() to stop function StartInternal(). Moreover, NS_ERROR_FAILURE
> will be returned under this circumstance (because StartDBus() will return
> false). The result for Enable() is not the same as the Disable() case since
> sBluedroid.Disable() will return true while Bluetooth was disabled. It seems
> to be a little bit inconsistent to me.

I'm not sure if I get the final two sentences, but with the requested changes to patch [02], Enable() would return true if the library has been loaded already. Wouldn't that fix the inconsistency?

Best regards
Thomas
Flags: needinfo?(echou)
> > 2. StartDBus() will be called, and it will return false since the event loop
> > has been started.
> 
> This behavior has been changed quite a while ago. Whenever you call
> StartDBus(), it'll assert that no connection has been opened.

Sorry, that statement is wrong. StartDBus returns true if a connection has already been opened, which means we'd run the init code a second time for the existing connection. Not nice, but probably not a problem here.
Hi Thomas,

Sorry for the unclear description. My major concern is that different values will be returned for the similar two cases after applying these patches (including the change based on my comments).

  1. StartInternal() is called when BT is enabled
     => NS_ERROR_FAILURE will be returned
  2. StopInternal() is called when BT is disabled
     => NS_OK will be returned

That's what I meant 'inconsistent'.
Flags: needinfo?(echou)
(In reply to Eric Chou [:ericchou] [:echou] from comment #11)
> Hi Thomas,
> 
> Sorry for the unclear description. My major concern is that different values
> will be returned for the similar two cases after applying these patches
> (including the change based on my comments).
> 
>   1. StartInternal() is called when BT is enabled
>      => NS_ERROR_FAILURE will be returned

Per discussion with Thomas on irc, this case won't return NS_ERROR_FAILURE but NS_OK.
Attachment #8373901 - Flags: review?(echou) → review+
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #10)
> > > 2. StartDBus() will be called, and it will return false since the event loop
> > > has been started.
> > 
> > This behavior has been changed quite a while ago. Whenever you call
> > StartDBus(), it'll assert that no connection has been opened.
> 
> Sorry, that statement is wrong. StartDBus returns true if a connection has
> already been opened, which means we'd run the init code a second time for
> the existing connection. Not nice, but probably not a problem here.

I meant that *my* statement about StartDBus calling assert is wrong. The whole issue will be fixed once bug 972253 landed.
Attachment #8375470 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.