Closed Bug 1171807 Opened 9 years ago Closed 9 years ago

[WebTelephony] use enum for TelephonyCall.state and TelephonyCallGroup.state

Categories

(Firefox OS Graveyard :: RIL, defect)

Product:
Firefox OS Graveyard
Component:
RIL
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(tracking-b2g:backlog, firefox44 fixed)

Status:
RESOLVED FIXED
Milestone:
FxOS-S9 (16Oct)
Project Flags:
tracking-b2g backlog
Tracking Flags:
Tracking Status
firefox44 --- fixed

People

(Reporter: aknow, Assigned: bhsu)

Details

Attachments

(2 files, 13 obsolete files)

(V4) Part 1: Add enums for TelephonyCall::State and TelephonyCallGroup::State (WebIDL)
2.42 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
(V10) Part 2: Move to enums and deprecate TelephonyCall.mCallState and TelephonyCallGroup.mCallState (DOM)
40.86 KB, patch
bhsu
: review+
Details | Diff | Splinter Review 
Currently, we use DOMString for the field [1]. It's not easy for user to understand the possible values. Making it an enum like disconnectedReason will be much better.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/webidl/TelephonyCall.webidl?from=TelephonyCall.webidl#18
Ben,
Would you like to work on this bug?
Flags: needinfo?(bhsu)
Sure, I'll take a look at this, but I am current working on the emulator support for CDMA.
Flags: needinfo?(bhsu)
Assignee: nobody → bhsu
Comment on attachment 8639178 [details] [diff] [review]
Part 1: Add enums for TelephonyCall::State and TelephonyCallGroup::State (WebIDL)

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

looks good
Attachment #8639178 - Flags: review?(szchen) → review?(htsai)
[Tracking Requested - why for this release]:
tracking-b2g: --- → backlog
Comment on attachment 8639179 [details] [diff] [review]
Part 2: Move to enums and deprecate TelephonyCall.mCallState and TelephonyCallGroup.mCallState (DOM)

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

::: dom/telephony/Telephony.cpp
@@ +637,5 @@
>  
>  NS_IMETHODIMP
>  Telephony::EnumerateCallStateComplete()
>  {
>    // Set conference state.

Suggest to extract the following code into a new function, e.g., DetectConferenceState.
Maybe the function could be put in TelephonyCallGroup.

@@ +652,1 @@
>          break;

In the new function, we can have the following code here.

ChangeState(TelephonyCallGroupState::_empty);
return;

So, we don't need a weird 'Alerting' state.

@@ +660,5 @@
> +      case TelephonyCallState::Held:
> +        mGroup->ChangeState(TelephonyCallGroupState::Held);
> +        break;
> +      default:
> +        mGroup->ChangeState(TelephonyCallGroupState::_empty);

NS_NOTREACHED("Invalid state!");

::: dom/telephony/Telephony.h
@@ +191,5 @@
>                 uint16_t aNamePresentation = nsITelephonyService::CALL_PRESENTATION_ALLOWED);
>  
>    already_AddRefed<TelephonyCall>
>    CreateCall(TelephonyCallId* aId,
> +

Do not introduce the empty line.

@@ +195,5 @@
> +
> +             uint32_t aServiceId,
> +             uint32_t aCallIndex,
> +             TelephonyCallState aState,
> +

ditto

::: dom/telephony/TelephonyCall.cpp
@@ +64,5 @@
>  // static
>  already_AddRefed<TelephonyCall>
> +TelephonyCall::Create(Telephony* aTelephony,
> +                      TelephonyCallId* aId,
> +

no blank line

@@ +130,5 @@
>      }
> +  }
> +
> +  // Handle newly added calls
> +  else if (!mLive) {

I prefer the original format. ( You could append the comment after '}' )

@@ +142,5 @@
>  
>    if (aFireEvents) {
> +    // We need to save mState here, since the statechange handler might invoke
> +    // APIs that have the ability to change the mState here.
> +    TelephonyCallState prevState = mState;

After line 120, prevState == aState.
So, you could simply refer to aState without introduce a new variable.

@@ +150,5 @@
>      }
>  
> +    // Check whether mState remains the same after statechange handler.
> +    if (mState == prevState) {
> +      nsString stateString;

For the following switch case, you can use the string value of the enum
mozilla::dom::TelephonyCallStateValues::strings[mState]

@@ +322,2 @@
>      NS_WARNING(nsPrintfCString("Answer on non-incoming call is rejected!"
> +                               " (State: %u)", mState).get());

For easier debugging, let's print out the string value of the enum.

@@ +344,2 @@
>      NS_WARNING(nsPrintfCString("HangUp on previously disconnected call"
> +                               " is rejected! (State: %u)", mState).get());

For easier debugging, let's print out the string value of the enum.

@@ +368,2 @@
>      NS_WARNING(nsPrintfCString("Hold non-connected call is rejected!"
> +                               " (State: %u)", mState).get());

For easier debugging, let's print out the string value of the enum.

@@ +409,2 @@
>      NS_WARNING(nsPrintfCString("Resume non-held call is rejected!"
> +                               " (State: %u)", mState).get());

For easier debugging, let's print out the string value of the enum.

::: dom/telephony/TelephonyCallGroup.cpp
@@ +92,4 @@
>  
> +  // We need to save mState here, since the statechange handler might invoke
> +  // APIs that have the ability to change the mState here.
> +  TelephonyCallGroupState prevState = mState;

use aState as prevState

@@ +100,5 @@
> +
> +  // Check whether mState remains the same after statechange handler and whether
> +  // the conference call exists.
> +  if (mState != TelephonyCallGroupState::_empty && mState == prevState) {
> +    nsString stateString;

Use the string value of the enum to replace the following switch block.

@@ +138,5 @@
> +      default:
> +        NS_NOTREACHED("Unknown state!");
> +    }
> +
> +    // TODO MOZ_ASSERT(call->CallState() == aCallState);

What's the final version of this line?
Attachment #8639179 - Flags: review?(szchen) → review-
Comment on attachment 8639178 [details] [diff] [review]
Part 1: Add enums for TelephonyCall::State and TelephonyCallGroup::State (WebIDL)

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

::: dom/webidl/TelephonyCallGroup.webidl
@@ +35,5 @@
>    attribute EventHandler onerror;
>  };
> +
> +enum TelephonyCallGroupState {
> +  "",

I understand this is the current API behaviour, but the meaning of "" is really implicit. I'd like to take this time to correct this. How about we do
|readonly attribute TelephonyCallGroupState? state;| that this attribute is non-null when calls length isn't zero ?
Attachment #8639178 - Flags: review?(htsai)
Attachment #8639178 - Attachment is obsolete: true
Attached patch Part 3: Update related testcases (obsolete) — — Splinter Review 

        Attachment #8644811 -
        Flags: review?(szchen)

    
    

    
  
Comment on attachment 8644807 [details] [diff] [review]
Part 1: Add enums for TelephonyCall::State and TelephonyCallGroup::State (WebIDL) (V2)

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

Looks good to me!

This somehow changes webAPI behaviour, please make sure it won't break existing features before landing, thanks!

        Attachment #8644807 -
        Flags: review?(htsai) → review+

    
    

    
  
Comment on attachment 8644809 [details] [diff] [review]
Part 2: Move to enums and deprecate TelephonyCall.mCallState and TelephonyCallGroup.mCallState (DOM) (V2)

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

::: dom/telephony/TelephonyCall.cpp
@@ +38,5 @@
> +#undef TELEPHONY_CALL_STATE
> +#endif
> +
> +#define TELEPHONY_CALL_STATE(_state) \
> +  (TelephonyCallStateValues::strings[int32_t(_state)])

use static cast
static_cast<int>

@@ +162,4 @@
>    }
> +
> +  // Check whether |mState| remains the same after the statechange handler.
> +  if (mState != prevState) {

I don't understand this line and line 157.

::: dom/telephony/TelephonyCallGroup.cpp
@@ +14,5 @@
>  
> +#include "nsPrintfCString.h"
> +
> +#ifdef TELEPHONY_CALL_STATE
> +#undef TELEPHONY_CALL_STATE

TELEPHONY_CALLGROUP_STATE for above two.

@@ +18,5 @@
> +#undef TELEPHONY_CALL_STATE
> +#endif
> +
> +#define TELEPHONY_CALLGROUP_STATE(_state) \
> +  (TelephonyCallGroupStateValues::strings[int32_t(_state)])

same, static cast

@@ +102,1 @@
>    }

why not use assert in the begining of the function

assert(mCalls.Length() != 1);
if (mCalls.Length() == 0) {
  ...
  return;
}

@@ +104,5 @@
> +  TelephonyCallState state = mCalls[0]->State();
> +  for (uint32_t i = 1; i < mCalls.Length(); i++) {
> +    if (mCalls[i]->State() != state) {
> +      NS_WARNING("Various call states are found in a call group!");
> +      ChangeStateInternal(false);

Here, you'll set the state to null and that means there is no conference group now.  However, it indeed has a group although the calls have different states.  So I don't think the handling here is reasonable enough.

@@ +170,5 @@
> +
> +  // Check whether |mState| remains the same after the statechange handler.
> +  // Besides, If there is no conference call at all, then we dont't have to
> +  // dispatch the state evnet.
> +  if (isExisting && !mState.IsNull() && mState.Value() == prevState) {

same problem here for line 160 and this line. why? how could that happen?

@@ +182,5 @@
> +  }
> +
> +  // Notify each call within to dispatch call state change event
> +  for (uint32_t index = 0; index < mCalls.Length(); index++) {
> +    res = mCalls[index]->NotifyStateChange();

A failed result previously stored in |res| may be overwritten.

::: dom/telephony/TelephonyCallGroup.h
@@ +101,2 @@
>    void
> +  ChangeState();

Suggest to rename the function name to make it conform with the comment.

@@ +111,5 @@
>    explicit TelephonyCallGroup(nsPIDOMWindow* aOwner);
>    ~TelephonyCallGroup();
>  
> +  void
> +  ChangeStateInternal(bool aIsExisting,

|aIsExisting| is not so easy to understand
rename or add a comment

@@ +112,5 @@
>    ~TelephonyCallGroup();
>  
> +  void
> +  ChangeStateInternal(bool aIsExisting,
> +                      TelephonyCallGroupState aState = TelephonyCallGroupState::Connected);

I don't like to provide a default value here. It's not obvious.

        Attachment #8644809 -
        Flags: review?(szchen) → review-

    
    

    
  
Comment on attachment 8644811 [details] [diff] [review]
Part 3: Update related testcases

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

I believed this part of patch is correct. Otherwise, you must get the failed result when doing marionette test.

        Attachment #8644811 -
        Flags: review?(szchen) → review+

    
    

    
  
Hi Hsin-Yi and Aknow,

Thanks for your review and suggestions. When revising those patches, I found out that making |TelephonyCallGroup.state| nullable may not be a good idea. One hand, I think there is still some ambiguity resides in a null state, since null doesn't have an actual meaning. On the other hand, a null state makes the implementation of the DOM object become more nasty. Thus, I suggest we keep the origin behaivor here and try to find a better replacement in an other bug.
Flags: needinfo?(htsai)

    
    

    
  
(In reply to Ben Hsu [:bhsu] from comment #15)
> Hi Hsin-Yi and Aknow,
> 
> Thanks for your review and suggestions. When revising those patches, I found
> out that making |TelephonyCallGroup.state| nullable may not be a good idea.
> One hand, I think there is still some ambiguity resides in a null state,
> since null doesn't have an actual meaning. On the other hand, a null state
> makes the implementation of the DOM object become more nasty. Thus, I
> suggest we keep the origin behaivor here and try to find a better
> replacement in an other bug.

I am convinced that null is still implicit and we probably need to rethink about the "TelephonyCallGroup" API to have a better shape. So, Ben's suggestion sounds good enough to me, thanks!
Flags: needinfo?(htsai)

    
    

    
  


  

        Attachment #8644807 -
        Attachment is obsolete: true

        Attachment #8650331 -
        Flags: review?(htsai)

    
    

    
  
Comment on attachment 8644811 [details] [diff] [review]
Part 3: Update related testcases

Since the behavior of these WebAPIs remain unchanged, this patch is no longer needed.

        Attachment #8644811 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8650331 [details] [diff] [review]
Part 1: Add enums for TelephonyCall::State and TelephonyCallGroup::State (WebIDL) (V3)

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

::: dom/webidl/TelephonyCallGroup.webidl
@@ +25,5 @@
>  
>    [NewObject, Throws]
>    Promise<void> resume();
>  
> +  readonly attribute TelephonyCallGroupState? state;

Ben, if I understood your latest suggestion correctly, you meant to have a non-nullable attribute here, no?

    
    

    
  
Comment on attachment 8650331 [details] [diff] [review]
Part 1: Add enums for TelephonyCall::State and TelephonyCallGroup::State (WebIDL) (V3)

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

see comment 20

        Attachment #8650331 -
        Flags: review?(htsai)

    
    

    
  


  
Hi Hsin-Yi

Sorry for the last patch, it's a rebasing mistake. Do you mind reviewing this part again?

        Attachment #8650331 -
        Attachment is obsolete: true

        Attachment #8655950 -
        Flags: review?(htsai)

    
    

    
  
Comment on attachment 8655950 [details] [diff] [review]
(V4) Part 1: Add enums for TelephonyCall::State and TelephonyCallGroup::State (WebIDL)

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

Looks good to me, thanks.

        Attachment #8655950 -
        Flags: review?(htsai) → review+

    
    

    
  
Comment on attachment 8655951 [details] [diff] [review]
(V4) Part 2: Move to enums and deprecate TelephonyCall.mCallState and TelephonyCallGroup.mCallState (DOM)

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

Please see my comments as followed.

Thanks!

::: dom/telephony/TelephonyCall.cpp
@@ +44,5 @@
>  using namespace mozilla::dom;
>  using namespace mozilla::dom::telephony;
>  using mozilla::ErrorResult;
>  
> +// Static

nit: // static

::: dom/telephony/TelephonyCallGroup.cpp
@@ +18,5 @@
> +#undef TELEPHONY_CALLGROUP_STATE
> +#endif
> +
> +#define TELEPHONY_CALLGROUP_STATE(_state) \
> +  (TelephonyCallGroupStateValues::strings[static_cast<int32_t>(_state)])

TelephonyCallGroupStateValues::strings[static_cast<int32_t>(_state)].value  ??

@@ +94,3 @@
>  {
> +  MOZ_ASSERT(mCalls.Length() != 1);
> +  if (mCalls.Length() == 0 ) {

nit: remove extra <sp> right after 0.

@@ +100,5 @@
>  
> +  TelephonyCallState state = mCalls[0]->State();
> +  for (uint32_t i = 1; i < mCalls.Length(); i++) {
> +    if (mCalls[i]->State() != state) {
> +      MOZ_ASSERT(false, "Various call states are found in a call group!");

The following 2 lines of ChangeStateInternal() and the return statement are dead code if you set MOS_ASSERT(false) here.
Please remove the following 2 lines if ASSERTION is expected as the original implementation in line#125.

@@ +105,5 @@
> +      ChangeStateInternal(TelephonyCallGroupState::_empty);
> +      return;
> +    }
> +  }
> +

TelephonyCallGroupState groupState = TelephonyCallGroupState::_empty;

@@ +108,5 @@
> +  }
> +
> +  switch (state) {
> +    case TelephonyCallState::Connected:
> +      ChangeStateInternal(TelephonyCallGroupState::Connected);

groupState = TelephonyCallGroupState::Connected;

@@ +113,3 @@
>        break;
> +    case TelephonyCallState::Held:
> +      ChangeStateInternal(TelephonyCallGroupState::Held);

groupState = TelephonyCallGroupState::Held;

@@ +116,5 @@
>        break;
>      default:
> +      NS_NOTREACHED(nsPrintfCString("Invavild call state for a call group(%s)!",
> +                                    TELEPHONY_CALL_STATE(state)).get());
> +      ChangeStateInternal(TelephonyCallGroupState::_empty);

remove : ChangeStateInternal(TelephonyCallGroupState::_empty);

@@ +121,1 @@
>    }

ChangeStateInternal(groupState);

@@ +143,5 @@
>  
>  nsresult
> +TelephonyCallGroup::NotifyStateChange()
> +{
> +  nsresult res = NS_OK;

remove this line and

@@ +149,5 @@
> +  // Since |mState| can be changed after statechange handler called back here,
> +  // we must save current state. Maybe we should figure out something smarter.
> +  TelephonyCallGroupState prevState = mState;
> +
> +  res = DispatchCallEvent(NS_LITERAL_STRING("statechange"), nullptr);

nsresult res = DispatchCallEvent(...);

@@ +158,5 @@
> +  // Check whether |mState| remains the same after the statechange handler.
> +  // Besides, If there is no conference call at all, then we dont't have to
> +  // dispatch the state evnet.
> +  if (mState == prevState) {
> +    nsString stateString;

We prefer to use nsAutoString inside a function for short strings instead of nsString which always allocates memory from the heap.

"
[Local variables]
Local variables within a function are usually stored on the stack. The nsAutoString/nsAutoCString classes are derivatives of the nsString/nsCString classes. They own a 64-character buffer allocated in the same storage space as the string itself. If the nsAutoString is allocated on the stack, then it has at its disposal a 64-character stack buffer. This allows the implementation to avoid allocating extra memory when dealing with small strings.
"

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings#Local_variables

@@ +160,5 @@
> +  // dispatch the state evnet.
> +  if (mState == prevState) {
> +    nsString stateString;
> +    stateString.AssignASCII(TELEPHONY_CALLGROUP_STATE(mState).value,
> +                            TELEPHONY_CALLGROUP_STATE(mState).length);

stateString.AssignASCII(TELEPHONY_CALLGROUP_STATE(mState));

To be more efficient, I think we could rewrite this as followed without the local variable of stateString.
res = DispatchCallEvent(NS_ConvertASCIItoUTF16(TELEPHONY_CALLGROUP_STATE(mState)), nullptr);

        Attachment #8655951 -
        Flags: review?(btseng)

    
    

    
  
Comment on attachment 8658008 [details] [diff] [review]
(V5) Part 2: Move to enums and deprecate TelephonyCall.mCallState and TelephonyCallGroup.mCallState (DOM)

It seems that you upload the same patch of v4 instead of v5.

Do you need a RedBull? :)

        Attachment #8658008 -
        Flags: review?(btseng) → review-

    
    

    
  


  
It's embarrassing. Here is the update-to-date patch, and I think I'll get home earlier today :P

        Attachment #8658536 -
        Attachment is obsolete: true

        Attachment #8658568 -
        Flags: review?(btseng)

    
    

    
  
Comment on attachment 8658568 [details] [diff] [review]
(V7) Part 2: Move to enums and deprecate TelephonyCall.mCallState and TelephonyCallGroup.mCallState (DOM)

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

Looks good to me.

Thank you!

        Attachment #8658568 -
        Flags: review?(btseng) → review+

    
    

    
  


  
A rebased version.

        Attachment #8668860 -
        Attachment is obsolete: true

        Attachment #8669497 -
        Flags: review+

    
    

    
  
part 2 failed to apply:

adding 1171807 to series file
renamed 1171807 -> 1171807-2.patch
applying 1171807-2.patch
patching file dom/telephony/Telephony.cpp
Hunk #6 FAILED at 604
1 out of 6 hunks FAILED -- saving rejects to file dom/telephony/Telephony.cpp.rej
patching file dom/telephony/TelephonyCall.cpp
Hunk #5 FAILED at 324
Hunk #6 FAILED at 389
2 out of 6 hunks FAILED -- saving rejects to file dom/telephony/TelephonyCall.cpp.rej
patching file dom/telephony/TelephonyCall.h
Hunk #5 FAILED at 187
1 out of 5 hunks FAILED -- saving rejects to file dom/telephony/TelephonyCall.h.rej
patching file dom/telephony/TelephonyCallGroup.cpp
Hunk #7 FAILED at 392
Hunk #8 FAILED at 415
2 out of 8 hunks FAILED -- saving rejects to file dom/telephony/TelephonyCallGroup.cpp.rej
patching file dom/telephony/TelephonyCallGroup.h
Hunk #1 succeeded at 5 with fuzz 2 (offset 0 lines).
Hunk #3 FAILED at 90
1 out of 3 hunks FAILED -- saving rejects to file dom/telephony/TelephonyCallGroup.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh 1171807-2.patch

could you take a look, thanks!
Flags: needinfo?(bhsu)

    
    

    
  
Sure, I'll handle this, and sorry for your inconvenience.
Flags: needinfo?(bhsu)

    
  
Keywords: checkin-needed

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: