Closed Bug 1308436 Opened 4 years ago Closed 2 years ago

Change behavior of setValueCurveAtTime to add an event at the end of the curve

Categories

(Core :: Web Audio, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed
firefox64 --- fixed

People

(Reporter: padenot, Assigned: padenot)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(30 files)

59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
karlt
: review+
Details
59 bytes, text/x-review-board-request
karlt
: review-
Details
59 bytes, text/x-review-board-request
karlt
: review-
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
karlt
: review+
Details
59 bytes, text/x-review-board-request
karlt
: review+
Details
59 bytes, text/x-review-board-request
karlt
: review+
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
karlt
: review-
Details
59 bytes, text/x-review-board-request
karlt
: review-
Details
59 bytes, text/x-review-board-request
karlt
: review+
Details
59 bytes, text/x-review-board-request
karlt
: review+
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
karlt
: review+
Details
46 bytes, text/x-phabricator-request
karlt
: review+
Details | Review
46 bytes, text/x-phabricator-request
karlt
: review+
Details | Review
46 bytes, text/x-phabricator-request
karlt
: review+
Details | Review
46 bytes, text/x-phabricator-request
karlt
: review+
Details | Review
46 bytes, text/x-phabricator-request
karlt
: review+
Details | Review
46 bytes, text/x-phabricator-request
karlt
: review+
Details | Review
46 bytes, text/x-phabricator-request
karlt
: review+
Details | Review
46 bytes, text/x-phabricator-request
karlt
: review+
Details | Review
46 bytes, text/x-phabricator-request
karlt
: review+
Details | Review
46 bytes, text/x-phabricator-request
karlt
: review+
Details | Review
46 bytes, text/x-phabricator-request
karlt
: review+
Details | Review
46 bytes, text/x-phabricator-request
karlt
: review+
Details | Review
46 bytes, text/x-phabricator-request
karlt
: review+
Details | Review
46 bytes, text/x-phabricator-request
karlt
: review+
Details | Review
57.62 KB, patch
Details | Diff | Splinter Review
`setValueCurveAtTime` is modified so that an implicit call to `setValueAtTime` is made at the end of the curve duration.  This makes any following automation events start from the end of the `setValueCurveAtTime` event with the last value of the curve.

Spec changes at https://github.com/webaudio/web-audio-api/commit/ee1090becbcc71ca21b08abeb5ad4b8061d65627.
Rank: 25
Summary: Specify behavior of setValueCurveAtTime → Change behavior of setValueCurveAtTime to add an event at the end of the curve
Depends on: 1184057
Mass change P2->P3 to align with new Mozilla triage process.
Priority: P2 → P3
Assignee: nobody → theo.rabut
I've opened a spec bug for this: https://github.com/WebAudio/web-audio-api/issues/1434
Comment on attachment 8922764 [details]
Bug 1308436 - Implicit call to SetValueAtTime in SetValueCurve

https://reviewboard.mozilla.org/r/193910/#review198992

::: dom/media/webaudio/AudioEventTimeline.h:183
(Diff revision 2)
> -          TimeOf(mEvents[i]) + mEvents[i].mDuration >= TimeOf(aEvent)) {
> +          TimeOf(mEvents[i]) + mEvents[i].mDuration > TimeOf(aEvent)) {
>          aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
>          return false;
>        }
>      }
> -
> +      

Trailing spaces.

::: dom/media/webaudio/AudioParam.h
(Diff revision 2)
>      if (!WebAudioUtils::IsTimeValid(aStartTime)) {
>        aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
>        return this;
>      }
>      aValues.ComputeLengthAndData();
> -

Leave this blank line, please.

::: dom/media/webaudio/AudioParam.h:62
(Diff revision 2)
> -
>      aStartTime = std::max(aStartTime, GetParentObject()->CurrentTime());
>      EventInsertionHelper(aRv, AudioTimelineEvent::SetValueCurve,
>                           aStartTime, 0.0f, 0.0f, aDuration, aValues.Data(),
>                           aValues.Length());
> +    

Trailing spaces.

::: dom/media/webaudio/AudioParam.h:64
(Diff revision 2)
>      EventInsertionHelper(aRv, AudioTimelineEvent::SetValueCurve,
>                           aStartTime, 0.0f, 0.0f, aDuration, aValues.Data(),
>                           aValues.Length());
> +    
> +    if(aDuration > 0){
> +      aStartTime = aStartTime + aDuration;

Don't overwrite the parameter, use another variable with a meaningful name.

::: dom/media/webaudio/AudioParam.h:66
(Diff revision 2)
>                           aValues.Length());
> +    
> +    if(aDuration > 0){
> +      aStartTime = aStartTime + aDuration;
> +      EventInsertionHelper(aRv, AudioTimelineEvent::SetValueAtTime, aStartTime,
> +                           aValues.Data()[aValues.Length()-1]);

Spaces around '-'.
Attachment #8922764 - Flags: review?(padenot)
When running the test:

[task 2017-11-03T13:45:52.888Z] 13:45:52     INFO - TEST-UNEXPECTED-FAIL | dom/media/webaudio/test/test_audioParamSetCurveAtTimeTwice.html | uncaught exception - SyntaxError: An invalid or illegal string was specified at createGraph@http://mochi.test:8888/tests/dom/media/webaudio/test/test_audioParamSetCurveAtTimeTwice.html:35:5
[task 2017-11-03T13:45:52.888Z] 13:45:52     INFO - runTestOnContext@http://mochi.test:8888/tests/dom/media/webaudio/test/webaudio.js:202:20
[task 2017-11-03T13:45:52.888Z] 13:45:52     INFO - testOnNormalContext@http://mochi.test:8888/tests/dom/media/webaudio/test/webaudio.js:222:7
[task 2017-11-03T13:45:52.889Z] 13:45:52     INFO - runTestFunction@http://mochi.test:8888/tests/dom/media/webaudio/test/webaudio.js:253:5
[task 2017-11-03T13:45:52.890Z] 13:45:52     INFO - rval@http://mochi.test:8888/tests/SimpleTest/SimpleTest.js:146:17
[task 2017-11-03T13:45:52.891Z] 13:45:52     INFO - EventHandlerNonNull*this.addLoadEvent@http://mochi.test:8888/tests/SimpleTest/SimpleTest.js:171:13
[task 2017-11-03T13:45:52.892Z] 13:45:52     INFO - @http://mochi.test:8888/tests/SimpleTest/SimpleTest.js:1441:5
Flags: needinfo?(theo.rabut)
Comment on attachment 8922764 [details]
Bug 1308436 - Implicit call to SetValueAtTime in SetValueCurve

https://reviewboard.mozilla.org/r/193910/#review201492

This fails tests.
Attachment #8922764 - Flags: review?(padenot)
This test is supposed to fail. I asked for a new review with a good one.
Flags: needinfo?(theo.rabut)
I've picked this up again, it's slightly more complicated that it looks like.
Assignee: theo.rabut → padenot
Depends on: 1478837
https://treeherder.mozilla.org/#/jobs?repo=try&revision=728473580461486734e879ce90a49410f7f1284d

This went a bit out of the initial scope, but it's mostly test fixes to align with what the spec says, related to AudioParams.
Comment on attachment 8995572 [details]
Bug 1308436 - Don't convert the curve duration from double to float in the EventInsertionHelper to avoid losing precision.

https://reviewboard.mozilla.org/r/259974/#review267422
Attachment #8995572 - Flags: review?(karlt) → review+
Comment on attachment 8995573 [details]
Bug 1308436 - Allow inserting an event strictly after the a value curve.

https://reviewboard.mozilla.org/r/259976/#review267434

::: dom/media/webaudio/AudioEventTimeline.h:173
(Diff revision 2)
>        if (mEvents[i].mType == AudioTimelineEvent::SetValueCurve &&
>            !(aEvent.mType == AudioTimelineEvent::SetValueCurve &&
>              TimeOf(aEvent) == TimeOf(mEvents[i])) &&
>            TimeOf(mEvents[i]) <= TimeOf(aEvent) &&
> -          TimeOf(mEvents[i]) + mEvents[i].mDuration >= TimeOf(aEvent)) {
> -        aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
> +          TimeOf(mEvents[i]) + mEvents[i].mDuration > TimeOf(aEvent)) {
> +          aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);

2 space indent inside block.
Attachment #8995573 - Flags: review?(karlt) → review-
Comment on attachment 8995575 [details]
Bug 1308436 - Throw correct exceptions when calling SetValueCurveAtTime with invalid parameters.

https://reviewboard.mozilla.org/r/259980/#review267448

::: dom/media/webaudio/AudioEventTimeline.h:166
(Diff revision 2)
>        aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
>        return false;
>      }
>  
> +    if (aEvent.mType == AudioTimelineEvent::SetValueCurve) {
> +      if (aEvent.mCurveLength < 2) {

Why not use the existing SetValueCurve block above and just tighten the mCurveLength check?

It seems odd to throw for length of zero and one point and throw for length of one elsewhere.
Attachment #8995575 - Flags: review?(karlt)
Comment on attachment 8995576 [details]
Bug 1308436 - Fix typo in audioparam-setValueCurve-exceptions.html.

https://reviewboard.mozilla.org/r/259982/#review267450
Attachment #8995576 - Flags: review?(karlt) → review+
Comment on attachment 8995578 [details]
Bug 1308436 - Update other WPT expectations for event-insertion.html.

https://reviewboard.mozilla.org/r/259986/#review267452
Attachment #8995578 - Flags: review?(karlt) → review+
Comment on attachment 8995580 [details]
Bug 1308436 - Throw a range error when the curve duration is negative, when calling SetValueCurveAtTime.

https://reviewboard.mozilla.org/r/259990/#review267456

::: dom/media/webaudio/AudioEventTimeline.h:158
(Diff revision 2)
>        if (!aEvent.mCurve || !aEvent.mCurveLength) {
>          aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>          return false;
>        }
> +      if (aEvent.mDuration <= 0) {
> +        aRv.Throw(NS_ERROR_RANGE_ERR);

The spec says a "RangeError exception" and links to the "simple exception" corresponding to the ECMAScript error object.  I assume this should be ThrowRangeError().
Attachment #8995580 - Flags: review?(karlt) → review-
Comment on attachment 8995581 [details]
Bug 1308436 - Throw proper exceptions when passing in out of range time values in automation methods.

https://reviewboard.mozilla.org/r/259992/#review267458

::: dom/media/webaudio/AudioEventTimeline.h:148
(Diff revision 2)
>      };
>  
>      // Validate the event itself
>      if (!WebAudioUtils::IsTimeValid(TimeOf(aEvent)) ||
>          !WebAudioUtils::IsTimeValid(aEvent.mTimeConstant)) {
> -      aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
> +      aRv.Throw(NS_ERROR_RANGE_ERR);

I assume this and elsewhere should be the ECMAScript object.

::: dom/media/webaudio/AudioEventTimeline.h:163
(Diff revision 2)
>      bool timeAndValueValid = IsValid(aEvent.mValue) &&
>                               IsValid(aEvent.mDuration);
>      if (!timeAndValueValid) {
> -      aRv.Throw(NS_ERROR_DOM_SYNTAX_ERR);
> +      aRv.Throw(NS_ERROR_RANGE_ERR);
>        return false;
>      }

Is this actually reachable or can this just be an assertion?

I expect webidl provides that these are finite.
Attachment #8995581 - Flags: review?(karlt) → review-
Comment on attachment 8995582 [details]
Bug 1308436 - Remove expectation file for audioparam-exceptional-values.html.ini, it now passes.

https://reviewboard.mozilla.org/r/259994/#review267460
Attachment #8995582 - Flags: review?(karlt) → review+
Comment on attachment 8995580 [details]
Bug 1308436 - Throw a range error when the curve duration is negative, when calling SetValueCurveAtTime.

https://reviewboard.mozilla.org/r/259990/#review267462

::: dom/media/webaudio/AudioEventTimeline.h:157
(Diff revision 2)
>      if (aEvent.mType == AudioTimelineEvent::SetValueCurve) {
>        if (!aEvent.mCurve || !aEvent.mCurveLength) {
>          aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>          return false;
>        }
> +      if (aEvent.mDuration <= 0) {

I don't like the spec throwing for zero duration because that case could easily be handled, but interoperability wins here.
Comment on attachment 8996011 [details]
Bug 1308436 - Remove test_audioParamSetCurveAtTimeZeroDuration.html.

https://reviewboard.mozilla.org/r/260284/#review267464
Attachment #8996011 - Flags: review?(karlt) → review+
Comment on attachment 8996012 [details]
Bug 1308436 - Fix test: webaudioeditor/test/browser_callwatcher-02.js.

https://reviewboard.mozilla.org/r/260286/#review267466

::: devtools/client/webaudioeditor/test/browser_callwatcher-02.js:32
(Diff revision 1)
>    error = await evalInDebuggee("throwDOMException()");
>    is(error.lineNumber, 37, "exception has correct lineNumber");
>    is(error.columnNumber, 0, "exception has correct columnNumber");
> -  is(error.code, 9, "exception has correct code");
> +  is(error.code, 0, "exception has correct code");

This isn't going make much sense when this is an error object.

I guess throwDOMException should be updated to something that still throws a DOMException.
Attachment #8996012 - Flags: review?(karlt)
Comment on attachment 8996013 [details]
Bug 1308436 - Update WPT for audioparam-method-chaining.html.

https://reviewboard.mozilla.org/r/260288/#review267468
Attachment #8996013 - Flags: review?(karlt) → review+
Comment on attachment 8995574 [details]
Bug 1308436 - Insert a SetValueAtTime event right after the curve, when doing a SetValueCurveAtTime.

https://reviewboard.mozilla.org/r/259978/#review267438

::: commit-message-c417f:1
(Diff revision 2)
> +Bug 1308436 - Insert a SetValueAtTime event right after the curve, when doing a SetValueCurveAtTime. r?karlt

Can you update the comment, please, in the code here that depends on this?
https://searchfox.org/mozilla-central/rev/033d45ca70ff32acf04286244644d19308c359d5/dom/media/webaudio/AudioEventTimeline.cpp#236-237

There will now always be a next node.
And the reason why aNext doesn't need to be considered in determining
values after the curve is that it is always SetValueAtTime.
A MOZ_ASSERT() to confirm that would also be useful.

::: dom/media/webaudio/AudioParam.h:66
(Diff revision 2)
> +    EventInsertionHelper(aRv, AudioTimelineEvent::SetValueAtTime,
> +                         aStartTime + aDuration, aValues[aValues.Length() - 1]);

The intention is that "following automations will start from the end of the
setValueCurveAtTime() event."

Calling EventInsertionHelper() will place the SetValueAtTime after any
following automations already at T + D, essentially erasing existing following
automations.

I expect it will be easier to add the implicit SetValueAtTime immediately
after SetValueCurve in AudioEventTimeline::InsertEvent().

I expect there are alternative approaches also that do not require having what
is really a redundant AudioTimelineEvent.  e.g. aValues[aValues.Length() - 1]
could be passed as the aValue parameter to EventInsertionHelper and
TimeOf(aPrevious) in AudioEventTimeline::GetValuesAtTimeHelperInternal()
could be changed to include mDuration.
Attachment #8995574 - Flags: review?(karlt) → review-
Comment on attachment 8995579 [details]
Bug 1308436 - Implement proper behaviour for SetValueCurveAtTime.

https://reviewboard.mozilla.org/r/259988/#review267444

::: commit-message-9a871:6
(Diff revision 2)
> +Bug 1308436 - Implement proper behaviour for SetValueCurveAtTime. r?karlt
> +
> +We're using a special event, not exposed to content, that behaves like a
> +SetValueAtTime, but represents the end of a curve. This allows calling
> +SetValueCurveAtTime twice with the same start time and different durations
> +(which is valid, events of the same type at the same time replace each other).

Can you point me at the part of the spec, please, that implies that "events of
the same type at the same time replace each other".

"If one of these events is added at a time where there is already one or more
events, then it will be placed in the list after them".

But the curves would overlap in that situation, and so I would apply
"a NotSupportedError exception MUST be thrown if any automation method is
called at a time which is inside of the time interval of a SetValueCurve event
at time and duration" to that situation.

"inside" is not particularly specific about whether it includes events at T,
but the same overlap problem would exist for any type of event placed at T.

If this is really the desired behavior, then I suspect this would be
simpler without the redundant event.
Attachment #8995579 - Flags: review?(karlt)
Comment on attachment 8995577 [details]
Bug 1308436 - Update wpt expectations for audioparamn-setValueCurve-exceptions.html.

https://reviewboard.mozilla.org/r/259984/#review267470

::: testing/web-platform/meta/webaudio/the-audio-api/the-audioparam-interface/audioparam-setValueCurve-exceptions.html.ini:2
(Diff revision 2)
> -  [X setValueAtTime(1, 0.018750000000000003) threw "SyntaxError" instead of NotSupportedError.]
> +  [X exponentialRampToValueAtTime(1, 0.0075) incorrectly threw SyntaxError: "An invalid or illegal string was specified".]
>      expected: FAIL
>  
> -  [X linearRampToValueAtTime(1, 0.018750000000000003) threw "SyntaxError" instead of NotSupportedError.]
> +  [< [start-end\] 1 out of 9 assertions were failed.]
>      expected: FAIL
>  
> -  [X exponentialRampToValueAtTime(1, 0.018750000000000003) threw "SyntaxError" instead of NotSupportedError.]
> +  [# AUDIT TASK RUNNER FINISHED: 1 out of 5 tasks were failed.]

I wonder why
Attachment #8995577 - Flags: review?(karlt) → review+
Comment on attachment 8995577 [details]
Bug 1308436 - Update wpt expectations for audioparamn-setValueCurve-exceptions.html.

https://reviewboard.mozilla.org/r/259984/#review267470

> I wonder why

I was wondering why the exponentialRampToValueAtTime subtest name had changed, but it hasn't.
I guess that linearRampToValueAtTime now passing means that
exponentialRampToValueAtTime(1, 0.0075) has started to run.

Anyway, I meant to delete this comment.
Comment on attachment 8995579 [details]
Bug 1308436 - Implement proper behaviour for SetValueCurveAtTime.

https://reviewboard.mozilla.org/r/259988/#review267518

::: commit-message-9a871:6
(Diff revision 2)
> +Bug 1308436 - Implement proper behaviour for SetValueCurveAtTime. r?karlt
> +
> +We're using a special event, not exposed to content, that behaves like a
> +SetValueAtTime, but represents the end of a curve. This allows calling
> +SetValueCurveAtTime twice with the same start time and different durations
> +(which is valid, events of the same type at the same time replace each other).

huh, this got removed from the spec in:

https://github.com/WebAudio/web-audio-api/issues/573#issuecomment-151032325
Looks like I implemented badly the wrong behaviour, trying to preserve some of what gecko does today and that we encode in tests, but it's wrong.
Comment on attachment 9002466 [details]
Bug 1308436 - Update WPT expectations for audioparam-method-chaining.html. r?karlt

Karl Tomlinson (:karlt) has approved the revision.
Attachment #9002466 - Flags: review+
I filed [0] so that we make the rules for inserting events more precise. I've addressed all the comments, but we're failing the wpt case where two curves are trying to be inserted at the same time [1].

[0]: https://github.com/WebAudio/web-audio-api/issues/1721
[1]: https://searchfox.org/mozilla-central/source/testing/web-platform/tests/webaudio/the-audio-api/the-audioparam-interface/event-insertion.html#60
Looks like that test is out of date.  Attempting to insert the second (overlapping) curve should throw.
Ramps now take into account the end of the curve as a start value for their
automation.
MozReview-Commit-ID: DDAUkkwdACv
Per spec, it should throw. This is tested in WPT.

MozReview-Commit-ID: InzdbPQM6HD
It's too difficult to maintain, obsolete and also it duplicates the
functionnality of wpt.
Comment on attachment 9003508 [details]
Bug 1308436 - Don't convert the curve duration from double to float in the EventInsertionHelper to avoid losing precision. r?karlt

Karl Tomlinson (:karlt) has approved the revision.
Attachment #9003508 - Flags: review+
Comment on attachment 9003513 [details]
Bug 1308436 - Fix typo in audioparam-setValueCurve-exceptions.html. r?karlt

Karl Tomlinson (:karlt) has approved the revision.
Attachment #9003513 - Flags: review+
Comment on attachment 9003518 [details]
Bug 1308436 - Remove audioparam-setValueCurve-exceptions.html.ini, it's now passing. r?karlt

Karl Tomlinson (:karlt) has approved the revision.
Attachment #9003518 - Flags: review+
Comment on attachment 9003516 [details]
Bug 1308436 - Update other WPT expectations for event-insertion.html. r?karlt

Karl Tomlinson (:karlt) has approved the revision.
Attachment #9003516 - Flags: review+
Comment on attachment 9003520 [details]
Bug 1308436 - Remove expectation file for audioparam-exceptional-values.html.ini, it now passes. r?karlt

Karl Tomlinson (:karlt) has approved the revision.
Attachment #9003520 - Flags: review+
Comment on attachment 9003514 [details]
Bug 1308436 - Remove test_audioParamSetCurveAtTimeZeroDuration.html. r?karlt

Karl Tomlinson (:karlt) has approved the revision.
Attachment #9003514 - Flags: review+
I wonder whether it is the phabsend command from
https://bitbucket.org/kmaglione/hgext/src/default/phabricator.py
that caused the funny dependency graph in the "Stack" at
https://phabricator.services.mozilla.com/D4108
which is not connected to the Stack at
https://phabricator.services.mozilla.com/D3791

"arc patch --nobranch D4103" doesn't apply due to conflicts, but using "arc
patch --nobranch D4099" followed by "arc patch --skip-dependencies --nobranch
<rev>" for each revision in the order of bugzilla attachments applies fine.

I don't know whether that is related to the disconnected stacks, attempts to
apply to https://hg.mozilla.org/mozilla-central/rev/3f99a091261f (the resting
place of D3507) and silently rebase (but leave behind pruned changesets), or
something else.

Anyway I can find my way around now.
Comment on attachment 9003509 [details]
Bug 1308436 - Add a test case for a negative curve duration in audioparam-exceptional-values.html. r?karlt

Karl Tomlinson (:karlt) has approved the revision.
Attachment #9003509 - Flags: review+
(In reply to Karl Tomlinson (:karlt) from comment #80)
> I don't know whether that is related to the disconnected stacks,

https://phabricator.services.mozilla.com/D4099 was missing from the
dependencies of the other stack, which is why it did not apply.

> attempts to and silently rebase (but leave behind pruned changesets)

This was due to the --nobranch argument.
Comment on attachment 9003517 [details]
Bug 1308436 - Update devtools tests with another Web Audio API call the throws the right exception. r?karlt

Karl Tomlinson (:karlt) has approved the revision.
Attachment #9003517 - Flags: review+
Comment on attachment 9003511 [details]
Bug 1308436 - Adjust the AudioParam processing algorithm to match the spec.  r?karlt

Karl Tomlinson (:karlt) has approved the revision.
Attachment #9003511 - Flags: review+
Comment on attachment 9003519 [details]
Bug 1308436 - Remove TestAudioEventTimeline.cpp. r?karlt

Karl Tomlinson (:karlt) has approved the revision.
Attachment #9003519 - Flags: review+
Comment on attachment 9003521 [details]
Bug 1308436 - Fix a test case and add more test cases for overlapping SetValueCurveAtTime. r?karlt

Karl Tomlinson (:karlt) has approved the revision.
Attachment #9003521 - Flags: review+
Comment on attachment 9003522 [details]
Bug 1308436 - Remove test_audioParamSetCurveAtTimeTwice.html because it's now wrong per spec. r?karlt

Karl Tomlinson (:karlt) has approved the revision.
Attachment #9003522 - Flags: review+
Comment on attachment 9002466 [details]
Bug 1308436 - Update WPT expectations for audioparam-method-chaining.html. r?karlt

Karl Tomlinson (:karlt) has requested changes to the revision.
Attachment #9002466 - Flags: review+
Comment on attachment 9003513 [details]
Bug 1308436 - Fix typo in audioparam-setValueCurve-exceptions.html. r?karlt

Karl Tomlinson (:karlt) has requested changes to the revision.
Attachment #9003513 - Flags: review+
Comment on attachment 9003509 [details]
Bug 1308436 - Add a test case for a negative curve duration in audioparam-exceptional-values.html. r?karlt

Karl Tomlinson (:karlt) has requested changes to the revision.
Attachment #9003509 - Flags: review+
Comment on attachment 9003521 [details]
Bug 1308436 - Fix a test case and add more test cases for overlapping SetValueCurveAtTime. r?karlt

Karl Tomlinson (:karlt) has requested changes to the revision.
Attachment #9003521 - Flags: review+
If bug 1473915 is not fixed (and it doesn't look like it will be soon), then
the changesets that modify web platform tests (not expectations) need changes
to MANIFEST.json.

Bug 1480644 may mean that sending MANIFEST.json changes to Phabricator is not
a good idea.  (Sometimes people have had more success than other times.)  If
not, then just apply the changes locally before pushing.
I've marked the revisions that need such changes with "Request changes" to
help identify, even though Phabricator may not be the best place to perform
those changes.
Attachment #9002466 - Attachment description: Bug 1308436 - Update WPT for audioparam-method-chaining.html. r?karlt → Bug 1308436 - Update WPT expectations for audioparam-method-chaining.html. r?karlt
Comment on attachment 9002466 [details]
Bug 1308436 - Update WPT expectations for audioparam-method-chaining.html. r?karlt

Karl Tomlinson (:karlt) has approved the revision.
Attachment #9002466 - Flags: review+
Comment on attachment 9003509 [details]
Bug 1308436 - Add a test case for a negative curve duration in audioparam-exceptional-values.html. r?karlt

Karl Tomlinson (:karlt) has approved the revision.
Attachment #9003509 - Flags: review+
Comment on attachment 9003513 [details]
Bug 1308436 - Fix typo in audioparam-setValueCurve-exceptions.html. r?karlt

Karl Tomlinson (:karlt) has approved the revision.
Attachment #9003513 - Flags: review+
Comment on attachment 9003521 [details]
Bug 1308436 - Fix a test case and add more test cases for overlapping SetValueCurveAtTime. r?karlt

Karl Tomlinson (:karlt) has approved the revision.
Attachment #9003521 - Flags: review+
Comment on attachment 9003510 [details]
Bug 1308436 - Rework the AudioParam event insertion algorithm to be spec compliant, and fix the exception types and values. r?karlt

Karl Tomlinson (:karlt) has approved the revision.
Attachment #9003510 - Flags: review+
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c781bf4c4970
Don't convert the curve duration from double to float in the EventInsertionHelper to avoid losing precision. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/08897a32ca05
Add a test case for a negative curve duration in audioparam-exceptional-values.html. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a729b75c155
Rework the AudioParam event insertion algorithm to be spec compliant, and fix the exception types and values. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f2140bc66e3
Adjust the AudioParam processing algorithm to match the spec.  r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ad67bac8643
Fix typo in audioparam-setValueCurve-exceptions.html. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dfd506e16f4
Remove test_audioParamSetCurveAtTimeZeroDuration.html. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/09a3186f939e
Update other WPT expectations for event-insertion.html. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b69df966f74
Update WPT for audioparam-method-chaining.html. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/33972bbac0d6
Update devtools tests with another Web Audio API call the throws the right exception. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd578b3bcf54
Remove audioparam-setValueCurve-exceptions.html.ini, it's now passing. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/a61e3c9c4ba7
Remove expectation file for audioparam-exceptional-values.html.ini, it now passes. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3233377aec2
Remove TestAudioEventTimeline.cpp. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c8691a86020
Fix a test case and add more test cases for overlapping SetValueCurveAtTime. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7d905753c20
Remove test_audioParamSetCurveAtTimeTwice.html because it's now wrong per spec. r=karlt
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12773 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
https://phabricator.services.mozilla.com/D4099?vs=10240&id=12155&whitespace=ignore-most#toc
indicates that the commits that landed were the first ones submitted to phabricator, not the ones that were reviewed.
Phabricator says "This revision was automatically updated to reflect the committed changes" for every revision, even the ones that did not change during the review process, which makes the message unhelpful at indicating which revisions pushed differ from the ones reviewed.  The order of commits is also the order prior to review.
Depends on: 1488218
Flags: needinfo?(padenot)
I don't know what happened. I'm going to back everything out and reland the right ones.
Flags: needinfo?(padenot)
I've manually gone through all the patches and grafted the revisions on top of central, on top of a backout patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e13834bf32e6adb986ffd8405f38a74d7ee379d
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
It's green. I'm going to request uplift on the backout so we return to the previous code in 63 (it's too late), and then re-land everything on inbound (backout + new patches).
The wrong patch have landed. I need those patches to be backed out from beta 63, to restore the previous state of the code.

This is the output of a `hg diff --reverse` command and is green on try.
Attachment #9006274 - Flags: approval-mozilla-beta?
Pushed by paul@paul.cx:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1452a7d4442
Backout all patches of bug 1308436, wrong patches have landed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5d40e82bb57
Don't convert the curve duration from double to float in the EventInsertionHelper to avoid losing precision. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba41d875df9b
Add a test case for a negative curve duration in audioparam-exceptional-values.html. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7d0486eaac8
Remove TestAudioEventTimeline.cpp. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/65901b920626
Rework the AudioParam event insertion algorithm to be spec compliant, and fix the exception types and values. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/6321efdf65d5
Adjust the AudioParam processing algorithm to match the spec. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5a98b2f4fdc
Fix typo in audioparam-setValueCurve-exceptions.html. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/8834636b7209
Remove test_audioParamSetCurveAtTimeZeroDuration.html. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaf9bdaa1bfb
Update other WPT expectations for event-insertion.html. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/41fbd4fd0622
Update devtools tests with another Web Audio API call the throws the right exception. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/57720a1c67dc
Remove audioparam-setValueCurve-exceptions.html.ini, it's now passing. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/290b7ee630e3
Remove expectation file for audioparam-exceptional-values.html.ini, it now passes. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6a1baa45d1c
Fix a test case and add more test cases for overlapping SetValueCurveAtTime. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fcef05c05bb
Remove test_audioParamSetCurveAtTimeTwice.html because it's now wrong per spec. r=karlt
https://hg.mozilla.org/integration/mozilla-inbound/rev/f58ddafd8a65
Update WPT expectations for audioparam-method-chaining.html. r=karlt
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12831 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
(In reply to Paul Adenot (:padenot) from comment #107)
> Created attachment 9006274 [details] [diff] [review]
> Backout patch for 63
> 
> The wrong patch have landed. I need those patches to be backed out from beta
> 63, to restore the previous state of the code.
> 
> This is the output of a `hg diff --reverse` command and is green on try.

Forgot to put the try for this:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=282a241af571236919c4790725e9d978c0708358
Comment on attachment 9006274 [details] [diff] [review]
Backout patch for 63

Approved for 63 beta 4, thanks for the quick action.
Attachment #9006274 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Documentation updated:

https://developer.mozilla.org/en-US/docs/Web/API/AudioParam/setValueCurveAtTime

And listed on Firefox 63 for developers.
Depends on: 1502748
Depends on: 1503950
Attachment #8922764 - Flags: review?(padenot)
You need to log in before you can comment on or make changes to this bug.