Deprecate Audio Data API

RESOLVED FIXED in Firefox 22

Status

()

Core
Audio/Video
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kinetik, Assigned: kinetik)

Tracking

({dev-doc-complete, site-compat})

Trunk
mozilla23
dev-doc-complete, site-compat
Points:
---

Firefox Tracking Flags

(firefox22 fixed, firefox23 fixed, firefox24 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Once our Web Audio implementation has MediaElementAudioSourceNode we can deprecate all of the old Audio Data API.
(Assignee)

Comment 1

4 years ago
Created attachment 730472 [details] [diff] [review]
patch v0
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.
(Assignee)

Comment 5

4 years ago
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.
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+
(Assignee)

Comment 8

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1aee691b14b7
https://hg.mozilla.org/mozilla-central/rev/1aee691b14b7
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23

Updated

4 years ago
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
https://developer.mozilla.org/en-US/docs/Introducing_the_Audio_API_Extension
Keywords: dev-doc-needed → dev-doc-complete
(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)
(Assignee)

Comment 14

4 years ago
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.
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".
(Assignee)

Comment 17

4 years ago
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.
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?

Updated

4 years ago
Attachment #751539 - Flags: approval-mozilla-beta?
Attachment #751539 - Flags: approval-mozilla-beta+
Attachment #751539 - Flags: approval-mozilla-aurora?
Attachment #751539 - Flags: approval-mozilla-aurora+
(Assignee)

Updated

4 years ago
Attachment #751565 - Attachment is obsolete: true
(Assignee)

Comment 20

4 years ago
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?
(Assignee)

Comment 21

4 years ago
followup v0 landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c06be9e2cf4f
https://hg.mozilla.org/releases/mozilla-aurora/rev/45ab407e8747
(Assignee)

Updated

4 years ago
Attachment #751565 - Attachment description: followup v0 → followup v1
https://hg.mozilla.org/mozilla-central/rev/c06be9e2cf4f
status-firefox22: --- → affected
status-firefox23: --- → fixed
status-firefox24: --- → fixed

Updated

4 years ago
Attachment #732122 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/df58e8e3e743

With the follow-up patch folded in.
status-firefox22: affected → fixed
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)
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!
Attachment #753562 - Flags: review?(bzbarsky)
(Assignee)

Comment 33

4 years ago
(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+
https://hg.mozilla.org/releases/mozilla-beta/rev/e4456cef81be
Reflected status-firefox22 to the documents:
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_22
https://developer.mozilla.org/en-US/docs/Introducing_the_Audio_API_Extension
You need to log in before you can comment on or make changes to this bug.