1155072 - Deprecate nsITelephonyListener.conferenceCallStateChanged

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

Originally, we have to fire |callStateChanged| for every calls and then |conferenceCallStateChanged|. The purpose is:

"""
This is used to notify that every conference participant has been visited. So that TelephonyDOM could know when is the correct time to dispatch conference state change event to gaia.
"""

In bug 1155063, callStateChanged could accepts an array of call information. As long as we ensure that the information of all the conference participants is carry by one callStateChanged, we could safely remove conferenceCallStateChanged and let TelephonyDOM itself detects the conference states.

Comment 1

[Ben Hsu [:HoPang]]

Attachment: Deprecate nsITelephonyListener::conferenceCallStateChanged

Comment 2

[Ben Hsu [:HoPang]]

Attachment: Deprecate nsITelephonyListener::conferenceCallStateChanged (Bluetooth)

Comment 3

[Ben Hsu [:HoPang]]

Attachment: Deprecate nsITelephonyListener::conferenceCallStateChanged

Comment 4

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

[Tracking Requested - why for this release]:

Comment 5

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

Comment on attachment 8610042 [details] [diff] [review]
Deprecate nsITelephonyListener::conferenceCallStateChanged

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

::: dom/telephony/Telephony.cpp
@@ +633,5 @@
> +        break;
> +      }
> +    }
> +  }
> +  mGroup->ChangeState(conferenceCallState);

The set of valid conference call state is different from the set of call state. The value could only be CONNECTED, HELD, UNKNOWN. Let's have an assert that make sure the value we compute is valid.

::: dom/telephony/gonk/TelephonyService.js
@@ +1655,5 @@
>      this._notifyAllListeners("supplementaryServiceNotification",
>                               [aClientId, callIndex, notification]);
>    },
>  
>    _handleConferenceCallStateChanged: function(aState) {

Is it possible to remove everything about conferenceCallState?
I'll expect we don't need this kind of information in TelephonyService.js.

Comment 6

[Ben Hsu [:HoPang]]

Comment on attachment 8610042 [details] [diff] [review]
Deprecate nsITelephonyListener::conferenceCallStateChanged

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

::: dom/telephony/Telephony.cpp
@@ +633,5 @@
> +        break;
> +      }
> +    }
> +  }
> +  mGroup->ChangeState(conferenceCallState);

You're absolutely right, I'll add assertions here.

::: dom/telephony/gonk/TelephonyService.js
@@ +1655,5 @@
>      this._notifyAllListeners("supplementaryServiceNotification",
>                               [aClientId, callIndex, notification]);
>    },
>  
>    _handleConferenceCallStateChanged: function(aState) {

Well, I think removing everything related is totally doable, since new conference states now can be computed at DOM by itself. However, there is still some control logic depending on conference state in |TelephonyService.js|, so it might take me some time to get rid all of them.

Comment 7

[Ben Hsu [:HoPang]]

Attachment: Part 1: Deprecate nsITelephonyListener::conferenceCallStateChanged (Telephony)

Comment 8

[Ben Hsu [:HoPang]]

Attachment: Part 2: Deprecate nsITelephonyListener::conferenceCallStateChanged (Bluetooth)

Comment 9

[Ben Hsu [:HoPang]]

Attachment: Part 1: Deprecate nsITelephonyListener::conferenceCallStateChanged (Telephony)

Comment 10

[Ben Hsu [:HoPang]]

Comment on attachment 8672485 [details] [diff] [review]
Part 1: Deprecate nsITelephonyListener::conferenceCallStateChanged (Telephony)

>-  this._currentConferenceState = nsITelephonyService.CALL_STATE_UNKNOWN;

Since this data is only used by hangUpConference(), I also remove this data to simplify Telephony Service.

Comment 11

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8672485 [details] [diff] [review]
Part 1: Deprecate nsITelephonyListener::conferenceCallStateChanged (Telephony)

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

Please see my comments below and 
please also remove conferenceCallStateChanged in 
https://dxr.mozilla.org/mozilla-central/source/dom/telephony/test/marionette/test_TelephonyUtils.js#48
https://dxr.mozilla.org/mozilla-central/source/dom/telephony/gonk/TelephonyUtils.jsm#100

Thanks!

::: dom/telephony/gonk/TelephonyService.js
@@ +2135,5 @@
> +    }
> +
> +    // There is no conference call.
> +    if (DEBUG) debug("hangUpConference: " +
> +                     "No conference call in modem[" + aClientId + "].");

We should reply a error if there is no conference call to hangup.

Comment 12

[Ben Hsu [:HoPang]]

Attachment: Part 1 (V2): Deprecate nsITelephonyListener::conferenceCallStateChanged (Telephony)

Comment 13

[Ben Hsu [:HoPang]]

Comment on attachment 8672520 [details] [diff] [review]
Part 1 (V2): Deprecate nsITelephonyListener::conferenceCallStateChanged (Telephony)

Here is an updated version :P

Comment 14

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8672520 [details] [diff] [review]
Part 1 (V2): Deprecate nsITelephonyListener::conferenceCallStateChanged (Telephony)

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

LGTM, thanks!

Comment 15

[Ben Hsu [:HoPang]]

Comment on attachment 8672449 [details] [diff] [review]
Part 2: Deprecate nsITelephonyListener::conferenceCallStateChanged (Bluetooth)

Hi Ben

We are now refactoring some telephony-relevant interfaces, and |nsITelephonyListener::conferenceCallStateChanged| is going to be deprecated in this bug. Since |TelephonyListener::ConferenceCallStateChanged()| seems just to implement the method without futher meaning, I simply removed it. Do you mind reviewing this patch?

Comment 16

[Ben Tian (inactive)]

Comment on attachment 8672449 [details] [diff] [review]
Part 2: Deprecate nsITelephonyListener::conferenceCallStateChanged (Bluetooth)

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

LGTM

Comment 17

[Ben Hsu [:HoPang]]

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

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/6d06e276a2b9
https://hg.mozilla.org/integration/b2g-inbound/rev/2b959bec2e88

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/6d06e276a2b9
https://hg.mozilla.org/mozilla-central/rev/2b959bec2e88