Deprecate nsITelephonyListener.conferenceCallStateChanged

RESOLVED FIXED in Firefox 44

Status

Firefox OS
RIL
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: aknow, Assigned: HoPang)

Tracking

unspecified
FxOS-S10 (30Oct)
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(tracking-b2g:backlog, firefox44 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Assignee: nobody → bhsu
(Assignee)

Comment 1

3 years ago
Created attachment 8610038 [details] [diff] [review]
Deprecate nsITelephonyListener::conferenceCallStateChanged
(Assignee)

Comment 2

3 years ago
Created attachment 8610039 [details] [diff] [review]
Deprecate nsITelephonyListener::conferenceCallStateChanged (Bluetooth)
(Assignee)

Comment 3

3 years ago
Created attachment 8610042 [details] [diff] [review]
Deprecate nsITelephonyListener::conferenceCallStateChanged
Attachment #8610038 - Attachment is obsolete: true
[Tracking Requested - why for this release]:
tracking-b2g: --- → backlog
(Assignee)

Updated

3 years ago
Attachment #8610042 - Flags: review?(szchen)
(Reporter)

Comment 5

3 years ago
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.
Attachment #8610042 - Flags: review?(szchen) → review-
(Assignee)

Comment 6

3 years ago
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.
(Assignee)

Comment 7

3 years ago
Created attachment 8672448 [details] [diff] [review]
Part 1: Deprecate nsITelephonyListener::conferenceCallStateChanged (Telephony)
Attachment #8610042 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8672448 - Attachment description: Part 1: Deprecate nsITelephonyListener::conferenceCallStateChanged (Telephony)) → Part 1: Deprecate nsITelephonyListener::conferenceCallStateChanged (Telephony)
(Assignee)

Comment 8

3 years ago
Created attachment 8672449 [details] [diff] [review]
Part 2: Deprecate nsITelephonyListener::conferenceCallStateChanged (Bluetooth)
Attachment #8610039 - Attachment is obsolete: true
(Assignee)

Comment 9

3 years ago
Created attachment 8672485 [details] [diff] [review]
Part 1: Deprecate nsITelephonyListener::conferenceCallStateChanged (Telephony)
Attachment #8672448 - Attachment is obsolete: true
(Assignee)

Comment 10

3 years ago
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.
Attachment #8672485 - Flags: review?(btseng)
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.
Attachment #8672485 - Flags: review?(btseng)
(Assignee)

Comment 12

3 years ago
Created attachment 8672520 [details] [diff] [review]
Part 1 (V2): Deprecate nsITelephonyListener::conferenceCallStateChanged (Telephony)
Attachment #8672485 - Attachment is obsolete: true
(Assignee)

Comment 13

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

Here is an updated version :P
Attachment #8672520 - Flags: review?(btseng)
Comment on attachment 8672520 [details] [diff] [review]
Part 1 (V2): Deprecate nsITelephonyListener::conferenceCallStateChanged (Telephony)

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

LGTM, thanks!
Attachment #8672520 - Flags: review?(btseng) → review+
(Assignee)

Comment 15

3 years ago
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?
Attachment #8672449 - Flags: review?(btian)

Comment 16

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

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

LGTM
Attachment #8672449 - Flags: review?(btian) → review+
https://hg.mozilla.org/mozilla-central/rev/6d06e276a2b9
https://hg.mozilla.org/mozilla-central/rev/2b959bec2e88
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S10 (30Oct)
You need to log in before you can comment on or make changes to this bug.