1131047 - Consider to obsolete TelephonyCallGroup.onholding and TelephonyCallGroup.onresuming

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Ben Hsu [:HoPang]]

Similar to Bug 1095366, since the return types of holding and resuming a TelephonyCallGroup are changed to promises which can show whether the requests are accepted, both the two events (onholding and onresuming) are no longer needed. It's time to obsolete them.

Comment 1

[a2882525224]

Attachment: Part 1:Obsolete some events (WebIDL)

Comment 2

[a2882525224]

Attachment: Part 2:Obsolete some events (DOM)

Comment 3

[a2882525224]

Attachment: Part 3:Update the related testcases

Comment 4

[a2882525224]

Attachment: Bug 1131047 - Part 1:Obsolete some events (WebIDL). r=hsinyi

Comment 5

[a2882525224]

Attachment: Bug 1131047 - Part 2:Obsolete some events (DOM) (V2). r=aknow

Comment 6

[a2882525224]

Attachment: Part 1:Obsolete some events (WebIDL). r=hsinyi

Comment 7

[a2882525224]

Attachment: Part 2:Obsolete some events (DOM) (V2)

Comment 8

[a2882525224]

Attachment: Part 3:Update the related testcases. r=aknow

Comment 9

[a2882525224]

Hi Hsinyi and Aknow,
I finally finish solving this issue. I have tested the patches locally with marionette-webapi tests, and them passed perfectly. Do you mind reviewing these patches?

Comment 10

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8593450 [details] [diff] [review]
Part 2:Obsolete some events (DOM) (V2)

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

Looks good.
There are some parts we could make it better. So, I'd like to have a second round review to see the final version.
Thanks.

::: dom/telephony/TelephonyCall.cpp
@@ +84,5 @@
>  
>  void
>  TelephonyCall::ChangeStateInternal(uint16_t aCallState, bool aFireEvents)
>  {
>    nsRefPtr<TelephonyCall> kungFuDeathGrip(this);

Maybe we could add

if (mCallState == aCallState) {
  return;
}

to skip the unnecessary action, just like what we did in TelephonyCallGroup.

@@ +88,5 @@
>    nsRefPtr<TelephonyCall> kungFuDeathGrip(this);
> +  // Since the return types are changed to promises, both of holding and
> +  // resuming are only used internally without dispatching the event. If
> +  // aCallState isn't those states used internally, this variable will remain
> +  // true and event will be dispatched.

Contains too much unrelated information in the comment, for example, the promise part. Actually, the return type of this function is not promises. The wording here may cause misunderstanding.

I'll suggest only write down the meaning of this variable. Others could be easily read from code.
// Indicate whether the external state should be changed.

@@ +94,2 @@
>  
>    nsString stateString;

Remove this line. It's not used.

@@ +94,3 @@
>  
>    nsString stateString;
>    switch (aCallState) {

Add comment before switch
// Update the external state.

so that we have a comment that is parallel to what you added for "Update the internal state".

@@ +131,1 @@
>    mCallState = aCallState;

nits:  I'd prefer to move this part to the place that immediately after

if (mCallState == aCallState) {
  return;
}

because they are highly related.

::: dom/telephony/TelephonyCallGroup.cpp
@@ +86,5 @@
>  {
> +  // Since the return types are changed to promises, both of holding and
> +  // resuming are only used internally without dispatching the event.
> +  // If aCallState isn't those states used internally, this variable will remain
> +  // true and event will be dispatched.

The same, as what I wrote in TelephonyCall::ChangeStateInternal.

@@ +93,1 @@
>    if (mCallState == aCallState) {

Move this |if| block to the beginning. We can perform the check before declare |externalStateChanged|.

@@ +93,5 @@
>    if (mCallState == aCallState) {
>      return;
>    }
>  
>    nsString stateString;

Remove the line.

@@ +94,5 @@
>      return;
>    }
>  
>    nsString stateString;
>    switch (aCallState) {

Comment before |switch|.
// Update the external state.

@@ +122,3 @@
>    mCallState = aCallState;
>  
> +  if(externalStateChanged){

Add a space after 'if' and before '{'.

Comment 11

[a2882525224]

Hi Aknow,

Thanks for your reviewing. I will modify the TelephonyCall.cpp and TelephonyCallGroup.cpp according to your
advice.

Comment 12

[Hsin-Yi Tsai (she/her)[:hsinyi]]

Comment on attachment 8593449 [details] [diff] [review]
Part 1:Obsolete some events (WebIDL). r=hsinyi

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

Thanks for the patch!

Comment 13

[a2882525224]

Attachment: log_with_the_state_check.log

Comment 14

[a2882525224]

Attachment: log_without_the_state_check.log

Comment 15

[a2882525224]

Attachment: Part 2:Obsolete some events (DOM) (V3). r=aknow

Comment 16

[a2882525224]

Hi Aknow,

The new patch doesn’t add the state check to the beginning of |TelephonyCall::ChangeStateInternal(…)|, because the state check would fail many of marionette-webapi tests, where the most of them are failed by |timeout|. To identify the root cause, I did a little experiment. The other two attachments are the logs of running |test_conference_two_hangup_one.js| with and without the state check. By comparing the difference between the two logs, I think some event firings are spared by the state check. In “Log_without_state_check.log”, the calls received both “statechange” and “conneted” events after the conference’s receiving “callschanged” (around line 899). However, in “log_with_the_state_check”, those events are gone (near line 811). Besides, I also found a comment in function |Telephony::HandleCallInfo(…)| might be high related to this fact. The comment says the following,

 // We don't fire the statechange event on a call in conference here.
 // Instead, the event will be fired later in
 // TelephonyCallGroup::ChangeState(). Thus the sequence of firing the
 // statechange events is guaranteed: first on TelephonyCallGroup then on
 // individual TelephonyCall objects.

Therefore, I came to a conclusion that |TelephonyCall::ChangeStateInternal(…)| might be invoke twice when the call within a conference group changes, where the first time we update the call’s state and the second time we fire events to it, so I don’t think we should add the state check in the |TelephonyCall::ChangeStateInternal(…)|.

Comment 17

[Szu-Yu Chen [:aknow] (inactive)]

(In reply to a2882525224 from comment #16)
> Hi Aknow,
> 
> The new patch doesn’t add the state check to the beginning of
> |TelephonyCall::ChangeStateInternal(…)|, because the state check would fail
> many of marionette-webapi tests, where the most of them are failed by
> |timeout|. To identify the root cause, I did a little experiment. The other
> two attachments are the logs of running |test_conference_two_hangup_one.js|
> with and without the state check. By comparing the difference between the
> two logs, I think some event firings are spared by the state check. In
> “Log_without_state_check.log”, the calls received both “statechange” and
> “conneted” events after the conference’s receiving “callschanged” (around
> line 899). However, in “log_with_the_state_check”, those events are gone
> (near line 811). Besides, I also found a comment in function
> |Telephony::HandleCallInfo(…)| might be high related to this fact. The
> comment says the following,
> 
>  // We don't fire the statechange event on a call in conference here.
>  // Instead, the event will be fired later in
>  // TelephonyCallGroup::ChangeState(). Thus the sequence of firing the
>  // statechange events is guaranteed: first on TelephonyCallGroup then on
>  // individual TelephonyCall objects.
> 
> Therefore, I came to a conclusion that
> |TelephonyCall::ChangeStateInternal(…)| might be invoke twice when the call
> within a conference group changes, where the first time we update the call’s
> state and the second time we fire events to it, so I don’t think we should
> add the state check in the |TelephonyCall::ChangeStateInternal(…)|.

Oh yes, you are right.  Sorry that I just forgot this part and thanks for your thorough experiment.

Comment 18

[Szu-Yu Chen [:aknow] (inactive)]

Comment on attachment 8594772 [details] [diff] [review]
Part 2:Obsolete some events (DOM) (V3). r=aknow

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

Perfect! Thanks.

Comment 19

[a2882525224]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6510d5a4ca1c&exclusion_profile=false

Comment 20

[Ryan VanderMeulen (PTO, back July 27)]

https://hg.mozilla.org/integration/b2g-inbound/rev/24dae699d9f8
https://hg.mozilla.org/integration/b2g-inbound/rev/9f2de9f82b1a
https://hg.mozilla.org/integration/b2g-inbound/rev/d50e013210af

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/24dae699d9f8
https://hg.mozilla.org/mozilla-central/rev/9f2de9f82b1a
https://hg.mozilla.org/mozilla-central/rev/d50e013210af