Last Comment Bug 855570 - Deprecate Audio Data API
: Deprecate Audio Data API
Status: RESOLVED FIXED
: dev-doc-complete, site-compat
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla23
Assigned To: Matthew Gregan [:kinetik]
:
Mentors:
Depends on: 855568
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-27 18:35 PDT by Matthew Gregan [:kinetik]
Modified: 2013-06-24 23:30 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
fixed


Attachments
patch v0 (3.71 KB, patch)
2013-03-27 18:37 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Review
patch v1 (4.87 KB, patch)
2013-04-01 16:33 PDT, Matthew Gregan [:kinetik]
ehsan: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Review
followup v0 (2.78 KB, patch)
2013-05-19 20:14 PDT, Matthew Gregan [:kinetik]
ehsan: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Review
followup v1 (4.45 KB, patch)
2013-05-19 22:45 PDT, Matthew Gregan [:kinetik]
ehsan: review-
Details | Diff | Review
Hardcode the string on beta (5.29 KB, patch)
2013-05-23 17:39 PDT, :Ehsan Akhgari (busy, don't ask for review please)
bzbarsky: review+
Details | Diff | Review

Description Matthew Gregan [:kinetik] 2013-03-27 18:35:46 PDT
Once our Web Audio implementation has MediaElementAudioSourceNode we can deprecate all of the old Audio Data API.
Comment 1 Matthew Gregan [:kinetik] 2013-03-27 18:37:00 PDT
Created attachment 730472 [details] [diff] [review]
patch v0
Comment 2 :Ehsan Akhgari (busy, don't ask for review please) 2013-03-30 15:02:18 PDT
Why do we want to wait for that?  Our current Web Audio implementation should be able to do everything that the old Audio Data API does, right?
Comment 3 :Ehsan Akhgari (busy, don't ask for review please) 2013-03-30 15:03:07 PDT
Comment on attachment 730472 [details] [diff] [review]
patch v0

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

::: dom/locales/en-US/chrome/dom/dom.properties
@@ +133,5 @@
>  # LOCALIZATION NOTE: Do not translate "X-Content-Security-Policy/Report-Only" or "Content-Security-Policy/Report-Only"
>  BothCSPHeadersPresent=This site specified both an X-Content-Security-Policy/Report-Only header and a Content-Security-Policy/Report-Only header. The X-Content-Security-Policy/Report-Only header(s) will be ignored.
>  # LOCALIZATION NOTE: Do not translate "NodeIterator" or "detach()".
>  NodeIteratorDetachWarning=Calling detach() on a NodeIterator no longer has an effect.
> +MozAudioDataWarning=The Mozilla Audio Data API is deprecated.  Please use the Web Audio API instead.

You probably want to add a LOCALIZATION NOTE here as well.
Comment 4 :Ehsan Akhgari (busy, don't ask for review please) 2013-03-31 16:00:25 PDT
Oh, and also, you should make these deprecation warnings conditional on whether the media.webaudio.enabled pref is true.
Comment 5 Matthew Gregan [:kinetik] 2013-04-01 16:33:29 PDT
Created attachment 732122 [details] [diff] [review]
patch v1

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> Why do we want to wait for that?  Our current Web Audio implementation
> should be able to do everything that the old Audio Data API does, right?

Don't we need MediaElementAudioSourceNode to replicate the functionality of the MozAudioAvailable event?  Or am I confused?

We don't have to wait, though.  It's probably better to discourage use of this API as soon as possible.
Comment 6 :Ehsan Akhgari (busy, don't ask for review please) 2013-04-02 08:00:23 PDT
(In reply to Matthew Gregan [:kinetik] from comment #5)
> Created attachment 732122 [details] [diff] [review]
> patch v1
> 
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> > Why do we want to wait for that?  Our current Web Audio implementation
> > should be able to do everything that the old Audio Data API does, right?
> 
> Don't we need MediaElementAudioSourceNode to replicate the functionality of
> the MozAudioAvailable event?  Or am I confused?

Nope, you're right, sorry, I ignored MozAudioAvailable!

> We don't have to wait, though.  It's probably better to discourage use of
> this API as soon as possible.

+1.  Er, make that +100.  :-)
Comment 7 :Ehsan Akhgari (busy, don't ask for review please) 2013-04-02 08:01:26 PDT
Comment on attachment 732122 [details] [diff] [review]
patch v1

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

::: dom/locales/en-US/chrome/dom/dom.properties
@@ +133,5 @@
>  # LOCALIZATION NOTE: Do not translate "X-Content-Security-Policy/Report-Only" or "Content-Security-Policy/Report-Only"
>  BothCSPHeadersPresent=This site specified both an X-Content-Security-Policy/Report-Only header and a Content-Security-Policy/Report-Only header. The X-Content-Security-Policy/Report-Only header(s) will be ignored.
>  # LOCALIZATION NOTE: Do not translate "NodeIterator" or "detach()".
>  NodeIteratorDetachWarning=Calling detach() on a NodeIterator no longer has an effect.
> +# LOCALIZATION NOTE: Do not translate "Mozilla Audio Data API" or "Web Audio API".

Nit: and
Comment 8 Matthew Gregan [:kinetik] 2013-04-02 17:34:18 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/1aee691b14b7
Comment 9 Ed Morley [:emorley] 2013-04-03 03:43:00 PDT
https://hg.mozilla.org/mozilla-central/rev/1aee691b14b7
Comment 10 Kohei Yoshino [:kohei] 2013-05-17 13:13:30 PDT
I've added this bug to the compatibility doc. Please correct the info if wrong.
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_23
Comment 12 :Ehsan Akhgari (busy, don't ask for review please) 2013-05-17 17:00:41 PDT
(In reply to comment #10)
> I've added this bug to the compatibility doc. Please correct the info if wrong.
> https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_23

Looks good, thanks!
Comment 13 :Ehsan Akhgari (busy, don't ask for review please) 2013-05-17 17:02:18 PDT
Hmm, actually thinking more about this, the deprecation messages being hidden behind the webaudio pref means that once 23 gets into beta, those messages will be hidden (as the pref is off by default there.)  Perhaps I was mistaken to ask you to hide it behind the pref.  What do you think, Matthew?
Comment 14 Matthew Gregan [:kinetik] 2013-05-19 20:14:19 PDT
Created attachment 751539 [details] [diff] [review]
followup v0

I think it makes sense to start warning that these APIs are going away as soon as possible, so let's remove the pref check.
Comment 15 Masatoshi Kimura [:emk] 2013-05-19 22:13:52 PDT
Then will beta users be warned while the alternative is not available? I don't think it makes sense.
Comment 16 Masatoshi Kimura [:emk] 2013-05-19 22:19:58 PDT
>+MozAudioDataWarning=The Mozilla Audio Data API is deprecated.  Please use the Web Audio API instead.

At least this very confusing message should be changed because beta users cannot "use the Web Audio API instead".
Comment 17 Matthew Gregan [:kinetik] 2013-05-19 22:45:50 PDT
Created attachment 751565 [details] [diff] [review]
followup v1

I think we'd want to warn people away from using this API even if Web Audio wasn't (becoming) available, because the Audio Data API is buggy and we have not been fixing those bugs.

I agree that we should remove the recommendation to use the Web Audio API from the warning if we land my follow up patch.  Updated patch attached.
Comment 18 :Ehsan Akhgari (busy, don't ask for review please) 2013-05-20 07:54:04 PDT
Comment on attachment 751565 [details] [diff] [review]
followup v1

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

Actually I think it makes sense for us to tell developers to switch to Web Audio, even if it's not available on beta, since it will be available there in less than 6 weeks anyway, and we're not removing the Moz Audio API quite yet, so they don't need to have a replacement today.  Also, changing localized strings means that we probably won't be able to take the patch on beta anyway, which would suck.

Let's take your other patch.
Comment 19 :Ehsan Akhgari (busy, don't ask for review please) 2013-05-20 07:56:05 PDT
Comment on attachment 751539 [details] [diff] [review]
followup v0

This patch just adds a warning to the web developer's console when using the deprecated moz audio API, and it has a very low risk of breaking anything, plus it has baked on m-c for a while.

I'd like to take this patch on both aurora and beta to start evangelizing people away from this API as soon as possible.
Comment 20 Matthew Gregan [:kinetik] 2013-05-20 18:21:54 PDT
Comment on attachment 732122 [details] [diff] [review]
patch v1

The initial patch only landed on 23, so if we want these warnings in beta we'll need to land this and the followup there.
Comment 22 Ryan VanderMeulen [:RyanVM] 2013-05-21 10:45:49 PDT
https://hg.mozilla.org/mozilla-central/rev/c06be9e2cf4f
Comment 23 Ryan VanderMeulen [:RyanVM] 2013-05-22 14:32:05 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/df58e8e3e743

With the follow-up patch folded in.
Comment 24 Francesco Lodolo [:flod] 2013-05-22 23:21:32 PDT
To be completely honest I don't believe that breaking string freeze on all repositories, even more for a warning, is a good idea (the new string wasn't even mentioned in the approval request).
Comment 25 :Ehsan Akhgari (busy, don't ask for review please) 2013-05-23 08:44:16 PDT
Ah, I thought that the string was already on beta... :(  Given the fact that it was not, I think we should back out https://hg.mozilla.org/releases/mozilla-beta/rev/df58e8e3e743.  CCing more RelMan folks.
Comment 26 :Ehsan Akhgari (busy, don't ask for review please) 2013-05-23 08:44:36 PDT
(And sorry for my mistake here.)
Comment 27 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-05-23 08:52:38 PDT
We can just hardcode the English string for Aurora/Beta.
Comment 28 Francesco Lodolo [:flod] 2013-05-23 08:57:35 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #27)
> We can just hardcode the English string for Aurora/Beta.

Considering the target of these warnings (developers), that would fine :-)

And the string is already on Aurora, so it's just a Beta problem
http://mxr.mozilla.org/l10n-mozilla-aurora/source/it/dom/chrome/dom/dom.properties#108
Comment 29 :Ehsan Akhgari (busy, don't ask for review please) 2013-05-23 11:06:09 PDT
OK, I can hardcode the string too, just waiting for confirmation from RelMan on what to do here.
Comment 30 Alex Keybl [:akeybl] 2013-05-23 11:58:37 PDT
a=akeybl on hardcoding the string for beta instead of introducing new strings

Sorry I missed the strings in the patch - you gotta call these things out :)
Comment 31 :Ehsan Akhgari (busy, don't ask for review please) 2013-05-23 17:09:25 PDT
(In reply to Alex Keybl [:akeybl] from comment #30)
> Sorry I missed the strings in the patch - you gotta call these things out :)

I own the blame entirely, as I was under the impression that the patch that added the strings _is_ already on Beta.
Comment 32 :Ehsan Akhgari (busy, don't ask for review please) 2013-05-23 17:39:04 PDT
Created attachment 753562 [details] [diff] [review]
Hardcode the string on beta

This is hackish but it's a one time fix.  Boris, do you mind giving this a quick review?  Thanks!
Comment 33 Matthew Gregan [:kinetik] 2013-05-23 17:51:52 PDT
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #31)
> (In reply to Alex Keybl [:akeybl] from comment #30)
> > Sorry I missed the strings in the patch - you gotta call these things out :)
> 
> I own the blame entirely, as I was under the impression that the patch that
> added the strings _is_ already on Beta.

I should have known better, I just haven't dealt with l10n issues on branches before and it never crossed my mind.  If I had filled out the proper approval form, it would have been explicit.  Sorry everyone!
Comment 34 Boris Zbarsky [:bz] 2013-05-23 18:58:41 PDT
Comment on attachment 753562 [details] [diff] [review]
Hardcode the string on beta

r=me
Comment 35 :Ehsan Akhgari (busy, don't ask for review please) 2013-05-23 19:34:10 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/e4456cef81be

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