[IMS] TTY Mode Support.

RESOLVED FIXED in Firefox OS v2.2r

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bevis, Assigned: freesamael)

Tracking

unspecified
FxOS-S6 (04Sep)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(feature-b2g:2.2r+, b2g-v2.2r fixed, b2g-master wontfix)

Details

Attachments

(7 attachments, 14 obsolete attachments)

1.59 KB, patch
bevis
: review+
Details | Diff | Splinter Review
6.09 KB, patch
freesamael
: review+
Details | Diff | Splinter Review
5.05 KB, patch
freesamael
: review+
Details | Diff | Splinter Review
4.08 KB, patch
freesamael
: review+
Details | Diff | Splinter Review
3.95 KB, patch
freesamael
: review+
Details | Diff | Splinter Review
5.84 KB, patch
freesamael
: review+
Details | Diff | Splinter Review
5.95 KB, patch
freesamael
: review+
Details | Diff | Splinter Review
This bug is to have TTY feature enabled to
1. Provide a User Settings to change current TTY mode.
2. Notify the TTY mode change to the modem/ims stack to change the audio path  to the TTY terminal accordingly.
3. Receive the Notification from remote party that the TTY mode is changed to redirect the user to change the TTY setting of the device to the correct one accordingly.
(Reporter)

Updated

3 years ago
Group: tai-confidential, qualcomm-confidential
CC list accessible: false
Not accessible to reporter
feature-b2g: --- → 2.2r+
Hi Samael, please help this! We need to work closely with our partners for the detailed design.
Assignee: nobody → sawang
(Reporter)

Comment 2

3 years ago
TTY study from AOSP:
1. A predefined user preference for TTYMode (Off, Full, HCO, VCO) and default set to Off.

2. TtyManager in CallManager: (currentTtyMode to Off by default)
   http://androidxref.com/5.1.1_r6/xref/packages/services/Telecomm/src/com/android/server/telecom/TtyManager.java#72
    - Listen to user preference change & headset plugged in change:
    - if wired headset is plugged, defined newTtyMode to the preferred one. Set to Off, Otherwise.
        - if currentTtyMode != newTtyMode, set currentTtyMode to newTtyMode and
            - notify tty_mode to AudioManager via AudioManager.setParameters.
            - notify tty_mode to TtyManager in each phone instance to set TTY mode properly.
              http://androidxref.com/5.1.1_r6/xref/packages/services/Telephony/src/com/android/services/telephony/TtyManager.java#updateTtyMode
            - notify status bar policy to update the status on status bar that currently applied to the audio manager.

3. TtyManager in PhoneGlobals:
    - Listen to user preference change to update UI TTY Mode to each phone. (IMS Phone currently. This is weird!)
    - Update TTY Mode notify from TtyManager in CallManager to each modem/phone instances.

4. CallStateMonitor for PhoneGlobals:
    - Listen to the TTY Mode changed from remote party and display a dialog to inform the user to change the tty mode accordingly. (Only available from IMS currently)
(Reporter)

Comment 3

3 years ago
For to support TTY in Telephony.webidl:
1. provide getter/setter of ttyMode.
2. provide a system message API to notify the TTYMode requested from remote party.
Created attachment 8639112 [details] [diff] [review]
Part 1: Add "telephony-tty-mode-changed" system message
Created attachment 8639113 [details] [diff] [review]
Part 2: Add ttyMode attribute to nsITelephonyAudioService
Created attachment 8639114 [details] [diff] [review]
Part 4: Update IPC implementation for nsITelephony change
Created attachment 8639115 [details] [diff] [review]
Part 5: Add ttyMode attribute to Telephony Web IDL
Created attachment 8639162 [details] [diff] [review]
Part 3: Add ttyMode attribute to nsITelephonyService
Created attachment 8639705 [details] [diff] [review]
Part 5(v2): Add ttyMode attribute to Telephony Web IDL
Attachment #8639705 - Attachment description: Part 5: Add ttyMode attribute to Telephony Web IDL → Part 5(v2): Add ttyMode attribute to Telephony Web IDL
Attachment #8639115 - Attachment is obsolete: true

Updated

3 years ago
Blocks: 1189467
Attachment #8639112 - Flags: review?(btseng)
Attachment #8639113 - Flags: review?(btseng)
Attachment #8639114 - Flags: review?(btseng)
Attachment #8639162 - Flags: review?(btseng)
Comment on attachment 8639705 [details] [diff] [review]
Part 5(v2): Add ttyMode attribute to Telephony Web IDL

Hi Bevis,

Could you help to review above patches?
Attachment #8639705 - Flags: review?(btseng)
Comment on attachment 8639112 [details] [diff] [review]
Part 1: Add "telephony-tty-mode-changed" system message

LGTM, thanks!
Attachment #8639112 - Flags: review?(btseng) → review+
Comment on attachment 8639113 [details] [diff] [review]
Part 2: Add ttyMode attribute to nsITelephonyAudioService

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

Please see my comment below:

::: dom/telephony/gonk/TelephonyAudioService.js
@@ +81,5 @@
>      } catch (e) {}
>    },
>  
> +  _updateTtyMode: function() {
> +    let mode = 0;

Please assign the default value to newly defined constant of Ci.nsIITelephonyAudioService.TTY_MODE_OFF.

::: dom/telephony/nsITelephonyAudioService.idl
@@ +23,5 @@
>  
>    /**
> +   * TTY mode.
> +   */
> +  attribute unsigned long ttyMode;

Please define constants of all the ttyModes.
Attachment #8639113 - Flags: review?(btseng) → review-
Comment on attachment 8639114 [details] [diff] [review]
Part 4: Update IPC implementation for nsITelephony change

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

Looks good to me, thanks!
Attachment #8639114 - Flags: review?(btseng) → review+
Comment on attachment 8639162 [details] [diff] [review]
Part 3: Add ttyMode attribute to nsITelephonyService

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

Please see my comment below:

::: dom/telephony/nsITelephonyService.idl
@@ +273,5 @@
>    void cancelUSSD(in unsigned long cliendId, in nsITelephonyCallback callback);
>  
>    attribute bool microphoneMuted;
>    attribute bool speakerEnabled;
> +  attribute unsigned long ttyMode;

Please set as |unsigned short| as well.
Attachment #8639162 - Flags: review?(btseng)
Comment on attachment 8639705 [details] [diff] [review]
Part 5(v2): Add ttyMode attribute to Telephony Web IDL

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

Please have marionette test case separated in another patch and then request review again.

Thanks!
Attachment #8639705 - Flags: review?(btseng)
Comment on attachment 8639113 [details] [diff] [review]
Part 2: Add ttyMode attribute to nsITelephonyAudioService

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

::: dom/telephony/gonk/TelephonyAudioService.js
@@ +81,5 @@
>      } catch (e) {}
>    },
>  
> +  _updateTtyMode: function() {
> +    let mode = 0;

Just realized that the constants are defined in nsITelephonyService, then
please assign the default value to Ci.nsIITelephonyService.TTY_MODE_OFF.

Thanks!

::: dom/telephony/nsITelephonyAudioService.idl
@@ +23,5 @@
>  
>    /**
> +   * TTY mode.
> +   */
> +  attribute unsigned long ttyMode;

Just realized that they were defined in nsITelephonyService.
Attachment #8639113 - Flags: review-
Created attachment 8646783 [details] [diff] [review]
Part 1(v2): Add "telephony-tty-mode-changed" system message
Created attachment 8646785 [details] [diff] [review]
Part 2(v2): Add ttyMode attribute to nsITelephonyAudioService
Created attachment 8646786 [details] [diff] [review]
Part 3(v2): Add ttyMode attribute to nsITelephonyService
Created attachment 8646787 [details] [diff] [review]
Part 4(v2): Update IPC implementation for nsITelephony change
Created attachment 8646788 [details] [diff] [review]
Part 5(v3): Add ttyMode attribute to Telephony Web IDL
Created attachment 8646790 [details] [diff] [review]
Part 6: Add marionette test case. r=btseng
Attachment #8646783 - Attachment description: Part 1: Add "telephony-tty-mode-changed" system message → Part 1(v2): Add "telephony-tty-mode-changed" system message
Attachment #8646785 - Attachment description: Part 2: Add ttyMode attribute to nsITelephonyAudioService → Part 2(v2): Add ttyMode attribute to nsITelephonyAudioService
Attachment #8646786 - Attachment description: Part 3: Add ttyMode attribute to nsITelephonyService → Part 3(v2): Add ttyMode attribute to nsITelephonyService
Attachment #8646787 - Attachment description: Part 4: Update IPC implementation for nsITelephony change → Part 4(v2): Update IPC implementation for nsITelephony change
Attachment #8646788 - Attachment description: Part 5: Add ttyMode attribute to Telephony Web IDL → Part 5(v3): Add ttyMode attribute to Telephony Web IDL
Attachment #8639112 - Attachment is obsolete: true
Attachment #8639113 - Attachment is obsolete: true
Attachment #8639114 - Attachment is obsolete: true
Attachment #8639162 - Attachment is obsolete: true
Attachment #8639705 - Attachment is obsolete: true
Attachment #8646783 - Flags: review?(btseng)
Attachment #8646785 - Flags: review?(btseng)
Attachment #8646786 - Flags: review?(btseng)
Attachment #8646787 - Flags: review?(btseng)
Attachment #8646788 - Flags: review?(btseng)
Comment on attachment 8646790 [details] [diff] [review]
Part 6: Add marionette test case. r=btseng

Hi Bevis,

Could you help to review those updated patches?
Attachment #8646790 - Flags: review?(btseng)
Comment on attachment 8646783 [details] [diff] [review]
Part 1(v2): Add "telephony-tty-mode-changed" system message

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

r=me after addressing the comments.

::: dom/system/gonk/tests/test_ril_system_messenger.js
@@ +119,5 @@
> +  add_test(function test_telephony_messenger_notify_tty_mode_changed() {
> +    const TTY_MODE_OFF = 0;
> +    const TTY_MODE_FULL = 1;
> +    const TTY_MODE_HCO = 2;
> +    const TTY_MODE_VCO = 3;

No need for this 4 const.
You can put Ci.nsITelephonyService.TTY_MODE_XXX directly in notifyTtyModeChanged().

@@ +123,5 @@
> +    const TTY_MODE_VCO = 3;
> +    const INVALID_VALUE = 4;
> +    let messenger = newRILSystemMessenger();
> +
> +    messenger.notifyTtyModeChanged(TTY_MODE_OFF);

messenger.notifyTtyModeChanged(Ci.nsITelephonyService.TTY_MODE_OFF);

@@ +126,5 @@
> +
> +    messenger.notifyTtyModeChanged(TTY_MODE_OFF);
> +    equal_received_system_message("telephony-tty-mode-changed",
> +                                  { ttyMode: "off" });
> +    messenger.notifyTtyModeChanged(TTY_MODE_FULL);

messenger.notifyTtyModeChanged(Ci.nsITelephonyService.TTY_MODE_FULL);

@@ +129,5 @@
> +                                  { ttyMode: "off" });
> +    messenger.notifyTtyModeChanged(TTY_MODE_FULL);
> +    equal_received_system_message("telephony-tty-mode-changed",
> +                                  { ttyMode: "full" });
> +    messenger.notifyTtyModeChanged(TTY_MODE_HCO);

messenger.notifyTtyModeChanged(Ci.nsITelephonyService.TTY_MODE_HCO);

@@ +132,5 @@
> +                                  { ttyMode: "full" });
> +    messenger.notifyTtyModeChanged(TTY_MODE_HCO);
> +    equal_received_system_message("telephony-tty-mode-changed",
> +                                  { ttyMode: "hco" });
> +    messenger.notifyTtyModeChanged(TTY_MODE_VCO);

messenger.notifyTtyModeChanged(Ci.nsITelephonyService.TTY_MODE_VCO);

::: dom/telephony/nsITelephonyMessenger.idl
@@ +54,5 @@
> +
> +  /**
> +   * To broadcast 'telephony-tty-mode-changed' system message.
> +   *
> +   * @param aMode The new TTY mode.

* @param aMode 
*        The new TTY mode of the remote party.
*        One of nsITelephonyService::TTY_MODE_*.
Attachment #8646783 - Flags: review?(btseng) → review+
Comment on attachment 8646785 [details] [diff] [review]
Part 2(v2): Add ttyMode attribute to nsITelephonyAudioService

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

r=me after addressing the comments.

Thanks!

::: dom/telephony/nsITelephonyAudioService.idl
@@ +22,5 @@
>    attribute boolean speakerEnabled;
>  
>    /**
> +   * TTY mode.
> +   */

/**
 * Current TTY mode.
 *
 * One of the nsITelephonyService::TTY_MODE_* values.
 */
Attachment #8646785 - Flags: review?(btseng) → review+
Comment on attachment 8646786 [details] [diff] [review]
Part 3(v2): Add ttyMode attribute to nsITelephonyService

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

r=me after addressing the comments.

Thanks!

::: dom/telephony/nsITelephonyService.idl
@@ +272,5 @@
>     */
>    void cancelUSSD(in unsigned long cliendId, in nsITelephonyCallback callback);
>  
>    attribute bool microphoneMuted;
>    attribute bool speakerEnabled;

// new line
/**
 * Current TTY mode.
 *
 * One of the nsITelephonyService::TTY_MODE_* values.
 */
Attachment #8646786 - Flags: review?(btseng) → review+
Comment on attachment 8646787 [details] [diff] [review]
Part 4(v2): Update IPC implementation for nsITelephony change

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

r=me after addressing the comments.

Thanks!

::: dom/telephony/ipc/TelephonyParent.cpp
@@ +292,5 @@
>  }
>  
> +bool
> +TelephonyParent::RecvGetTtyMode(uint16_t* aMode)
> +{

*aMode = nsITelephonyService::TTY_MODE_OFF;
Attachment #8646787 - Flags: review?(btseng) → review+
Comment on attachment 8646788 [details] [diff] [review]
Part 5(v3): Add ttyMode attribute to Telephony Web IDL

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

Looks good to me.

Forward the review to Hsinyi for webidl changes.

Thanks!
Attachment #8646788 - Flags: review?(htsai)
Attachment #8646788 - Flags: review?(btseng)
Attachment #8646788 - Flags: review+
Comment on attachment 8646790 [details] [diff] [review]
Part 6: Add marionette test case. r=btseng

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

Looks good to me.

Thanks!
Attachment #8646790 - Flags: review?(btseng) → review+
Comment on attachment 8646785 [details] [diff] [review]
Part 2(v2): Add ttyMode attribute to nsITelephonyAudioService

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

Please r=me after uuid is updated.

::: dom/telephony/nsITelephonyAudioService.idl
@@ +24,5 @@
>    /**
> +   * TTY mode.
> +   */
> +  attribute unsigned short ttyMode;
> +

Just found that there is no uuid change for adding this attribute.
Comment on attachment 8646786 [details] [diff] [review]
Part 3(v2): Add ttyMode attribute to nsITelephonyService

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

no uuid change for nsITelephonyService.idl.

Please address this before landing the patch.

Thanks!
Created attachment 8647448 [details] [diff] [review]
Part 1(v3): Add "telephony-tty-mode-changed" system message. r=btseng
Created attachment 8647449 [details] [diff] [review]
Part 2(v3): Add ttyMode attribute to nsITelephonyAudioService. r=btseng
Created attachment 8647450 [details] [diff] [review]
Part 3(v3): Add ttyMode attribute to nsITelephonyService. r=btseng
Created attachment 8647451 [details] [diff] [review]
Part 4(v3): Update IPC implementation for nsITelephony change. r=btseng
Created attachment 8647452 [details] [diff] [review]
Part 7: Add mochitest test case
Attachment #8646783 - Attachment is obsolete: true
Attachment #8647448 - Attachment description: Part 1: Add "telephony-tty-mode-changed" system message → Part 1(v3): Add "telephony-tty-mode-changed" system message. r=btseng
Attachment #8647448 - Flags: review+
Attachment #8647449 - Attachment description: Part 2: Add ttyMode attribute to nsITelephonyAudioService → Part 2(v3): Add ttyMode attribute to nsITelephonyAudioService. r=btseng
Attachment #8647449 - Flags: review+
Attachment #8646785 - Attachment is obsolete: true
Attachment #8647450 - Attachment description: Part 3: Add ttyMode attribute to nsITelephonyService → Part 3(v3): Add ttyMode attribute to nsITelephonyService. r=btseng
Attachment #8647450 - Flags: review+
Attachment #8646786 - Attachment is obsolete: true
Attachment #8647451 - Attachment description: Part 4: Update IPC implementation for nsITelephony change → Part 4(v3): Update IPC implementation for nsITelephony change. r=btseng
Attachment #8647451 - Flags: review+
Attachment #8646787 - Attachment is obsolete: true
Comment on attachment 8646788 [details] [diff] [review]
Part 5(v3): Add ttyMode attribute to Telephony Web IDL

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

::: dom/webidl/Telephony.webidl
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/.
>   */
>  
> +enum TtyMode { "off", "full", "hco", "vco" };

"hco" and "vco" are really implicit to understand. Please have comment for more description, thanks.
Attachment #8646788 - Flags: review?(htsai) → review+
Created attachment 8650359 [details] [diff] [review]
Part 5(v4): Add ttyMode attribute to Telephony Web IDL. r=htsai, btseng
Attachment #8646790 - Attachment description: Part 6: Add marionette test case → Part 6: Add marionette test case. r=btseng
Attachment #8650359 - Attachment description: Part 5: Add ttyMode attribute to Telephony Web IDL → Part 5(v5): Add ttyMode attribute to Telephony Web IDL. r=htsai, btseng
Attachment #8650359 - Flags: review+
Attachment #8650359 - Attachment description: Part 5(v5): Add ttyMode attribute to Telephony Web IDL. r=htsai, btseng → Part 5(v4): Add ttyMode attribute to Telephony Web IDL. r=htsai, btseng
Attachment #8646788 - Attachment is obsolete: true
Comment on attachment 8647452 [details] [diff] [review]
Part 7: Add mochitest test case

Hi Bevis,

Could you help to review the mochitest test case?
Attachment #8647452 - Flags: review?(btseng)
(Reporter)

Updated

3 years ago
status-b2g-v2.2r: --- → affected
status-b2g-master: --- → wontfix
Comment on attachment 8647452 [details] [diff] [review]
Part 7: Add mochitest test case

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

Please see my comments below.

Thanks!

::: dom/telephony/test/mochitest/chrome_telephony_messenger_helper.js
@@ +21,5 @@
> +addMessageListener("notifyTtyModeChanged",
> +  (aMode) => {
> +    gSystemMessenger.registerPage("telephony-tty-mode-changed",
> +      Services.io.newURI(PAGE_URI, null, null),
> +      Services.io.newURI(MANIFEST_URI, null, null));

Please separate registerPage/newURI stuff into another event for setting up the test environment.

::: dom/telephony/test/mochitest/test_telephony_messenger.html
@@ +29,5 @@
> +  var promises = [];
> +
> +  promises.push(new Promise((aResolve, aReject) => {
> +    info("waiting for system message: " + MSG_TTY_MODE_CHANGED);
> +    navigator.mozSetMessageHandler(MSG_TTY_MODE_CHANGED, (aMessage) => {

Why to set MessageHandler multiple times during the test?

@@ +38,5 @@
> +  }).then((msg) => is(msg.ttyMode, aModeString)));
> +
> +  promises.push(new Promise((aResolve, aReject) => {
> +    info("triggering system message: " + MSG_TTY_MODE_CHANGED);
> +    gTelephonyMessengerHelper.addMessageListener("notifyTtyModeChangedCompleted",

We to add the same listener multiple times during the test?

@@ +55,5 @@
> +    .then(() => testNotifyTtyModeChanged(SpecialPowers.Ci.nsITelephonyService.TTY_MODE_HCO, "hco"))
> +    .then(() => {
> +      ok(true, "test finishes");
> +      SimpleTest.finish()
> +    });

replace line#56-59 to:
    .catch(function(e) {
      ok(false, 'promise rejects during test: ' + e);
    })
    .then(cleanUp);

The cleanUp function has to do something address in next comment.

@@ +61,5 @@
> +
> +SpecialPowers.pushPermissions([{ type: 'telephony', allow: true, context: document }],
> +  () => SpecialPowers.pushPrefEnv({'set': [[ 'ril.debugging.enabled', true ]]}, runTest));
> +
> +SimpleTest.waitForExplicitFinish();

Please have a setUp function to
pushPermissions, 
SpecialPowers.waitForExplicitFinish(), 
mozSetMessageHandler, 
addMessageListener

and 
have a cleanUp function to 
gTelephonyMessengerHelper.removeMessageListener, 
gTelephonyMessengerHelper.destroy
SimpleTest.finish()
Attachment #8647452 - Flags: review?(btseng)
Created attachment 8650933 [details] [diff] [review]
Part 7: Add mochitest test case
Attachment #8650933 - Attachment is obsolete: true
Attachment #8647452 - Attachment is obsolete: true
Created attachment 8650980 [details] [diff] [review]
Part 7(v2): Add mochitest test case
Attachment #8650980 - Attachment description: Part 7: Add mochitest test case → Part 7(v2): Add mochitest test case
Comment on attachment 8650980 [details] [diff] [review]
Part 7(v2): Add mochitest test case

Hi Bevis,

Would you help to review the updated test case?
Attachment #8650980 - Flags: review?(btseng)
Comment on attachment 8650980 [details] [diff] [review]
Part 7(v2): Add mochitest test case

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

Looks good to me. r=me after the comment below is addressed.

Thanks!

::: dom/telephony/test/mochitest/test_telephony_messenger.html
@@ +28,5 @@
> +function setup() {
> +  return new Promise((aResolve, aReject) => {
> +    SpecialPowers.pushPermissions(
> +      [{ type: 'telephony', allow: true, context: document }], function() {
> +      SpecialPowers.pushPrefEnv({'set': [[ 'ril.debugging.enabled', true ]]}, function() {

nit: It seems nothing to debug according to our implmentation even we set ril.debugging.enabled to true, IMO. We can remove this if not needed. :)
Attachment #8650980 - Flags: review?(btseng) → review+
Created attachment 8651673 [details] [diff] [review]
Part 7(v3): Add mochitest test case. r=btseng
Attachment #8651673 - Attachment description: Part 7: Add mochitest test case → Part 7(v3): Add mochitest test case. r=btseng
Attachment #8651673 - Flags: review+
Attachment #8650980 - Attachment is obsolete: true
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.