Closed
Bug 969314
Opened 10 years ago
Closed 10 years ago
[Bluetooth] Merge BluetoothGonkService into BluetoothDBusService
Categories
(Firefox OS Graveyard :: Bluetooth, defect)
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.
Assignee | ||
Comment 1•10 years ago
|
||
Only moves code around.
Attachment #8372249 -
Flags: review?(echou)
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8372250 -
Flags: review?(echou)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8372251 -
Flags: review?(echou)
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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 6•10 years ago
|
||
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-
Assignee | ||
Comment 7•10 years ago
|
||
Oops, this should be the correct patch. ;)
Attachment #8372251 -
Attachment is obsolete: true
Attachment #8373901 -
Flags: review?(echou)
Comment 8•10 years ago
|
||
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.
Assignee | ||
Comment 9•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(echou)
Assignee | ||
Comment 10•10 years ago
|
||
> > 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.
Comment 11•10 years ago
|
||
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)
Comment 12•10 years ago
|
||
(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.
Updated•10 years ago
|
Attachment #8373901 -
Flags: review?(echou) → review+
Assignee | ||
Comment 13•10 years ago
|
||
(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.
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8372250 -
Attachment is obsolete: true
Attachment #8375469 -
Flags: review+
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8373901 -
Attachment is obsolete: true
Attachment #8375470 -
Flags: review+
Assignee | ||
Comment 16•10 years ago
|
||
Rebased onto bug 969447.
Attachment #8372249 -
Attachment is obsolete: true
Attachment #8375528 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8375470 -
Attachment is obsolete: true
Assignee | ||
Comment 18•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/fad8e6966ea6 https://hg.mozilla.org/integration/b2g-inbound/rev/7b73db4d0d23 https://hg.mozilla.org/integration/b2g-inbound/rev/a4140e8bea6d https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=a4140e8bea6d
https://hg.mozilla.org/mozilla-central/rev/fad8e6966ea6 https://hg.mozilla.org/mozilla-central/rev/7b73db4d0d23 https://hg.mozilla.org/mozilla-central/rev/a4140e8bea6d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S1 (14feb)
You need to log in
before you can comment on or make changes to this bug.
Description
•