Last Comment Bug 927852 - don't use Moz Audio API in the Gaia TonePlayer (dialer and system/emergency-call)
: don't use Moz Audio API in the Gaia TonePlayer (dialer and system/emergency-c...
Status: RESOLVED FIXED
:
Product: Firefox OS
Classification: Client Software
Component: Gaia::Dialer (show other bugs)
: unspecified
: x86 Mac OS X
-- normal (vote)
: 1.3 Sprint 4 - 11/8
Assigned To: Matthew Gregan [:kinetik]
:
:
Mentors:
Depends on: 942751 943138 961986 963220
Blocks: 927245
  Show dependency treegraph
 
Reported: 2013-10-17 06:31 PDT by Andreas Gal :gal
Modified: 2014-02-03 16:23 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
1.3+


Attachments
bug927852_v0.patch (9.64 KB, patch)
2013-10-23 20:52 PDT, Matthew Gregan [:kinetik]
etienne: feedback+
Details | Diff | Splinter Review
fix (46 bytes, text/x-github-pull-request)
2013-10-30 16:31 PDT, Matthew Gregan [:kinetik]
etienne: review+
Details | Review | Splinter Review
fix (46 bytes, text/x-github-pull-request)
2013-11-06 17:01 PST, Matthew Gregan [:kinetik]
kinetik: review+
Details | Review | Splinter Review

Description User image Andreas Gal :gal 2013-10-17 06:31:37 PDT
The Moz Audio API is deprecated and we should use Web Audio instead. We are about to drop Moz Audio API so we should replace this for 1.3.

apps/communications/dialer/js/keypad.js:    this._audio.mozSetup(1, this._sampleRate);
apps/system/emergency-call/js/keypad.js:   this._audio.mozSetup(2, this._sampleRate);
Comment 1 User image Etienne Segonzac (:etienne) 2013-10-20 09:43:38 PDT
An old WIP patch can be found here: https://github.com/etiennesegonzac/gaia/compare/web-audio-tone-player
Comment 2 User image Matthew Gregan [:kinetik] 2013-10-23 20:52:24 PDT
Created attachment 821455 [details] [diff] [review]
bug927852_v0.patch

Something like this?  Tested locally on desktop, not on actual phone hardware.
Comment 3 User image Etienne Segonzac (:etienne) 2013-10-25 07:30:42 PDT
(In reply to Matthew Gregan [:kinetik] from comment #2)
> Created attachment 821455 [details] [diff] [review]
> bug927852_v0.patch
> 
> Something like this?

Well yes, something *exactly* like this :)
Taking the liberty to assign this to you since it's pretty much done here.
Comment 4 User image Etienne Segonzac (:etienne) 2013-10-25 07:32:58 PDT
Comment on attachment 821455 [details] [diff] [review]
bug927852_v0.patch

* bug 922569 should introduce some unit-test coverage for the TonePlayer, which would help with adding some here
* it's probably a good time to put the TonePlayer in shared/ and have a little bit less duplication between the dialer and the system app

But those 2 things could be filed as follow up bug, depends on how much time you have :)
Comment 5 User image Matthew Gregan [:kinetik] 2013-10-30 16:29:39 PDT
Thanks Etienne, I've put a pull request up: https://github.com/mozilla-b2g/gaia/pull/13245

I don't have much time to spend on this (just want to kill the old audio API as soon as possible), so I'll leave your excellent suggestions for a followup bug.
Comment 6 User image Matthew Gregan [:kinetik] 2013-10-30 16:31:27 PDT
Created attachment 824973 [details] [review]
fix
Comment 7 User image Etienne Segonzac (:etienne) 2013-10-31 08:08:16 PDT
Follow up bug 933278 is open for the unit-tests.
Comment 8 User image Etienne Segonzac (:etienne) 2013-10-31 08:14:26 PDT
Comment on attachment 824973 [details] [review]
fix

r=me with a small nit:

the kShortPressDuration is 0.25 in the emergency call keypad, and 0.3 in the dialer.

We should probably use the same value in both places (I like the result better with 0.25 fwiw).
Comment 9 User image Matthew Gregan [:kinetik] 2013-11-03 10:41:47 PST
(In reply to Etienne Segonzac (:etienne) from comment #8)
> the kShortPressDuration is 0.25 in the emergency call keypad, and 0.3 in the
> dialer.
> 
> We should probably use the same value in both places (I like the result
> better with 0.25 fwiw).

That's what the original code had, so I kept it, but it makes sense to standardize on something.  I pushed a small followup that uses 0.25 everywhere.

I think this is ready to be merged now.  Since the pull request is already active, is there anything else I need to do?
Comment 10 User image Etienne Segonzac (:etienne) 2013-11-04 14:02:35 PST
(In reply to Matthew Gregan [:kinetik] from comment #9)
> I think this is ready to be merged now.  Since the pull request is already
> active, is there anything else I need to do?

Can you squash the commits into one? Thanks, then it's ready to merge.
Comment 11 User image Matthew Gregan [:kinetik] 2013-11-04 14:37:53 PST
(In reply to Etienne Segonzac (:etienne) from comment #10)
> Can you squash the commits into one? Thanks, then it's ready to merge.

Thanks, done!
Comment 12 User image Ryan VanderMeulen [:RyanVM] 2013-11-06 05:49:19 PST
Is there an updated pull request? I don't see a merge option on that one.
Comment 13 User image Matthew Gregan [:kinetik] 2013-11-06 17:00:54 PST
Comment on attachment 824973 [details] [review]
fix

Weird, maybe it's because I did a fixup + rebase and pushed?  Or because Travis-CI is horked?  It updated the pull request with the correct commits...

Anyway, I closed that and opened a new one: https://github.com/mozilla-b2g/gaia/pull/13450
Comment 14 User image Matthew Gregan [:kinetik] 2013-11-06 17:01:23 PST
Created attachment 828362 [details] [review]
fix
Comment 15 User image Matthew Gregan [:kinetik] 2013-11-06 17:39:33 PST
FYI, I landed the patch to disable the Audio Data API (bug 927245) on mozilla-inbound just now, so you'll need to merge this to have a working dialer.
Comment 17 User image Matthew Gregan [:kinetik] 2013-11-06 18:54:50 PST
Thanks!

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