Closed Bug 1053804 Opened 6 years ago Closed 6 years ago

[Bluetooth] Convert HFP callbacks to backend-neutral interface

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(6 files, 5 obsolete files)

17.81 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
10.58 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
4.42 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
7.50 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
16.30 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
6.09 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
No description provided.
Attachment #8473572 - Flags: review?(shuang)
Attachment #8473572 - Flags: feedback?(btian)
Attachment #8473573 - Flags: review?(shuang)
Attachment #8473573 - Flags: feedback?(btian)
Attachment #8473574 - Flags: review?(shuang)
Attachment #8473574 - Flags: feedback?(btian)
Attachment #8473576 - Flags: review?(shuang)
Attachment #8473576 - Flags: feedback?(btian)
Comment on attachment 8473572 [details] [diff] [review]
[01] Bug 1053804: Add Bluetooth Handsfree notifications

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

f=me with nit addressed.

::: dom/bluetooth/bluedroid/BluetoothInterface.h
@@ +136,5 @@
> +  CopsNotification()
> +  { }
> +
> +  virtual void
> +  AtClccNotification()

nit: Rename to |ClccNotification| for consistency with |CopsNotification|, |CindNotification|, and |CnumNotification|.
Attachment #8473572 - Flags: feedback?(btian) → feedback+
Comment on attachment 8473573 [details] [diff] [review]
[02] Bug 1053804: Implement Bluetooth Handsfree notifications

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

f=me with comment addressed.

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +1729,5 @@
> +  mConnectionState = aState;
> +
> +  if (aState == HFP_CONNECTION_STATE_SLC_CONNECTED) {
> +    mDeviceAddress = aBdAddress;
> +    BT_HF_DISPATCH_MAIN(MainThreadTaskCmd::NOTIFY_CONN_STATE_CHANGED,

This macro was created to run main-thread only code. Please open a followup bug to remove this macro since all callbacks become running on main thread.
Attachment #8473573 - Flags: feedback?(btian) → feedback+
Comment on attachment 8473574 [details] [diff] [review]
[03] Bug 1053804: Use Bluetooth Handsfree notifications

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

LGTM
Attachment #8473574 - Flags: feedback?(btian) → feedback+
(In reply to Ben Tian [:btian] from comment #7)
> > +    BT_HF_DISPATCH_MAIN(MainThreadTaskCmd::NOTIFY_CONN_STATE_CHANGED,
> 
> This macro was created to run main-thread only code. Please open a followup
> bug to remove this macro since all callbacks become running on main thread.

Ignore this comment since patch [04] does it.
Comment on attachment 8473572 [details] [diff] [review]
[01] Bug 1053804: Add Bluetooth Handsfree notifications

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

::: dom/bluetooth/BluetoothCommon.h
@@ +305,5 @@
>    HFP_NETWORK_STATE_AVAILABLE
>  };
>  
> +enum BluetoothHandsfreeNoiseReductionState {
> +  HFP_NOISE_REDUCTION_STOPPED,

nit: Noise Reduction and Echo Cancellation, so I prefer to keep Nrec wording. Otherwise EC is missing. What do you think?
Attachment #8473572 - Flags: review?(shuang) → review+
Comment on attachment 8473575 [details] [diff] [review]
[04] Bug 1053804: Integrate runnables into Handsfree notifications

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

f=me with nit addressed.

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +1854,3 @@
>      nsAutoCString newMsg("ATD");
>      newMsg += StringHead(message, message.Length() - 1);
> +                        NotifyDialer(NS_ConvertUTF8toUTF16(newMsg));

nit: indent too long.
Attachment #8473575 - Flags: feedback?(btian) → feedback+
Comment on attachment 8473574 [details] [diff] [review]
[03] Bug 1053804: Use Bluetooth Handsfree notifications

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

::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
@@ +2342,5 @@
> +    .cops_cmd_cb = BluetoothHandsfreeCallback::Cops,
> +    .clcc_cmd_cb = BluetoothHandsfreeCallback::AtClcc,
> +    .unknown_at_cmd_cb = BluetoothHandsfreeCallback::UnknownAt,
> +    .key_pressed_cmd_cb = BluetoothHandsfreeCallback::KeyPressed
> +  };

CAF repo defines extra |codec_negotiated_callback| here again. I guess we need to deal with it. :(
Attachment #8473574 - Flags: review?(shuang)
Comment on attachment 8473576 [details] [diff] [review]
[05] Bug 1053804: Cleanup Bluetooth Handsfree manager

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

f=me with comment addressed.

::: dom/bluetooth/bluedroid/hfp/BluetoothHfpManager.cpp
@@ +62,1 @@
>  enum MainThreadTaskCmd {

Remove |MainThreaTaskCmd| and |BluetoothHfpManager::MainThreadTask| since only macro |BT_HF_PROCESS_CB| uses them.

@@ +1578,5 @@
>    MOZ_ASSERT(NS_IsMainThread());
>  
>    nsCString atString(aAtString);
>  
>    BT_LOGR("[%s]", atString.get());

Print |NS_ConvertUTF8toUTF16(aAtString).get()| directly and remove redundant |atString|.

@@ +1580,5 @@
>    nsCString atString(aAtString);
>  
>    BT_LOGR("[%s]", atString.get());
>  
>    NS_ENSURE_TRUE_VOID(sBluetoothHfpInterface);

Remove this check since |SendResponse| checks |sBluetoothHfpInterface| inside.
Attachment #8473576 - Flags: feedback?(btian) → feedback+
(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #10)
> Comment on attachment 8473572 [details] [diff] [review]
> [01] Bug 1053804: Add Bluetooth Handsfree notifications
> 
> Review of attachment 8473572 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/BluetoothCommon.h
> @@ +305,5 @@
> >    HFP_NETWORK_STATE_AVAILABLE
> >  };
> >  
> > +enum BluetoothHandsfreeNoiseReductionState {
> > +  HFP_NOISE_REDUCTION_STOPPED,
> 
> nit: Noise Reduction and Echo Cancellation, so I prefer to keep Nrec
> wording. Otherwise EC is missing. What do you think?

Shall we call this 'NREC' in all capital letters then?
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #14)
> (In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #10)
> > Comment on attachment 8473572 [details] [diff] [review]
> > [01] Bug 1053804: Add Bluetooth Handsfree notifications
> > 
> > Review of attachment 8473572 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/bluetooth/BluetoothCommon.h
> > @@ +305,5 @@
> > >    HFP_NETWORK_STATE_AVAILABLE
> > >  };
> > >  
> > > +enum BluetoothHandsfreeNoiseReductionState {
> > > +  HFP_NOISE_REDUCTION_STOPPED,
> > 
> > nit: Noise Reduction and Echo Cancellation, so I prefer to keep Nrec
> > wording. Otherwise EC is missing. What do you think?
> 
> Shall we call this 'NREC' in all capital letters then?
That will be fine.
Comment on attachment 8473576 [details] [diff] [review]
[05] Bug 1053804: Cleanup Bluetooth Handsfree manager

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

Let's fix the problem that Ben mentioned :)
Attachment #8473576 - Flags: review?(shuang)
Depends on: 1054218
(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #12)
> Comment on attachment 8473574 [details] [diff] [review]
> [03] Bug 1053804: Use Bluetooth Handsfree notifications
> 
> Review of attachment 8473574 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
> @@ +2342,5 @@
> > +    .cops_cmd_cb = BluetoothHandsfreeCallback::Cops,
> > +    .clcc_cmd_cb = BluetoothHandsfreeCallback::AtClcc,
> > +    .unknown_at_cmd_cb = BluetoothHandsfreeCallback::UnknownAt,
> > +    .key_pressed_cmd_cb = BluetoothHandsfreeCallback::KeyPressed
> > +  };
> 
> CAF repo defines extra |codec_negotiated_callback| here again. I guess we
> need to deal with it. :(

I remembered that if we miss the last designated initializer, we will have compiling error, but if we don't use designated initializers, we won't see compiling error.
(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #17)
> (In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #12)
> > Comment on attachment 8473574 [details] [diff] [review]
> > [03] Bug 1053804: Use Bluetooth Handsfree notifications
> > 
> > Review of attachment 8473574 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/bluetooth/bluedroid/BluetoothInterface.cpp
> > @@ +2342,5 @@
> > > +    .cops_cmd_cb = BluetoothHandsfreeCallback::Cops,
> > > +    .clcc_cmd_cb = BluetoothHandsfreeCallback::AtClcc,
> > > +    .unknown_at_cmd_cb = BluetoothHandsfreeCallback::UnknownAt,
> > > +    .key_pressed_cmd_cb = BluetoothHandsfreeCallback::KeyPressed
> > > +  };
> > 
> > CAF repo defines extra |codec_negotiated_callback| here again. I guess we
> > need to deal with it. :(
> 
> I remembered that if we miss the last designated initializer, we will have
> compiling error, but if we don't use designated initializers, we won't see
> compiling error.

I was able to build these patches for flame-kk without changes. \o/ Can we keep them?
Changes since v1:

  - fixed |AtClccNotification|
  - use NREC
Attachment #8473572 - Attachment is obsolete: true
Attachment #8475840 - Flags: review+
Changes since v1:

  - rebased onto [01] (v2)
Attachment #8473573 - Attachment is obsolete: true
Attachment #8475841 - Flags: review+
Comment on attachment 8473574 [details] [diff] [review]
[03] Bug 1053804: Use Bluetooth Handsfree notifications

I'm reopening this for review because I found that no special handling for flame-kk is required.
Attachment #8473574 - Flags: review?(shuang)
Changes since v1:

  - rebased onto [01] (v2)
Attachment #8473574 - Attachment is obsolete: true
Attachment #8473574 - Flags: review?(shuang)
Attachment #8475844 - Flags: review?(shuang)
Changes since v2:

  - fixed indention
Attachment #8473575 - Attachment is obsolete: true
Attachment #8475846 - Flags: review+
Changes since v1:

  - removed |atString|
  - removed duplicated test for |sBluetoothHfpInterface|
Attachment #8473576 - Attachment is obsolete: true
Attachment #8475847 - Flags: review?(shuang)
I cannot completely remove the functionality of MainThreadTask, so I moved the last used bits into a separate class and deleted the rest.
Attachment #8475851 - Flags: review?(shuang)
You need to log in before you can comment on or make changes to this bug.