Paul Adenot (:padenot) Assignee
	Description • 8 years ago

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

	Bulk Bug Changes for mreavy's org
	Comment 1 • 7 years ago

Mass change P2->P3 to align with new Mozilla triage process.

	Paul Adenot (:padenot) Assignee
	Comment 4 • 7 years ago

I've opened a spec bug for this: https://github.com/WebAudio/web-audio-api/issues/1434

	Paul Adenot (:padenot) Assignee
	Comment 5 • 7 years ago
mozreview-review

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

	Paul Adenot (:padenot) Assignee
	Comment 7 • 7 years ago

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

	Paul Adenot (:padenot) Assignee
	Comment 8 • 7 years ago
mozreview-review

Comment on attachment 8922764 [details]
Bug 1308436 - Implicit call to SetValueAtTime in SetValueCurve

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

This fails tests.

	theo.rabut
	Comment 11 • 6 years ago

This test is supposed to fail. I asked for a new review with a good one.

	Paul Adenot (:padenot) Assignee
	Comment 12 • 6 years ago

I've picked this up again, it's slightly more complicated that it looks like.

	Paul Adenot (:padenot) Assignee
	Comment 38 • 6 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 39 • 6 years ago
mozreview-review

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

	Karl Tomlinson (:karlt)
	Comment 40 • 6 years ago
mozreview-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.

	Karl Tomlinson (:karlt)
	Comment 41 • 6 years ago
mozreview-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.

	Karl Tomlinson (:karlt)
	Comment 42 • 6 years ago
mozreview-review

Comment on attachment 8995576 [details]
Bug 1308436 - Fix typo in audioparam-setValueCurve-exceptions.html.

https://reviewboard.mozilla.org/r/259982/#review267450

	Karl Tomlinson (:karlt)
	Comment 43 • 6 years ago
mozreview-review

Comment on attachment 8995578 [details]
Bug 1308436 - Update other WPT expectations for event-insertion.html.

https://reviewboard.mozilla.org/r/259986/#review267452

	Karl Tomlinson (:karlt)
	Comment 44 • 6 years ago
mozreview-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().

	Karl Tomlinson (:karlt)
	Comment 45 • 6 years ago
mozreview-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.

	Karl Tomlinson (:karlt)
	Comment 46 • 6 years ago
mozreview-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

	Karl Tomlinson (:karlt)
	Comment 47 • 6 years ago
mozreview-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.

	Karl Tomlinson (:karlt)
	Comment 48 • 6 years ago
mozreview-review

Comment on attachment 8996011 [details]
Bug 1308436 - Remove test_audioParamSetCurveAtTimeZeroDuration.html.

https://reviewboard.mozilla.org/r/260284/#review267464

	Karl Tomlinson (:karlt)
	Comment 49 • 6 years ago
mozreview-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.

	Karl Tomlinson (:karlt)
	Comment 50 • 6 years ago
mozreview-review

Comment on attachment 8996013 [details]
Bug 1308436 - Update WPT for audioparam-method-chaining.html.

https://reviewboard.mozilla.org/r/260288/#review267468

	Karl Tomlinson (:karlt)
	Comment 51 • 6 years ago
mozreview-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.

	Karl Tomlinson (:karlt)
	Comment 52 • 6 years ago
mozreview-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.

	Karl Tomlinson (:karlt)
	Comment 53 • 6 years ago
mozreview-review

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

	Karl Tomlinson (:karlt)
	Comment 54 • 6 years ago
mozreview-review-reply

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.

	Paul Adenot (:padenot) Assignee
	Comment 55 • 6 years ago
mozreview-review

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

	Paul Adenot (:padenot) Assignee
	Comment 56 • 6 years ago

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.

	Paul Adenot (:padenot) Assignee
	Comment 57 • 6 years ago

MozReview-Commit-ID: KmfDil9LKGS

	Karl Tomlinson (:karlt)
	Comment 58 • 6 years ago

Comment on attachment 9002466 [details]
Bug 1308436 - Update WPT expectations for audioparam-method-chaining.html. r?karlt

Karl Tomlinson (:karlt) has approved the revision.

	Paul Adenot (:padenot) Assignee
	Comment 59 • 6 years ago

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

	Karl Tomlinson (:karlt)
	Comment 60 • 6 years ago

Looks like that test is out of date.  Attempting to insert the second (overlapping) curve should throw.

	Paul Adenot (:padenot) Assignee
	Comment 61 • 6 years ago

MozReview-Commit-ID: 9vazrYi3J8C

	Paul Adenot (:padenot) Assignee
	Comment 64 • 6 years ago

Ramps now take into account the end of the curve as a start value for their
automation.

	Paul Adenot (:padenot) Assignee
	Comment 65 • 6 years ago

MozReview-Commit-ID: DDAUkkwdACv

	Paul Adenot (:padenot) Assignee
	Comment 66 • 6 years ago

Per spec, it should throw. This is tested in WPT.

MozReview-Commit-ID: InzdbPQM6HD

	Paul Adenot (:padenot) Assignee
	Comment 67 • 6 years ago

MozReview-Commit-ID: BRcethELDJB

	Paul Adenot (:padenot) Assignee
	Comment 70 • 6 years ago

It's too difficult to maintain, obsolete and also it duplicates the
functionnality of wpt.

	Paul Adenot (:padenot) Assignee
	Comment 71 • 6 years ago

MozReview-Commit-ID: 4iXmUDgr81x

	Karl Tomlinson (:karlt)
	Comment 74 • 6 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 75 • 6 years ago

Comment on attachment 9003513 [details]
Bug 1308436 - Fix typo in audioparam-setValueCurve-exceptions.html. r?karlt

Karl Tomlinson (:karlt) has approved the revision.

	Karl Tomlinson (:karlt)
	Comment 76 • 6 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 77 • 6 years ago

Comment on attachment 9003516 [details]
Bug 1308436 - Update other WPT expectations for event-insertion.html. r?karlt

Karl Tomlinson (:karlt) has approved the revision.

	Karl Tomlinson (:karlt)
	Comment 78 • 6 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 79 • 6 years ago

Comment on attachment 9003514 [details]
Bug 1308436 - Remove test_audioParamSetCurveAtTimeZeroDuration.html. r?karlt

Karl Tomlinson (:karlt) has approved the revision.

	Karl Tomlinson (:karlt)
	Comment 80 • 6 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 81 • 6 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 82 • 6 years ago

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

	Karl Tomlinson (:karlt)
	Comment 83 • 6 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 84 • 6 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 85 • 6 years ago

Comment on attachment 9003519 [details]
Bug 1308436 - Remove TestAudioEventTimeline.cpp. r?karlt

Karl Tomlinson (:karlt) has approved the revision.

	Karl Tomlinson (:karlt)
	Comment 86 • 6 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 87 • 6 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 88 • 6 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 89 • 6 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 90 • 6 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 91 • 6 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 92 • 6 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 93 • 6 years ago

Comment on attachment 9002466 [details]
Bug 1308436 - Update WPT expectations for audioparam-method-chaining.html. r?karlt

Karl Tomlinson (:karlt) has approved the revision.

	Karl Tomlinson (:karlt)
	Comment 94 • 6 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 95 • 6 years ago

Comment on attachment 9003513 [details]
Bug 1308436 - Fix typo in audioparam-setValueCurve-exceptions.html. r?karlt

Karl Tomlinson (:karlt) has approved the revision.

	Karl Tomlinson (:karlt)
	Comment 96 • 6 years ago

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.

	Karl Tomlinson (:karlt)
	Comment 97 • 6 years ago

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.

	Pulsebot
	Comment 98 • 6 years ago

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

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 99 • 6 years ago

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12773 for changes under testing/web-platform/tests

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 100 • 6 years ago

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

	Cristina Coroiu [:ccoroiu]
	Comment 101 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/c781bf4c4970
https://hg.mozilla.org/mozilla-central/rev/08897a32ca05
https://hg.mozilla.org/mozilla-central/rev/0a729b75c155
https://hg.mozilla.org/mozilla-central/rev/9f2140bc66e3
https://hg.mozilla.org/mozilla-central/rev/9ad67bac8643
https://hg.mozilla.org/mozilla-central/rev/7dfd506e16f4
https://hg.mozilla.org/mozilla-central/rev/09a3186f939e
https://hg.mozilla.org/mozilla-central/rev/6b69df966f74
https://hg.mozilla.org/mozilla-central/rev/33972bbac0d6
https://hg.mozilla.org/mozilla-central/rev/cd578b3bcf54
https://hg.mozilla.org/mozilla-central/rev/a61e3c9c4ba7
https://hg.mozilla.org/mozilla-central/rev/f3233377aec2
https://hg.mozilla.org/mozilla-central/rev/1c8691a86020
https://hg.mozilla.org/mozilla-central/rev/a7d905753c20

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 102 • 6 years ago

Upstream PR merged

	Karl Tomlinson (:karlt)
	Comment 103 • 6 years ago

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.

	Paul Adenot (:padenot) Assignee
	Comment 104 • 6 years ago

I don't know what happened. I'm going to back everything out and reland the right ones.

	Paul Adenot (:padenot) Assignee
	Comment 105 • 6 years ago

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

	Paul Adenot (:padenot) Assignee
	Comment 106 • 6 years ago

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

	Paul Adenot (:padenot) Assignee
	Comment 107 • 6 years ago

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.

	Pulsebot
	Comment 108 • 6 years ago

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

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 109 • 6 years ago

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12831 for changes under testing/web-platform/tests

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 110 • 6 years ago

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 111 • 6 years ago

Upstream PR merged

	Dorel Luca [:dluca]
	Comment 112 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/d5d40e82bb57
https://hg.mozilla.org/mozilla-central/rev/ba41d875df9b
https://hg.mozilla.org/mozilla-central/rev/d7d0486eaac8
https://hg.mozilla.org/mozilla-central/rev/65901b920626
https://hg.mozilla.org/mozilla-central/rev/6321efdf65d5
https://hg.mozilla.org/mozilla-central/rev/b5a98b2f4fdc
https://hg.mozilla.org/mozilla-central/rev/8834636b7209
https://hg.mozilla.org/mozilla-central/rev/eaf9bdaa1bfb
https://hg.mozilla.org/mozilla-central/rev/41fbd4fd0622
https://hg.mozilla.org/mozilla-central/rev/57720a1c67dc
https://hg.mozilla.org/mozilla-central/rev/290b7ee630e3
https://hg.mozilla.org/mozilla-central/rev/b6a1baa45d1c
https://hg.mozilla.org/mozilla-central/rev/7fcef05c05bb
https://hg.mozilla.org/mozilla-central/rev/f58ddafd8a65

	Paul Adenot (:padenot) Assignee
	Comment 113 • 6 years ago

(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

	Pascal Chevrel:pascalc (PTO until April 26)
	Comment 114 • 6 years ago

Comment on attachment 9006274 [details] [diff] [review]
Backout patch for 63

Approved for 63 beta 4, thanks for the quick action.

	Eric Shepherd [:sheppy]
	Comment 115 • 6 years ago

Documentation updated:

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

And listed on Firefox 63 for developers.