722841 - WebTelephony: implement navigator.mozTelephony.sendTones()

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[John Hammink]

from https://github.com/jonallengriffin/B2G
pulled and merged approx 11:59 a.m. p.s.t 1/31/2012
Samsung Galaxy S2 with working SIM card installed.

DTMF tones don't work correctly on IVR menu

Before test:  Made control call to testcall.com on my working android mobile to verify that it actually DOES recognize DTMF.  It does.

To repro:
1. Dial a number such as that of testcall.com [phone]
2. Verify also that IVR can verify the DTMF Tones from the B2G phone dialer.

NOTE: DTMF tones are not recognized by testcall IVR.

Comment 1

[Philipp von Weitershausen [:philikon]]

Bug 674726 never implemented navigator.mozTelephony.sendTones():

NS_IMETHODIMP
Telephony::SendTones(const nsAString& aTones, PRUint32 aToneDuration,
                     PRUint32 aIntervalDuration)
{
  NS_NOTYETIMPLEMENTED("Implement me!");
  return NS_ERROR_NOT_IMPLEMENTED;
}

It should be plumbed through from nsIRadioInterfaceLayer onwards.

Comment 2

[Shian-Yow Wu [:swu]]

I'd like to take this bug.

Comment 3

[Shian-Yow Wu [:swu]]

Attachment: Support sendTones() V1

To make DTMF tone during a call working, it also requires Gaia implementation at dialer.js.
It can be implemented by either:
1. Calling startTone() at keyDown and stopTone() at keyUp.
2. Calling sendTones() at keyDown, passing only one DTMF tone.

This first patch supports sendTones() by sending one tone with fixed duration.

The current ril_worker.js already implemented sending one tone by sendTone(), as it seems RIL supports only sending one tone with fixed duration(by REQUEST_DTMF).
I'm thinking how to send multiple tones with a requested duration, maybe to handle it in RIL layer by REQUEST_DTMF_START and REQUEST_DTMF_STOP.

Any comments are welcome.  Thank you very much!  :)

Comment 4

[Philipp von Weitershausen [:philikon]]

Comment on attachment 596273 [details] [diff] [review]
Support sendTones() V1

Nice first patch!

># HG changeset patch
># Parent d71dab82fff4325584406ae0ffac3ba89a73f13e
># User Shian-Yow Wu <[e-mail]>
>First patch to support navigator.mozTelephony.sendTones()

Please follow the mozilla-central commit msg convention:

  Bug NNN - bug summary. r=reviewer

(See https://developer.mozilla.org/En/Developer_Guide/Committing_Rules_and_Responsibilities)

>diff --git a/dom/system/b2g/nsIRadioInterfaceLayer.idl b/dom/system/b2g/nsIRadioInterfaceLayer.idl
>--- a/dom/system/b2g/nsIRadioInterfaceLayer.idl
>+++ b/dom/system/b2g/nsIRadioInterfaceLayer.idl
>@@ -153,16 +153,17 @@ interface nsIRadioInterfaceLayer : nsISu
>   /**
>    * Functionality for making and managing phone calls.
>    */
>   void dial(in DOMString number);
>   void hangUp(in unsigned long callIndex);
> 
>   void startTone(in DOMString dtmfChar);
>   void stopTone();
>+  void sendTone(in DOMString dtmfChar);

When changing an interface, you also need to give it a new UUID. (firebot can generate one for you.) r- from me for this.

>diff --git a/dom/telephony/Telephony.cpp b/dom/telephony/Telephony.cpp
>--- a/dom/telephony/Telephony.cpp
>+++ b/dom/telephony/Telephony.cpp
>@@ -353,18 +353,29 @@ Telephony::StopTone()
> 
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> Telephony::SendTones(const nsAString& aTones, PRUint32 aToneDuration,
>                      PRUint32 aIntervalDuration)
> {
>-  NS_NOTYETIMPLEMENTED("Implement me!");
>-  return NS_ERROR_NOT_IMPLEMENTED;
>+  if (aTones.IsEmpty()) {
>+    NS_WARNING("Empty tone string will be ignored");
>+    return NS_OK;
>+  }
>+
>+  if (aTones.Length() > 1) {
>+    return NS_ERROR_INVALID_ARG;
>+  }

The method and parameter name is plural ("tones" rather than "tone"), which leads me to think that multiple tones should be supported. But I wonder for what use case...

It seems "sendTones()" method as proposed in the WebTelephony API isn't actually very useful in reality. A "real" phone starts sending the phone when you touch the button and stops the tone when you let go of the button. So the length of the tone actually depends on how long you were holding the button. This doesn't seem currently implementable with the "sendTones()" API. We might want to consider exposing the start/stop tone functionality instead. Asking bent for some feedback on this.

Comment 5

[Philipp von Weitershausen [:philikon]]

(In reply to Philipp von Weitershausen [:philikon] from comment #4)
> We might want to consider exposing the start/stop tone functionality
> instead. Asking bent for some feedback on this.

So, uh, I forgot that there was actually navigator.mozTelephony.startTone()/stopTone(), which renders this point moot. Still I wonder about the point of sendTones(), hoping bent can clarify.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

The idea was to be able to do something like this:

  sendTones("*2,12345,2");

This lets people program a series of dial tones that need to be sent when calling automated numbers or something (not sure if you can still do this on smart phones, but you used to be able to with most programmable land-line handsets).

This would send the tones for "*" with some standard duration, pause very briefly, then send "2", then pause for some standard number of seconds (the ","). Then it would send "1", "2", "3", "4", "5", pause again, and then send "2".

It's entirely possible for the page to do this on its own, of course, with timeouts and startTone/stopTone. Seems to me that there is value in having a built-in way to do this, but I'm not sure how much value really. We could punt and remove this for the time being if others disagree.

Comment 7

[Shian-Yow Wu [:swu]]

Thanks for review comments from philikon and clarification from bent.  :)

There is one RIL command "REQUEST_CDMA_BURST_DTMF" should be what we need.  

/**
 * RIL_REQUEST_CDMA_BURST_DTMF
 *
 * Send DTMF string
 *
 * "data" is const char **
 * ((const char **)data)[0] is a DTMF string
 * ((const char **)data)[1] is the DTMF ON length in milliseconds, or 0 to use
 *                          default
 * ((const char **)data)[2] is the DTMF OFF length in milliseconds, or 0 to use
 *                          default
 *
 * "response" is NULL
 *
 * Valid errors:
 *  SUCCESS
 *  RADIO_NOT_AVAILABLE
 *  GENERIC_FAILURE
 *
 */

However, RIL is proprietary and provided as binary blob by device vendors, it still depends on whether the device implemented this command.  On SGS2, this command is supported, but seems it always ignores the tone & interval duration parameters and uses 500ms for each by default, it means sending a DTMF string "1234567890" would take about 10 seconds.  Not sure about the result on other devices.

Implementing by this command provides a working but not perfect result at least on SGS2 phone.  I think we can implement sendTones() this way for at this moment, and leave the built-in support status to be platform dependent issue.  Will do it this way if no other comments.

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

BTW, this did get implemented, in bug 709565. I think it got stomped when we moved telephony back to C++.

Comment 9

[Shian-Yow Wu [:swu]]

Currently startTone()/stopTone()/sendTone() were implemented in ril_worker.js, and telephony.cpp implemented startTone()/stopTone().

This bug requires implementation of sendTones().  So we need another implementation of sendTones() in ril_worker.js and telephony.cpp.

Comment 10

[Mozilla RelEng Bot]

Try run for cbaf30163c8d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=cbaf30163c8d
Results (out of 209 total builds):
    success: 170
    warnings: 23
    failure: 14
    other: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-cbaf30163c8d

Comment 11

[Philipp von Weitershausen [:philikon]]

(In reply to ben turner [:bent] from comment #6)
> The idea was to be able to do something like this:
> 
>   sendTones("*2,12345,2");
> 
> This lets people program a series of dial tones that need to be sent when
> calling automated numbers or something (not sure if you can still do this on
> smart phones, but you used to be able to with most programmable land-line
> handsets).
> 
> This would send the tones for "*" with some standard duration, pause very
> briefly, then send "2", then pause for some standard number of seconds (the
> ","). Then it would send "1", "2", "3", "4", "5", pause again, and then send
> "2".

Where are the lengths of tones and pauses defined, and the valid characters for the `sendTones()` string argument? It seems to me the draft proposal is lacking some details here (and sadly not just here.)

> It's entirely possible for the page to do this on its own, of course, with
> timeouts and startTone/stopTone. Seems to me that there is value in having a
> built-in way to do this, but I'm not sure how much value really. We could
> punt and remove this for the time being if others disagree.

I'm not sure I even understand the use case for this. Where in mobile phone functionality does this come up?

(In reply to Shian-Yow Wu from comment #9)
> This bug requires implementation of sendTones().  So we need another
> implementation of sendTones() in ril_worker.js and telephony.cpp.

I'm not sure about the "need" here. I think it makes more sense to punt on `sendTones()` for the time being. Dialer apps can use `startTone()` and `stopTone()` to implement DTMF tones quite easily.

Unless I hear objections, I'm going to close this bug as WONTFIX.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

The use case is wanting to have a series of tones that can be programed ahead of time and then replayed during a phone call. For example, say I was to call my bank's automated telephone system. Every time the prompt is the same ("press 1 for english", then a pause, then "press 4 for account balance", pause, "enter the last 4 digits of your social security number", pause, etc.). Rather than doing this manually all the time some phones have the ability to program this in and execute it automatically. Then I would just call the bank and press some key to have the prompts automatically answered. Make sense?

We can do this later or not at all, sure. As I said, it's possible for the dialer to do this already.

Comment 13

[Philipp von Weitershausen [:philikon]]

(In reply to ben turner [:bent] from comment #12)
> Rather than doing this manually all the time some phones have
> the ability to program this in and execute it automatically.

I did not know this.

> Then I would
> just call the bank and press some key to have the prompts automatically
> answered. Make sense?

Yes.

> We can do this later or not at all, sure. As I said, it's possible for the
> dialer to do this already.

Right. If I were to prioritize this, it would be pretty low on my list, IMHO. I'd rather take error events (bug 717462). I'm resolving this as WONTFIX for now. We can always reopen if we decide to come back to it.

Shian-Yow, thanks for the patch, it's very nice, but given the above discussion, I don't think we want to go forward with it. As you noted, the biggest missing piece for DTMF tones at this point is in Gaia and I see you've already filed https://github.com/andreasgal/gaia/issues/406 for that. If you would like to work on something else RIL/Telephony-related (e.g. bug 717462), you're more than welcome to. At least now you know your way around the code :)

Comment 14

[David Garcia [:davidg][retired]]

It would be nice if implementing this could be reconsidered, as I think there are some cases in which the use of startTone/stopTone is not the best option or even not an option at all.
For example in bug 911055, we need to send a series of DTMF tones of certain duration (300 ms). The problem is that you have to use setTimeout to accurately call stopTone at an specific time so the tone has the right duration, and also to wait certain time before calling the next startTone. This can lead to inaccurate tone timing, for example when the application process goes to background, setTimeout callback will not be called in less than 1 second, so you will not be able to send tones shorter than a second using startTone/setTimeout/stopTone.

HsinYi, do you think it makes sense to reopen this bug?

Comment 15

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

Yes, as I mentioned in bug 911055, reopening this bug makes sense to me. Thanks.

Comment 16

[Mike Hoye [:mhoye]]

Removing Good First Bug status; happy to re-add it if the expected outcome is clarified.

Comment 17

[David Garcia [:davidg][retired]]

One question about the signature of the method.
The draft [1] says startTone and sendTones should take a ToneOptions parameter

dictionary ToneOptions {
    unsigned long duration;
    unsigned long gap;
    DOMString     serviceId;
};

But startTone is not implemented that way. It takes a tone and a serviceId.
Should I do something similar with sendTones? Something like:

sendTones(tones, duration, gap, serviceId);

What do you think Hsin-Yi?

[1] http://www.w3.org/2012/sysapps/telephony/#idl-def-ToneOptions

Comment 18

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

(In reply to David Garcia [:davidg] from comment #17)
> One question about the signature of the method.
> The draft [1] says startTone and sendTones should take a ToneOptions
> parameter
> 
> dictionary ToneOptions {
>     unsigned long duration;
>     unsigned long gap;
>     DOMString     serviceId;
> };
> 
> But startTone is not implemented that way. It takes a tone and a serviceId.
> Should I do something similar with sendTones? Something like:
> 
> sendTones(tones, duration, gap, serviceId);
> 
> What do you think Hsin-Yi?
> 
> [1] http://www.w3.org/2012/sysapps/telephony/#idl-def-ToneOptions

Hi David,

Thanks for taking this bug :)

Per previous discussion, I thought Dialer would send one tone at a time to gecko but not a series. According to the latest comment of bug 911055, I am wondering if we need to have an API for sending a series of tones.
Have you seen any problem if sending tones one by one? Could you please elaborate more if I miss something? Thank you!

Comment 19

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

Anshul,

Our partner is trying to make this happen in v2.2. I am looping you in cc so that you won't miss ril interface change. :)

Comment 20

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

Anshul,

Our partner is trying to make this happen in v2.2. I am looping you in cc so that you won't miss ril interface change. :)

Comment 21

[David Garcia [:davidg][retired]]

Hi Hsinyi,

NP. Thanks for having a look :)

The problem is that if you want to be precise on the duration of the tones, and the time gap between the tones, it is pretty hard to do it in GAIA.
So lets say you want to play a series of tones in gaia, each tone played for 300ms and 500ms between each tone, you could do something like:

telephony.startTone('1');
setTimeout(function() {
    telephony.stopTone();
    setTimeout(function() {
        // Keep playing the rest of the tones
    }, 500);
}, 300);

That is what I did for bug 911055. The problem is that setTimeout is not precise, and if the app goes to background (that was our problem), then the minimum setTimeout is 1000ms, so the code above doesn't work properly. We couldn't figure out a way to solve this problem in a clean way (maybe there is a better way, I don't know).

Then I checked the draft and saw the sendTones method was defined, and it did exactly what we needed.

By sending a single tone, do you mean something like sendTone instead of sendTones? The problem is that you still need to deal with the time gap between tones in GAIA, having the same problem I said above.
I personally prefer the way it is defined in the draft, because if you want to send a single tone, you still can call sendTones with just 1 character. 

Thanks!

Comment 22

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

(In reply to David Garcia [:davidg] from comment #21)
> Hi Hsinyi,
> 
> NP. Thanks for having a look :)
> 
> The problem is that if you want to be precise on the duration of the tones,
> and the time gap between the tones, it is pretty hard to do it in GAIA.
> So lets say you want to play a series of tones in gaia, each tone played for
> 300ms and 500ms between each tone, you could do something like:
> 
> telephony.startTone('1');
> setTimeout(function() {
>     telephony.stopTone();
>     setTimeout(function() {
>         // Keep playing the rest of the tones
>     }, 500);
> }, 300);
> 
> That is what I did for bug 911055. The problem is that setTimeout is not
> precise, and if the app goes to background (that was our problem), then the
> minimum setTimeout is 1000ms, so the code above doesn't work properly. We
> couldn't figure out a way to solve this problem in a clean way (maybe there
> is a better way, I don't know).
> 
> Then I checked the draft and saw the sendTones method was defined, and it
> did exactly what we needed.
> 
> By sending a single tone, do you mean something like sendTone instead of
> sendTones? The problem is that you still need to deal with the time gap
> between tones in GAIA, having the same problem I said above.
> I personally prefer the way it is defined in the draft, because if you want
> to send a single tone, you still can call sendTones with just 1 character. 
> 
> Thanks!

Your explanation makes sense to me. Let's do this way as you proposed. :)

Comment 23

[Anshul]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #20)
> Anshul,
> 
> Our partner is trying to make this happen in v2.2. I am looping you in cc so
> that you won't miss ril interface change. :)

Thank you Hsin-Yi. As always very much appreciate it.

Comment 24

[David Garcia [:davidg][retired]]

Attachment: patch to add sendTones

This patch adds sendTones method to mozTelephony.

I'm aware that the way I hardcoded default values for duration and gap in dom/telephony/Telephony.cpp, is probably not the best way to do it.
Where do you think is better to put this default values?

Comment 25

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

Comment on attachment 8501111 [details]
patch to add sendTones

Thanks for the patch, David, though the format is wrong. Please do follow [1] and [2] to submit a nice-orgnized patch, and ask for review again.

Quick comments for the patch, and please have them addressed in the next revision:
1) We should return Promise for sendTones
2) Define the default values for "gap" and "duration" in Telephony.webidl. [3] is an example. How are the default values determined, any reference or standard?
3) Use "let" instead of "var" in Telephony js code. 

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
[3] http://dxr.mozilla.org/mozilla-central/source/dom/webidl/AudioBufferSourceNode.webidl?from=AudioBufferSourceNode.webidl&case=true#24

Comment 26

[David Garcia [:davidg][retired]]

Attachment: WIP

Comment 27

[David Garcia [:davidg][retired]]

One question Hsin-Yi.
I have seen that when a method returns a promise it is defined the promise type. For example:

Promise<(TelephonyCall or DOMRequest)> dial(DOMString number, optional unsigned long serviceId);

In the case of the sendTones method, what do you think it make more sense? It should be a DOMRequest?

Thanks a lot!

Comment 28

[David Garcia [:davidg][retired]]

Another question :)

If a define the defaults in the WebIdl I still need to do this?

  if (aDuration.WasPassed()) {
    duration = aDuration.Value();
  } else {
    duration = 300;
  }

or maybe I can just do:

duration = aDuration.Value()

?

Thanks again

Comment 29

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

(In reply to David Garcia [:davidg] from comment #27)
> One question Hsin-Yi.
> I have seen that when a method returns a promise it is defined the promise
> type. For example:
> 
> Promise<(TelephonyCall or DOMRequest)> dial(DOMString number, optional
> unsigned long serviceId);
> 
> In the case of the sendTones method, what do you think it make more sense?
> It should be a DOMRequest?
> 
> Thanks a lot!

Promise is used for asynchronous operation. In this case, when sendTones succeeds, we don't need to return a specific data, so |Promise<void>| makes sense.

(In reply to David Garcia [:davidg] from comment #28)
> Another question :)
> 
> If a define the defaults in the WebIdl I still need to do this?
> 
>   if (aDuration.WasPassed()) {
>     duration = aDuration.Value();
>   } else {
>     duration = 300;
>   }
> 
> or maybe I can just do:
> 
> duration = aDuration.Value()
> 
> ?
> 
> Thanks again

I believe you don't need either .WasPassed() or .Value(). Use "aDuration" directly. 
(I haven't tried it yet ;) )

Comment 30

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #29)
> (In reply to David Garcia [:davidg] from comment #27)
> > One question Hsin-Yi.
> > I have seen that when a method returns a promise it is defined the promise
> > type. For example:
> > 
> > Promise<(TelephonyCall or DOMRequest)> dial(DOMString number, optional
> > unsigned long serviceId);
> > 
> > In the case of the sendTones method, what do you think it make more sense?
> > It should be a DOMRequest?
> > 
> > Thanks a lot!
> 
> Promise is used for asynchronous operation. In this case, when sendTones
> succeeds, we don't need to return a specific data, so |Promise<void>| makes
> sense.
> 

One thing deserves more discussion: when is Promise supposed to reject? My opinion is if any of a tone sending fails, the promise should reject. Makes sense?



> (In reply to David Garcia [:davidg] from comment #28)
> > Another question :)
> > 
> > If a define the defaults in the WebIdl I still need to do this?
> > 
> >   if (aDuration.WasPassed()) {
> >     duration = aDuration.Value();
> >   } else {
> >     duration = 300;
> >   }
> > 
> > or maybe I can just do:
> > 
> > duration = aDuration.Value()
> > 
> > ?
> > 
> > Thanks again
> 
> I believe you don't need either .WasPassed() or .Value(). Use "aDuration"
> directly. 
> (I haven't tried it yet ;) )

Comment 31

[David Garcia [:davidg][retired]]

Attachment: WIP

Comment 32

[Anshul]

Hsin-Yi, this bug is precisely the reason why I was thinking that the pause character as being discussed in bug 911055 should be handled by gecko so Gaia doesn't need to worry about it. What if there is more than one pause characters in the dial string (definitely possible); how is gaia going to know that the previous sendTones is finished before processing the second pause character?

Comment 33

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

Hi Anshul, since sendTones returns a promise, api user can simply deal with multiple pause characters by a chain of "then" handlers. The cases you mentioned are covered too. 

If we want to support "dial waiting" in the future, the api could support that, too. But if we parse the string in dial(), then the api isn't flexible enough to "dial waiting." In addition, that makes dial() much vague. For example, how do we express "a call was successfully made but the following dtmf tones weren't sent successfully" via the api itself? Do my concerns make sense to you?

Comment 34

[Albert [:albert]]

Attachment: Patch

Changes from comment 25

Comment 35

[Maria Angeles Oteo (:oteo)]

Wesley, Wilfred, we need someone to review the patch if Hsin-Yi is busy, we want to land this in master before Jan 12th (so it can be part of 2.2 branch) and part of my team will be in Holidays the next two weeks.
Could please help me to speed up the review? Thanks a lot

Comment 36

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

(In reply to Maria Angeles Oteo (:oteo) from comment #35)
> Wesley, Wilfred, we need someone to review the patch if Hsin-Yi is busy, we
> want to land this in master before Jan 12th (so it can be part of 2.2
> branch) and part of my team will be in Holidays the next two weeks.
> Could please help me to speed up the review? Thanks a lot

I'll do the review today.

Comment 37

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

Comment on attachment 8537150 [details] [diff] [review]
Patch

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

Thanks for the patch, Albert.
I still have some questions about the API design. Please see my comments below, thank you.

::: dom/telephony/Telephony.cpp
@@ +388,5 @@
> +  }
> +
> +  if (aDTMFChars.IsEmpty()) {
> +    NS_WARNING("Empty tone string will be ignored");
> +    return nullptr;

We should |return promise.reject();|.

Please do:
promise->MaybeReject(NS_ERROR_DOM_INVALID_ACCESS_ERR);
return promise.forget();

@@ +393,5 @@
> +  }
> +
> +  if (!IsValidServiceId(serviceId)) {
> +    aRv.Throw(NS_ERROR_INVALID_ARG);
> +    return nullptr;

ditto.

::: dom/telephony/gonk/TelephonyService.js
@@ +893,5 @@
>        this._sendToRilWorker(aClientId, "hangUp", { callIndex: aCallIndex });
>      }
>    },
>  
> +  sendTones: function(aClientId, aDtmfChars, duration, gap, aCallback) {

s/duration/aDuration/  and s/gap/aGap/

@@ +894,5 @@
>      }
>    },
>  
> +  sendTones: function(aClientId, aDtmfChars, duration, gap, aCallback) {
> +    var self = this;

Please use "let" instead of "var" in Telephony gecko code. And in this case, we usually use |.bind()| in gecko.

@@ +899,5 @@
> +
> +    function playSingleTone(tone, cb) {
> +      self._sendToRilWorker(aClientId, "startTone",
> +                            { dtmfChar: tone });
> +      setTimeout(function() {

Please just use Ci.nsITimer (contractid: mozilla.org/timer;1), instead of Timer.jsm.

You may refer to [1].
[1] http://dxr.mozilla.org/mozilla-central/source/dom/telephony/gonk/TelephonyService.js?from=TelephonyService.js#171

@@ +902,5 @@
> +                            { dtmfChar: tone });
> +      setTimeout(function() {
> +        self._sendToRilWorker(aClientId, "stopTone");
> +        setTimeout(function() {
> +          cb(); 

nit: no trailing white space

@@ +903,5 @@
> +      setTimeout(function() {
> +        self._sendToRilWorker(aClientId, "stopTone");
> +        setTimeout(function() {
> +          cb(); 
> +        }, gap);

The implementation here is different from my understanding of the API design.
My understanding is, "gap" is used to indicate the "pause" duration between each series of tones, but not between each tone. For example, for the dialing string "00000,123,45", gap is the indication of the pause duration b/w the sub-strings "123" and "45." However, this is not what your code tries to achieve. Could you explain? Thank you.

@@ +913,5 @@
> +        playSingleTone(tones[0], function() {
> +          playTones(tones.substr(1));
> +        });
> +      } else {
> +        aCallback.notifySuccess();

In addition to "notifySuccess", we should also take care of "error" cases which will call "notifyError()"

::: dom/telephony/ipc/TelephonyIPCService.cpp
@@ +290,5 @@
> +                               uint32_t aDuration, uint32_t aGap,
> +                               nsITelephonyCallback *aCallback)
> +{
> +  return SendRequest(nullptr, aCallback,
> +            SendTonesRequest(aClientId, nsString(aDtmfChars), aDuration, aGap));

nit: align "SendTonesRequest" with "nullptr", and wrap a line at 80th character

::: dom/webidl/Telephony.webidl
@@ +28,5 @@
>    [Throws]
>    Promise<TelephonyCall> dialEmergency(DOMString number, optional unsigned long serviceId);
>  
>    [Throws]
> +  Promise<void> sendTones(DOMString tone, optional unsigned long duration = 300, optional unsigned long gap = 500, optional unsigned long serviceId);

s/tone/tones/

Could you point me out how the default values are determined here? Thank you.

Comment 38

[Albert [:albert]]

Attachment: Patch V2

Changes from comment 37

Comment 39

[Albert [:albert]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #37)
> Comment on attachment 8537150 [details] [diff] [review]
> Patch
> 
> Review of attachment 8537150 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/telephony/gonk/TelephonyService.js
> @@ +893,5 @@
> >        this._sendToRilWorker(aClientId, "hangUp", { callIndex: aCallIndex });
> >      }
> >    },
> >  
> > +  sendTones: function(aClientId, aDtmfChars, duration, gap, aCallback) {
> 
> s/duration/aDuration/  and s/gap/aGap/
> 

As talked offline aGap is not exposed to gaia, now is a constant. And I've added pauseDuration.

> 
> @@ +903,5 @@
> > +      setTimeout(function() {
> > +        self._sendToRilWorker(aClientId, "stopTone");
> > +        setTimeout(function() {
> > +          cb(); 
> > +        }, gap);
> 
> The implementation here is different from my understanding of the API design.
> My understanding is, "gap" is used to indicate the "pause" duration between
> each series of tones, but not between each tone. For example, for the
> dialing string "00000,123,45", gap is the indication of the pause duration
> b/w the sub-strings "123" and "45." However, this is not what your code
> tries to achieve. Could you explain? Thank you.
> 

There are three parameters involved.
- toneDuration: amount of time that a digit sound is being generated
- gap: interval of silence inserted between each successive DTMF character
- pauseDuration: silent interval when ',' char is present

I noticed that I was sending the ',' pause char and it was wrong. Now the ',' char makes a pause and then the following char is processed.

>
> ::: dom/webidl/Telephony.webidl
> @@ +28,5 @@
> >    [Throws]
> >    Promise<TelephonyCall> dialEmergency(DOMString number, optional unsigned long serviceId);
> >  
> >    [Throws]
> > +  Promise<void> sendTones(DOMString tone, optional unsigned long duration = 300, optional unsigned long gap = 500, optional unsigned long serviceId);
> 
> s/tone/tones/
> 
> Could you point me out how the default values are determined here? Thank you.

I used those values because David used them. I tried to find the standard values but no luck, only found that 3GPP specifies that pauses should be of 3000ms. However, after having a look to AOSP I see that android uses 2000ms. Now I use 2000ms because 3000ms seems very long.

Comment 40

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

Comment on attachment 8540670 [details] [diff] [review]
Patch V2

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

Hi Albert, 
thanks for the revision!

Two main concerns:
1) this patch still misses the error handling, please see [comment 1] inline for details
2) regarding ',' parsing, seems there's mis-communication b/w us over irc. please see [comment 2]

Thank you very much.

::: dom/telephony/gonk/TelephonyService.js
@@ +10,5 @@
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/Promise.jsm");
>  Cu.import("resource://gre/modules/systemlibs.js");
> +Cu.import("resource://gre/modules/Timer.jsm");

We don't need this.

@@ +47,5 @@
>  const DIAL_ERROR_BAD_NUMBER = RIL.GECKO_CALL_ERROR_BAD_NUMBER;
>  
>  const DEFAULT_EMERGENCY_NUMBERS = ["112", "911"];
>  
> +const TONES_GAP_DURATION = 300;

Per http://www.etsi.org/deliver/etsi_ts/101200_101299/10123502/01.01.01_60/ts_10123502v010101p.pdf, the minimum tones gap duration is 65ms. Let's use 70 for safety.

@@ +900,5 @@
> +                      aCallback) {
> +    let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +
> +    let playSingleTone = (function (tone, cb) {
> +      this.startTone(aClientId, tone);

[comment 1]
We need to consider error cases, notifyError, as I mentioned in my last review.

startTone could fail. Once it fails, we need to reject the promise. Please take care of the error handling. That means, you need to wait until the startTone callback comes from [1], then to either proceed to the next dtmf char or notifyError(), based on the callback result.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js&case=true#6127

@@ +909,5 @@
> +        }, TONES_GAP_DURATION, Ci.nsITimer.TYPE_ONE_SHOT);
> +      }.bind(this), aToneDuration, Ci.nsITimer.TYPE_ONE_SHOT);
> +    }).bind(this);
> +
> +    let playTones = function (tones) {

nit: no space between "function" and "("

@@ +911,5 @@
> +    }).bind(this);
> +
> +    let playTones = function (tones) {
> +      if (tones.length) {
> +        if (tones[0] == ',') {

[comment 2]
Hmm, I don't think we need to check ',' in gecko. I am sorry if I didn't express myself clearly enough when we chatted on IRC yesterday. We don't check ',' in gecko, but we should instead do that in gaia. That means, gaia application should NOT treat ',' being part of aDtmfChars.

The use case in mind looks like: 
For the dialing string "00000,123,,45", gaia will do:
telephony.dial("00000")
.then (() => waitForCallConnected();)
.then (() => telephony.sendTones("123",70,3000))
.then (() => telephony.sendTones("45",70, 6000))  // 6000 due to "2" pause characters

So, considering [comment 1] and [comment 2], the whole concept might look like below (the code might not be 100% correct but I'd like to show what I meant by code):

 sendTones: function(aClientId, aDtmfChars, aToneDuration, aPauseDuration, aCallback) {
     let tones = aDtmfChars;
     let playTone = (tone) => {
        this.startTone(aClientId, tone, () => {
           if (!response.success) {
             aCallback.notifyError(response.errorMsg);
             return;
           }
 
         timer.initWithCallback(() => {
            this.stopTone();
            timer.initWithCallback(() => {
              if (tones.length === 1) {
                aCallback.notifySuccess();
              } else {
                tones = tones.substr(1);
                playTone(tones[0]);
              }
            }, TONES_GAP_DURATION, Ci.nsITimer.TYPE_ONE_SHOT);
          }, aToneDuration, Ci.nsITimer.TYPE_ONE_SHOT);
        });
      };
  
      timer.initWithCallback(() => {
        playTone(tones[0]);
      }, aPauseDuration, Ci.nsITimer.TYPE_ONE_SHOT);
    },

::: dom/webidl/Telephony.webidl
@@ +28,5 @@
>    [Throws]
>    Promise<TelephonyCall> dialEmergency(DOMString number, optional unsigned long serviceId);
>  
>    [Throws]
> +  Promise<void> sendTones(DOMString tones, optional unsigned long toneDuration = 500, optional unsigned long pauseDuration = 2000, optional unsigned long serviceId);

Per http://www.etsi.org/deliver/etsi_ts/101200_101299/10123502/01.01.01_60/ts_10123502v010101p.pdf, minimum tone duration is 65ms. Let's use 70 ms as the default value of toneDuration.

Agree that 3000ms seems long ;) but let's keep what spec suggests for API. If UI requires a shorter value, Gaia could always easily adjust that. Thank you.

In addition, please also comment on the "unit" of duration.

Comment 41

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #40)
> Comment on attachment 8540670 [details] [diff] [review]
> Patch V2
> 
> Review of attachment 8540670 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Albert, 
> thanks for the revision!
> 
> Two main concerns:
> 1) this patch still misses the error handling, please see [comment 1] inline
> for details
> 2) regarding ',' parsing, seems there's mis-communication b/w us over irc.
> please see [comment 2]
> 
> Thank you very much.
> 
> ::: dom/telephony/gonk/TelephonyService.js
> @@ +10,5 @@
> >  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> >  Cu.import("resource://gre/modules/Services.jsm");
> >  Cu.import("resource://gre/modules/Promise.jsm");
> >  Cu.import("resource://gre/modules/systemlibs.js");
> > +Cu.import("resource://gre/modules/Timer.jsm");
> 
> We don't need this.
> 
> @@ +47,5 @@
> >  const DIAL_ERROR_BAD_NUMBER = RIL.GECKO_CALL_ERROR_BAD_NUMBER;
> >  
> >  const DEFAULT_EMERGENCY_NUMBERS = ["112", "911"];
> >  
> > +const TONES_GAP_DURATION = 300;
> 
> Per
> http://www.etsi.org/deliver/etsi_ts/101200_101299/10123502/01.01.01_60/
> ts_10123502v010101p.pdf, the minimum tones gap duration is 65ms. Let's use
> 70 for safety.
> 
> @@ +900,5 @@
> > +                      aCallback) {
> > +    let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> > +
> > +    let playSingleTone = (function (tone, cb) {
> > +      this.startTone(aClientId, tone);
> 
> [comment 1]
> We need to consider error cases, notifyError, as I mentioned in my last
> review.
> 
> startTone could fail. Once it fails, we need to reject the promise. Please
> take care of the error handling. That means, you need to wait until the
> startTone callback comes from [1], then to either proceed to the next dtmf
> char or notifyError(), based on the callback result.
> 
> [1]
> http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.
> js?from=ril_worker.js&case=true#6127

To be more clear, we need:
1) 
RilObject.prototype[REQUEST_DTMF_START] = function REQUEST_DTMF_START(length, options) {
  options.success = (options.rilRequestError === 0);
  if (!options.success) {
    options.errorMsg = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError];
  }
  this.sendChromeMessage(options);
};

2) to replace |Buf.newParcel(REQUEST_DTMF_START);| with |Buf.newParcel(REQUEST_DTMF_START, options);|

Comment 42

[Albert [:albert]]

Attachment: Patch V3

Changes from comment 40

Comment 43

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

Comment on attachment 8541267 [details] [diff] [review]
Patch V3

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

Looks good. I also played with this and patch on bug 911055. Everything works well, thank you.

r=me with comments addressed.

::: dom/telephony/gonk/TelephonyService.js
@@ +904,5 @@
> +        if (!response.success) {
> +          aCallback.notifyError(response.errorMsg);
> +          return;
> +        }
> + 

nit: no trailing white space

@@ +921,5 @@
> +    };
> +
> +    timer.initWithCallback(() => {
> +      playTone(tones[0]);
> +    }, aPauseDuration, Ci.nsITimer.TYPE_ONE_SHOT);  

nit: no trailing white space

::: dom/webidl/Telephony.webidl
@@ +28,5 @@
>    [Throws]
>    Promise<TelephonyCall> dialEmergency(DOMString number, optional unsigned long serviceId);
>  
>    [Throws]
> +  Promise<void> sendTones(DOMString tones, optional unsigned long pauseDuration = 3000 /* ms */, optional unsigned long toneDuration = 70 /* ms */, optional unsigned long serviceId);

Thanks for adding "ms"! But let's try to not have comments in an API function to have better readability.

Let's have below written in front of the function:
/**
  * Send a series of DTMF tones.
  *
  * @param tones
  *    DTMF chars.
  * @param pauseDuraton (ms) [optional]
  *    Time to wait before sending tones. Default value is 3000 ms.
  * @param toneDuration (ms) [optional]
  *    Duration of each tone. Default value is 70 ms.
  * @param serviceId [optional]
  *    Default value is as user setting dom.telephony.defaultServiceId.
  */

Comment 44

[Albert [:albert]]

Attachment: Patch V4

Fixed nits from comment 43

Comment 45

[Carsten Book [:Tomcat]]

can we get a try run here ?

Comment 46

[Beatriz Rodríguez [:brg]]

Antonio, could you please run the try after your holiday period? Thanks in advance.

Comment 47

[Antonio Manuel Amaya Calvo (:amac)]

No need to wait for my holidays to end. There you go: https://hg.mozilla.org/try/pushloghtml?changeset=821425cb0bc6

Comment 48

[Antonio Manuel Amaya Calvo (:amac)]

Keeping the NI to me so I remember to check the try results.

Comment 49

[Antonio Manuel Amaya Calvo (:amac)]

And of course, the link wasn't exactly helpful. Correct link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=821425cb0bc6

The try looks good to me. There are some failures but appear to be previously reported ones (I retriggered them just in case anyway, but flagged them already).

Re-requesting checkin.

Comment 50

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing against bug 1077075. Also, please use ./mach update-uuids for making UUID changes to make sure that any inheriting interfaces are also updated. In this case, nsIGonkTelephonyService will also be automatically updated if you do.

Comment 51

[Albert [:albert]]

Attachment: Patch V4

Rebased and updated uuids

Comment 52

[Albert [:albert]]

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=cc29a261c52b

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/841495120f82

Comment 54

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

https://hg.mozilla.org/mozilla-central/rev/841495120f82

Comment 55

[Chris Mills [:cmills]]

I've documented this method, at https://developer.mozilla.org/en-US/docs/Web/API/Telephony.sendTones. Could someone please give it a technical review?

Comment 56

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

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #55)
> I've documented this method, at
> https://developer.mozilla.org/en-US/docs/Web/API/Telephony.sendTones. Could
> someone please give it a technical review?

I could help. But I am busy on other stuff so it might take some time.
anyway, I flag myself for tracking.

Comment 57

[Chris Mills [:cmills]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #56)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #55)
> > I've documented this method, at
> > https://developer.mozilla.org/en-US/docs/Web/API/Telephony.sendTones. Could
> > someone please give it a technical review?
> 
> I could help. But I am busy on other stuff so it might take some time.
> anyway, I flag myself for tracking.

Thank you - I really appreciate it! It isn't super urgent, so whenever you have some time is good.

Comment 58

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

Mdn review done & some errors were corrected.

Comment 59

[Chris Mills [:cmills]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #58)
> Mdn review done & some errors were corrected.

cool, thanks!