Closed Bug 925638 Opened 6 years ago Closed 6 years ago

[Bluetooth][HFP][CDMA] (Gecko) Inform gecko bluetooth of call manipulations for CLCC

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

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

References

Details

Attachments

(3 files, 6 obsolete files)

The bug tracks implementation of gecko bluetooth 1) CDMA-handling functions and 2) new webidl functions to be informed by dialer in CDMA.
Attachment #815742 - Flags: review?(echou)
Attachment #815743 - Flags: review?(echou)
Attachment #815744 - Flags: review?(echou)
Comment on attachment 815744 [details] [diff] [review]
Patch 3/3 (v1): webidl file change

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

r=me with nits addressed. Redirect this bug to mrbkap for superreview.

::: dom/webidl/BluetoothAdapter.webidl
@@ +128,5 @@
>    DOMRequest disconnectSco();
>    [Creator, Throws]
>    DOMRequest isScoConnected();
>  
> +  // HFP methods for CDMA

Please add more comments to explain why we need these additional methods to handle CDMA network.

@@ +130,5 @@
>    DOMRequest isScoConnected();
>  
> +  // HFP methods for CDMA
> +  [Creator, Throws]
> +  DOMRequest onAnswerWaitingCall();

I can understand the reason why you want to name these functions like callbacks which initialized with 'on', however it's not very common to do so. I would like to propose using 

  answerWaitingCall / ignoreWaitingCall / toggleCalls

directly. That would be clear enough to me.

I think I'll let superreviewer to decide which one is better, so let's keep it for a while.
Attachment #815744 - Flags: superreview?(mrbkap)
Attachment #815744 - Flags: review?(echou)
Attachment #815744 - Flags: review+
Comment on attachment 815742 [details] [diff] [review]
Patch 1/3 (v1): hfp cdma-handling functions

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

Hi Ben, please check my comments below.

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +303,5 @@
>           (aType == CINDType::CALLSETUP);
>  }
>  
> +// FIXME: Query phone type from RIL after RIL implements new API (bug 912019)
> +static bool

I was thinking that maybe we could merge IsGsmPhone() and IsCdmaPhone() into one getter function such as GetPhoneType() or GetTelephonyNetwork() which returns PhoneType.

@@ +338,5 @@
> +Call::Reset()
> +{
> +  mState = nsITelephonyProvider::CALL_STATE_DISCONNECTED;
> +  mDirection = false;
> +  mNumber = EmptyString();

Please use mNumber.Truncate() to avoid creating a new EmptyString().

@@ +342,5 @@
> +  mNumber = EmptyString();
> +  mType = TOA_UNKNOWN;
> +}
> +
> +bool Call::IsActive()

Please wrap.

@@ +594,5 @@
>  
> +  nsString type;
> +  voiceInfo->GetType(type);
> +  mPhoneType = (IsGsmPhone(type)) ? PhoneType::GSM :
> +                 (IsCdmaPhone(type)) ? PhoneType::CDMA : PhoneType::NONE;

Please add a new line here. Besides,

@@ +1266,5 @@
>      for (uint32_t i = 1; i < callNumbers; i++) {
> +      rv &= SendCLCC(mCurrentCallArray[i], i);
> +
> +      if (mPhoneType == PhoneType::CDMA) {
> +        if (i == 1 && mCdmaSecondCall.mNumber.Length()) {

OK, this part might be a little hard for readers to understand what's going on here. Since there would be only 1 call object while CDMA network is present, we can handle mCdmaSecondCall after the for-loop, like:

uint32_t callNumbers = mCurrentCallArray.Length();
uint32_t i;

for (i = 1; i < callNumbers; i++) {
  rv &= SendCLCC(mCurrentCallArray[i], i);
}

if (!mCdmaSecondCall.mNumber.IsEmpty()) {
  MOZ_ASSERT(mPhoneType == PhoneType::CDMA);
  MOZ_ASSERT(i == 2);

  rv &= SendCLCC(mCdmaSecondCall, 2);
}

return rv;

@@ +1491,5 @@
>  }
>  
>  void
> +BluetoothHfpManager::UpdateSecondNumber(const nsAString& aNumber)
> +{

Please MOZ_ASSERT(mPhoneType == PhoneType::CDMA).

@@ +1496,5 @@
> +  if (mPhoneType == PhoneType::CDMA) {
> +    // Always regard second call as incoming call since v1.2 RIL
> +    // doesn't support outgoing second call in CDMA.
> +    //
> +    // FIXME: Revise after RIL supports outgoing second call in CDMA.

Do we have a bug for this?

::: dom/bluetooth/BluetoothHfpManager.h
@@ +50,5 @@
>    NETWORK_NOT_ALLOWED = 32
>  };
>  
> +class Call {
> +  public:

We don't usually insert whitespaces in front of keyword such as 'public' and 'private'.

@@ +161,5 @@
>    bool mCMEE;
>    bool mCMER;
>    bool mFirstCKPD;
>    int mNetworkSelectionMode;
> +  int mPhoneType;

You may want to move "enum PhoneType" to this .h so that you'll be able to declare this variable as a PhoneType variable.

@@ +170,5 @@
>    nsString mDeviceAddress;
>    nsString mMsisdn;
>    nsString mOperatorName;
> +  // CDMA-specific variable
> +  Call mCdmaSecondCall;

Please move this kind of 'xxx-specific' variables to the bottom, so that we won't be confused with 'only one variable is CDMA-specific' or 'all variables under this description are CDMA-specific'.
Attachment #815742 - Flags: review?(echou) → review-
Comment on attachment 815743 [details] [diff] [review]
Patch 2/3 (v1): implement new webidl functions

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

Looks good. r=me with nits addressed.

::: dom/bluetooth/BluetoothHfpManager.cpp
@@ +1510,5 @@
>  }
>  
>  void
> +BluetoothHfpManager::OnAnswerWaitingCall()
> +{

Please add a thread-check assertion here. Here and below.

@@ +1519,5 @@
> +
> +    sCINDItems[CINDType::CALLHELD].value = CallHeldState::ONHOLD_ACTIVE;
> +    SendCommand("+CIEV: ", CINDType::CALLHELD);
> +  } else {
> +    BT_WARNING("Shouldn't get here under non-CDMA network");

We could use MOZ_ASSERT(mPhoneType == PhoneType::CDMA) instead of warnings. Also, please put the assertion at the beginning of this function in order to return earlier.

@@ +1530,5 @@
> +  if (mPhoneType == PhoneType::CDMA) {
> +    mCdmaSecondCall.Reset();
> +    UpdateCIND(CINDType::CALLSETUP, CallSetupState::NO_CALLSETUP, true);
> +  } else {
> +    BT_WARNING("Shouldn't get here under non-CDMA network");

Ditto.

@@ +1543,5 @@
> +    mCdmaSecondCall.mState = (mCdmaSecondCall.IsActive()) ?
> +                               nsITelephonyProvider::CALL_STATE_HELD :
> +                               nsITelephonyProvider::CALL_STATE_CONNECTED;
> +  } else {
> +    BT_WARNING("Shouldn't get here under non-CDMA network");

Ditto.
Attachment #815743 - Flags: review?(echou) → review+
> @@ +594,5 @@
> >  
> > +  nsString type;
> > +  voiceInfo->GetType(type);
> > +  mPhoneType = (IsGsmPhone(type)) ? PhoneType::GSM :
> > +                 (IsCdmaPhone(type)) ? PhoneType::CDMA : PhoneType::NONE;
> 
> Please add a new line here. Besides,

Oops, it's a typo. I just wanted to say that we should use a getter function replacing IsGsmPhone() and IsCdmaPhone, which I have already mentioned in another comment.
Fix nits.
Attachment #815742 - Attachment is obsolete: true
Attachment #816518 - Flags: review?(echou)
Fix nits.
Attachment #815743 - Attachment is obsolete: true
Comment on attachment 816518 [details] [diff] [review]
Patch 1/3 (v2): hfp cdma-handling functions

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

r=me with nit addressed. Thanks.

::: dom/bluetooth/BluetoothHfpManager.h
@@ +57,5 @@
> +};
> +
> +class Call {
> +public:
> +    Call();

nit: two-space indentation, please.
Attachment #816518 - Flags: review?(echou) → review+
Attachment #816518 - Attachment is obsolete: true
Comment on attachment 815744 [details] [diff] [review]
Patch 3/3 (v1): webidl file change

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

sr=me with the renaming that was already hinted at here.

::: dom/webidl/BluetoothAdapter.webidl
@@ +130,5 @@
>    DOMRequest isScoConnected();
>  
> +  // HFP methods for CDMA
> +  [Creator, Throws]
> +  DOMRequest onAnswerWaitingCall();

Yeah, the on* names are weird for me as they usually indicate an event handler instead of a function call. answerWaitingCall() etc. make much more sense to me.
Attachment #815744 - Flags: superreview?(mrbkap) → superreview+
Rename webidl functions based on reviewers' comment.
Attachment #816521 - Attachment is obsolete: true
Rename webidl functions and add comments.
Attachment #815744 - Attachment is obsolete: true
Keywords: checkin-needed
Depends on: 928135
You need to log in before you can comment on or make changes to this bug.