AudioContext.decodeAudioData returns a Promise

AudioContext.decodeAudioData returns a Promise

See the spec: http://webaudio.github.io/web-audio-api/#widl-AudioContext-decodeAudioData-Promise-AudioBuffer--ArrayBuffer-audioData-DecodeSuccessCallback-successCallback-DecodeErrorCallback-errorCallback

A few other issues are also fixed:

No longer throw an error if the audio data arg is null.  This is supposed to reject the promise and also call the errorcallback.

The error callback is supposed to be invoked with the error. Previously, we just used null.

BUG=420107, 464130
TEST=decode-audio-data-promise.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193648

[Raymond Toy, 2015-03-20 20:19:54]

rtoy@chromium.org changed reviewers:
+ hongchan@chromium.org

[Raymond Toy, 2015-03-20 20:19:55]

PTAL

[hongchan, 2015-03-23 16:18:49]

LGTM with nits.

Just a suggestion: can pass & fail checks in |decode-audio-data-promise.html|
refactored? Not sure if it is possible but I am thinking about it.

https://codereview.chromium.org/1006963003/diff/40001/LayoutTests/webaudio/de...
File LayoutTests/webaudio/decode-audio-data-basic-expected.txt (right):

https://codereview.chromium.org/1006963003/diff/40001/LayoutTests/webaudio/de...
LayoutTests/webaudio/decode-audio-data-basic-expected.txt:1: CONSOLE ERROR: line
21: Uncaught (in promise) NotSupportedError: invalid ArrayBuffer for audioData.
We don't want this kind of error messages in the expected result. Do we?

https://codereview.chromium.org/1006963003/diff/40001/LayoutTests/webaudio/de...
File LayoutTests/webaudio/decode-audio-data-basic.html (right):

https://codereview.chromium.org/1006963003/diff/40001/LayoutTests/webaudio/de...
LayoutTests/webaudio/decode-audio-data-basic.html:21:
context.decodeAudioData(null,
I think we should wrap this with try & catch to avoid the console error message
in the result.

[Raymond Toy, 2015-03-23 18:44:57]

On 2015/03/23 16:18:49, hoch wrote:
> LGTM with nits.
> 
> Just a suggestion: can pass & fail checks in |decode-audio-data-promise.html|
> refactored? Not sure if it is possible but I am thinking about it.

You don't describe how to refactor, so there isn't really anything I can change.
:-(

> 
>
https://codereview.chromium.org/1006963003/diff/40001/LayoutTests/webaudio/de...
> File LayoutTests/webaudio/decode-audio-data-basic-expected.txt (right):
> 
>
https://codereview.chromium.org/1006963003/diff/40001/LayoutTests/webaudio/de...
> LayoutTests/webaudio/decode-audio-data-basic-expected.txt:1: CONSOLE ERROR:
line
> 21: Uncaught (in promise) NotSupportedError: invalid ArrayBuffer for
audioData.
> We don't want this kind of error messages in the expected result. Do we?
> 
>
https://codereview.chromium.org/1006963003/diff/40001/LayoutTests/webaudio/de...
> File LayoutTests/webaudio/decode-audio-data-basic.html (right):
> 
>
https://codereview.chromium.org/1006963003/diff/40001/LayoutTests/webaudio/de...
> LayoutTests/webaudio/decode-audio-data-basic.html:21:
> context.decodeAudioData(null,
> I think we should wrap this with try & catch to avoid the console error
message
> in the result.

[Raymond Toy, 2015-03-23 18:45:20]

https://codereview.chromium.org/1006963003/diff/40001/LayoutTests/webaudio/de...
File LayoutTests/webaudio/decode-audio-data-basic-expected.txt (right):

https://codereview.chromium.org/1006963003/diff/40001/LayoutTests/webaudio/de...
LayoutTests/webaudio/decode-audio-data-basic-expected.txt:1: CONSOLE ERROR: line
21: Uncaught (in promise) NotSupportedError: invalid ArrayBuffer for audioData.
On 2015/03/23 16:18:49, hoch wrote:
> We don't want this kind of error messages in the expected result. Do we?

I'm undecided.  I kind of didn't want to write decodeAudioData().then(), because
it's not required, even if promises are supported. But it's also annoying to
have that in the expected result.

[Raymond Toy, 2015-04-07 22:39:08]

rtoy@chromium.org changed reviewers:
+ haraken@chromium.org, tkent@chromium.org, yhirano@chromium.org

[Raymond Toy, 2015-04-07 22:39:09]

PTAL.

haraken@: For the oilpan related changes
yhirano@: For the Promise usage.
tkent@: As API owner

[tkent, 2015-04-07 23:24:48]

Public API change LGTM.  This was already approved in blink-dev.

https://codereview.chromium.org/1006963003/diff/60001/Source/modules/webaudio...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/1006963003/diff/60001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:115: , m_closedContextSampleRate(-1)
This member isn't initialized in another constructor.

[haraken, 2015-04-07 23:39:14]

https://codereview.chromium.org/1006963003/diff/60001/Source/modules/webaudio...
File Source/modules/webaudio/AsyncAudioDecoder.h (right):

https://codereview.chromium.org/1006963003/diff/60001/Source/modules/webaudio...
Source/modules/webaudio/AsyncAudioDecoder.h:59:
RefPtrWillBeMember<ScriptPromiseResolver> m_resolver;

Since AsyncAudioDecoder is not on-heap, this needs to be RefPtrWillBePersistent.

Just check that this CL compiles with enable_oilpan=1.

https://codereview.chromium.org/1006963003/diff/60001/Source/modules/webaudio...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/1006963003/diff/60001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:157: for (auto& resolver :
m_audioDecoderResolvers) {

This is unsafe. You're not allowed to touch other on-heap objects in a
destructor. Can we move this reject logic to AudioContext::uninitialize or
something like that?

[yhirano, 2015-04-08 03:19:27]

https://codereview.chromium.org/1006963003/diff/60001/Source/modules/webaudio...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/1006963003/diff/60001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:157: for (auto& resolver :
m_audioDecoderResolvers) {
On 2015/04/07 23:39:14, haraken wrote:
> 
> This is unsafe. You're not allowed to touch other on-heap objects in a
> destructor. Can we move this reject logic to AudioContext::uninitialize or
> something like that?
> 

Rejection is needed when deleting a resolver if 
 - the resolver is not resolved nor rejected, and
 - the execution context is not stopped.

In other words, even when m_audioDecoderResolvers is not empty, you don't have
to reject them if the execution context is already stopped (You need a comment
and / or assertion though).

I'm not sure if the above assumption holds: Just FYI

https://codereview.chromium.org/1006963003/diff/60001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:306:
m_audioDecoder.decodeAsync(audioData, rate, successCallback, errorCallback,
resolver.get(), this);
Just to confirm: Is this |resolver.get()| safe in non-oilpan environment? Does
AudioContext live long enough?

[Raymond Toy, 2015-04-08 16:47:11]

https://codereview.chromium.org/1006963003/diff/60001/Source/modules/webaudio...
File Source/modules/webaudio/AsyncAudioDecoder.h (right):

https://codereview.chromium.org/1006963003/diff/60001/Source/modules/webaudio...
Source/modules/webaudio/AsyncAudioDecoder.h:59:
RefPtrWillBeMember<ScriptPromiseResolver> m_resolver;
On 2015/04/07 23:39:14, haraken wrote:
> 
> Since AsyncAudioDecoder is not on-heap, this needs to be
RefPtrWillBePersistent.
> 
> Just check that this CL compiles with enable_oilpan=1.

Oops. This isn't actually used anywhere.  Deleting it.

https://codereview.chromium.org/1006963003/diff/60001/Source/modules/webaudio...
File Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/1006963003/diff/60001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:157: for (auto& resolver :
m_audioDecoderResolvers) {
On 2015/04/07 23:39:14, haraken wrote:
> 
> This is unsafe. You're not allowed to touch other on-heap objects in a
> destructor. Can we move this reject logic to AudioContext::uninitialize or
> something like that?
> 

uninitialize should work.

https://codereview.chromium.org/1006963003/diff/60001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:157: for (auto& resolver :
m_audioDecoderResolvers) {
On 2015/04/08 03:19:27, yhirano wrote:
> On 2015/04/07 23:39:14, haraken wrote:
> > 
> > This is unsafe. You're not allowed to touch other on-heap objects in a
> > destructor. Can we move this reject logic to AudioContext::uninitialize or
> > something like that?
> > 
> 
> Rejection is needed when deleting a resolver if 
>  - the resolver is not resolved nor rejected, and
>  - the execution context is not stopped.
> 
> In other words, even when m_audioDecoderResolvers is not empty, you don't have
> to reject them if the execution context is already stopped (You need a comment
> and / or assertion though).
> 
> I'm not sure if the above assumption holds: Just FYI

If this is moved to uninitialize(), then the execution context should still be
around because hasPendingActivity() is true until uninitialize is finished, at
which point hasPendingActivity() returns false.

https://codereview.chromium.org/1006963003/diff/60001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.cpp:306:
m_audioDecoder.decodeAsync(audioData, rate, successCallback, errorCallback,
resolver.get(), this);
On 2015/04/08 03:19:27, yhirano wrote:
> Just to confirm: Is this |resolver.get()| safe in non-oilpan environment? Does
> AudioContext live long enough?

I believe so.  As long as uninitialize() has not been called, the AudioContext
should still be alive.

[haraken, 2015-04-09 00:43:43]

Oilpan part LGTM.

[yhirano, 2015-04-10 04:55:23]

This is not a problem of this issue, but I think having a promise-capable test
infrastructure is great. See
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
for example.

https://codereview.chromium.org/1006963003/diff/100001/LayoutTests/webaudio/d...
File LayoutTests/webaudio/decode-audio-data-promise.html (right):

https://codereview.chromium.org/1006963003/diff/100001/LayoutTests/webaudio/d...
LayoutTests/webaudio/decode-audio-data-promise.html:129: p =
context.decodeAudioData(request.response,
context.decodeAudioData(request.response, function() {
  testPassed(...);
}, function() {
  testFailed(...);
}).then(function(buffer) {
  ...
}, then(function(e) {
  testFailed(...);
}).then(done, done);

https://codereview.chromium.org/1006963003/diff/100001/LayoutTests/webaudio/d...
LayoutTests/webaudio/decode-audio-data-promise.html:160: p =
context.decodeAudioData(request.response,
context.decodeAudioData(request.response, function() {
  testFailed(...);
}, function() {
  testPassed(...);
}).then(function() {
  testFailed(...);
}, function(e) {
  testPassed(...);
}).then(done, done);

https://codereview.chromium.org/1006963003/diff/100001/LayoutTests/webaudio/d...
LayoutTests/webaudio/decode-audio-data-promise.html:197: context.close()
You don't need nested |then|.

context.clonse().then(function() {
  return context.decodeAudioData(encodedAudioData);
}).then(function(b) {
  ...
}, function(e) {
  ...
}).then(done, done);

https://codereview.chromium.org/1006963003/diff/100001/Source/modules/webaudi...
File Source/modules/webaudio/AudioContext.h (right):

https://codereview.chromium.org/1006963003/diff/100001/Source/modules/webaudi...
Source/modules/webaudio/AudioContext.h:439:
WillBeHeapVector<RefPtrWillBeMember<ScriptPromiseResolver>>
m_audioDecoderResolvers;
Is using WillBeHeapHashSet better?

[Raymond Toy, 2015-04-10 20:54:57]

https://codereview.chromium.org/1006963003/diff/100001/Source/modules/webaudi...
File Source/modules/webaudio/AudioContext.h (right):

https://codereview.chromium.org/1006963003/diff/100001/Source/modules/webaudi...
Source/modules/webaudio/AudioContext.h:439:
WillBeHeapVector<RefPtrWillBeMember<ScriptPromiseResolver>>
m_audioDecoderResolvers;
On 2015/04/10 04:55:23, yhirano wrote:
> Is using WillBeHeapHashSet better?

I was just following what was done for the suspend and resume resolvers.  If you
think WillBeHeapHashSet is better, I will change it for the suspend and resume
resolvers too.  (In a separate CL.)

[Raymond Toy, 2015-04-10 21:00:45]

https://codereview.chromium.org/1006963003/diff/100001/LayoutTests/webaudio/d...
File LayoutTests/webaudio/decode-audio-data-promise.html (right):

https://codereview.chromium.org/1006963003/diff/100001/LayoutTests/webaudio/d...
LayoutTests/webaudio/decode-audio-data-promise.html:129: p =
context.decodeAudioData(request.response,
On 2015/04/10 04:55:23, yhirano wrote:
> context.decodeAudioData(request.response, function() {
>   testPassed(...);
> }, function() {
>   testFailed(...);
> }).then(function(buffer) {
>   ...
> }, then(function(e) {
>   testFailed(...);
> }).then(done, done);

Done.

https://codereview.chromium.org/1006963003/diff/100001/LayoutTests/webaudio/d...
LayoutTests/webaudio/decode-audio-data-promise.html:160: p =
context.decodeAudioData(request.response,
On 2015/04/10 04:55:23, yhirano wrote:
> context.decodeAudioData(request.response, function() {
>   testFailed(...);
> }, function() {
>   testPassed(...);
> }).then(function() {
>   testFailed(...);
> }, function(e) {
>   testPassed(...);
> }).then(done, done);
>   

Done.

https://codereview.chromium.org/1006963003/diff/100001/LayoutTests/webaudio/d...
LayoutTests/webaudio/decode-audio-data-promise.html:197: context.close()
On 2015/04/10 04:55:23, yhirano wrote:
> You don't need nested |then|.
> 
> context.clonse().then(function() {
>   return context.decodeAudioData(encodedAudioData);
> }).then(function(b) {
>   ...
> }, function(e) {
>   ...
> }).then(done, done);

Done.

[yhirano, 2015-04-11 01:13:43]

lgtm

[Raymond Toy, 2015-04-13 16:09:19]

On 2015/04/11 01:13:43, yhirano wrote:
> lgtm

Thanks, everyone!

[Raymond Toy, 2015-04-13 16:09:27]

The CQ bit was checked by rtoy@chromium.org

[Raymond Toy, 2015-04-13 16:09:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
hongchan@chromium.org, tkent@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1006963003/#ps120001
(title: "Update for review")

[commit-bot: I haz the power, 2015-04-13 16:09:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1006963003/120001

[commit-bot: I haz the power, 2015-04-13 20:01:27]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193648

[Raymond Toy, 2015-04-20 20:51:11]

A revert of this CL (patchset #7 id:120001) has been created in
https://chromiumcodereview.appspot.com/1097873003/ by rtoy@chromium.org.

The reason for reverting is: Causes problems noted in issue  477089.