Last Comment Bug 709565 - B2G telephony: implement DTMF
: B2G telephony: implement DTMF
Status: RESOLVED FIXED
[good first bug][lang=js][mentor=phil...
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla11
Assigned To: Kyle Machulis [:qdot]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: b2g-telephony
  Show dependency treegraph
 
Reported: 2011-12-11 03:21 PST by Philipp von Weitershausen [:philikon]
Modified: 2011-12-17 09:32 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First pass for B2G DTMF (7.25 KB, patch)
2011-12-16 15:09 PST, Kyle Machulis [:qdot]
no flags Details | Diff | Splinter Review
DTMF addition to RIL and DOM for B2G (7.33 KB, patch)
2011-12-16 15:44 PST, Kyle Machulis [:qdot]
philipp: review+
Details | Diff | Splinter Review
Final version of DTMF addition for B2G RIL (7.34 KB, patch)
2011-12-16 17:26 PST, Kyle Machulis [:qdot]
no flags Details | Diff | Splinter Review

Description Philipp von Weitershausen [:philikon] 2011-12-11 03:21:42 PST
This should be pretty easy:

* DOM message & parcel handling in ril_worker.js
* extending nsITelephone and its implementation, nsTelephonyWorker
* implement relevant WebTelephony API in our current throw-away DOM implementation, Telephony.js

If anybody wants to tackle this, poke me and I'll happily provide some guidance.
Comment 1 Philipp von Weitershausen [:philikon] 2011-12-11 03:22:44 PST
Oh, in case anybody is wondering what DTMF is: these are the tones the keypad makes when you're in a call. Very useful for operating, say, the Mozilla conference bridge :)
Comment 2 Kyle Machulis [:qdot] 2011-12-16 15:09:23 PST
Created attachment 582403 [details] [diff] [review]
First pass for B2G DTMF

First pass patch for DTMF. Tested via b2g-js-ril, RIL level functionality works. Still needs to line up with proposed telephony DOM api, doesn't actually work with DOM test yet.
Comment 3 Kyle Machulis [:qdot] 2011-12-16 15:44:28 PST
Created attachment 582418 [details] [diff] [review]
DTMF addition to RIL and DOM for B2G

Implements startTone and stopTone functions for the Telephony API. sendTones not yet implemented because I'm not quite sure how the timing would work, but that can probably be punted.
Comment 4 Philipp von Weitershausen [:philikon] 2011-12-16 16:49:14 PST
Comment on attachment 582418 [details] [diff] [review]
DTMF addition to RIL and DOM for B2G

>@@ -356,6 +356,14 @@ Telephony.prototype = {
>     return call;
>   },
> 
>+  startTone: function startTone(dtmfChar) {
>+	this.telephone.startTone(dtmfChar);
>+  },
>+
>+  stopTone: function stopTone() {
>+	this.telephone.stopTone();
>+  },

Nit: two space indentation, not four.

(you might want to adjust your default values for js2-mode.)

>@@ -685,6 +685,30 @@ let RIL = {
>   },
> 
>   /**
>+   * Start a DTMF Tone.
>+   *
>+   * @param dtmfChar
>+   *        DTMF signal to send, 0-9, *, +
>+   */
>+
>+  startDTMF: function startDTMF(dtmfChar) {
>+	Buf.newParcel(REQUEST_DTMF_START);
>+	Buf.writeString(dtmfChar);
>+	Buf.sendParcel();
>+  },
>+
>+  stopDTMF: function startDTMF() {
>+	Buf.newParcel(REQUEST_DTMF_STOP);
>+	Buf.sendParcel();
>+  },

Buf.simpleRequest(REQUEST_DTMF_STOP);

>+  sendDTMF: function sendDTMF(dtmfChar) {
>+	Buf.newParcel(REQUEST_DTMF);
>+	Buf.writeString(dtmfChar);
>+	Buf.sendParcel();
>+  },

Nit: indentation.

>@@ -832,7 +856,9 @@ RIL[REQUEST_OPERATOR] = function REQUEST_OPERATOR(length) {
>   Phone.onOperator(operator);
> };
> RIL[REQUEST_RADIO_POWER] = null;
>-RIL[REQUEST_DTMF] = null;
>+RIL[REQUEST_DTMF] = function REQUEST_DTMF() {
>+  Phone.onDTMFSend();
>+};
...
>+RIL[REQUEST_DTMF_START] = function REQUEST_DTMF_START() {
>+  Phone.onDTMFStart();
>+};
>+RIL[REQUEST_DTMF_STOP] = function REQUEST_DTMF_STOP() {
>+  Phone.onDTMFStop();
>+};
...
>@@ -1270,11 +1300,19 @@ let Phone = {
>   onSetMute: function onSetMute() {
>   },
> 
>+  onDTMFSend: function onDTMFSend() {
>+  },
>+
>+  onDTMFStart: function onDTMFStart() {
>+  },
>+
>+  onDTMFStop: function onDTMFStop() {
>+  },

For now the convention has been that if RIL.fooBar() method, the response will call Phone.onFooBar(). Your RIL methods above are called {start|stop|send}DTMF(), so I would prefer if these were called on{Start|Stop|Send}DTMF() as well.

>@@ -1310,6 +1348,33 @@ let Phone = {
>   },
> 
>   /**
>+   * Send DTMF Tone
>+   *
>+   * @param dtmfChar
>+   *        String containing the DTMF signal to send.
>+   */
>+  sendTone: function sendTone(options) {
>+    RIL.sendDTMF(options.dtmfChar);
>+  },
>+
>+  /**
>+   * Start DTMF Tone
>+   *
>+   * @param dtmfChar
>+   *        String containing the DTMF signal to send.
>+   */
>+  startTone: function startTone(options) {
>+    RIL.startDTMF(options.dtmfChar);
>+  },
>+
>+  /**
>+   * Stop DTMF Tone
>+   */
>+  stopTone: function stopTone() {
>+    RIL.stopDTMF();
>+  },
>+
>+  /**

Up until now, the methods on Phone mirror the names of the corresponding methods on RIL. I see no reason to break this convention. Makes it easier to remember how stuff is wired up.

Rest looks good! r=me with nits + conventions addressed.
Comment 5 Kyle Machulis [:qdot] 2011-12-16 17:26:40 PST
Created attachment 582440 [details] [diff] [review]
Final version of DTMF addition for B2G RIL

nits picked, functions renamed. Landable.
Comment 6 Philipp von Weitershausen [:philikon] 2011-12-16 18:09:59 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/7121c07c3e7f
Comment 7 Matt Brubeck (:mbrubeck) 2011-12-17 09:32:02 PST
https://hg.mozilla.org/mozilla-central/rev/7121c07c3e7f

Note You need to log in before you can comment on or make changes to this bug.