AudioBufferSourceNode.buffer can only be set once.

AudioBufferSourceNode.buffer can only be set once.

According to https://github.com/WebAudio/web-audio-api/issues/288, the
buffer attribute of an AudioBufferSourceNode can only be set once. Display a deprecation warning when this happens. A use counter is also added to see how often this actually happens.

BUG=448960
TEST=dom-exceptions.html updated with new test

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

[Raymond Toy, 2015-01-16 22:50:49]

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

[Raymond Toy, 2015-01-16 22:50:49]

PTAL

[hongchan, 2015-01-16 23:42:09]

lgtm with nits

https://codereview.chromium.org/825603005/diff/1/LayoutTests/webaudio/audiobu...
File LayoutTests/webaudio/audiobuffersource-channels.html (right):

https://codereview.chromium.org/825603005/diff/1/LayoutTests/webaudio/audiobu...
LayoutTests/webaudio/audiobuffersource-channels.html:38: source =
context.createBufferSource();
This is something I am not comfortable with. Even thought the code is fully
functional, but the variable 'source' is defined globally and used for all the
test cases. I prefer to encapsulate each test from others.

I have seen a lot like this from tests which is written by crogers@, but am
quite not sure about it. I will  leave this up to you.

https://codereview.chromium.org/825603005/diff/1/LayoutTests/webaudio/dom-exc...
File LayoutTests/webaudio/dom-exceptions-expected.txt (right):

https://codereview.chromium.org/825603005/diff/1/LayoutTests/webaudio/dom-exc...
LayoutTests/webaudio/dom-exceptions-expected.txt:62: PASS source.buffer = buffer
threw exception InvalidStateError: Failed to set the 'buffer' property on
'AudioBufferSourceNode': buffer can only be set once..
I have noticed this problem in other places: some exception messages are being
printed with two periods. It would be better if we can avoid this.

https://codereview.chromium.org/825603005/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioBufferSourceNode.cpp (right):

https://codereview.chromium.org/825603005/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioBufferSourceNode.cpp:346: "buffer can only be set
once.");
Perhaps the first word should be capitalized? Other places in chromium code base
are throwing properly capitalized message when it is a complete sentence.

WDYT?

[Raymond Toy, 2015-01-17 00:03:16]

https://codereview.chromium.org/825603005/diff/1/LayoutTests/webaudio/audiobu...
File LayoutTests/webaudio/audiobuffersource-channels.html (right):

https://codereview.chromium.org/825603005/diff/1/LayoutTests/webaudio/audiobu...
LayoutTests/webaudio/audiobuffersource-channels.html:38: source =
context.createBufferSource();
On 2015/01/16 23:42:09, hoch wrote:
> This is something I am not comfortable with. Even thought the code is fully
> functional, but the variable 'source' is defined globally and used for all the
> test cases. I prefer to encapsulate each test from others.
> 
> I have seen a lot like this from tests which is written by crogers@, but am
> quite not sure about it. I will  leave this up to you.

The global source is needed for the shouldThrow() stuff, but I can make the
other uses local vars.

https://codereview.chromium.org/825603005/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioBufferSourceNode.cpp (right):

https://codereview.chromium.org/825603005/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioBufferSourceNode.cpp:346: "buffer can only be set
once.");
On 2015/01/16 23:42:09, hoch wrote:
> Perhaps the first word should be capitalized? Other places in chromium code
base
> are throwing properly capitalized message when it is a complete sentence.
> 
> WDYT?

Will do. And get rid of the trailing period.

[Raymond Toy, 2015-01-20 18:06:00]

https://codereview.chromium.org/825603005/diff/1/LayoutTests/webaudio/audiobu...
File LayoutTests/webaudio/audiobuffersource-channels.html (right):

https://codereview.chromium.org/825603005/diff/1/LayoutTests/webaudio/audiobu...
LayoutTests/webaudio/audiobuffersource-channels.html:38: source =
context.createBufferSource();
On 2015/01/17 00:03:16, Raymond Toy wrote:
> On 2015/01/16 23:42:09, hoch wrote:
> > This is something I am not comfortable with. Even thought the code is fully
> > functional, but the variable 'source' is defined globally and used for all
the
> > test cases. I prefer to encapsulate each test from others.
> > 
> > I have seen a lot like this from tests which is written by crogers@, but am
> > quite not sure about it. I will  leave this up to you.
> 
> The global source is needed for the shouldThrow() stuff, but I can make the
> other uses local vars.

Done.

https://codereview.chromium.org/825603005/diff/1/LayoutTests/webaudio/dom-exc...
File LayoutTests/webaudio/dom-exceptions-expected.txt (right):

https://codereview.chromium.org/825603005/diff/1/LayoutTests/webaudio/dom-exc...
LayoutTests/webaudio/dom-exceptions-expected.txt:62: PASS source.buffer = buffer
threw exception InvalidStateError: Failed to set the 'buffer' property on
'AudioBufferSourceNode': buffer can only be set once..
On 2015/01/16 23:42:09, hoch wrote:
> I have noticed this problem in other places: some exception messages are being
> printed with two periods. It would be better if we can avoid this.

Done.

https://codereview.chromium.org/825603005/diff/1/Source/modules/webaudio/Audi...
File Source/modules/webaudio/AudioBufferSourceNode.cpp (right):

https://codereview.chromium.org/825603005/diff/1/Source/modules/webaudio/Audi...
Source/modules/webaudio/AudioBufferSourceNode.cpp:346: "buffer can only be set
once.");
On 2015/01/16 23:42:09, hoch wrote:
> Perhaps the first word should be capitalized? Other places in chromium code
base
> are throwing properly capitalized message when it is a complete sentence.
> 
> WDYT?

Done.

[Raymond Toy, 2015-01-20 18:37:57]

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

[Raymond Toy, 2015-01-20 18:37:57]

PTAL

[Ken Russell (switch to Gerrit), 2015-01-20 21:26:16]

What's the compatibility impact of this change? Intuitively, it seems
potentially large.

https://codereview.chromium.org/825603005/diff/40001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioBufferSourceNode.cpp (right):

https://codereview.chromium.org/825603005/diff/40001/Source/modules/webaudio/...
Source/modules/webaudio/AudioBufferSourceNode.cpp:350: // The context must be
locked since changing the buffer can re-configure the number of channels that
are output.
This comment, and probably later ones, should be updated.

[Raymond Toy, 2015-01-20 21:35:03]

Yes, this change has a potentially large compatibility issue.  I have found that
(only) one of the Chrome WebAudio demos was setting the buffer more than once.

Should the error be changed to a warning?  Then at some future time, the warning
can be changed to an error?

https://codereview.chromium.org/825603005/diff/40001/Source/modules/webaudio/...
File Source/modules/webaudio/AudioBufferSourceNode.cpp (right):

https://codereview.chromium.org/825603005/diff/40001/Source/modules/webaudio/...
Source/modules/webaudio/AudioBufferSourceNode.cpp:350: // The context must be
locked since changing the buffer can re-configure the number of channels that
are output.
On 2015/01/20 21:26:16, Ken Russell wrote:
> This comment, and probably later ones, should be updated.

I believe this is still true.  On creation without a buffer, the
AudioBufferSource is configured for a single channel (of silence).  When the
buffer is assigned, the source node reconfigures the number of channels to match
the buffer.

[Ken Russell (switch to Gerrit), 2015-01-21 01:18:14]

On 2015/01/20 21:35:03, Raymond Toy wrote:
> Yes, this change has a potentially large compatibility issue.  I have found
that
> (only) one of the Chrome WebAudio demos was setting the buffer more than once.
> 
> Should the error be changed to a warning?  Then at some future time, the
warning
> can be changed to an error?

I would say so, yes. A deprecation warning should be issued for at least one
full Chrome release. Additionally, you might want to add a use counter to see
how often the old code path is taken.


>
https://codereview.chromium.org/825603005/diff/40001/Source/modules/webaudio/...
> File Source/modules/webaudio/AudioBufferSourceNode.cpp (right):
> 
>
https://codereview.chromium.org/825603005/diff/40001/Source/modules/webaudio/...
> Source/modules/webaudio/AudioBufferSourceNode.cpp:350: // The context must be
> locked since changing the buffer can re-configure the number of channels that
> are output.
> On 2015/01/20 21:26:16, Ken Russell wrote:
> > This comment, and probably later ones, should be updated.
> 
> I believe this is still true.  On creation without a buffer, the
> AudioBufferSource is configured for a single channel (of silence).  When the
> buffer is assigned, the source node reconfigures the number of channels to
match
> the buffer.

OK, that's fine then.

[Raymond Toy, 2015-01-21 20:22:06]

On 2015/01/21 01:18:14, Ken Russell wrote:
> On 2015/01/20 21:35:03, Raymond Toy wrote:
> > Yes, this change has a potentially large compatibility issue.  I have found
> that
> > (only) one of the Chrome WebAudio demos was setting the buffer more than
once.
> > 
> > Should the error be changed to a warning?  Then at some future time, the
> warning
> > can be changed to an error?
> 
> I would say so, yes. A deprecation warning should be issued for at least one
> full Chrome release. Additionally, you might want to add a use counter to see
> how often the old code path is taken.

Done.  Use counter added too.
> 
> 
> >
>
https://codereview.chromium.org/825603005/diff/40001/Source/modules/webaudio/...
> > File Source/modules/webaudio/AudioBufferSourceNode.cpp (right):
> > 
> >
>
https://codereview.chromium.org/825603005/diff/40001/Source/modules/webaudio/...
> > Source/modules/webaudio/AudioBufferSourceNode.cpp:350: // The context must
be
> > locked since changing the buffer can re-configure the number of channels
that
> > are output.
> > On 2015/01/20 21:26:16, Ken Russell wrote:
> > > This comment, and probably later ones, should be updated.
> > 
> > I believe this is still true.  On creation without a buffer, the
> > AudioBufferSource is configured for a single channel (of silence).  When the
> > buffer is assigned, the source node reconfigures the number of channels to
> match
> > the buffer.
> 
> OK, that's fine then.

[Ken Russell (switch to Gerrit), 2015-01-21 22:58:46]

lgtm

https://codereview.chromium.org/825603005/diff/60001/Source/core/frame/UseCou...
File Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/825603005/diff/60001/Source/core/frame/UseCou...
Source/core/frame/UseCounter.cpp:834: return "Setting
AudioBufferSourceNode.buffer more than once is deprecated and will no longer
work in M45.";
Better to say "Chrome 45"? Also, feel free to pull move this up to an earlier
release like Chrome 43. Also please communicate this change as widely as
possible on public mailing lists.

https://codereview.chromium.org/825603005/diff/60001/Source/core/frame/UseCou...
File Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/825603005/diff/60001/Source/core/frame/UseCou...
Source/core/frame/UseCounter.h:622: // to update the UMA mapping.
Don't forget to do this after committing.

[Raymond Toy, 2015-01-21 23:16:27]

https://codereview.chromium.org/825603005/diff/60001/Source/core/frame/UseCou...
File Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/825603005/diff/60001/Source/core/frame/UseCou...
Source/core/frame/UseCounter.cpp:834: return "Setting
AudioBufferSourceNode.buffer more than once is deprecated and will no longer
work in M45.";
On 2015/01/21 22:58:46, Ken Russell wrote:
> Better to say "Chrome 45"? Also, feel free to pull move this up to an earlier
> release like Chrome 43. Also please communicate this change as widely as
> possible on public mailing lists.

Chrome 43 it is.  Will add entry to the chrome status dashboard

[Raymond Toy, 2015-01-21 23:16:41]

The CQ bit was checked by rtoy@chromium.org

[commit-bot: I haz the power, 2015-01-21 23:17:13]

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

[commit-bot: I haz the power, 2015-01-22 01:43:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-22 01:43:21]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/47228)

[Raymond Toy, 2015-01-23 16:48:41]

The CQ bit was checked by rtoy@chromium.org

[commit-bot: I haz the power, 2015-01-23 16:49:33]

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

[commit-bot: I haz the power, 2015-01-23 19:02:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-23 19:02:04]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/47523)

[Raymond Toy, 2015-01-27 17:54:10]

The CQ bit was checked by rtoy@chromium.org

[commit-bot: I haz the power, 2015-01-27 17:54:46]

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

[commit-bot: I haz the power, 2015-01-27 17:54:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-27 17:54:58]

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

Patch:       Source/core/frame/UseCounter.h
Index: Source/core/frame/UseCounter.h
diff --git a/Source/core/frame/UseCounter.h b/Source/core/frame/UseCounter.h
index
ff5ecd6f02172a6ef93c5871d03af6994a3335db..dfb6ccb258813292e4bacbb6226479cc3d99040d
100644
--- a/Source/core/frame/UseCounter.h
+++ b/Source/core/frame/UseCounter.h
@@ -614,6 +614,7 @@ public:
         BlockedSniffingImageToScript = 674,
         Fetch = 675,
         FetchBodyStream = 676,
+        AudioBufferSourceBufferOnce = 677,
 
         // Add new features immediately above this line. Don't change assigned
         // numbers of any item, and don't reuse removed slots.

[Raymond Toy, 2015-01-27 18:12:34]

The CQ bit was checked by rtoy@chromium.org

[commit-bot: I haz the power, 2015-01-27 18:12:52]

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

[commit-bot: I haz the power, 2015-01-27 18:13:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-27 18:13:06]

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

Patch:       Source/core/frame/UseCounter.h
Index: Source/core/frame/UseCounter.h
diff --git a/Source/core/frame/UseCounter.h b/Source/core/frame/UseCounter.h
index
442b5f119590432c3a5e8d6a6d36286aab210bb3..0fcf8569831c19ac629c55c06768f1479bb74cef
100644
--- a/Source/core/frame/UseCounter.h
+++ b/Source/core/frame/UseCounter.h
@@ -616,6 +616,7 @@ public:
         BlockedSniffingImageToScript = 674,
         Fetch = 675,
         FetchBodyStream = 676,
+        AudioBufferSourceBufferOnce = 677,
 
         // Add new features immediately above this line. Don't change assigned
         // numbers of any item, and don't reuse removed slots.

[Raymond Toy, 2015-01-28 17:04:03]

The CQ bit was checked by rtoy@chromium.org

[commit-bot: I haz the power, 2015-01-28 17:04:12]

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

[commit-bot: I haz the power, 2015-01-28 18:28:38]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189127