Drop [LegacyInterfaceTypeChecking] for the Web Audio API

Drop [LegacyInterfaceTypeChecking] for the Web Audio API

This aligns Web Audio with what WebIDL requires given the IDL it uses.

The risk of these changes is bounded by these use counters:

AnalyserNode ~0.01%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/631

BiquadFilterNode ~0.001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/632

ConvolverNode ~0.0001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/636

OscillatorNode ~0.001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/643

WaveShaperNode <0.0001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/648

AudioParam.prototype.setValueCurveAtTime <0.0001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/913

With such lower usage, fixing corner cases like this is unlikely to
cause any trouble. Usage of AudioContext itself is at most ~0.2%, but
that counter is not for the constructor but merely access to
window.AudioContext, and thus not a good indicator of real usage:
https://www.chromestatus.com/metrics/feature/timeline/popularity/652

BUG=561338
Committed: https://crrev.com/eb440261d8872ac47d0c826aa38fba055b9be866
Cr-Commit-Position: refs/heads/master@{#363297}

[philipj_slow, 2015-12-04 10:47:15]

Description was changed from

==========
Drop [LegacyInterfaceTypeChecking] for the Web Audio API

BUG=561338
==========

to

==========
Drop [LegacyInterfaceTypeChecking] for the Web Audio API

This aligns Web Audio with what WebIDL requires given the IDL it uses.

The risk of these changes is bounded by these use counters:

AnalyserNode:
https://www.chromestatus.com/metrics/feature/timeline/popularity/631

BiquadFilterNode:
https://www.chromestatus.com/metrics/feature/timeline/popularity/632

ConvolverNode:
https://www.chromestatus.com/metrics/feature/timeline/popularity/636

OscillatorNode:
https://www.chromestatus.com/metrics/feature/timeline/popularity/643

WaveShaperNode:
https://www.chromestatus.com/metrics/feature/timeline/popularity/648

AudioParam.prototype.setValueCurveAtTime:
https://www.chromestatus.com/metrics/feature/timeline/popularity/913

BUG=561338
==========

[philipj_slow, 2015-12-04 10:49:11]

Description was changed from

==========
Drop [LegacyInterfaceTypeChecking] for the Web Audio API

This aligns Web Audio with what WebIDL requires given the IDL it uses.

The risk of these changes is bounded by these use counters:

AnalyserNode:
https://www.chromestatus.com/metrics/feature/timeline/popularity/631

BiquadFilterNode:
https://www.chromestatus.com/metrics/feature/timeline/popularity/632

ConvolverNode:
https://www.chromestatus.com/metrics/feature/timeline/popularity/636

OscillatorNode:
https://www.chromestatus.com/metrics/feature/timeline/popularity/643

WaveShaperNode:
https://www.chromestatus.com/metrics/feature/timeline/popularity/648

AudioParam.prototype.setValueCurveAtTime:
https://www.chromestatus.com/metrics/feature/timeline/popularity/913

BUG=561338
==========

to

==========
Drop [LegacyInterfaceTypeChecking] for the Web Audio API

This aligns Web Audio with what WebIDL requires given the IDL it uses.

The risk of these changes is bounded by these use counters:

AnalyserNode:
https://www.chromestatus.com/metrics/feature/timeline/popularity/631

BiquadFilterNode:
https://www.chromestatus.com/metrics/feature/timeline/popularity/632

ConvolverNode:
https://www.chromestatus.com/metrics/feature/timeline/popularity/636

OscillatorNode:
https://www.chromestatus.com/metrics/feature/timeline/popularity/643

WaveShaperNode:
https://www.chromestatus.com/metrics/feature/timeline/popularity/648

AudioParam.prototype.setValueCurveAtTime:
https://www.chromestatus.com/metrics/feature/timeline/popularity/913

AnalyserNode is around 0.01% and the others are much lower, which means that
fixing corner cases like this is unlikely to cause any trouble.

BUG=561338
==========

[philipj_slow, 2015-12-04 10:49:20]

The CQ bit was checked by philipj@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-04 10:49:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1493753003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1493753003/20001

[commit-bot: I haz the power, 2015-12-04 12:09:36]

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

[commit-bot: I haz the power, 2015-12-04 12:09:37]

Dry run: This issue passed the CQ dry run.

[philipj_slow, 2015-12-04 12:25:32]

philipj@opera.com changed reviewers:
+ hongchan@chromium.org, kbr@chromium.org, rtoy@chromium.org

[philipj_slow, 2015-12-04 12:25:32]

PTAL. I think the compat risk here seems very low, but you know best how these
APIs are likely to be used. If you're at all worried about breakage, please say
so!

[Raymond Toy, 2015-12-04 17:30:42]

https://codereview.chromium.org/1493753003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp (right):

https://codereview.chromium.org/1493753003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:204:
ASSERT(curve);
In a release build, what should we do if curve is null?  This shouldn't happen,
but if it's worth putting in a debug assert we should think about the release
build.

Chris Rogers was fairly careful but silently returning on release builds for bad
situations like this.

[Raymond Toy, 2015-12-04 17:32:53]

If you're going to include links to the metrics, can you also include the
current rough percentages.  The values in the links will change value over time.

Also, it might be useful to include a link to the AudioContext metric
(https://www.chromestatus.com/metrics/feature/timeline/popularity/652) to put
the percentages in the appropriate context.

[Raymond Toy, 2015-12-04 17:43:38]

On Fri, Dec 4, 2015 at 9:30 AM, <rtoy@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1493753003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp
> (right):
>
>
>
https://codereview.chromium.org/1493753003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:204:
> ASSERT(curve);
> In a release build, what should we do if curve is null?  This shouldn't
> happen, but if it's worth putting in a debug assert we should think
> about the release build.
>
> Chris Rogers was fairly careful but silently returning on release builds
> for bad situations like this.
>

Ugh.  He was careful about silently returning.

>
> https://codereview.chromium.org/1493753003/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Raymond Toy, 2015-12-04 17:43:38]

On Fri, Dec 4, 2015 at 9:30 AM, <rtoy@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1493753003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp
> (right):
>
>
>
https://codereview.chromium.org/1493753003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:204:
> ASSERT(curve);
> In a release build, what should we do if curve is null?  This shouldn't
> happen, but if it's worth putting in a debug assert we should think
> about the release build.
>
> Chris Rogers was fairly careful but silently returning on release builds
> for bad situations like this.
>

Ugh.  He was careful about silently returning.

>
> https://codereview.chromium.org/1493753003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[philipj_slow, 2015-12-04 18:22:28]

Description was changed from

==========
Drop [LegacyInterfaceTypeChecking] for the Web Audio API

This aligns Web Audio with what WebIDL requires given the IDL it uses.

The risk of these changes is bounded by these use counters:

AnalyserNode:
https://www.chromestatus.com/metrics/feature/timeline/popularity/631

BiquadFilterNode:
https://www.chromestatus.com/metrics/feature/timeline/popularity/632

ConvolverNode:
https://www.chromestatus.com/metrics/feature/timeline/popularity/636

OscillatorNode:
https://www.chromestatus.com/metrics/feature/timeline/popularity/643

WaveShaperNode:
https://www.chromestatus.com/metrics/feature/timeline/popularity/648

AudioParam.prototype.setValueCurveAtTime:
https://www.chromestatus.com/metrics/feature/timeline/popularity/913

AnalyserNode is around 0.01% and the others are much lower, which means that
fixing corner cases like this is unlikely to cause any trouble.

BUG=561338
==========

to

==========
Drop [LegacyInterfaceTypeChecking] for the Web Audio API

This aligns Web Audio with what WebIDL requires given the IDL it uses.

The risk of these changes is bounded by these use counters:

AnalyserNode ~0.01%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/631

BiquadFilterNode ~0.001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/632

ConvolverNode ~0.0001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/636

OscillatorNode ~0.001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/643

WaveShaperNode <0.0001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/648

AudioParam.prototype.setValueCurveAtTime <0.0001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/913

With such lower usage, fixing corner cases like this is unlikely to
cause any trouble. Usage of AudioContext itself is at most ~0.2%, but
that counter is not for the constructor but merely access to
window.AudioContext, and thus not a good indicator of real usage:
https://www.chromestatus.com/metrics/feature/timeline/popularity/652

BUG=561338
==========

[philipj_slow, 2015-12-04 18:24:10]

On 2015/12/04 17:32:53, Raymond Toy wrote:
> If you're going to include links to the metrics, can you also include the
> current rough percentages.  The values in the links will change value over
time.
> 
> Also, it might be useful to include a link to the AudioContext metric
> (https://www.chromestatus.com/metrics/feature/timeline/popularity/652) to put
> the percentages in the appropriate context.

Done, but see the new description for why the the AudioContext metric is not
reliable. Compare to the entirely useless window.offscreenBuffering, which sees
similar "usage", likely due to enumeration:
https://www.chromestatus.com/metrics/feature/timeline/popularity/356

[philipj_slow, 2015-12-04 18:31:57]

https://codereview.chromium.org/1493753003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp (right):

https://codereview.chromium.org/1493753003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:204:
ASSERT(curve);
On 2015/12/04 17:30:42, Raymond Toy wrote:
> In a release build, what should we do if curve is null?  This shouldn't
happen,
> but if it's worth putting in a debug assert we should think about the release
> build.
> 
> Chris Rogers was fairly careful but silently returning on release builds for
bad
> situations like this.

Would you like me to revert all changes to cpp files, or something else? In all
other cases where bindings guarantees that the input is not null, I've checked
that there are no internal calls and ASSERTed it, to make responsibilities
clear, but I will do whatever you're most comfortable with.

[Raymond Toy, 2015-12-04 21:06:26]

lgtm.

https://codereview.chromium.org/1493753003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp (right):

https://codereview.chromium.org/1493753003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:204:
ASSERT(curve);
On 2015/12/04 18:31:57, philipj wrote:
> On 2015/12/04 17:30:42, Raymond Toy wrote:
> > In a release build, what should we do if curve is null?  This shouldn't
> happen,
> > but if it's worth putting in a debug assert we should think about the
release
> > build.
> > 
> > Chris Rogers was fairly careful but silently returning on release builds for
> bad
> > situations like this.
> 
> Would you like me to revert all changes to cpp files, or something else? In
all
> other cases where bindings guarantees that the input is not null, I've checked
> that there are no internal calls and ASSERTed it, to make responsibilities
> clear, but I will do whatever you're most comfortable with.

Thanks for the info.  I think this is fine, plus you've added additional tests
for this.

[philipj_slow, 2015-12-04 21:12:27]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2015-12-04 21:14:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1493753003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1493753003/20001

[commit-bot: I haz the power, 2015-12-04 21:20:53]

Description was changed from

==========
Drop [LegacyInterfaceTypeChecking] for the Web Audio API

This aligns Web Audio with what WebIDL requires given the IDL it uses.

The risk of these changes is bounded by these use counters:

AnalyserNode ~0.01%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/631

BiquadFilterNode ~0.001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/632

ConvolverNode ~0.0001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/636

OscillatorNode ~0.001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/643

WaveShaperNode <0.0001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/648

AudioParam.prototype.setValueCurveAtTime <0.0001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/913

With such lower usage, fixing corner cases like this is unlikely to
cause any trouble. Usage of AudioContext itself is at most ~0.2%, but
that counter is not for the constructor but merely access to
window.AudioContext, and thus not a good indicator of real usage:
https://www.chromestatus.com/metrics/feature/timeline/popularity/652

BUG=561338
==========

to

==========
Drop [LegacyInterfaceTypeChecking] for the Web Audio API

This aligns Web Audio with what WebIDL requires given the IDL it uses.

The risk of these changes is bounded by these use counters:

AnalyserNode ~0.01%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/631

BiquadFilterNode ~0.001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/632

ConvolverNode ~0.0001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/636

OscillatorNode ~0.001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/643

WaveShaperNode <0.0001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/648

AudioParam.prototype.setValueCurveAtTime <0.0001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/913

With such lower usage, fixing corner cases like this is unlikely to
cause any trouble. Usage of AudioContext itself is at most ~0.2%, but
that counter is not for the constructor but merely access to
window.AudioContext, and thus not a good indicator of real usage:
https://www.chromestatus.com/metrics/feature/timeline/popularity/652

BUG=561338
==========

[commit-bot: I haz the power, 2015-12-04 21:20:54]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-04 21:22:22]

Description was changed from

==========
Drop [LegacyInterfaceTypeChecking] for the Web Audio API

This aligns Web Audio with what WebIDL requires given the IDL it uses.

The risk of these changes is bounded by these use counters:

AnalyserNode ~0.01%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/631

BiquadFilterNode ~0.001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/632

ConvolverNode ~0.0001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/636

OscillatorNode ~0.001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/643

WaveShaperNode <0.0001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/648

AudioParam.prototype.setValueCurveAtTime <0.0001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/913

With such lower usage, fixing corner cases like this is unlikely to
cause any trouble. Usage of AudioContext itself is at most ~0.2%, but
that counter is not for the constructor but merely access to
window.AudioContext, and thus not a good indicator of real usage:
https://www.chromestatus.com/metrics/feature/timeline/popularity/652

BUG=561338
==========

to

==========
Drop [LegacyInterfaceTypeChecking] for the Web Audio API

This aligns Web Audio with what WebIDL requires given the IDL it uses.

The risk of these changes is bounded by these use counters:

AnalyserNode ~0.01%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/631

BiquadFilterNode ~0.001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/632

ConvolverNode ~0.0001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/636

OscillatorNode ~0.001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/643

WaveShaperNode <0.0001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/648

AudioParam.prototype.setValueCurveAtTime <0.0001%:
https://www.chromestatus.com/metrics/feature/timeline/popularity/913

With such lower usage, fixing corner cases like this is unlikely to
cause any trouble. Usage of AudioContext itself is at most ~0.2%, but
that counter is not for the constructor but merely access to
window.AudioContext, and thus not a good indicator of real usage:
https://www.chromestatus.com/metrics/feature/timeline/popularity/652

BUG=561338

Committed: https://crrev.com/eb440261d8872ac47d0c826aa38fba055b9be866
Cr-Commit-Position: refs/heads/master@{#363297}
==========

[commit-bot: I haz the power, 2015-12-04 21:22:23]

Patchset 2 (id:??) landed as
https://crrev.com/eb440261d8872ac47d0c826aa38fba055b9be866
Cr-Commit-Position: refs/heads/master@{#363297}