Closed Bug 855570 Opened 11 years ago Closed 11 years ago

Deprecate Audio Data API

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23
Tracking Status
firefox22 --- fixed
firefox23 --- fixed
firefox24 --- fixed

People

(Reporter: kinetik, Assigned: kinetik)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(3 files, 2 obsolete files)

Once our Web Audio implementation has MediaElementAudioSourceNode we can deprecate all of the old Audio Data API.
Attached patch patch v0 (obsolete) — Splinter Review
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 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.
Oh, and also, you should make these deprecation warnings conditional on whether the media.webaudio.enabled pref is true.
Attached patch patch v1Splinter Review
(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.
Attachment #730472 - Attachment is obsolete: true
Attachment #732122 - Flags: review?(ehsan)
(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 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
Attachment #732122 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/1aee691b14b7
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Keywords: dev-doc-needed
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
Keywords: site-compat
(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!
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?
Flags: needinfo?(kinetik)
Attached patch followup v0Splinter Review
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.
Attachment #751539 - Flags: review?(ehsan)
Flags: needinfo?(kinetik)
Then will beta users be warned while the alternative is not available? I don't think it makes sense.
>+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".
Attached patch followup v1 (obsolete) — Splinter Review
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.
Attachment #751539 - Attachment is obsolete: true
Attachment #751539 - Flags: review?(ehsan)
Attachment #751565 - Flags: review?(ehsan)
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.
Attachment #751565 - Flags: review?(ehsan) → review-
Attachment #751539 - Attachment is obsolete: false
Attachment #751539 - Flags: review+
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.
Attachment #751539 - Flags: approval-mozilla-beta?
Attachment #751539 - Flags: approval-mozilla-aurora?
Attachment #751539 - Flags: approval-mozilla-beta?
Attachment #751539 - Flags: approval-mozilla-beta+
Attachment #751539 - Flags: approval-mozilla-aurora?
Attachment #751539 - Flags: approval-mozilla-aurora+
Attachment #751565 - Attachment is obsolete: true
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.
Attachment #732122 - Flags: approval-mozilla-beta?
Attachment #751565 - Attachment description: followup v0 → followup v1
Attachment #732122 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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).
Flags: needinfo?(akeybl)
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.
Flags: needinfo?(lsblakk)
Flags: needinfo?(bbajaj)
(And sorry for my mistake here.)
We can just hardcode the English string for Aurora/Beta.
(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
OK, I can hardcode the string too, just waiting for confirmation from RelMan on what to do here.
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 :)
Flags: needinfo?(akeybl)
(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.
Flags: needinfo?(lsblakk)
Flags: needinfo?(bbajaj)
This is hackish but it's a one time fix.  Boris, do you mind giving this a quick review?  Thanks!
Attachment #753562 - Flags: review?(bzbarsky)
(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 on attachment 753562 [details] [diff] [review]
Hardcode the string on beta

r=me
Attachment #753562 - Flags: review?(bzbarsky) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: