Closed Bug 1095366 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

defect
Not set

Tracking

(firefox38 fixed)

RESOLVED FIXED
2.2 S6 (20feb)
Tracking Status
firefox38 --- fixed

People

(Reporter: hsinyi, Assigned: bhsu)

Details

Attachments

(3 files, 5 obsolete files)

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.
(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.
Assignee: nobody → bhsu
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.
Flags: needinfo?(htsai)
(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.
Flags: needinfo?(htsai)
Attachment #8553597 - Flags: review?(htsai)
Attachment #8553598 - Flags: review?(szchen)
Attachment #8553598 - Attachment is obsolete: true
Attachment #8553598 - Flags: review?(szchen)
Attachment #8553600 - Flags: review?(szchen)
Attachment #8553622 - Flags: review?(szchen)
Attachment #8553622 - Attachment description: 3.patch → Part 3: Update the related testcases
Comment on attachment 8553597 [details] [diff] [review]
Part 1: Remove some events (WebIDL). r=htsai

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

Thank you.
Attachment #8553597 - Flags: review?(htsai) → review+
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.
Attachment #8553622 - Flags: review?(szchen) → review-
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.
Attachment #8553600 - Flags: review?(szchen) → review-
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
Flags: needinfo?(bhsu)
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|.
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.
Flags: needinfo?(bhsu)
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.
(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.
(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.
Attachment #8553600 - Attachment is obsolete: true
Attachment #8553622 - Attachment is obsolete: true
Attachment #8555045 - Attachment is obsolete: true
Attachment #8555049 - Flags: review?(szchen)
Attachment #8555047 - Flags: review?(szchen)
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);
Attachment #8555049 - Flags: review?(szchen) → review-
Attachment #8555047 - Flags: review?(szchen) → review+
Attachment #8555049 - Attachment is obsolete: true
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.
Attachment #8555700 - Flags: review?(szchen)
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.
Attachment #8555700 - Flags: review?(szchen) → review+
Attachment #8553597 - Attachment description: Part 1: Remove some events (WebIDL) → Part 1: Remove some events (WebIDL). r=htsai
Attachment #8555047 - Attachment description: Part 3: Update the related testcases (V2) → Part 3: Update the related testcases (V2). r=aknow
Attachment #8555700 - Attachment description: Part 2: Remove some event firing (DOM) (V3) → Part 2: Remove some event firing (DOM) (V3). r=aknow
You need to log in before you can comment on or make changes to this bug.