Get rid of custom bindings for AudioContext's constructor

Get rid of custom bindings for AudioContext's constructor

Get rid of custom bindings for AudioContext's constructor by leveraging
existing generator functionality and adding a new factory method to
AudioContext that constructs an OfflineAudioContext.

R=haraken, kbr@chromium.org
TEST=webaudio/audionode.html
BUG=

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

[haraken, 2013-10-11 15:39:30]

LGTM. Please wait for kbr's review.

https://codereview.chromium.org/27037002/diff/1/Source/bindings/v8/custom/V8A...
File Source/bindings/v8/custom/V8AudioContextCustom.cpp (left):

https://codereview.chromium.org/27037002/diff/1/Source/bindings/v8/custom/V8A...
Source/bindings/v8/custom/V8AudioContextCustom.cpp:50: if (!audioContext.get())
{

Just to confirm: This branch won't be generated in the auto-generated code, but
it's ok because the branch cannot be hit. Right?

https://codereview.chromium.org/27037002/diff/1/Source/bindings/v8/custom/V8A...
Source/bindings/v8/custom/V8AudioContextCustom.cpp:64: if (!audioContext.get())
{

Ditto.

[Inactive, 2013-10-11 16:07:05]

https://codereview.chromium.org/27037002/diff/1/Source/bindings/v8/custom/V8A...
File Source/bindings/v8/custom/V8AudioContextCustom.cpp (left):

https://codereview.chromium.org/27037002/diff/1/Source/bindings/v8/custom/V8A...
Source/bindings/v8/custom/V8AudioContextCustom.cpp:50: if (!audioContext.get())
{
On 2013/10/11 15:39:30, haraken wrote:
> 
> Just to confirm: This branch won't be generated in the auto-generated code,
but
> it's ok because the branch cannot be hit. Right?

Actually, you are right. AudioContext::create() can return 0 and we should throw
an exception in this case. I will move the exception throwing to
AudioContext::create().

https://codereview.chromium.org/27037002/diff/1/Source/bindings/v8/custom/V8A...
Source/bindings/v8/custom/V8AudioContextCustom.cpp:64: if (!audioContext.get())
{
On 2013/10/11 15:39:30, haraken wrote:
> 
> Ditto.

Looks like this condition can never be true. It would have returned earlier if
it has been null.

[Inactive, 2013-10-11 16:22:47]

https://codereview.chromium.org/27037002/diff/1/Source/bindings/v8/custom/V8A...
File Source/bindings/v8/custom/V8AudioContextCustom.cpp (left):

https://codereview.chromium.org/27037002/diff/1/Source/bindings/v8/custom/V8A...
Source/bindings/v8/custom/V8AudioContextCustom.cpp:50: if (!audioContext.get())
{
On 2013/10/11 15:39:30, haraken wrote:
> 
> Just to confirm: This branch won't be generated in the auto-generated code,
but
> it's ok because the branch cannot be hit. Right?

Fixed. And I added a layout test to cover this scenario.

[Ken Russell (switch to Gerrit), 2013-10-11 19:19:00]

The code changes LGTM but please get a review from rtoy@ as well.

I have one concern about the newly added test, though I do see that it's
dependent on MaxHardwareContexts in AudioContext.cpp so will behave
deterministically.

https://codereview.chromium.org/27037002/diff/7001/LayoutTests/webaudio/audio...
File LayoutTests/webaudio/audiocontext-max-contexts.html (right):

https://codereview.chromium.org/27037002/diff/7001/LayoutTests/webaudio/audio...
LayoutTests/webaudio/audiocontext-max-contexts.html:19:
shouldThrow("reachHardwareContextsLimit()");
Is this test really viable? What if the implementation is clever and can
support, say, 100 simultaneous contexts, all software-mixed together?

[Inactive, 2013-10-11 19:29:49]

https://codereview.chromium.org/27037002/diff/7001/LayoutTests/webaudio/audio...
File LayoutTests/webaudio/audiocontext-max-contexts.html (right):

https://codereview.chromium.org/27037002/diff/7001/LayoutTests/webaudio/audio...
LayoutTests/webaudio/audiocontext-max-contexts.html:19:
shouldThrow("reachHardwareContextsLimit()");
On 2013/10/11 19:19:01, Ken Russell wrote:
> Is this test really viable? What if the implementation is clever and can
> support, say, 100 simultaneous contexts, all software-mixed together?

This is currently hardcoded in Blink so this is not currently a problem and it
is deterministic.
If the situation changes and it becomes hard to test, we may have to get rid of
this test at some point. However, in the mean time I think it adds some
coverage, which is valuable.

As you can see, my first patch set was wrong and I did not notice because of
lack of test coverage.
That said, if you prefer I remove the test (or if you have a better idea how to
test it), I won't mind.

[Ken Russell (switch to Gerrit), 2013-10-11 19:36:26]

On 2013/10/11 19:29:49, Chris Dumez wrote:
>
https://codereview.chromium.org/27037002/diff/7001/LayoutTests/webaudio/audio...
> File LayoutTests/webaudio/audiocontext-max-contexts.html (right):
> 
>
https://codereview.chromium.org/27037002/diff/7001/LayoutTests/webaudio/audio...
> LayoutTests/webaudio/audiocontext-max-contexts.html:19:
> shouldThrow("reachHardwareContextsLimit()");
> On 2013/10/11 19:19:01, Ken Russell wrote:
> > Is this test really viable? What if the implementation is clever and can
> > support, say, 100 simultaneous contexts, all software-mixed together?
> 
> This is currently hardcoded in Blink so this is not currently a problem and it
> is deterministic.
> If the situation changes and it becomes hard to test, we may have to get rid
of
> this test at some point. However, in the mean time I think it adds some
> coverage, which is valuable.
> 
> As you can see, my first patch set was wrong and I did not notice because of
> lack of test coverage.
> That said, if you prefer I remove the test (or if you have a better idea how
to
> test it), I won't mind.

No, that's fine -- it's great that the test caught an issue.

[Inactive, 2013-10-14 13:20:15]

rtoy@, could you please take a look as well?

[Raymond Toy (Google), 2013-10-14 16:55:18]

On 2013/10/14 13:20:15, Chris Dumez wrote:
> rtoy@, could you please take a look as well?

LGTM.

[commit-bot: I haz the power, 2013-10-14 17:22:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/27037002/7001

[commit-bot: I haz the power, 2013-10-14 17:22:34]

Failed to apply patch for Source/bindings/bindings.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/bindings/bindings.gypi
  Hunk #1 FAILED at 152.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/bindings/bindings.gypi.rej

Patch:       Source/bindings/bindings.gypi
Index: Source/bindings/bindings.gypi
diff --git a/Source/bindings/bindings.gypi b/Source/bindings/bindings.gypi
index
2d222dd0193cd9191290cf675f672236d76b8c0d..49d6c20c6728cdbfecc122ed2fad0b57af615e91
100644
--- a/Source/bindings/bindings.gypi
+++ b/Source/bindings/bindings.gypi
@@ -152,7 +152,6 @@
             'v8/custom/V8ArrayBufferViewCustom.cpp',
             'v8/custom/V8ArrayBufferViewCustom.h',
             'v8/custom/V8AudioBufferSourceNodeCustom.cpp',
-            'v8/custom/V8AudioContextCustom.cpp',
             'v8/custom/V8AudioNodeCustom.cpp',
             'v8/custom/V8BiquadFilterNodeCustom.cpp',
             'v8/custom/V8BlobCustom.cpp',

[commit-bot: I haz the power, 2013-10-14 20:03:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/27037002/19001

[commit-bot: I haz the power, 2013-10-14 23:27:51]

Change committed as 159613