1095366 - Consider to obsolete some telephonyCall.on***ing events, onconnecting, onholding, onresuming...

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

Please see Bug 1077075 comment #2 and comment #4.

In addition to phase these events out, how about the state 'connecting', 'holding', 'resuming' and 'disconnecting'? I suspect that these are not helpful anymore after bug 1077075.

Comment 1

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #0)
> Please see Bug 1077075 comment #2 and comment #4.
> 
> In addition to phase these events out, how about the state 'connecting',
> 'holding', 'resuming' and 'disconnecting'? I suspect that these are not
> helpful anymore after bug 1077075.

Agree.
I'd like to phase out those states and events.

Comment 2

[Ben Hsu [:HoPang]]

After tracing the related code and discussed with Szu-Yu, we found out that if we remove those states thoroughly, we can no longer reject consecutive requests of the same type. For example, consecutive hold requests can be sent, and the call is possible be resumed unintentionally. Therefore, I suggest keeping those states internally and hiding them from the WebApps.

Comment 3

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

(In reply to Ben Hsu [:benhsu] from comment #2)
> After tracing the related code and discussed with Szu-Yu, we found out that
> if we remove those states thoroughly, we can no longer reject consecutive
> requests of the same type. For example, consecutive hold requests can be
> sent, and the call is possible be resumed unintentionally. Therefore, I
> suggest keeping those states internally and hiding them from the WebApps.

Agree. We do need to have internal mechanism for well handling consecutive requests. Keeping those transient states internally is a solution.

Comment 4

[Ben Hsu [:HoPang]]

Attachment: Part 1: Remove some events (WebIDL). r=htsai

Comment 5

[Ben Hsu [:HoPang]]

Attachment: Part 2: Remove some event firing (DOM)

Comment 6

[Ben Hsu [:HoPang]]

Attachment: Part 2: Remove some event firing (DOM)

Comment 7

[Ben Hsu [:HoPang]]

Attachment: Part 3: Update the related testcases

Comment 8

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

Comment on attachment 8553597 [details] [diff] [review]
Part 1: Remove some events (WebIDL). r=htsai

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

Thank you.

Comment 9

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

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

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

Please check my comment and see whether we could remove that test . Thank you.

::: dom/telephony/test/marionette/test_incoming_connecting_hangup.js
@@ +28,5 @@
>  
>  function testConnectingHangUp() {
>    log("= testConnectingHangUp =");
>    return incoming()
> +    .then(() => inCall.answer())

The purpose of this test case is to test hangUp when the call is in "connecting" state. However, after we remove the "ing" state, the test is not valid and I think that we don't need them anymore. Please consider to remove the entire test.

Moreover, after applying your change here, the test case is just like testing a normal call (answer and hangup). It's redundant because we already have the same one in other test files.

Comment 10

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

Comment on attachment 8553600 [details] [diff] [review]
Part 2: Remove some event firing (DOM)

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

::: dom/telephony/TelephonyCall.cpp
@@ +239,5 @@
>  
>    nsRefPtr<Promise> promise = Promise::Create(global, aRv);
>    NS_ENSURE_TRUE(!aRv.Failed(), nullptr);
>  
> +  if (mCallState == nsITelephonyService::CALL_STATE_CONNECTING) {

We don't need this 'if' block. I think the case is also covered by the 'if' condition below.
- if (mCallState != nsITelephonyService::CALL_STATE_INCOMING) {

@@ +277,5 @@
> +    promise->MaybeReject(NS_ERROR_DOM_INVALID_STATE_ERR);
> +    return promise.forget();
> +  }
> +
> +  if (mCallState == nsITelephonyService::CALL_STATE_DISCONNECTED) {

Let's just merge these two condition. I cannot see the value of separating them.

@@ +305,5 @@
>  
>    nsRefPtr<Promise> promise = Promise::Create(global, aRv);
>    NS_ENSURE_TRUE(!aRv.Failed(), nullptr);
>  
> +  if (mCallState == nsITelephonyService::CALL_STATE_HOLDING) {

The same. The 'if' here is redundant seems it's covered by next if condition.

@@ +356,5 @@
>  
>    nsRefPtr<Promise> promise = Promise::Create(global, aRv);
>    NS_ENSURE_TRUE(!aRv.Failed(), nullptr);
>  
> +  if (mCallState == nsITelephonyService::CALL_STATE_RESUMING) {

The same. The 'if' here is redundant seems it's covered by next if condition.

Comment 11

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

Ben,

There are 'holding' and 'resuming' on TelephonyCallGroup [1]. Are you going to remove them as well in this bug?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/webidl/TelephonyCallGroup.webidl#33

Comment 12

[Ben Hsu [:HoPang]]

Hi aknow,

Thanks for your review. I think those |if blocks| are necessary, since the warning messages are different from their following state checks to explicitly show the fail cause. However, if you think we don't have to differ those warning messages, I am fine to remove the |if blocks|.

Comment 13

[Ben Hsu [:HoPang]]

Sure, those events should be deprecated, but I think we should update the summary of this bug first, or we had better file another bug for it.

Comment 14

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

Ben and Hsinyi,

Beside modifying .webidl, we should also remove those 'ing' states from nsITelephonyService.  In my opinion, those states has to be designed for internal use only in DOM.  Otherwise, they are possible to be altered by TelephonyService (ex: calling nsITelephonyListener.callStateChanged(...) with callstate = 'xxxing').

However, it will touch our freezed interface and I cannot think of an easy way to do that.

Comment 15

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

(In reply to Ben Hsu [:benhsu] from comment #12)
> Hi aknow,
> 
> Thanks for your review. I think those |if blocks| are necessary, since the
> warning messages are different from their following state checks to
> explicitly show the fail cause. However, if you think we don't have to
> differ those warning messages, I am fine to remove the |if blocks|.

How about having just one |if block| but showing the warning with current call state?  Then we will have enough information to differ those cases.

Comment 16

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

(In reply to Szu-Yu Chen [:aknow] from comment #14)
> Ben and Hsinyi,
> 
> Beside modifying .webidl, we should also remove those 'ing' states from
> nsITelephonyService.  

You are absolutely right, Aknow!

Eventually we should remove those -ing states from nsITelephonyService.idl but we cannot do that due to the freezing agreement. 

> In my opinion, those states has to be designed for
> internal use only in DOM.  Otherwise, they are possible to be altered by
> TelephonyService (ex: calling nsITelephonyListener.callStateChanged(...)
> with callstate = 'xxxing').
> 

That's a concern, though I am not too worried about if comRIL uses those values because dialer app has never relied on them. Ignoring them as what Ben proposed won't harm anything. 

> However, it will touch our freezed interface and I cannot think of an easy
> way to do that.

For now there's no explicit way to indicate 'things to be deprecated' except dropping a comment :(
So what we can and should do now per our offline meeting is 1) file a bug for the deprecation, 2) Add TODO comment in nsITelephonyService.idl , and 3) add the dependency to bug 1125733.

Comment 17

[Ben Hsu [:HoPang]]

Attachment: Part 2: Remove some event firing (DOM) (V2)

Comment 18

[Ben Hsu [:HoPang]]

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

Comment 19

[Ben Hsu [:HoPang]]

Attachment: Part 2: Remove some event firing (DOM) (V2)

Comment 20

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

Comment on attachment 8555049 [details] [diff] [review]
Part 2: Remove some event firing (DOM) (V2)

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

::: dom/telephony/TelephonyCall.cpp
@@ +176,5 @@
> +void
> +TelephonyCall::DebugWarning(const char *aMsg)
> +{
> +
> +#ifdef DEBUG

Don't use #ifdef DEBUG.

Try to assume that you don't know the magic behind NS_WARNING.  Maybe It still do something when !DEBUG.  However, in that case, your code will skip the WARNING.

If you are worried about the waste of running time or memory of creating a string but not used, you could do somthing like

  #define WARN(...) NS_WARNING(nsPrintfCString(__VA_ARGS__).get())

the nsPrintfCString will only be created when NS_WARNING is not empty.

@@ +179,5 @@
> +
> +#ifdef DEBUG
> +  nsPrintfCString debugMsg("%s, (State: %u, Conference: %u,"
> +                           " Mergeable: %u, Switchable: %u)",
> +                           aMsg, mGroup? 1:0, mMergeable, mSwitchable);

Missing one argument for callstate.

@@ +269,2 @@
>    if (mCallState != nsITelephonyService::CALL_STATE_INCOMING) {
> +    DebugWarning("This call cannot be answered!");

I'd prefer to have a custom debug message in every case.  In current code, your DebugWanring simply dump all the parameters.  However, not all of them are useful in each case.

For example, here, only call state is useful.  With my previous defined macro, you can do
WARN("The call cannot be answered! state: %u", mCallState);

Comment 21

[Ben Hsu [:HoPang]]

Attachment: Part 2: Remove some event firing (DOM) (V3). r=aknow

Comment 22

[Ben Hsu [:HoPang]]

Comment on attachment 8555700 [details] [diff] [review]
Part 2: Remove some event firing (DOM) (V3). r=aknow

Hi Aknow,

Thanks for your advice, but I think defining a new macro for warnings isn't so necassary, since only four of the warnings need to be extended.

Comment 23

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

Comment on attachment 8555700 [details] [diff] [review]
Part 2: Remove some event firing (DOM) (V3). r=aknow

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

Looks good to me.  Thank you.

Comment 24

[Ben Hsu [:HoPang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=59b2af37e7ef&exclusion_state=all

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/de43b958f3f4
https://hg.mozilla.org/integration/b2g-inbound/rev/20ad233975c2
https://hg.mozilla.org/integration/b2g-inbound/rev/fff3275b879f

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/de43b958f3f4
https://hg.mozilla.org/mozilla-central/rev/20ad233975c2
https://hg.mozilla.org/mozilla-central/rev/fff3275b879f