Closed Bug 939672 Opened 11 years ago Closed 10 years ago

[Bluedroid] Add fallback HFP manager for MOZ_B2G_RIL disabled build

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ben.tian, Assigned: ben.tian)

References

Details

Attachments

(3 files, 3 obsolete files)

Per bug 938524 comment 6, add MOZ_B2G_RIL flag into bluedroid HFP manager in this bug.
Blocks: flatfish
Depends on: 938524, 920551
Can't the patch from bug: 957485 be applied here?
https://bugzilla.mozilla.org/attachment.cgi?id=8359027&action=edit
Add fallback BluetoothHfpManager for RIL-disabled build.
Assignee: nobody → btian
Attachment #8378091 - Flags: review?(echou)
Attachment #8378091 - Flags: feedback?(shuang)
(In reply to Nikola from comment #3)
> Can't the patch from bug: 957485 be applied here?
> https://bugzilla.mozilla.org/attachment.cgi?id=8359027&action=edit
I think the patch is too messy for code maintenance and prefer an isolated fallback HFP manager as in comment 4.
Comment on attachment 8378091 [details] [diff] [review]
Patch 1 (v1): Fallback HFP manager for MOZ_B2G_RIL disabled build

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

Overall is workable. Maybe we can replace hfp-fallback with hfp-voip? Since the only case I can think this callfack can be used for HFP voip case (such as Skype VOIP + Bluetooth Handsfree headset) on non-phone products (tablet) in the near future.

::: dom/bluetooth/bluedroid/hfp-fallback/BluetoothHfpManager.cpp
@@ +6,5 @@
> +
> +#include "base/basictypes.h"
> +#include "BluetoothHfpManager.h"
> +#include "BluetoothProfileController.h"
> +

We shall explain why this 'fallback' version exists.
Attachment #8378091 - Flags: feedback?(shuang) → feedback+
Changes:
- Return non-nullptr in BluetoothHfpManager::Get()
- Add nsIObserver functions
- Explain the purpose in hfp-fallback/BluetoothHfpManager.h
Attachment #8378091 - Attachment is obsolete: true
Attachment #8378091 - Flags: review?(echou)
Attachment #8378915 - Flags: review?(gyeh)
Also revise log in BluetoothService.cpp
Attachment #8378917 - Flags: review?(gyeh)
Comment on attachment 8378915 [details] [diff] [review]
Patch 1/2 (v2): Add fallback BluetoothHfpManager for bluedroid

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

For functions which is inherited from base class but no implementation, we can add |MOZ_ASSERT(false)| to avoid someone uses them accidentally.

::: dom/bluetooth/bluedroid/hfp-fallback/BluetoothHfpManager.cpp
@@ +7,5 @@
> +#include "base/basictypes.h"
> +#include "BluetoothHfpManager.h"
> +#include "BluetoothProfileController.h"
> +#include "mozilla/StaticPtr.h"
> +#include "nsThreadUtils.h"

We can also move these to BluetoothHfpManagerBase.h

@@ +102,5 @@
> +  }
> +
> +  // Create a new instance and return
> +  sBluetoothHfpManager = new BluetoothHfpManager();
> +  return sBluetoothHfpManager;

It should register topic NS_XPCOM_SHUTDOWN_OBSERVER_ID and release |sBluetoothHfpManager| whenever it's notified.

@@ +108,5 @@
> +
> +bool
> +BluetoothHfpManager::ConnectSco()
> +{
> +  return false;

Some applications may want to create a SCO link without a HFP connection. Let's handle this scenario in another bug.

::: dom/bluetooth/bluedroid/hfp-fallback/BluetoothHfpManager.h
@@ +33,5 @@
> +  static BluetoothHfpManager* Get();
> +  virtual ~BluetoothHfpManager() { }
> +
> +  bool ConnectSco();
> +  bool DisconnectSco();

I found something weird when reviewing this patch. Instead of repling the runnable in BluetoothServiceBluedroid::ConnectSco(), we should hold the runnable and reply it after the SCO is established. Once we fix that part, the above two methods can be moved into BluetoothHfpManagerBase.h.

@@ +38,5 @@
> +
> +  // CDMA-specific functions
> +  void AnswerWaitingCall();
> +  void IgnoreWaitingCall();
> +  void ToggleCalls();

Since we haven't implemented these CDMA-specific functions for bluedroid, these function aren't necessary for now.

Or, you might want to move these functions into BluetoothHfpManagerBase.h and keep them.
Revise based on Gina's suggestion.
Attachment #8378915 - Attachment is obsolete: true
Attachment #8378915 - Flags: review?(gyeh)
Attachment #8379534 - Flags: review?(gyeh)
Comment on attachment 8379534 [details] [diff] [review]
Patch 1/2 (v3): Add fallback BluetoothHfpManager for bluedroid

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

Looks great to me. :)

::: dom/bluetooth/bluedroid/hfp-fallback/BluetoothHfpManager.cpp
@@ +155,5 @@
> +}
> +
> +bool
> +BluetoothHfpManager::ConnectSco()
> +{

Suggest to add the thread checking.

@@ +160,5 @@
> +  /**
> +   * TODO:
> +   *   Implement ConnectSco() for applications that want to create SCO link
> +   *   without a HFP connection (e.g., VoIP).
> +   */

Please file a follow-up.

@@ +166,5 @@
> +}
> +
> +bool
> +BluetoothHfpManager::DisconnectSco()
> +{

Suggest to add the thread checking.
Attachment #8379534 - Flags: review?(gyeh) → review+
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #11)
> @@ +160,5 @@
> > +  /**
> > +   * TODO:
> > +   *   Implement ConnectSco() for applications that want to create SCO link
> > +   *   without a HFP connection (e.g., VoIP).
> > +   */
> 
> Please file a follow-up.
Filed bug 976403 to track.
Comment on attachment 8378917 [details] [diff] [review]
Patch 2/2 (v1): Move NS_DECL_ISUPPORTS and NS_DECL_NSIOBSERVER into BluetoothProfileManagerBase.h

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

The patch is ready to ship.
Attachment #8378917 - Flags: review?(gyeh) → review+
Revise based on Gina's comment. Also rebase onto bug 972732 patches.
Attachment #8379534 - Attachment is obsolete: true
Depends on: 972732
Summary: [Bluetooth] Add MOZ_B2G_RIL to bluedroid HFP manager → [Bluedroid] Add fallback HFP manager for MOZ_B2G_RIL disabled build
Attachment #8381210 - Flags: review?(gyeh) → review+
Why did this require a clobber?
Flags: needinfo?(btian)
(In reply to Gregory Szorc [:gps] from comment #19)
> Why did this require a clobber?
The file relocation results in build failure if the clobber build is not triggered. I've been backed out for the reason in bug 915533 comment 22. The sheriff suggested us touch the CLOBBER file to avoid such temporary build failure. Ever since that I always touch the CLOBBER file when files are moved.
Flags: needinfo?(btian)
Please don't blindly touch CLOBBER because something used to not work. Instead, ask a build peer or a sheriff before touching CLOBBER.

Please file bugs blocking bug 941904 every time a clobber is needed so that we can fix the clobber issues and eradicate their necessity.
ni? sheriff Ryan for clear explanation.

Ryan, in bug 915533 comment 22 we've been backed out due to file relocation and you've suggested Eric and I touch clobber to avoid such build failure. Since Gregory suggests another method in comment 21, I have some questions:
1) Which method should I follow? (or either way is fine?)
2) Should I ask you or other sheriff before each time I touch clobber?
3) Under which conditions exactly should I touch clobber?
Flags: needinfo?(ryanvm)
Not sure why gps brought me or my team into this. CLOBBER issues are a build system issue. I think his main point is that if you're hitting random bustage due to needs-clobber, you should at least be filing a build system bug for it rather than just quietly touching CLOBBER and nothing more. Ideally, you shouldn't *ever* have to touch CLOBBER and having to do so can be considered a bug :)
Flags: needinfo?(ryanvm)
You need to log in before you can comment on or make changes to this bug.