Implement cancelValuesAndHoldAtTime

Implement cancelValuesAndHoldAtTime

cancelValuesAndHoldAtTime(time) should cancel all events after time |time|
and when the context current time reaches |time|, the automation value
that is current at that time is held as if setValueAtTime were called
with the appropriate value and time.

Spec: https://webaudio.github.io/web-audio-api/#widl-AudioParam-cancelAndHoldAtTime-AudioParam-double-cancelTime
Intent: https://groups.google.com/a/chromium.org/d/msg/blink-dev/IftXib6yFyw/CLoxngG9BgAJ

BUG=432934
TEST=audioparam-cancel-scheduled-values.html

Review-Url: https://codereview.chromium.org/1533103002
Cr-Commit-Position: refs/heads/master@{#442916}
Committed: https://chromium.googlesource.com/chromium/src/+/4260025dd80eed380e0c1fff0e4be2bdd0ae0e8a

[Raymond Toy, 2016-02-05 21:38:18]

Description was changed from

==========
cancelScheduledValues should hold that last value

cancelScheduledValues(time) should cancel at event after time |time|
and when the context current time reaches |time|, the automation value
that is current at that time is held as if setValueAtTime were called
with the appropriate value and time.

BUG=432934
TEST=
==========

to

==========
cancelScheduledValues should hold that last value

cancelScheduledValues(time) should cancel at event after time |time|
and when the context current time reaches |time|, the automation value
that is current at that time is held as if setValueAtTime were called
with the appropriate value and time.

BUG=432934
TEST=audioparam-cancel-scheduled-values.html
==========

[Raymond Toy, 2016-02-26 22:56:25]

Description was changed from

==========
cancelScheduledValues should hold that last value

cancelScheduledValues(time) should cancel at event after time |time|
and when the context current time reaches |time|, the automation value
that is current at that time is held as if setValueAtTime were called
with the appropriate value and time.

BUG=432934
TEST=audioparam-cancel-scheduled-values.html
==========

to

==========
cancelScheduledValues should hold that last value

cancelScheduledValues(time) should cancel all events after time |time|
and when the context current time reaches |time|, the automation value
that is current at that time is held as if setValueAtTime were called
with the appropriate value and time.

BUG=432934
TEST=audioparam-cancel-scheduled-values.html
==========

[Raymond Toy, 2016-08-03 16:05:40]

The CQ bit was checked by rtoy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 16:06:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-03 17:37:52]

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

[commit-bot: I haz the power, 2016-08-03 17:37:53]

Dry run: This issue passed the CQ dry run.

[Raymond Toy, 2016-12-07 17:58:04]

Description was changed from

==========
cancelScheduledValues should hold that last value

cancelScheduledValues(time) should cancel all events after time |time|
and when the context current time reaches |time|, the automation value
that is current at that time is held as if setValueAtTime were called
with the appropriate value and time.

BUG=432934
TEST=audioparam-cancel-scheduled-values.html
==========

to

==========
Implement cancelValuesAndHoldAtTime

cancelValuesAndHoldAtTime(time) should cancel all events after time |time|
and when the context current time reaches |time|, the automation value
that is current at that time is held as if setValueAtTime were called
with the appropriate value and time.

Spec:
https://webaudio.github.io/web-audio-api/#widl-AudioParam-cancelAndHoldAtTime...

BUG=432934
TEST=audioparam-cancel-scheduled-values.html
==========

[Raymond Toy, 2016-12-14 00:00:07]

Description was changed from

==========
Implement cancelValuesAndHoldAtTime

cancelValuesAndHoldAtTime(time) should cancel all events after time |time|
and when the context current time reaches |time|, the automation value
that is current at that time is held as if setValueAtTime were called
with the appropriate value and time.

Spec:
https://webaudio.github.io/web-audio-api/#widl-AudioParam-cancelAndHoldAtTime...

BUG=432934
TEST=audioparam-cancel-scheduled-values.html
==========

to

==========
Implement cancelValuesAndHoldAtTime

cancelValuesAndHoldAtTime(time) should cancel all events after time |time|
and when the context current time reaches |time|, the automation value
that is current at that time is held as if setValueAtTime were called
with the appropriate value and time.

Spec:
https://webaudio.github.io/web-audio-api/#widl-AudioParam-cancelAndHoldAtTime...
Intent:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/IftXib6yFyw/CLoxngG9...

BUG=432934
TEST=audioparam-cancel-scheduled-values.html
==========

[Raymond Toy, 2016-12-14 00:17:28]

The CQ bit was checked by rtoy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 00:18:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 02:32:52]

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

[commit-bot: I haz the power, 2016-12-14 02:32:53]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Raymond Toy, 2016-12-20 16:30:11]

The CQ bit was checked by rtoy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 16:30:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Raymond Toy, 2016-12-20 16:31:05]

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

[Raymond Toy, 2016-12-20 16:31:06]

PTAL

[commit-bot: I haz the power, 2016-12-20 17:54:27]

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

[commit-bot: I haz the power, 2016-12-20 17:54:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hongchan, 2016-12-20 19:48:59]

The first pass - few superficial feedback. I started looking at the logic for
the second pass of the review.

https://codereview.chromium.org/1533103002/diff/500001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/webaudio/AudioParam/audioparam-cancel-and-hold.html
(right):

https://codereview.chromium.org/1533103002/diff/500001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webaudio/AudioParam/audioparam-cancel-and-hold.html:1:
<!doctype html>
Can we replace all 'var's to 'let'?

https://codereview.chromium.org/1533103002/diff/500001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webaudio/AudioParam/audioparam-cancel-and-hold.html:340:
// Run the cancellation test. A set of automations is created and canceled.
It would be great if we can apply the 80-col rule here as well.

[Raymond Toy, 2016-12-20 22:40:10]

https://codereview.chromium.org/1533103002/diff/500001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/webaudio/AudioParam/audioparam-cancel-and-hold.html
(right):

https://codereview.chromium.org/1533103002/diff/500001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webaudio/AudioParam/audioparam-cancel-and-hold.html:1:
<!doctype html>
On 2016/12/20 19:48:59, hongchan wrote:
> Can we replace all 'var's to 'let'?

Done.

https://codereview.chromium.org/1533103002/diff/500001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webaudio/AudioParam/audioparam-cancel-and-hold.html:340:
// Run the cancellation test. A set of automations is created and canceled.
On 2016/12/20 19:48:58, hongchan wrote:
> It would be great if we can apply the 80-col rule here as well.

Done.

[hongchan, 2016-12-22 18:41:37]

The second pass: I focused on the style and the convention.

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp (right):

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:227: }
What happens if |savedEvent| is not valid after the |if| clause?

If there is no possibility that |saveEvent| is an invalid pointer, perhaps we
can unwrap the |if| clause?

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:253:
DCHECK(getType() == ParamEvent::CancelValues);
DCHECK_EQ() might be better.

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:258:
DCHECK(getType() == ParamEvent::CancelValues);
ditto.

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:263:
DCHECK(getType() == ParamEvent::CancelValues);
ditto.

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:700: 
nit: an empty line.

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h (right):

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h:110: //
cancelValuesAndHoldAtTime() API.
Just a thought: how about renaming this variable to 'CancelValuesInternal'?

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h:135: static
std::unique_ptr<ParamEvent> createGeneralEvent(
Can we have a comment why this method is needed? Perhaps the adjective 'Base' or
'Abstract' is better than 'General'?

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h:163: // For
CancelValues events. Not valid for any other event
nit: a missing period. Also there is a couple of missing periods in the comments
below.

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h:182: // Create
simplest event needing just a value and time, like setValueAtTime
nit: a missing period.

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h:186: // time in
case
nit: the line breaks at the wrong position?

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h:228: // The
following items are only used for SetValueCurve events
nit: perhaps a colon at the end?

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h:238: //    
SetValueCurve events.
These comments should be located just above each variable. I think that's our
convention. No?

[Raymond Toy, 2016-12-22 19:06:23]

If you're not going to finish reviewing this by today, I'd hold off because it's
all going to have to be rebased after The Great Blink Renaming on Dec 31.

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp (right):

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:227: }
On 2016/12/22 18:41:36, hongchan wrote:
> What happens if |savedEvent| is not valid after the |if| clause?
> 
> If there is no possibility that |saveEvent| is an invalid pointer, perhaps we
> can unwrap the |if| clause?

savedEvent is sometimes nullptr depending on the type of the event that is being
cancelled, if any.

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h (right):

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h:110: //
cancelValuesAndHoldAtTime() API.
On 2016/12/22 18:41:37, hongchan wrote:
> Just a thought: how about renaming this variable to 'CancelValuesInternal'?

Actually, the comment is kind of wrong. None of these are actually exposed to
Javascript.  So internal or external doesn't really mean anything.

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h:135: static
std::unique_ptr<ParamEvent> createGeneralEvent(
On 2016/12/22 18:41:37, hongchan wrote:
> Can we have a comment why this method is needed? Perhaps the adjective 'Base'
or
> 'Abstract' is better than 'General'?

It's needed to create a saved event (which is basically a copy of the event). 
Not sure how this would be "Abstract", since it's a real event, and there's now
"Base" event.  There's just the ParamEvent.

[Raymond Toy, 2016-12-22 19:08:47]

I guess I should have mentioned that a lot of the changes are because I changed
the event list to hold pointers instead of objects so that the ownership of the
actual event is made clear. I think (it's been too long) that if you don't do
this, some events would get destructed while it was still being used because we
had pointers or refs to them.

[blink-reviews, 2016-12-22 19:10:17]

Thanks. I was wondering why unique_ptr was introduced.

On Thu, Dec 22, 2016 at 11:08 AM rtoy@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> I guess I should have mentioned that a lot of the changes are because I
> changed
> the event list to hold pointers instead of objects so that the ownership
> of the
> actual event is made clear. I think (it's been too long) that if you don't
> do
> this, some events would get destructed while it was still being used
> because we
> had pointers or refs to them.
>
> https://codereview.chromium.org/1533103002/
>

-- 
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.

[chromium-reviews, 2016-12-22 19:10:18]

Thanks. I was wondering why unique_ptr was introduced.

On Thu, Dec 22, 2016 at 11:08 AM rtoy@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> I guess I should have mentioned that a lot of the changes are because I
> changed
> the event list to hold pointers instead of objects so that the ownership
> of the
> actual event is made clear. I think (it's been too long) that if you don't
> do
> this, some events would get destructed while it was still being used
> because we
> had pointers or refs to them.
>
> https://codereview.chromium.org/1533103002/
>

-- 
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.

[Raymond Toy, 2016-12-22 19:21:35]

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp (right):

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:253:
DCHECK(getType() == ParamEvent::CancelValues);
On 2016/12/22 18:41:36, hongchan wrote:
> DCHECK_EQ() might be better.

Done.

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:258:
DCHECK(getType() == ParamEvent::CancelValues);
On 2016/12/22 18:41:36, hongchan wrote:
> ditto.

Done.

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:263:
DCHECK(getType() == ParamEvent::CancelValues);
On 2016/12/22 18:41:36, hongchan wrote:
> ditto.

Done.

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.cpp:700: 
On 2016/12/22 18:41:36, hongchan wrote:
> nit: an empty line.

Done.

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h (right):

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h:163: // For
CancelValues events. Not valid for any other event
On 2016/12/22 18:41:37, hongchan wrote:
> nit: a missing period. Also there is a couple of missing periods in the
comments
> below.

Done.

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h:182: // Create
simplest event needing just a value and time, like setValueAtTime
On 2016/12/22 18:41:37, hongchan wrote:
> nit: a missing period.

Done.

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h:186: // time in
case
On 2016/12/22 18:41:37, hongchan wrote:
> nit: the line breaks at the wrong position?

Done.

https://codereview.chromium.org/1533103002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioParamTimeline.h:238: //    
SetValueCurve events.
On 2016/12/22 18:41:37, hongchan wrote:
> These comments should be located just above each variable. I think that's our
> convention. No?

Done.

[hongchan, 2017-01-03 17:52:42]

lgtm

This is a new feature, so I believe the only possible (sane) way to discover
weird corner cases is to get it out there.

Great work!

[Raymond Toy, 2017-01-03 21:28:13]

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

[Raymond Toy, 2017-01-03 21:28:14]

foolip: PTAL as API Owner

[Raymond Toy, 2017-01-03 21:29:20]

The linux test failure is a flaky test. It is being tracked in crbug.com/678069
and we will wait for that fix before landing this CL.

[Raymond Toy, 2017-01-05 22:59:58]

The CQ bit was checked by rtoy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 23:00:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 01:35:48]

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

[commit-bot: I haz the power, 2017-01-06 01:35:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Raymond Toy, 2017-01-09 19:20:38]

Friendly ping.

[Raymond Toy, 2017-01-10 16:19:21]

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

[Raymond Toy, 2017-01-10 16:19:23]

tkent: PTAL as API Owner

[Raymond Toy, 2017-01-10 23:07:29]

The CQ bit was checked by rtoy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 23:08:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tkent, 2017-01-10 23:32:56]

lgtm

[commit-bot: I haz the power, 2017-01-11 01:26:53]

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

[commit-bot: I haz the power, 2017-01-11 01:26:56]

Dry run: This issue passed the CQ dry run.

[foolip, 2017-01-11 13:45:09]

lgtm, I'm slow and sorry.

[Raymond Toy, 2017-01-11 16:02:59]

The CQ bit was checked by rtoy@chromium.org

[Raymond Toy, 2017-01-11 16:03:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
hongchan@chromium.org
Link to the patchset: https://codereview.chromium.org/1533103002/#ps640001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-11 16:03:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 16:09:31]

CQ is committing da patch.

Bot data: {"patchset_id": 640001, "attempt_start_ts": 1484150579722150,
"parent_rev": "97501194ee5fac524c6126c7e7bd2184d9fcfca6", "commit_rev":
"4260025dd80eed380e0c1fff0e4be2bdd0ae0e8a"}

[commit-bot: I haz the power, 2017-01-11 16:10:05]

Description was changed from

==========
Implement cancelValuesAndHoldAtTime

cancelValuesAndHoldAtTime(time) should cancel all events after time |time|
and when the context current time reaches |time|, the automation value
that is current at that time is held as if setValueAtTime were called
with the appropriate value and time.

Spec:
https://webaudio.github.io/web-audio-api/#widl-AudioParam-cancelAndHoldAtTime...
Intent:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/IftXib6yFyw/CLoxngG9...

BUG=432934
TEST=audioparam-cancel-scheduled-values.html
==========

to

==========
Implement cancelValuesAndHoldAtTime

cancelValuesAndHoldAtTime(time) should cancel all events after time |time|
and when the context current time reaches |time|, the automation value
that is current at that time is held as if setValueAtTime were called
with the appropriate value and time.

Spec:
https://webaudio.github.io/web-audio-api/#widl-AudioParam-cancelAndHoldAtTime...
Intent:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/IftXib6yFyw/CLoxngG9...

BUG=432934
TEST=audioparam-cancel-scheduled-values.html

Review-Url: https://codereview.chromium.org/1533103002
Cr-Commit-Position: refs/heads/master@{#442916}
Committed:
https://chromium.googlesource.com/chromium/src/+/4260025dd80eed380e0c1fff0e4b...
==========

[commit-bot: I haz the power, 2017-01-11 16:10:06]

Committed patchset #33 (id:640001) as
https://chromium.googlesource.com/chromium/src/+/4260025dd80eed380e0c1fff0e4b...