1185438 - [IMS] TTY Mode Support.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

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.

Comment 1

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

Hi Samael, please help this! We need to work closely with our partners for the detailed design.

Comment 2

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

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)

Comment 3

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

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.

Comment 4

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 1: Add "telephony-tty-mode-changed" system message

Comment 5

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 2: Add ttyMode attribute to nsITelephonyAudioService

Comment 6

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 4: Update IPC implementation for nsITelephony change

Comment 7

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 5: Add ttyMode attribute to Telephony Web IDL

Comment 8

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 3: Add ttyMode attribute to nsITelephonyService

Comment 9

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 5(v2): Add ttyMode attribute to Telephony Web IDL

Comment 10

[Samael Wang [:freesamael] (away for now)]

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?

Comment 11

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

Comment on attachment 8639112 [details] [diff] [review]
Part 1: Add "telephony-tty-mode-changed" system message

LGTM, thanks!

Comment 12

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

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.

Comment 13

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

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!

Comment 14

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

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.

Comment 15

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

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!

Comment 16

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

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.

Comment 17

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 1(v2): Add "telephony-tty-mode-changed" system message

Comment 18

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 2(v2): Add ttyMode attribute to nsITelephonyAudioService

Comment 19

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 3(v2): Add ttyMode attribute to nsITelephonyService

Comment 20

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 4(v2): Update IPC implementation for nsITelephony change

Comment 21

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 5(v3): Add ttyMode attribute to Telephony Web IDL

Comment 22

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 6: Add marionette test case. r=btseng

Comment 23

[Samael Wang [:freesamael] (away for now)]

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?

Comment 24

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

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_*.

Comment 25

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

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.
 */

Comment 26

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

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.
 */

Comment 27

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

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;

Comment 28

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

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!

Comment 29

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

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!

Comment 30

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

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 31

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

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!

Comment 32

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 1(v3): Add "telephony-tty-mode-changed" system message. r=btseng

Comment 33

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 2(v3): Add ttyMode attribute to nsITelephonyAudioService. r=btseng

Comment 34

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 3(v3): Add ttyMode attribute to nsITelephonyService. r=btseng

Comment 35

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 4(v3): Update IPC implementation for nsITelephony change. r=btseng

Comment 36

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 7: Add mochitest test case

Comment 37

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

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.

Comment 38

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 5(v4): Add ttyMode attribute to Telephony Web IDL. r=htsai, btseng

Comment 39

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8647452 [details] [diff] [review]
Part 7: Add mochitest test case

Hi Bevis,

Could you help to review the mochitest test case?

Comment 40

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

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()

Comment 41

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 7: Add mochitest test case

Comment 42

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 7(v2): Add mochitest test case

Comment 43

[Samael Wang [:freesamael] (away for now)]

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?

Comment 44

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

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. :)

Comment 45

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 7(v3): Add mochitest test case. r=btseng

Comment 46

[Samael Wang [:freesamael] (away for now)]

try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7f783c73e21

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/db05fc4b7cc6
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/bb095614315b
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/dc0a98a32279
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/b4d62ac206c4
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/9dda4a47b582
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/1599f550f86b
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/73843469b540