Closed Bug 709565 Opened 8 years ago Closed 8 years ago

B2G telephony: implement DTMF

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: philikon, Assigned: qdot)

References

Details

(Whiteboard: [good first bug][lang=js][mentor=philikon])

Attachments

(1 file, 2 obsolete files)

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.
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 :)
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][mentor=philikon]
Assignee: nobody → kyle
Component: DOM → DOM: Device Interfaces
QA Contact: general → device-interfaces
Version: unspecified → Trunk
Attached patch First pass for B2G DTMF (obsolete) — Splinter Review
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.
Attachment #582403 - Flags: review?(philipp)
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.
Attachment #582403 - Attachment is obsolete: true
Attachment #582403 - Flags: review?(philipp)
Attachment #582418 - Flags: review?(philipp)
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.
Attachment #582418 - Flags: review?(philipp) → review+
nits picked, functions renamed. Landable.
Attachment #582418 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/7121c07c3e7f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.