Closed Bug 1171807 Opened 10 years ago Closed 10 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
https://hg.mozilla.org/mozilla-central/rev/f891c4f77f67 https://hg.mozilla.org/mozilla-central/rev/718fb43568ad
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S9 (16Oct)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: