Closed Bug 1216843 Opened 9 years ago Closed 8 years ago

implement effect iteration composition

Categories

(Core :: DOM: Animation, defect)

Product:
Core
Component:
DOM: Animation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox44 --- affected
firefox51 --- fixed

People

(Reporter: heycam, Assigned: hiro)

References

(Depends on 1 open bug, Blocks 1 open bug,
URL
)

Details

Attachments

(15 files, 2 obsolete files)

Bug 1216843 - Part 1: Tests for effect iteration composition.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1216843 - Part 2: Implement effect iteration composition.
58 bytes, text/x-review-board-request
birtles
: review+
smaug
: review+
Details
Bug 1216843 - Part 3: Use nsCSSValue instead of nscolor to store over 1.0 color components as an accumulated value so that we can correctly calculate intermediate values in interpolation.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1216843 - Part 4: Pass nsCSSValue& to AddWeightedColors and DiluteColor.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1216843 - Part 5: Reuse AddWeightedColors and DiluteColor in AddShadowItems().
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1216843 - Part 7: Implement color accumulation.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1216843 - Part 8: Make AddShadowItems() return UniquePtr and rename it to AddWeightedShadowItems().
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1216843 - Part 9: Factor out AddWeightedShadowList.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1216843 - Part 10: Implement box-shadow/text-shadow color accumulation.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1216843 - Part 11: Make AddFilterFunction and AddFilterFunctionImpl return UniquePtr and rename them to AddWeightedFilterXX.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1216843 - Part 12: Factor out AddWeightedFilterList.
58 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1216843 - Part 13: Implement color accumulation in filter property.
58 bytes, text/x-review-board-request
Details
Bug 1216843 - Part 14: Reftest for iterationComposite.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1216843 - Part 15: Update styles when current iteration changed.
58 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1216843 - Part 16: Fix bug number for implementation of keyframe composition.
58 bytes, text/x-review-board-request
birtles
: review+
Details 
Script-created Web Animations support a composite operation -- "replace", "add" or "accumulate".  We don't currently ignore the composite value on a Keyframe dictionary passed to the KeyframeEffectReadOnly constructor.  We should store it on the AnimationPropertySegements and have the effect computation take it into account.
Depends on: 1211783
According to a bug number in KeyframeEffect.webidl, this bug seems to aim implementation of iteration composition.
URL: https://w3c.github.io/web-animations/...
Summary: implement animation composition → implement animation iteration composition
Summary: implement animation iteration composition → implement effect iteration composition
To accumulate transform properties we need to convert them to matrix using nsStyleTransformMatrix::ReadTransform.  And the function needs nsStyleContext to resolve unit conversion.  I'd like to do this conversion in KeyframeEffectReadOnly::UpdateProperties() and store the converted matrices in AnimationPropertySegment if iterationComposite is Accumulate.  The converted matrices will also be used for keyframe composition.

Brian, does this sound reasonable?  Or should we store the converted matrices into AnimationProperty as Maybe<nsTArray<Matrix4x4>> to reduce AnimationPropertySegment size?
Flags: needinfo?(bbirtles)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> To accumulate transform properties we need to convert them to matrix using
> nsStyleTransformMatrix::ReadTransform.

We should accumulate transform properties using their specified transform functions. e.g. rotate(180deg) accumulated with rotate(180deg) should be rotate(360deg). If we use matrices I think we will end up with rotate(0deg).

There is a rough spec for this here: https://w3c.github.io/web-animations/#transform-list-animation-type-section

However that spec text needs a lot of work.
Flags: needinfo?(bbirtles)
Thank you, Brian.  I've started writing test cases, I have no idea how to check the rotation value over 360deg though.

A fundamental question; Given that below animation, 

 div.animate({ margin-left: [ '0px', '10px' ]}, { duration: 1000, iterationComposite: 'accumulate', iterations: 10 });

which one is the correct value at 2500ms?

a) (5px + 10px) + (5px + 10px) = 30px
b) 5px + (10px + 10px) = 25px

According to the spec <https://w3c.github.io/web-animations/#the-effect-value-of-a-keyframe-animation-effect>;

 2. If this keyframe effect has an iteration composite operation of accumulate, apply the following step current iteration times:

 replace the property value of target property on keyframe with the result of combining the property value (Va) with the property value on the final keyframe in property-specific keyframes (Vb) using the accumulation procedure defined for target property.

It seems a) is correct, but it is not intuitive to me because it looks getting faster and faster.
Flags: needinfo?(bbirtles)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> Thank you, Brian.  I've started writing test cases, I have no idea how to
> check the rotation value over 360deg though.
> 
> A fundamental question; Given that below animation, 
> 
>  div.animate({ margin-left: [ '0px', '10px' ]}, { duration: 1000,
> iterationComposite: 'accumulate', iterations: 10 });
> 
> which one is the correct value at 2500ms?
> 
> a) (5px + 10px) + (5px + 10px) = 30px
> b) 5px + (10px + 10px) = 25px
> 
> According to the spec
> <https://w3c.github.io/web-animations/#the-effect-value-of-a-keyframe-
> animation-effect>;
> 
>  2. If this keyframe effect has an iteration composite operation of
> accumulate, apply the following step current iteration times:
> 
>  replace the property value of target property on keyframe with the result
> of combining the property value (Va) with the property value on the final
> keyframe in property-specific keyframes (Vb) using the accumulation
> procedure defined for target property.
> 
> It seems a) is correct, but it is not intuitive to me because it looks
> getting faster and faster.

It should be (b).

According to the spec:

> 10. Let interval endpoints be an empty sequence of keyframes.

Interval endpoints = []

> 11. Populate interval endpoints by following the steps from the first
>     matching condition from below:
> ... (iteration progress = 0.5 so skip to the last step) ...
>     Otherwise,
>     1. Append to interval endpoints the last keyframe in
>        property-specific keyframes whose computed keyframe offset is
>        less than or equal to iteration progress and less than 1. If
>        there is no such keyframe (because, for example, the iteration
>        progress is negative), add the last keyframe whose computed
>        keyframe offset is 0.

Interval endpoints = [ { marginLeft: '0px' } ]

>     2. Append to interval endpoints the next keyframe in
>        property-specific keyframes after the one added in the previous
>        step.

Interval endpoints = [ { marginLeft: '0px' }, { marginLeft: '10px' } ]

> 12. For each keyframe in interval endpoints:

Applying to the first keyframe: { marginLeft: '0px' }

>     1. If keyframe has a composite operation that is not replace, or
>        keyframe has no composite operation and the composite operation
>        of this keyframe effect is not replace, then perform the
>        following steps:

(Does not apply)

>     2. If this keyframe effect has an iteration composite operation of
>        accumulate, apply the following step current iteration times:

current iteration = 2

>          replace the property value of target property on keyframe
>          with the result of combining the property value (Va) with the
>          property value on the final keyframe in property-specific
>          keyframes (Vb) using the accumulation procedure defined for
>          target property.

First pass:
{ marginLeft: '0px' } -> { marginLeft: '0px' + '10px' }
                      -> { marginLeft: '10px' }

Second pass (since current iteration = 2):
{ marginLeft: '10px' } -> { marginLeft: '10px' + '10px' }
                       -> { marginLeft: '20px' }

Interval endpoints = [ { marginLeft: '20px' }, { marginLeft: '10px' } ]

After the next iteration of step 12, applied to the second keyframe in
interval endpoints, { marginLeft: '10px' } we end up with:

Interval endpoints = [ { marginLeft: '20px' }, { marginLeft: '30px' } ]

> 13. If there is only one keyframe in interval endpoints return the
>     property value of target property on that keyframe.

(Does not apply)

> 14. Let start offset be the computed keyframe offset of the first
>     keyframe in interval endpoints.

start offset = 0.0

> 15. Let end offset be the computed keyframe offset of last keyframe in
>     interval endpoints.

end offset = 1.0

> 16. Let interval distance be the result of evaluating (iteration
>     progress - start offset) / (end offset - start offset).

interval distance = 0.5

> 17. Let transformed distance be the result of evaluating the timing
>     function associated with the first keyframe in interval endpoints
>     passing interval distance as the input progress.

transformed distance = 0.5

> 18. Return the result of applying the interpolation procedure defined
>     by the animation type of the target property, to the values of the
>     target property specified on the two keyframes in interval
>     endpoints taking the first such value as Vstart and the second as
>     Vend and using transformed distance as the interpolation parameter
>     p.

result = (1 - p) * Vstart + p * Vend
       = (1 - 0.5) * 20px + 0.5 * 30px
       = 0.5 * 20px + 0.5 * 30px
       = 10px + 15px
       = 25px
Flags: needinfo?(bbirtles)
As I and Brian discussed about this, we'd like to re-use StyleAnimationValue::AddWeighted() to accumulate for all properties other than color.  For color accumulation we need to store over 1.0 color components.  For example;

 div.animate({ color: ['rgb(0,0,0)', 'rgb(200,200,200)']}, { duration: 100, iterations: 3, iterationComposite: 'accumulate'});

The color property of this animation on the second iteration should be interpolated from rgb(200,200,200) to rgb(400,400,400).  If we don't store over 1.0 color components, the color property is interpolated from rgb(200,200,200) to rgb(255,255,255).

So I'd like to do here in this bug.

1) Change mColor in StyleAnimationValue to nsCSSValue instead of nscolor.
2) Add eCSSUnit_OverflowRGBAColor to nsCSSValue to store over 1.0 color components. (Or should we extend eCSSUnit_PercentageRGBAColor?)
3) Factor out several new functions, which returns UniquePtr<>, e.g. returning interpolated color's nsCSSValue etc.,  to be reused by accumulation too.

Hi Daniel,  Do these sound reasonable?

For reference, here is a try with the patch series including 1) to 3).
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e36b8f1084e1b448dd1c55e742e874cb61ffd6d0
Flags: needinfo?(dholbert)
I think that sounds reasonable, yeah.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> 2) Add eCSSUnit_OverflowRGBAColor to nsCSSValue to store over 1.0 color
> components. (Or should we extend eCSSUnit_PercentageRGBAColor?)

We probably should extend eCSSUnit_PercentageRGBAColor (though I'm not super-familiar with that unit's usages, so maybe there's a reason we'd need a new unit after all).  I do notice we have this comment in the nsCSSValueFloatColor impl right now (the thin that backs eCSSUnit_PercentageRGBAColor):
    // FIXME: We should not be clamping specified RGB color components.
https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSValue.h#1692

So it looks like that type is already asking to be extended, so that author-provided eCSSUnit_PercentageRGBAColor values can store "overflow" values (regardless of any animation).

> For reference, here is a try with the patch series including 1) to 3).

A few nits from skimming the patches:
 - You have two patches labeled "Part 6". I think the first one ("Part 6: Factor out AddColor") really wants to be "Part 4".
 - Right now it looks like that "Factor out AddColor" patch moves that color-adding code, and then Part 5 moves it again (up higher in the file so it can be used by another function).  It'd be better to just move it to its final location in the initial refactoring patch, and then have Part 5 simply add a new caller & make tweaks as-needed rather than moving it again.
 - AddColor should really be named AddWeightedColors, since that's what it does (it adds two *weighted* colors, not simply two colors).  (This is also useful as a hint/reminder to the reader that its signature is similar to that of AddWeighted.)
Flags: needinfo?(dholbert)
Thank you, Daniel for the suggestions!  I've changed to extend eCSSUnit_PercentageRGBAColor and renamed couple of functions to AddWeightedXX.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=157e9a7b7144
Review commit: https://reviewboard.mozilla.org/r/69414/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69414/
Attachment #8778040 - Flags: review?(bbirtles)
Attachment #8778041 - Flags: review?(bugs)
Attachment #8778041 - Flags: review?(bbirtles)
Attachment #8778042 - Flags: review?(dholbert)
Attachment #8778043 - Flags: review?(dholbert)
Attachment #8778044 - Flags: review?(dholbert)
Attachment #8778045 - Flags: review?(dholbert)
Attachment #8778046 - Flags: review?(dholbert)
Attachment #8778047 - Flags: review?(dholbert)
Attachment #8778048 - Flags: review?(dholbert)
Attachment #8778049 - Flags: review?(dholbert)
Attachment #8778050 - Flags: review?(dholbert)
Attachment #8778051 - Flags: review?(dholbert)
Attachment #8778052 - Flags: review?(dholbert)
Attachment #8778053 - Flags: review?(bbirtles)
Attachment #8778054 - Flags: review?(bbirtles)
Attachment #8778055 - Flags: review?(bbirtles)
GetColorValue() for eCSSUnit_PercentageRGBAColor returns a clamped value,
Red/Green/Blue/Alpha() returns a raw value instead.

Review commit: https://reviewboard.mozilla.org/r/69424/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69424/
The content of this bug (1216843) has changed since it filed initially,
so we should change bug numbers in our source tree.

Re-generating ini file re-ordered items in the ini file.

Review commit: https://reviewboard.mozilla.org/r/69444/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69444/
Hello Olli, could you please take a look change of KeyframeEffect.webidl in part 2 patch?
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
Comment on attachment 8778041 [details]
Bug 1216843 - Part 2: Implement effect iteration composition.

https://reviewboard.mozilla.org/r/69416/#review66666

r+ for the .webidl
Attachment #8778041 - Flags: review?(bugs) → review+
Comment on attachment 8778040 [details]
Bug 1216843 - Part 1: Tests for effect iteration composition.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69414/diff/1-2/
Comment on attachment 8778041 [details]
Bug 1216843 - Part 2: Implement effect iteration composition.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69416/diff/1-2/
Comment on attachment 8778042 [details]
Bug 1216843 - Part 3: Use nsCSSValue instead of nscolor to store over 1.0 color components as an accumulated value so that we can correctly calculate intermediate values in interpolation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69418/diff/1-2/
Comment on attachment 8778043 [details]
Bug 1216843 - Part 4: Pass nsCSSValue& to AddWeightedColors and DiluteColor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69420/diff/1-2/
Comment on attachment 8778044 [details]
Bug 1216843 - Part 5: Reuse AddWeightedColors and DiluteColor in AddShadowItems().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69422/diff/1-2/
Comment on attachment 8778045 [details]
Bug 1216843 - Part 6: A dummy empty changeset.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69424/diff/1-2/
Comment on attachment 8778046 [details]
Bug 1216843 - Part 7: Implement color accumulation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69426/diff/1-2/
Comment on attachment 8778047 [details]
Bug 1216843 - Part 8: Make AddShadowItems() return UniquePtr and rename it to AddWeightedShadowItems().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69428/diff/1-2/
Comment on attachment 8778048 [details]
Bug 1216843 - Part 9: Factor out AddWeightedShadowList.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69430/diff/1-2/
Comment on attachment 8778049 [details]
Bug 1216843 - Part 10: Implement box-shadow/text-shadow color accumulation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69432/diff/1-2/
Comment on attachment 8778050 [details]
Bug 1216843 - Part 11: Make AddFilterFunction and AddFilterFunctionImpl return UniquePtr and rename them to AddWeightedFilterXX.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69434/diff/1-2/
Comment on attachment 8778051 [details]
Bug 1216843 - Part 12: Factor out AddWeightedFilterList.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69436/diff/1-2/
Comment on attachment 8778052 [details]
Bug 1216843 - Part 13: Implement color accumulation in filter property.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69438/diff/1-2/
Comment on attachment 8778053 [details]
Bug 1216843 - Part 14: Reftest for iterationComposite.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69440/diff/1-2/
Comment on attachment 8778054 [details]
Bug 1216843 - Part 15: Update styles when current iteration changed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69442/diff/1-2/
Comment on attachment 8778055 [details]
Bug 1216843 - Part 16: Fix bug number for implementation of keyframe composition.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69444/diff/1-2/
Comment on attachment 8778040 [details]
Bug 1216843 - Part 1: Tests for effect iteration composition.

https://reviewboard.mozilla.org/r/69414/#review66644

These tests are really really good, but I wonder what you think about combining them with the tests in bug 1277433? In particular, see part 3 of that bug, where we have animation-model/animation-types/type-per-property.html. The eventual idea is that in that we would test that each property uses the correct procedures for interpolation / composition / addition / distance.

So, if a new spec define a new property with a new animation type they&#39;d update that file with the new property and new procedures for interpolation / composition etc.? Would that be easier to maintain?

::: testing/web-platform/meta/web-animations/interfaces/KeyframeEffect/iterationComposite.html.ini:30
(Diff revision 1)
> +  [iterationComposite of filter drop-shadow animation]
> +    expected: FAIL
> +

Is this fixed by the end of this patch series? (I couldn't see it). If not, can we add a bug number here?

::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/iterationComposite.html:24
(Diff revision 2)
> +    'Animated margin-left style at 50s on the first iteration');
> +  anim.currentTime = anim.effect.timing.duration * 2;
> +  assert_equals(getComputedStyle(div).marginLeft, '20px',
> +    'Animated margin-left style at 0s on the third iteration');
> +  anim.currentTime += anim.effect.timing.duration / 2;
> +  assert_equals(getComputedStyle(div).marginLeft, '25px',
> +    'Animated margin-left style at 50s on the third iteration');

Nit: s/on the xxx iteration/of the xxx iteration/

(Likewise elsewhere in this file.)

::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/iterationComposite.html:36
(Diff revision 2)
> +}, 'iterationComposite of <length> type animation');
> +
> +test(function(t) {
> +  var div = createDiv(t);
> +  div.style.width = '100%';
> +  var initialWidth = parseFloat(getComputedStyle(div).width);

Are we sure we're not getting an initialWidth of 0px here? (If we do, all these tests will pass since all the values will be zero!)

Would it be easier to add a parent element with position:relative and a fixed width?

::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/iterationComposite.html:59
(Diff revision 2)
> +    div.animate({ color: ['rgb(0, 0, 0)', 'rgb(120, 120, 120)'] },
> +                { duration: 100 * MS_PER_SEC,
> +                  easing: 'linear',
> +                  iterations: 10,
> +                  iterationComposite: 'accumulate' });

This is mostly just for my own records because I'll probably have the same question again in the future.

I was wondering what happens if one or more of the channels is decreasing.

For example, if we have:

  rgb(0, 200, 0) -> rgb(100, 100, 100)

Then you might expect we should get something like:

  progress = 0.5 -> rgb(50, 150, 50)
  progress = 1.0 -> rgb(100, 100, 100)
  progress = 1.5 -> rgb(150, 50, 150)

But the spec says we do:

  progress = 1.5 -> rgb(150, 250, 150)

That seemed wrong but I think it's actually correct. That's what SMIL does and once we support additive animation I think you can produce the other effect by other means. I think the alternative--adjusting values of the current iteration so that the last value of the previous iteration equals the first value of the new iteration--is more complex and makes it difficult to add jumps between iterations when that is the desired effect.

At the end of the day, this feature is mostly added to support SMIL so I think doing the same thing as SMIL is probably fine. Perhaps we can add another type of accumulation in future if we need the other behavior.

But for now, I wonder if it's worth adjusting this test so that one of the channels is decreasing to check that we do the correct thing in that case?

::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/iterationComposite.html:119
(Diff revision 2)
> +  assert_equals(getComputedStyle(div).clip, 'rect(20px, 20px, 20px, 20px)',
> +    'Animated cli style at 0s on the third iteration');
> +  anim.currentTime += anim.effect.timing.duration / 2;
> +  assert_equals(getComputedStyle(div).clip, 'rect(25px, 25px, 25px, 25px)',
> +    'Animated clip style at 50s on the third iteration');
> +}, 'iterationComposite of <shape> type animation');

If you get a chance, would you mind making up an etherpad / Google doc with a rough outline of the procedures used here? Unless they're really obvious?

Before we ship this we need to spec this somewhere. According to [1] that's going to be CSS Values and Units Level 4 where we will probably write out the procedure for interpolation/addition/accumulation/distance for all types.

[1] https://github.com/w3c/csswg-drafts/issues/338

::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/iterationComposite.html:140
(Diff revision 2)
> +  assert_equals(getComputedStyle(div).width, '20px',
> +    'Animated calc width style at 0s on the third iteration');
> +  anim.currentTime += anim.effect.timing.duration / 2;
> +  assert_equals(getComputedStyle(div).width, '25px',
> +    'Animated calc width style at 50s on the third iteration');
> +}, 'iterationComposite of <calc()> value animation');

Can we add a test for something that can't be reduced, e.g. calc(10% + 20px) applied to something where getComputedStyle returns the computed style (and not the used style)?

There's only a pretty limited number of properties where getComputedStyle returns the used value but according to bug 1292447 we actually return the used value in a few cases where we shouldn't. I think there are some properties where we correctly return the computed style (perhaps some of the background-* properties?)

::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/iterationComposite.html:191
(Diff revision 2)
> +}, 'iterationComposite of box-shadow animation');
> +
> +test(function(t) {
> +  var div = createDiv(t);
> +  var anim =
> +    div.animate({ filter: ['blur(0px)', 'blur(10px)'] },

Should we add a test for when the function list doesn't match?

e.g.

a) "blur(0px) sepia(50%)" -> "blur(10px)"
b) "blur(0px) sepia(50%)" -> "sepia(50%) blur(10px)"
c) "blur(0px)" -> "none"

(b) and (c) should fall back to discrete animation and so I guess there should be no accumulation?

For (a) I'd be tempted to say that we expand the second value to "blur(10px) sepia(0%)" but I don't think we do that for transforms and I guess we should be consistent with transforms?


Oh, never mind, I see below that you have these tests. Great!

::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/iterationComposite.html:251
(Diff revision 2)
> +  assert_equals(getComputedStyle(div).filter,
> +    'brightness(0.5)',
> +    'Animated filter brightness style at 50s on the first iteration');
> +  anim.currentTime = anim.effect.timing.duration * 2;
> +  assert_equals(getComputedStyle(div).filter,
> +    'brightness(0)', // brightness(1) is not accumulated.

Why not?

::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/iterationComposite.html:326
(Diff revision 2)
> +    // We can't accumulate 'contrast(2) brightness(2)' onto
> +    // the first list 'brightness(1) contrast(1)' because of
> +    // mismatch of the order.
> +    'brightness(1) contrast(1)',

Excellent! That is exactly what I was looking for.

::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/iterationComposite.html:333
(Diff revision 2)
> +    // We *can* accumulate 'contrast(2) brightness(2)' onto
> +    // the same list 'contrast(2) brigheness(2)' here.
> +    'contrast(4) brightness(4)', // discrete

Oh, interesting! That seems a bit odd but maybe it is ok?

(Nit: s/brigheness/brightness/)

::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/iterationComposite.html:343
(Diff revision 2)
> +    div.animate({ filter: ['sepia(0)',
> +                           'sepia(1) contrast(2)'] },
> +                { duration: 100 * MS_PER_SEC,
> +                  easing: 'linear',
> +                  iterations: 10,
> +                  iterationComposite: 'accumulate' });
> +  anim.pause();
> +
> +  anim.currentTime = anim.effect.timing.duration / 2;
> +  assert_equals(getComputedStyle(div).filter,
> +    'sepia(0.5) contrast(1.5)',
> +    'Animated filter list at 50s on the first iteration');

Oh neat! This seems preferable to me, but is it inconsistent with what we do for transforms? Do we fill in missing functions for transforms?

In any case, we need to spec this so if you have a chance to summarize the different procedures that would be helpful.

::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/iterationComposite.html:381
(Diff revision 2)
> +  assert_equals(getComputedStyle(div).transform,
> +    'matrix(0, 1, -1, 0, 0, 0)', // rotate(90deg)
> +    'Animated transform(rotate) style at 50s on the first iteration');
> +  anim.currentTime = anim.effect.timing.duration * 2;
> +  assert_equals(getComputedStyle(div).transform,
> +    'matrix(1, 0, 0, 1, 0, 0)', // rotate(360deg) how can we check it?

There's no need to check it I think -- we probably need to check, however, that we continue rotating in the right direction?

::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/iterationComposite.html:405
(Diff revision 2)
> +  assert_equals(getComputedStyle(div).transform,
> +    'matrix(0.5, 0, 0, 0.5, 0, 0)', // scale(0.5)
> +    'Animated transform(scale) style at 50s on the first iteration');
> +  anim.currentTime = anim.effect.timing.duration * 2;
> +  assert_equals(getComputedStyle(div).transform,
> +    'matrix(0, 0, 0, 0, 0, 0)', // scale(0); scale(1) is not accumulated.

Why is that?

::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/iterationComposite.html:453
(Diff revision 2)
> +    'matrix(2, 0, 0, 2, 0, 0)', // scale(2)
> +    'Animated transform(scale) style at 0s on the third iteration');

I don't quite understand this but I haven't looked at the procedure we're using for accumulating scale yet.

::: testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/iterationComposite.html:484
(Diff revision 2)
> +    'Animated transform list at 0s on the third iteration');
> +  anim.currentTime += anim.effect.timing.duration / 2;
> +  assert_equals(getComputedStyle(div).transform,
> +    'matrix(0, 1, -1, 0, 0, 25)', // rotate(450deg) translateX(25px)
> +    'Animated transform list at 50s on the third iteration');
> +}, 'iterationComposite of transform list animation');

Do we need a test for mismatched transform lists? Including one where we *could* fill in the missing functions? (Although I'm not sure if we *should* fill them in or not)
Attachment #8778040 - Flags: review?(bbirtles) → review+
Comment on attachment 8778041 [details]
Bug 1216843 - Part 2: Implement effect iteration composition.

https://reviewboard.mozilla.org/r/69416/#review66648

::: dom/animation/KeyframeEffect.h:480
(Diff revision 1)
>    // that to update the properties rather than calling
>    // GetStyleContextForElement.
>    void SetTarget(const Nullable<ElementOrCSSPseudoElement>& aTarget);
>  
> +  void SetIterationComposite(
> +    const IterationCompositeOperation& aIterateComposite);

s/aIterateComposite/sIterationComposite/

::: dom/animation/KeyframeEffect.cpp:679
(Diff revision 2)
> +    StyleAnimationValue fromValue = segment->mFromValue;
> +    StyleAnimationValue toValue = segment->mToValue;

Is there a way we could avoid copying these two StyleAnimationValues in the common case where the composite mode is not accumulate or the current iteration is < 1 ?

::: dom/animation/KeyframeEffect.cpp:685
(Diff revision 2)
> +      const AnimationPropertySegment& lastSegment =
> +        prop.mSegments[prop.mSegments.Length() - 1];

Can we use LastElement() here?

::: dom/animation/KeyframeEffect.cpp:1695
(Diff revision 2)
> +  mEffectOptions.mIterationComposite = aIterationComposite;
> +  RequestRestyle(EffectCompositor::RestyleType::Layer);
> +}

We need to call nsNodeUtils::AnimationChanged here, something like:

  if (mAnimation && mAnimation->IsRelevant()) {
    nsNodeUtils::AnimationChanged(mAnimation);
  }

And I guess we also need to add a mutation observer test.

::: dom/animation/KeyframeEffectParams.h:12
(Diff revision 2)
>  #ifndef mozilla_KeyframeEffectParams_h
>  #define mozilla_KeyframeEffectParams_h
>  
>  #include "nsCSSProps.h"
>  #include "nsString.h"
> +#include "mozilla/dom/KeyframeEffectBinding.h" //For IterationCompositeOperation

(Really really really minor nit: Add a space between "//" and "For", or, if this makes the line exceed 80 characters, just drop the "For" altogether)

::: gfx/layers/composite/AsyncCompositionManager.cpp:568
(Diff revision 2)
> +  StyleAnimationValue startValue = aStart;
> +  StyleAnimationValue endValue = aEnd;
> +  // Iteration composition for accumulate
> +  if (static_cast<dom::IterationCompositeOperation>(aAnimation.iterationComposite()) ==
> +        dom::IterationCompositeOperation::Accumulate &&
> +      aCurrentIteration > 0) {
> +    StyleAnimationValue::Accumulate(aAnimation.property(),
> +                                    startValue,
> +                                    aLastValue,
> +                                    aCurrentIteration);
> +    StyleAnimationValue::Accumulate(aAnimation.property(),
> +                                    endValue,
> +                                    aLastValue,
> +                                    aCurrentIteration);
> +  }

This is a bit unfortunate to repeat the same code we have in KeyframeEffectReadOnly::ComposeStyle.

Can we factor out a utility function for the common code? Feel free to do this as a separate patch/bug, however.

::: gfx/layers/composite/AsyncCompositionManager.cpp:571
(Diff revision 2)
>                 "Must have same unit");
> -  StyleAnimationValue::Interpolate(aAnimation.property(), aStart, aEnd,
> +
> +  StyleAnimationValue startValue = aStart;
> +  StyleAnimationValue endValue = aEnd;
> +  // Iteration composition for accumulate
> +  if (static_cast<dom::IterationCompositeOperation>(aAnimation.iterationComposite()) ==

Nit: > 80 chars?

::: gfx/layers/composite/AsyncCompositionManager.cpp:705
(Diff revision 2)
>            // interpolate the property
>            Animatable interpolatedValue;
> -          SampleValue(portion, animation, animData.mStartValues[segmentIndex],
> -                      animData.mEndValues[segmentIndex], &interpolatedValue, layer);
> +          SampleValue(portion, animation,
> +                      animData.mStartValues[segmentIndex],
> +                      animData.mEndValues[segmentIndex],
> +                      animData.mEndValues[segmentSize - 1],

Would animData.mEndValues.LastElement() be better here?

::: layout/style/StyleAnimationValue.h:129
(Diff revision 2)
>                            double aCoeff1, const StyleAnimationValue& aValue1,
>                            double aCoeff2, const StyleAnimationValue& aValue2,
>                            StyleAnimationValue& aResultValue);
>  
> +  /**
> +   * Accumulates |aValueToAccumulte| onto |aDest| |aCount| times.

s/aValueToAccumulte/aValueToAccumulate/

::: layout/style/StyleAnimationValue.h:130
(Diff revision 2)
>                            double aCoeff2, const StyleAnimationValue& aValue2,
>                            StyleAnimationValue& aResultValue);
>  
> +  /**
> +   * Accumulates |aValueToAccumulte| onto |aDest| |aCount| times.
> +   * The result is stored in |aDest| if success.

s/if success/on success/

::: layout/style/StyleAnimationValue.h:134
(Diff revision 2)
> +   * Accumulates |aValueToAccumulte| onto |aDest| |aCount| times.
> +   * The result is stored in |aDest| if success.
> +   *
> +   * @param aDest              The base value to be accumulated.
> +   * @param aValueToAccumulate The value to accumulate.
> +   * @param aCount             The number of times to accmulate aValueToAdd.

s/accmulate/accumulate/

::: layout/style/StyleAnimationValue.cpp:2771
(Diff revision 2)
> +    //case eUnit_Shadow:
> +    //case eUnit_Filter:
> +    default:
> +      return Add(aProperty, aDest, aValueToAccumulate, aCount);
> +  }
> +  MOZ_ASSERT(false, "Can't accumulate using the given common unit");

Should we just use MOZ_ASSERT_UNREACHABLE here?
Attachment #8778041 - Flags: review?(bbirtles) → review+
Comment on attachment 8778053 [details]
Bug 1216843 - Part 14: Reftest for iterationComposite.

https://reviewboard.mozilla.org/r/69440/#review66990

Looks good but I should probably check the updated tests.

::: layout/reftests/web-animations/style-updates-on-iteration-composition-changed-from-accumulate-to-replace.html:22
(Diff revision 2)
> +    // rAF callback is usually 16.6ms interval, so now we are in the second
> +    // iteration. That means now margin-left of the element must be 400px.

This seems a bit flaky. First of all, Element.animate() doesn't begin right away. In fact, by the next animation frame the currentTime might still be zero.

Is there another approach that is more reliable that doesn't depend on the rAF interval. For example, could we continually call rAF until anim.effect.getComputedTiming().currentIteration == 1?

::: layout/reftests/web-animations/style-updates-on-iteration-composition-changed-from-replace-to-accumulate.html:22
(Diff revision 2)
> +    // rAF callback is usually 16.6ms interval, so now we are in the second
> +    // iteration.

As with the previous test, there's probably a more reliable way of checking this.
Attachment #8778053 - Flags: review?(bbirtles)
Attachment #8778054 - Flags: review?(bbirtles) → review+
Comment on attachment 8778054 [details]
Bug 1216843 - Part 15: Update styles when current iteration changed.

https://reviewboard.mozilla.org/r/69442/#review66992

r=me with the following changes, particularly the ComputedTimeHasChanged/HasComputedTimeChanged method, assuming you agree.

::: dom/animation/KeyframeEffect.h:420
(Diff revision 2)
>    // used to detect when the progress is not changing (e.g. due to a step
>    // timing function) so we can avoid unnecessary style updates.
>    Nullable<double> mProgressOnLastCompose;
> +  // The purpose of this value is the same as mProgressOnLastCompose but
> +  // this is used to detect when the current iteration is not changing
> +  // in case when iterationComposite is accumulate.

nit: s/in case/in the case/

::: dom/animation/KeyframeEffect.cpp:186
(Diff revision 2)
> -  //
> +  if (NeedsStyleUpdates()) {
> -  // Bug 1216843: When we implement iteration composite modes, we need to
> -  // also detect if the current iteration has changed.
> -  if (mAnimation &&
> -      !mProperties.IsEmpty() &&
> -      GetComputedTiming().mProgress != mProgressOnLastCompose) {

NeedsStyleUpdates() seems a little too general to me.

Do you think it would be more clear if we just made a function called something like:

a. HasTimingChangedSinceLastCompose()?
b. HasOutputTimeChanged()?
c. HasComputedTimeChanged()?

I think (c) is best -- perhaps even ComputedTimeHasChanged()

Then this would become:

  if (mAnimation && !mProperties.IsEmpty() && ComputedTimeHasChanged()) {
    ...
  }

What do you think?

::: dom/animation/KeyframeEffect.cpp:196
(Diff revision 2)
>      RequestRestyle(restyleType);
>    }
>  
>    // If we're no longer "in effect", our ComposeStyle method will never be
> -  // called and we will never have a chance to update mProgressOnLastCompose.
> -  // We clear mProgressOnLastCompose here to ensure that if we later become
> +  // called and we will never have a chance to update mProgressOnLastCompose
> +  // and mCUrrentIterationOnLastCompose.

s/mCUrrentIterationOnLastCompose/mCurrentIterationOnLastCompose/

::: dom/animation/KeyframeEffect.cpp:676
(Diff revision 2)
> +    mCurrentIterationOnLastCompose = computedTiming.mCurrentIteration;
> +

Any reason we can't set this at the start of the function along with mProgressOnLastCompose? If possible, that would be more clear, I think.

::: dom/animation/KeyframeEffect.cpp:1585
(Diff revision 2)
> +  // We don't need to update styles when neither the progress nor the current
> +  // iteration has changed since the last style composition

This might be a bit more clear as:

  // Typically we don't need to request a restyle if the progress hasn't
  // changed since the last call to ComposeStyle. The one exception is if the
  // iteration composite mode is 'accumulate' and the current iteration has
  // changed, since that will often produce a different result.

::: layout/reftests/web-animations/style-updates-on-current-iteration-changed.html:21
(Diff revision 2)
> +    // rAF callback is usually 16.6ms interval, so now
> +    // we are in the second iteration.

As with the previous patch in this series, I think we could do something a bit more robust here.

Also, does this test fail without the changes in this patch?
Comment on attachment 8778055 [details]
Bug 1216843 - Part 16: Fix bug number for implementation of keyframe composition.

https://reviewboard.mozilla.org/r/69444/#review66996

::: layout/style/nsAnimationManager.cpp:794
(Diff revision 2)
>      // If we have a keyframe at the same offset with the same timing
>      // function we should merge our (unique) values into it.
>      // Otherwise, we should update the existing keyframe with only the
>      // unique properties.
>      //
> -    // Bug 1216843: We should also match composite modes here.
> +    // Bug 1291468: We should also match composite modes here.

I guess this should actually be the bug where we implement animation-composition?
Attachment #8778055 - Flags: review?(bbirtles) → review+
Blocks: 1293492
Blocks: 1293495
Comment on attachment 8778040 [details]
Bug 1216843 - Part 1: Tests for effect iteration composition.

https://reviewboard.mozilla.org/r/69414/#review66644

> Is this fixed by the end of this patch series? (I couldn't see it). If not, can we add a bug number here?

Yes, sure.

> Are we sure we're not getting an initialWidth of 0px here? (If we do, all these tests will pass since all the values will be zero!)
> 
> Would it be easier to add a parent element with position:relative and a fixed width?

Thanks, I never thought using parent's style.

> This is mostly just for my own records because I'll probably have the same question again in the future.
> 
> I was wondering what happens if one or more of the channels is decreasing.
> 
> For example, if we have:
> 
>   rgb(0, 200, 0) -> rgb(100, 100, 100)
> 
> Then you might expect we should get something like:
> 
>   progress = 0.5 -> rgb(50, 150, 50)
>   progress = 1.0 -> rgb(100, 100, 100)
>   progress = 1.5 -> rgb(150, 50, 150)
> 
> But the spec says we do:
> 
>   progress = 1.5 -> rgb(150, 250, 150)
> 
> That seemed wrong but I think it's actually correct. That's what SMIL does and once we support additive animation I think you can produce the other effect by other means. I think the alternative--adjusting values of the current iteration so that the last value of the previous iteration equals the first value of the new iteration--is more complex and makes it difficult to add jumps between iterations when that is the desired effect.
> 
> At the end of the day, this feature is mostly added to support SMIL so I think doing the same thing as SMIL is probably fine. Perhaps we can add another type of accumulation in future if we need the other behavior.
> 
> But for now, I wonder if it's worth adjusting this test so that one of the channels is decreasing to check that we do the correct thing in that case?

Added a new test for decreasing channel instead of adjusting it into this test.

> Can we add a test for something that can't be reduced, e.g. calc(10% + 20px) applied to something where getComputedStyle returns the computed style (and not the used style)?
> 
> There's only a pretty limited number of properties where getComputedStyle returns the used value but according to bug 1292447 we actually return the used value in a few cases where we shouldn't. I think there are some properties where we correctly return the computed style (perhaps some of the background-* properties?)

Added it.

> Why not?

Because brightness(1) is an identity element, scale(1) for transform is the same.

> Oh neat! This seems preferable to me, but is it inconsistent with what we do for transforms? Do we fill in missing functions for transforms?
> 
> In any case, we need to spec this so if you have a chance to summarize the different procedures that would be helpful.

Yes, right. It is inconsistent with transforms.  All of transforms can be converted matricies so they are compounded as a single matrix.   As I can see our interpolation of transform code, it matches the spec. https://drafts.csswg.org/css-transforms/#interpolation-of-transforms

> Do we need a test for mismatched transform lists? Including one where we *could* fill in the missing functions? (Although I'm not sure if we *should* fill them in or not)

Added a mismatched transform list case.
Comment on attachment 8778041 [details]
Bug 1216843 - Part 2: Implement effect iteration composition.

https://reviewboard.mozilla.org/r/69416/#review66648

> Is there a way we could avoid copying these two StyleAnimationValues in the common case where the composite mode is not accumulate or the current iteration is < 1 ?

Actually I did it before but it was hard to read, lots of if-else blocks.  In bug 1291468, we'd also copy the values, I might think up a cleaner way in that bug.

> We need to call nsNodeUtils::AnimationChanged here, something like:
> 
>   if (mAnimation && mAnimation->IsRelevant()) {
>     nsNodeUtils::AnimationChanged(mAnimation);
>   }
> 
> And I guess we also need to add a mutation observer test.

Thanks, I totally forget about Mutation observers and added the notify process, but left a test case as bug 1293495.

> This is a bit unfortunate to repeat the same code we have in KeyframeEffectReadOnly::ComposeStyle.
> 
> Can we factor out a utility function for the common code? Feel free to do this as a separate patch/bug, however.

Filed bug 1293492.
Comment on attachment 8778054 [details]
Bug 1216843 - Part 15: Update styles when current iteration changed.

https://reviewboard.mozilla.org/r/69442/#review66992

> NeedsStyleUpdates() seems a little too general to me.
> 
> Do you think it would be more clear if we just made a function called something like:
> 
> a. HasTimingChangedSinceLastCompose()?
> b. HasOutputTimeChanged()?
> c. HasComputedTimeChanged()?
> 
> I think (c) is best -- perhaps even ComputedTimeHasChanged()
> 
> Then this would become:
> 
>   if (mAnimation && !mProperties.IsEmpty() && ComputedTimeHasChanged()) {
>     ...
>   }
> 
> What do you think?

Thanks, it looks nice.  I used c but changed it to HasComputedTimingChanged because it matches our class name.

> Any reason we can't set this at the start of the function along with mProgressOnLastCompose? If possible, that would be more clear, I think.

Thanks, fixed it.
Initially I did confine the setting inside if (mEffectOptions.mIterationComposite == ..) block, but it did not work, after the trial, I just moved it up to nearby there.
Comment on attachment 8778055 [details]
Bug 1216843 - Part 16: Fix bug number for implementation of keyframe composition.

https://reviewboard.mozilla.org/r/69444/#review66996

> I guess this should actually be the bug where we implement animation-composition?

Thanks, I did not know it.  Filed bug 1293490, and changed the bug number in nsAnimationManager.cpp.
Comment on attachment 8778053 [details]
Bug 1216843 - Part 14: Reftest for iterationComposite.

https://reviewboard.mozilla.org/r/69440/#review66990

> This seems a bit flaky. First of all, Element.animate() doesn't begin right away. In fact, by the next animation frame the currentTime might still be zero.
> 
> Is there another approach that is more reliable that doesn't depend on the rAF interval. For example, could we continually call rAF until anim.effect.getComputedTiming().currentIteration == 1?

Fixed to wait for changing currentIteration.

I came up with another idea that uses negative endDelay and waits for finished promise, but it gets complicated.
Comment on attachment 8778053 [details]
Bug 1216843 - Part 14: Reftest for iterationComposite.

https://reviewboard.mozilla.org/r/69440/#review67518

Thanks for making that change. On second glance, however, is there any reason we need to wait for an iteration to tick naturally rather than just seeking into the second iteration, waiting a frame, changing the iterationComposite mode, waiting a frame and then taking a snapshot?
Attachment #8778053 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #69)
> Comment on attachment 8778053 [details]
> Bug 1216843 - Part 14: Reftest for iterationComposite.
> 
> https://reviewboard.mozilla.org/r/69440/#review67518
> 
> Thanks for making that change. On second glance, however, is there any
> reason we need to wait for an iteration to tick naturally rather than just
> seeking into the second iteration, waiting a frame, changing the
> iterationComposite mode, waiting a frame and then taking a snapshot?

I had the same thing in my mind but I want to check that the accumulated result is painted as usual animating.
(In reply to Brian Birtles (:birtles) from comment #43)
> Comment on attachment 8778040 [details]
> Bug 1216843 - Part 1: Tests for effect iteration composition.
> 
> https://reviewboard.mozilla.org/r/69414/#review66644
> 
> These tests are really really good, but I wonder what you think about
> combining them with the tests in bug 1277433? In particular, see part 3 of
> that bug, where we have
> animation-model/animation-types/type-per-property.html. The eventual idea is
> that in that we would test that each property uses the correct procedures
> for interpolation / composition / addition / distance.
> 
> So, if a new spec define a new property with a new animation type they&#39;d
> update that file with the new property and new procedures for interpolation
> / composition etc.? Would that be easier to maintain?

I had forgotten to mention about this.
I agree that we should put the bunch of test cases into a single place, but I am not sure where the place should be.

We have already tests for interpolation in our tree, layout/style/test/test_transitions_per_property.html.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #71)
> We have already tests for interpolation in our tree,
> layout/style/test/test_transitions_per_property.html.

Yes, but I think we need something we can use cross-browser, i.e. something we can check in to web-platform-tests.
Comment on attachment 8778042 [details]
Bug 1216843 - Part 3: Use nsCSSValue instead of nscolor to store over 1.0 color components as an accumulated value so that we can correctly calculate intermediate values in interpolation.

https://reviewboard.mozilla.org/r/69418/#review68178

r=me -- one observation, and one nit to address:

::: layout/style/StyleAnimationValue.h:355
(Diff revision 3)
>      NS_ASSERTION(mUnit == eUnit_Float, "unit mismatch");
>      return mValue.mFloat;
>    }
>    nscolor GetColorValue() const {
>      NS_ASSERTION(mUnit == eUnit_Color, "unit mismatch");
> -    return mValue.mColor;
> +    return GetCSSValueValue()->GetColorValue();

Perhaps we can/should just get rid of this function, for consistency with other nsCSSValue-backed types (which don't have their own custom getter)?

The callsites in ComputeDistance & AddWeighted would be the only things that need to change, I think, to replace "GetColorValue()" with "GetCSSValueValue()->GetColorValue()".

Anyway, I guess this doesn't matter that much, and this could happen at the end of this patch-stack (if it's even worth doing), so no need to make that change yet.

::: layout/style/StyleAnimationValue.cpp:3074
(Diff revision 3)
>        aSpecifiedValue.
>          SetFloatValue(aComputedValue.GetFloatValue(), eCSSUnit_Number);
>        break;
>      case eUnit_Color:
>        // colors can be alone, or part of a paint server
>        aSpecifiedValue.SetColorValue(aComputedValue.GetColorValue());

Looks like we should merge this case (in UncomputeValue) into the other nsCSSValue-typed case down below, which does...
  nsCSSValue* val = aComputedValue.GetCSSValueValue();
  aSpecifiedValue = *val;

You'll want to also update the assertion in that case -- you'll want to add...
  (unit == eUnit_Color &&
   val->GetUnit() == eCSSUnit_Color)
...as one of the possibilities for that assertion, or something like that.

(This is similar to what you're already doing for operator= and operator==.  We don't give eUnit_Color its own special case in those functions anymore, as of this patch, so we shouldn't give it its own special case here either.)
Attachment #8778042 - Flags: review?(dholbert) → review+
Comment on attachment 8778043 [details]
Bug 1216843 - Part 4: Pass nsCSSValue& to AddWeightedColors and DiluteColor.

https://reviewboard.mozilla.org/r/69420/#review68182

::: layout/style/StyleAnimationValue.cpp:1169
(Diff revision 3)
> +                  double aCoeff2, const nsCSSValue& aValue2)
> +{
> +  MOZ_ASSERT(aValue1.IsNumericColorUnit() && aValue2.IsNumericColorUnit(),
> +              "The unit should be color");
> +
> +  UniquePtr<nsCSSValue> result(new nsCSSValue());

This should be:
  auto result = MakeUnique<nsCSSValue>();
(MakeUnique is strongly preferred over "new", when you're dealing with UniquePtr values, for reasons laid out at https://dxr.mozilla.org/mozilla-central/source/mfbt/UniquePtr.h#640 )

Also, please declare this variable just before it's used, at the very end of the function, rather than here at the beginning. In other words, this line should move to the end, and the function should end like so:

  auto result = MakeUnique<nsCSSValue>();
  result->SetColorValue(resultColor);
  return result;

::: layout/style/StyleAnimationValue.cpp:2390
(Diff revision 3)
>          aCoeff2 * aValue2.GetFloatValue()));
>        return true;
>      }
>      case eUnit_Color: {
> -      nscolor color1 = aValue1.GetColorValue();
> -      nscolor color2 = aValue2.GetColorValue();
> +      const nsCSSValue *value1 = aValue1.GetCSSValueValue();
> +      const nsCSSValue *value2 = aValue2.GetCSSValueValue();

Coding-style nit: for |value1| and |value2| here, move the "*" to the left of the space character (i.e. put it next to the type, not the variable-name).
Attachment #8778043 - Flags: review?(dholbert) → review+
Comment on attachment 8778044 [details]
Bug 1216843 - Part 5: Reuse AddWeightedColors and DiluteColor in AddShadowItems().

https://reviewboard.mozilla.org/r/69422/#review68416

::: layout/style/StyleAnimationValue.cpp:1243
(Diff revision 3)
>  
>    if (color1.GetUnit() != eCSSUnit_Null) {
> -    StyleAnimationValue color1Value
> -      (color1.GetColorValue(), StyleAnimationValue::ColorConstructor);
> -    StyleAnimationValue color2Value
> -      (color2.GetColorValue(), StyleAnimationValue::ColorConstructor);
> +    UniquePtr<nsCSSValue> result =
> +      AddWeightedColors(aCoeff1, color1, aCoeff2, color2);
> +    MOZ_ASSERT(result, "should not fail");
> +    resultArray->Item(4).AdoptColorValue(Move(result));

Hmm -- so since AdoptColorValue just copies the contents of |result| (as noted below), that means |result| is basically a temporary variable here.  So, it's unfortunate/inefficient for it to be heap allocated.

I think we need to change AddWeightedColors (in part 4) to take a nsCSSValue& outparam (similar to how AddWeighted() takes an outparam) -- and then here, we should just pass in resultArray->Item(4) as that outparam, for AddWeightedColors to directly populate.

Does that make sense?

::: layout/style/nsCSSValue.cpp:493
(Diff revision 3)
> +    aValue->mUnit = eCSSUnit_Null;
> +    aValue->mValue.mFloatColor = nullptr;
> +  } else {
> +    mValue.mColor = aValue->mValue.mColor;
> +  }
> +  aValue.reset();

"Adopt" is the wrong name choice for this function.  It implies "take ownership of this heap-allocated resource" -- but this new function does not do that -- instead, it copies the value of the resource and then frees it.

Does this function even need to exist?  It seems like the assignment operator should do the same thing.
Comment on attachment 8778044 [details]
Bug 1216843 - Part 5: Reuse AddWeightedColors and DiluteColor in AddShadowItems().

https://reviewboard.mozilla.org/r/69422/#review68420

r- on part 5 for now, per comment 75.
Attachment #8778044 - Flags: review?(dholbert) → review-
Comment on attachment 8778043 [details]
Bug 1216843 - Part 4: Pass nsCSSValue& to AddWeightedColors and DiluteColor.

https://reviewboard.mozilla.org/r/69420/#review68418

[Changing part 4 to r-, since we need to rework AddWeightedColors (added in part 4) a bit, so that part 5 can avoid introducing an inefficient extra heap allocation.)

::: layout/style/StyleAnimationValue.cpp:1163
(Diff revision 3)
>    aResult.SetFloatValue(RestrictValue(aValueRestrictions, result + aInitialVal),
>                          eCSSUnit_Number);
>  }
>  
> +static UniquePtr<nsCSSValue>
> +AddWeightedColors(double aCoeff1, const nsCSSValue& aValue1,

(so per comment 75, this function should actually return its result by reference, via an outparam.)

::: layout/style/StyleAnimationValue.cpp:2393
(Diff revision 3)
>      case eUnit_Color: {
> -      nscolor color1 = aValue1.GetColorValue();
> -      nscolor color2 = aValue2.GetColorValue();
> -      // FIXME (spec): The CSS transitions spec doesn't say whether
> -      // colors are premultiplied, but things work better when they are,
> -      // so use premultiplication.  Spec issue is still open per
> +      const nsCSSValue *value1 = aValue1.GetCSSValueValue();
> +      const nsCSSValue *value2 = aValue2.GetCSSValueValue();
> +      MOZ_ASSERT(value1 && value2, "Both of CSS value should be valid");
> +      UniquePtr<nsCSSValue> result =
> +        AddWeightedColors(aCoeff1, *value1, aCoeff2, *value2);

(Here, we'll really want to:
 - Allocate here (instead of in AddWeightedColors) using "auto result = MakeUnique<nsCSSValue>()"
 - Pass in "*result" as the outparam to AddWeightedColors, once it takes an outparam,
 - Keep the aResultValue.SetAndAdoptCSSValueValue clal that you've already got here.
Attachment #8778043 - Flags: review+ → review-
Comment on attachment 8778045 [details]
Bug 1216843 - Part 6: A dummy empty changeset.

https://reviewboard.mozilla.org/r/69424/#review68426

I'm unclear on whether this patch is correct/complete, per the comments I've added about RGB/RGBA. So, r- for now...

::: layout/style/nsCSSValue.h:378
(Diff revision 3)
>    eCSSUnit_HexColor            = 83,   // (nscolor) an opaque RGBA value specified as #rrggbb
>    eCSSUnit_ShortHexColor       = 84,   // (nscolor) an opaque RGBA value specified as #rgb
>    eCSSUnit_HexColorAlpha       = 85,   // (nscolor) an opaque RGBA value specified as #rrggbbaa
>    eCSSUnit_ShortHexColorAlpha  = 86,   // (nscolor) an opaque RGBA value specified as #rgba
>    eCSSUnit_PercentageRGBColor  = 87,   // (nsCSSValueFloatColor*)
> -  eCSSUnit_PercentageRGBAColor = 88,   // (nsCSSValueFloatColor*)
> +  eCSSUnit_PercentageRGBAColor = 88,   // (nsCSSValueFloatColor*) Each RGB

Nit: s/Each RGB/Each RGBA/ (since this is  for the RGBA unit)

Bigger question: why are we only allowing overflow-beyond-100% for eCSSUnit_Percentage*RGBA*Color?  (and not RGBColor, the unit directly above it)  Why is it only one and not the other?  If it really does need to be one and not the other, perhaps we should differentiate their unit-names a bit more clearly (beyond just the "A")...  But I'm not sure we do want them to differ.

::: layout/style/nsCSSValue.h:1709
(Diff revision 3)
>    size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const;
>  
>    NS_INLINE_DECL_REFCOUNTING(nsCSSValueFloatColor)
>  
>  private:
> -  // FIXME: We should not be clamping specified RGB color components.
> +  // eCSSUnit_PercentageRGBAColor allows over 1.0 values to store accumulated

Following up on my above question about RGBA vs RGB -- note that this FIXME is not RGBA-unit-specific.  Seems like it applies to all float-valued colors.

I believe the point of this comment was that we should accept and be able to internally represent overflowed values like "background:rgb(200%,0%,0%)" in this type -- not just rgba() values. Right?

::: layout/style/nsCSSValue.h:1714
(Diff revision 3)
> -  // FIXME: We should not be clamping specified RGB color components.
> -  float mComponent1;  // 0..1 for RGB, 0..360 for HSL
> -  float mComponent2;  // 0..1
> -  float mComponent3;  // 0..1
> -  float mAlpha;       // 0..1
> +  // eCSSUnit_PercentageRGBAColor allows over 1.0 values to store accumulated
> +  // values.
> +  float mComponent1;  // [0, 1] for RGBColor, RGBAColor, PercentageRGBColor
> +                      // [0, float::max()] for PercentageRGBAColor
> +                      // [0, 360] for HSL
> +  float mComponent2;  // [0, 1]

These comments are no longer correct ("[0, 1]") -- they need to be extended like you extended the mComponent1 comment.

Perhaps update them like so,
>  float mComponent2;  // [0, 1], or [0, float::max()] for PercentageRGBAColor

(and similar for mComponent3 and mAlpha)
Attachment #8778045 - Flags: review?(dholbert) → review-
Comment on attachment 8778046 [details]
Bug 1216843 - Part 7: Implement color accumulation.

https://reviewboard.mozilla.org/r/69426/#review68434

This patch on the right track, but r- for now simply because there are enough review comments to merit another round of review as a sanity-check after you've addressed the feedback.

::: layout/style/StyleAnimationValue.cpp:1162
(Diff revision 3)
>    float result = (n1 - aInitialVal) * aCoeff1 + (n2 - aInitialVal) * aCoeff2;
>    aResult.SetFloatValue(RestrictValue(aValueRestrictions, result + aInitialVal),
>                          eCSSUnit_Number);
>  }
>  
> +enum class OperationType {

This enum class seems generically/confusingly named.  Is it a color-specific thing? (Seems like it right now.) If so, it should have something color-specific in the name.  And the enum-values, "Addition" vs "Accumulation", are confusing to me too -- I didn't immediately know what they meant precisely.  And even now after I've learned what they mean, they're still confusing because they're both *flavors of addition*, so it's weird that one of the flavors is itself named "Addition".

Possible replacement (including clarifying comments):

// Enum class to to allow callers to control the color-channel
// clamping behavior of AddWeightedColors().
enum class ColorAdditionType {
  Clamped,   // Clamp each color channel after adding.
  Unclamped  // Do not clamp color channels after adding.
};

Alternately: if you want to keep using "Accumulation"/"OperationType" terminology (in particular, if this isn't going to remain color-specific), then please at least add comments like my suggested ones above to explain this stuff, and perhaps rename "Addition" to "ClampedAddition" to make the distinction clearer. (since as noted above, Accumulation is sort of a flavor of addition, so Accumulation/Addition don't make sense as mutually exclusive options.)

::: layout/style/StyleAnimationValue.cpp:1168
(Diff revision 3)
> +  Addition,
> +  Accumulation
> +};
> +
>  static UniquePtr<nsCSSValue>
> -AddWeightedColors(double aCoeff1, const nsCSSValue& aValue1,
> +AddWeightedOrAccumulateColors(double aCoeff1, const nsCSSValue& aValue1,

Let's not rename this function -- just keep it named AddWeightedColors, and let callers customize it by passing the "Accumulate" enum (or whatever the enum ends up being named).

(Even if we were renaming, "AddWeightedOrAccumulateColors" would not be a correct name -- better would be "AddOrAccumulateWeightedColors", since the colors are weighted regardless of whether we're adding or accumulating. BUT, really, I don't think we should rename the function, since it's still basically a customizeable addition function.)

::: layout/style/StyleAnimationValue.cpp:1179
(Diff revision 3)
>  
>    UniquePtr<nsCSSValue> result(new nsCSSValue());
>  
> -  nscolor color1 = aValue1.GetColorValue();
> -  nscolor color2 = aValue2.GetColorValue();
> +  // Color component value might be greater than 1.0 in case when the color
> +  // value is accumulated, so we can't use GetColorValue() here because
> +  // the function clamps its values.

s/the function/that function/ (to be a little more specific about which function you're talking about)

::: layout/style/StyleAnimationValue.cpp:1182
(Diff revision 3)
> -  nscolor color1 = aValue1.GetColorValue();
> -  nscolor color2 = aValue2.GetColorValue();
> +  // Color component value might be greater than 1.0 in case when the color
> +  // value is accumulated, so we can't use GetColorValue() here because
> +  // the function clamps its values.
> +  #define GET_COLOR_COMPONENT(name_, capital_)                \
> +    [](const nsCSSValue& value) -> double {                   \
> +      if (value.GetUnit() == eCSSUnit_PercentageRGBAColor) {  \

Per my previous patch, what if we happen to have eCSSUnit_PercentageRGBColor (RGB instead of RGBA)?  

Right now, we'd take the (clamped) GetColorValue() codepath -- is that really what we want?

::: layout/style/StyleAnimationValue.cpp:1210
(Diff revision 3)
> -  if (Aresf <= 0.0) {
> -    resultColor = NS_RGBA(0, 0, 0, 0);
> -  } else {
> -    if (Aresf > 1.0) {
> -      Aresf = 1.0;
> -    }
> +  double Ares = (A1 * aCoeff1 + A2 * aCoeff2);
> +
> +  if (Ares <= 0.0) {
> +    result->SetColorValue(NS_RGBA(0, 0, 0, 0));
> +    return result;
> +  } else if (Ares >= 1.0) {

Two nits:

(1) No "else after return" please. (It's redundant/misleading.)  Delete the "else" & drop the "if" down to the next line.

(2) It looks like you're clamping Ares to be a maximum of 1.0, without checking whether we're accumulating (i.e. whether we should be clamping).  Why?  Are alpha values not allowed to accumulate/overflow in the same way that other color-channels do?  Please add a comment to explain why this channel has this special-case (if we do really want this special case).

::: layout/style/StyleAnimationValue.cpp:1230
(Diff revision 3)
> +    Rres = Rres * (1.0 / 255);
> +    Gres = Gres * (1.0 / 255);
> +    Bres = Bres * (1.0 / 255);
> +
> +    result->SetFloatColorValue(Rres, Gres, Bres, Ares,
> +                               eCSSUnit_PercentageRGBAColor );

nit: drop the trailing space character before the ")" here

::: layout/style/StyleAnimationValue.cpp:1269
(Diff revision 3)
> -  if (color1.GetUnit() != color2.GetUnit() ||
> +  if ((color1.GetUnit() != color2.GetUnit() &&
> +       (!color1.IsNumericColorUnit() || !color2.IsNumericColorUnit())) ||
>        inset1.GetUnit() != inset2.GetUnit()) {
>      // We don't know how to animate between color and no-color, or
>      // between inset and not-inset.
> +    // NOTE: In case when both color's unit are eCSSUnit_Null, that means

grammar nits:
 s/color's/colors'/
 s/unit/units/

::: layout/style/StyleAnimationValue.cpp:1270
(Diff revision 3)
> +       (!color1.IsNumericColorUnit() || !color2.IsNumericColorUnit())) ||
>        inset1.GetUnit() != inset2.GetUnit()) {
>      // We don't know how to animate between color and no-color, or
>      // between inset and not-inset.
> +    // NOTE: In case when both color's unit are eCSSUnit_Null, that means
> +    // color value is not specified, so we can animate it.

Two clarifications to make here:
1) s/color value is not/neither color value was/
2) s/so we can animate it/so we can interpolate (and simply use a Null-unit color in the result)/

(The current patch's phrase "so we can animate it" sounds to me like we'd be doing some sort of visual animation on the unspecified colors, which doesn't make sense.)

::: layout/style/StyleAnimationValue.cpp:1277
(Diff revision 3)
>    }
>  
>    if (color1.GetUnit() != eCSSUnit_Null) {
>      UniquePtr<nsCSSValue> result =
> -      AddWeightedColors(aCoeff1, color1, aCoeff2, color2);
> +      AddWeightedOrAccumulateColors(aCoeff1, color1, aCoeff2, color2,
> +                                    OperationType::Addition);

Why are we using OperationType::Addition here, vs. Accumulate elsewhere? It's not clear to me why we'd ever want Addition once we've got the ability to do Accumulation. (If we're producing intermediate interpolated values, aren't we better-off putting off clamping to happen as late as possible?)

So: please add a comment (or several comments) somewhere, explaining why certain callers might want one OperationType vs. the other.  (E.g. you could add an explanation/hint at each callsite, or maybe an explanation above AddWeightedColors to say which sorts of callers should use which operation.)

::: layout/style/StyleAnimationValue.cpp:2422
(Diff revision 3)
>        MOZ_ASSERT(value1 && value2, "Both of CSS value should be valid");
>        UniquePtr<nsCSSValue> result =
> -        AddWeightedColors(aCoeff1, *value1, aCoeff2, *value2);
> -      MOZ_ASSERT(result, "AddWeightedColors never fails");
> +        AddWeightedOrAccumulateColors(aCoeff1, *value1,
> +                                      aCoeff2, *value2,
> +                                      OperationType::Addition);
> +      MOZ_ASSERT(result, "AddWeightedOrAccumulateColors never fails");

This MOZ_ASSERT can go away, since now AddWeightedColors will be returning its nsCSSValue by reference (using an allocated-by-the-caller nsCSSValue& outparam), as requested above in my comments on patches 4 & 5.

::: layout/style/StyleAnimationValue.cpp:2821
(Diff revision 3)
> +        AddWeightedOrAccumulateColors(1.0,
> +                                      *aDest.GetCSSValueValue(),
> +                                      aCount,
> +                                      *aValueToAccumulate.GetCSSValueValue(),
> +                                      OperationType::Accumulation);
> +      MOZ_ASSERT(result, "AddOrAccumulateColor should not fail");

Typo in the function name here (AddOrAccumulateColor) -- but similar to above, this MOZ_ASSERT can really go away once you've converted AddWeightedColors' result to be an outparam.
Attachment #8778046 - Flags: review?(dholbert) → review-
Blocks: 1294651
Comment on attachment 8778045 [details]
Bug 1216843 - Part 6: A dummy empty changeset.

https://reviewboard.mozilla.org/r/69424/#review68426

> Following up on my above question about RGBA vs RGB -- note that this FIXME is not RGBA-unit-specific.  Seems like it applies to all float-valued colors.
> 
> I believe the point of this comment was that we should accept and be able to internally represent overflowed values like "background:rgb(200%,0%,0%)" in this type -- not just rgba() values. Right?

Ah, thanks.  I misunderstood the comment implies that we can set over 100% value from style in some day and I had no idea about the setter. 
Incoming patch extends eCSSUnit_PercentageRGBColor as well.
Oops!  I am really sorry I did accidentally push the review request while I am writing replies to Daniel.
Danial, the patch set in this review request is incomplete.  I will re-request again.  Sorry for the confusing.
Comment on attachment 8778046 [details]
Bug 1216843 - Part 7: Implement color accumulation.

https://reviewboard.mozilla.org/r/69426/#review68434

> This enum class seems generically/confusingly named.  Is it a color-specific thing? (Seems like it right now.) If so, it should have something color-specific in the name.  And the enum-values, "Addition" vs "Accumulation", are confusing to me too -- I didn't immediately know what they meant precisely.  And even now after I've learned what they mean, they're still confusing because they're both *flavors of addition*, so it's weird that one of the flavors is itself named "Addition".
> 
> Possible replacement (including clarifying comments):
> 
> // Enum class to to allow callers to control the color-channel
> // clamping behavior of AddWeightedColors().
> enum class ColorAdditionType {
>   Clamped,   // Clamp each color channel after adding.
>   Unclamped  // Do not clamp color channels after adding.
> };
> 
> Alternately: if you want to keep using "Accumulation"/"OperationType" terminology (in particular, if this isn't going to remain color-specific), then please at least add comments like my suggested ones above to explain this stuff, and perhaps rename "Addition" to "ClampedAddition" to make the distinction clearer. (since as noted above, Accumulation is sort of a flavor of addition, so Accumulation/Addition don't make sense as mutually exclusive options.)

Yeah, thanks! Actually current implematation of AddWeightedColors just adds two values. ColorAdditionType::Clamped and Unclamped look really nice to me.

> Per my previous patch, what if we happen to have eCSSUnit_PercentageRGBColor (RGB instead of RGBA)?  
> 
> Right now, we'd take the (clamped) GetColorValue() codepath -- is that really what we want?

Will check eCSSUnit_PercentageRGBColor as well. Thanks.

> Two nits:
> 
> (1) No "else after return" please. (It's redundant/misleading.)  Delete the "else" & drop the "if" down to the next line.
> 
> (2) It looks like you're clamping Ares to be a maximum of 1.0, without checking whether we're accumulating (i.e. whether we should be clamping).  Why?  Are alpha values not allowed to accumulate/overflow in the same way that other color-channels do?  Please add a comment to explain why this channel has this special-case (if we do really want this special case).

Because of unpremultiplication issue over 1.0.  I had not checked it but now I noticed there is a bug (bug 1294614).  I will add a comment to refer that bug.
Comment on attachment 8778042 [details]
Bug 1216843 - Part 3: Use nsCSSValue instead of nscolor to store over 1.0 color components as an accumulated value so that we can correctly calculate intermediate values in interpolation.

https://reviewboard.mozilla.org/r/69418/#review68790

Final nit on one of the changes in the latest "Part 3" version:

::: layout/style/nsStyleContext.cpp:1415
(Diff revision 4)
>               nsStyleContext *aStyleContext)
>  {
>    StyleAnimationValue val;
>    ExtractAnimationValue(aProperty, aStyleContext, val);
>    return val.GetUnit() == StyleAnimationValue::eUnit_CurrentColor
> -    ? aStyleContext->StyleColor()->mColor : val.GetColorValue();
> +    ? aStyleContext->StyleColor()->mColor :

Please drop the ":" down to the next line here, aligned directly below the "?" character.

(Sometimes we have it at the end of the line when the "?" was also at the end of a line, like so...
  a == (b) ?
    bar :
    blah;

...but if the "?" operator is at the *start* of the line [as it is here & as gecko coding style technically says it should be], then the ":" operator should be at the start as well, for consistency.)
Comment on attachment 8778043 [details]
Bug 1216843 - Part 4: Pass nsCSSValue& to AddWeightedColors and DiluteColor.

https://reviewboard.mozilla.org/r/69420/#review68792

r=me on part 4
Attachment #8778043 - Flags: review?(dholbert) → review+
Comment on attachment 8778044 [details]
Bug 1216843 - Part 5: Reuse AddWeightedColors and DiluteColor in AddShadowItems().

https://reviewboard.mozilla.org/r/69422/#review68794

r=me on part 5
Attachment #8778044 - Flags: review?(dholbert) → review+
Comment on attachment 8778045 [details]
Bug 1216843 - Part 6: A dummy empty changeset.

https://reviewboard.mozilla.org/r/69424/#review68802

Sorry if this shakes things up a bit, but I think we should probably split "Part 6" off into its own separate bug, and it should include tests and update how our serialization code works for overflowed RGB/RGBA-valued nsCSSValue objects.

In particular:
 * Given <div style="color:rgb(150%,0%,0%)">, if an author queries elem.style.color (the specified color), they should probably get back a faithful serialization of the same color that they provided, 150%-and-all, if that's how we're representing it internally.  (Right now, they'll get back rgb(255,0,0), since we use GetColor() during serialization, and GetColor clamps.)  So, we need to fix that.
 * We need to add tests for that ^^ behavior (with rgb as well as rgba, exercising edge cases around negative percentages, and overflowed color channels vs. alpha channel behavior.)

(FWIW, the clamping/during/serialization happens during this GetColorValue() call inside of nsCSSValue::AppendToString():
  https://dxr.mozilla.org/mozilla-central/source/layout/style/nsCSSValue.cpp#1511
We probably need a separate IsOverflowedRGBColor() clause there which serializes as percentages, or something like that.)

So, anyway -- in light of the above, this is an independent-enough, substantial-enough, & independently-testable-enough change that it should probably happen on its own bug with its own place for discussion/backouts/etc., rather than in the middle of a 16-part patch-stack.  And I think it can land before everything else on this bug.

(BTW, probably no need to worry about renumbering all the patches after this one, after you've stitched this patch out.  Patch numbers are a convenience, but it's not the end of the world if this bug's patch numbering goes straight from "part 5" to "part 7", IMO.)
Attachment #8778045 - Flags: review?(dholbert) → review-
(BTW, I'd also like to get a sanity-check from heycam/dbaron about your RGBA-clamping changes in nsCSSValue, since they wrote/reviewed the patch that added the FIXME in question, here:  https://hg.mozilla.org/integration/mozilla-inbound/rev/411281eba8f7#l5.195 .  So, please CC them [and me] on that helper-bug, when you file it.)
Comment on attachment 8778043 [details]
Bug 1216843 - Part 4: Pass nsCSSValue& to AddWeightedColors and DiluteColor.

https://reviewboard.mozilla.org/r/69420/#review68852

::: layout/style/StyleAnimationValue.cpp:1168
(Diff revision 4)
> +AddWeightedColors(double aCoeff1, const nsCSSValue& aValue1,
> +                  double aCoeff2, const nsCSSValue& aValue2,
> +                  nsCSSValue& aResult)
> +{
> +  MOZ_ASSERT(aValue1.IsNumericColorUnit() && aValue2.IsNumericColorUnit(),
> +              "The unit should be color");

One more nit on part 4 (sorry -- I only just noticed when reviewing a later patch that touches this same code):

The second line of this MOZ_ASSERT expression is mis-indented -- looks like it's got 1 extra space of indentation before the assertion-message.  Please fix that. (The quote character should be directly below the "a" in "aValue1")
Comment on attachment 8778041 [details]
Bug 1216843 - Part 2: Implement effect iteration composition.

https://reviewboard.mozilla.org/r/69416/#review68870

::: layout/style/StyleAnimationValue.h:140
(Diff revision 4)
> +   *                           aValueToAccumulate.
> +   * @return true on success, false on failure.
> +   */
> +  static bool Accumulate(nsCSSProperty aProperty, StyleAnimationValue& aDest,
> +                         const StyleAnimationValue& aValueToAccumulate,
> +                         uint64_t aCount);

Hmm, I'm just noticing that this new function (StyleAnimationValue::Accumulate) looks suspiciously similar (in documentation & function-signature) to the already-existing function StyleAnimationValue::Add():
http://searchfox.org/mozilla-central/rev/b92ae9a263a05efefc0110eaad5e56369c9f1b10/layout/style/StyleAnimationValue.h#45

That older function happens to only be called by SMIL code, as part of of implementing nsSMILValue::Add(), which is for repeating SMIL animations and only used at these 2 callsites, FWIW:
https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22nsSMILValue%3A%3AAdd%28const+nsSMILValue+%26%2C+uint32_t%29%22

(I'm pointing out the callsites in case they're helpful for figuring out potential semantic differences between this old function & your new function.)

So, anyway -- when adding this new StyleAnimationValue API, please clarify in its documentation how it differs from the existing (and extremely-similar) Add() method. (Maybe they even want to be merged into a single function someday, perhaps with a flag to customize behavior? Not sure.  That merging could happen in a followup, presumably, if we even want to do it -- once Accumulate has settled.)
Comment on attachment 8778046 [details]
Bug 1216843 - Part 7: Implement color accumulation.

https://reviewboard.mozilla.org/r/69426/#review68834

::: layout/style/StyleAnimationValue.cpp:1168
(Diff revision 5)
> +  Clamped, // Clamp each color channel after adding.
> +  Unclamped // Do not clamp color channels after adding.
> +};
> +
> +// |aAdditionType| should be Clamped in case of interpolation or calculation of
> +// distance.  Now Unclamped is only for accumulation.

Two nits:
 (1) I think "calculation of distance" is wrong here.  AFAICT, nobody actually uses this function (AddWeightedColors) for distance-calculation.  The top-level distance function, StyleAnimationValue::ComputeDistance, has its own Color-specific chunk which does not use this function (not at this point in your patch queue at least).

 (2) I think you want s/Now/For now,/ in the second sentence.  (You're using "Now" to mean "Currently & temporarily", I think?)  Or if that's not what you mean, I don't think I understand what that sentence is trying to say.

::: layout/style/StyleAnimationValue.cpp:1210
(Diff revision 5)
> -  double Aresf = (A1 * aCoeff1 + A2 * aCoeff2);
> -  nscolor resultColor;
> -  if (Aresf <= 0.0) {
> -    resultColor = NS_RGBA(0, 0, 0, 0);
> -  } else {
> -    if (Aresf > 1.0) {
> +  #undef GET_COLOR_COMPONENT
> +
> +  double Ares;
> +  // FIXME: Bug 1294614: In the case when aCoeff1 + aCoeff2 != 1.0,
> +  // we should provide a new function for that purpose and the function
> +  // should have an accurate name what it does.

I don't entirely understand what this comment is saying.  In particular: "We should provide a new function for that purpose" is unclear to me. Is it saying AddWeightedColor should change to require that its coefficients add up to 1.0, and we need to create a new function for cases when they don't add up to 1.0?

If possible, I'd like to address the problem up-front, by just fixing bug 1294614, instead of adding this hard-to-understand comment and the mysterious special-case that follows it (for aCoeff1 != 1.0 && aCoeff2 == 0.0).  At least, we should fix the shadow-list callsite that you're referencing here, maybe in the way that I suggest in bug 1294614 comment 3; and then I think we can get rid of this special case.

::: layout/style/StyleAnimationValue.cpp:1219
(Diff revision 5)
> -    uint8_t Ares = NSToIntRound(Aresf * 255.0);
> -    uint8_t Rres = ClampColor((R1 * aCoeff1 + R2 * aCoeff2) * factor);
> -    uint8_t Gres = ClampColor((G1 * aCoeff1 + G2 * aCoeff2) * factor);
> -    uint8_t Bres = ClampColor((B1 * aCoeff1 + B2 * aCoeff2) * factor);
> -    resultColor = NS_RGBA(Rres, Gres, Bres, Ares);
> -  }
> +  // but this formula can not be used for over 1.0 values.
> +  if (aCoeff1 != 1.0 && aCoeff2 == 0.0) {
> +    Ares = (A1 * aCoeff1 + A2 * aCoeff2);
> +  } else {
> +    // In case of alpha component, 1.0 is the identity element.
> +    Ares = (A1 - 1.0) * aCoeff1 + (A2 - 1.0) * aCoeff2 + 1.0;

Nit: I think you mean to say identity _value_, not identity _element_. (better to avoid the word "element", to avoid any mixed-up association with HTML elements. :))

Also: please expand this comment to explain the "- 1.0 ... + 1.0" stuff a bit better.  The following things aren't immediately clear to me:
  (1) Why is 1.0 the identity value, and what does that even mean?  (I'm having a hard time conceptualizing what it even means for 1.0 to be the identity value, and what adding alpha-values really represents.)
  (2) What would go wrong if we didn't do this? (in particular, I'm pretty sure stuff only could go wrong if coefficients sum to something other than 1.0 - that's nice to clarify)
  (3) Why don't we have to do anything like this for the other color channels?

If you could extend this comment a bit to make those things clearer, that would help a lot.  Perhaps an example addition where this matters (in a code-comment here) would help make this clearer?

::: layout/style/StyleAnimationValue.cpp:1227
(Diff revision 5)
> +  if (Ares <= 0.0) {
> +    aResult.SetColorValue(NS_RGBA(0, 0, 0, 0));
> +    return;
> +  }
> +
> +  double factor = 1.0 / Ares;

Please add a comment to explain what this |factor| represents and what we're doing with it below this.

::: layout/style/StyleAnimationValue.cpp:1231
(Diff revision 5)
> +
> +  double factor = 1.0 / Ares;
> +
> +  double Rres = (R1 *aCoeff1 + R2 * aCoeff2) * factor;
> +  double Gres = (G1 *aCoeff1 + G2 * aCoeff2) * factor;
> +  double Bres = (B1 *aCoeff1 + B2 * aCoeff2) * factor;

Add a space character after the * in "*aCoeff1" in each of these 3 lines.

::: layout/style/StyleAnimationValue.cpp:1236
(Diff revision 5)
> +  double Bres = (B1 *aCoeff1 + B2 * aCoeff2) * factor;
> +
> +  if (aAdditionType == ColorAdditionType::Clamped) {
> +    aResult.SetColorValue(
> +      NS_RGBA(ClampColor(Rres), ClampColor(Gres), ClampColor(Bres),
> +              NSToIntRound(Ares * 255.0)));

Don't we need to clamp the Ares expression here, too, since this is in the Clamped-behavior codepath?  (Previously we clamped Ares to be maximum of 1.0, before this point, but I don't think we're doing that anymore.)

i.e. maybe this should be ClampColor(NSToIntRound(Ares * 255.0)) ?

::: layout/style/StyleAnimationValue.cpp:1279
(Diff revision 5)
> +       (!color1.IsNumericColorUnit() || !color2.IsNumericColorUnit())) ||
>        inset1.GetUnit() != inset2.GetUnit()) {
>      // We don't know how to animate between color and no-color, or
>      // between inset and not-inset.
> +    // NOTE: In case when both colors' units are eCSSUnit_Null, that means
> +    // neither color value was not specified, so we can interpolate.

nit: s/was not/was/

(Right now, this says the opposite of what you want it to say. The current text, "Neither...was not" approximately means "Both... were". You really want to say "Neither...was".)
Attachment #8778046 - Flags: review?(dholbert) → review-
Comment on attachment 8778047 [details]
Bug 1216843 - Part 8: Make AddShadowItems() return UniquePtr and rename it to AddWeightedShadowItems().

https://reviewboard.mozilla.org/r/69428/#review68910

This is nearly r=me, but I think we need to eliminate some duplicated list-appending code, as noted below.  r- for the moment, but likely r+ with that a suitable refactoring.

::: layout/style/StyleAnimationValue.cpp:2626
(Diff revision 5)
> +        if (!shadowValue) {
>            return false;
>          }
>          shadow1 = shadow1->mNext;
>          shadow2 = shadow2->mNext;
> +        // Append the result.

This 8-line chunk is repeated twice in this patch (twice in this same function, actually), and the logic is subtle, so it'd be nice to factor it into a helper-function.  Perhaps something like this (haven't tested this to see if it compiles, but it's hopefully pretty close):

// Appends a value to a nsCSSValueList.
// aHead is the head of the list (and may be null, if we're "appending" the
// first entry).
// *aTail is the (current) final nsCSSValueList (or null if aHead is null).
// This function will update *aTail to the (appended) new final entry, so
// the caller can keep track of the tail.
void
AppendToCSSValueList(UniquePtr<nsCSSValueList>& aHead,
                     UniquePtr<nsCSSValueList>&& aValToAppend,
                     nsCSSValueList** aTail)
{
  MOZ_ASSERT(!aHead == !*aTail,
             "Can't have head w/o tail, & vice versa");

  if (!aHead) {
    aHead = Move(aToAppend);
    *aTail = aHead->mNext;
  } else {
    (*aTail) = (*aTail)->mNext = aValToAppend.release();
  }
}

...and then you'd call this like so, replacing each of your "Append the result" chunks in this patch:
  // Append the result.
  AppendToCSSValueList(result, Move(shadowValue), &tail);

::: layout/style/StyleAnimationValue.cpp:2658
(Diff revision 5)
> +          if (!shadowValue) {
>              return false;
>            }
>  
>            longShadow = longShadow->mNext;
> +          // Append the result.

(I think you could use my above-sugggested "AppendToCSSValueList" function here, as well.)
Attachment #8778047 - Flags: review?(dholbert) → review-
(In reply to Daniel Holbert [:dholbert] (recovering from vacation reviews/bugmail) from comment #117)
> > +        // Append the result.
> 
> This 8-line chunk is repeated twice in this patch

To clarify (in case it's not clear) -- the 8 lines I'm referring to here are the 8 lines *starting with* this "Append the result" comment".  (The lines about appending to & updating the list-tail.)
Comment on attachment 8778048 [details]
Bug 1216843 - Part 9: Factor out AddWeightedShadowList.

https://reviewboard.mozilla.org/r/69430/#review68918

Part 9 looks good (just a straight code-move/refactor).

(Though, note that it'll need to rebased, after you've addressed the AppendToCSSValueList refactoring suggested in comment 117.)
Attachment #8778048 - Flags: review?(dholbert) → review+
Comment on attachment 8778049 [details]
Bug 1216843 - Part 10: Implement box-shadow/text-shadow color accumulation.

https://reviewboard.mozilla.org/r/69432/#review68922

r=me
Attachment #8778049 - Flags: review?(dholbert) → review+
Comment on attachment 8778050 [details]
Bug 1216843 - Part 11: Make AddFilterFunction and AddFilterFunctionImpl return UniquePtr and rename them to AddWeightedFilterXX.

https://reviewboard.mozilla.org/r/69434/#review68926

r=me with nits addressed:

::: layout/style/StyleAnimationValue.cpp:1773
(Diff revision 5)
>    nsCSSKeyword filterFunction = a1->Item(0).GetKeywordValue();
> -  if (filterFunction != a2->Item(0).GetKeywordValue())
> -    return false; // Can't add two filters of different types.
> +  if (filterFunction != a2->Item(0).GetKeywordValue()) {
> +    return nullptr; // Can't add two filters of different types.
> +  }
>  
> -  nsAutoPtr<nsCSSValueList> resultListEntry(new nsCSSValueList);
> +  UniquePtr<nsCSSValueList> resultList(new nsCSSValueList);

Use auto/MakeUnique instead of "new" here, as noted for earlier patches.

::: layout/style/StyleAnimationValue.cpp:2724
(Diff revision 5)
>            return false;
>          }
>  
> +        // Append the result function.
> +        if (!tail) {
> +          result = Move(resultFunction);

Please replace this chunk with a call to AppendToCSSValueList (new function suggested in comment 117 for an earlier patch).
Attachment #8778050 - Flags: review?(dholbert) → review+
Comment on attachment 8778051 [details]
Bug 1216843 - Part 12: Factor out AddWeightedFilterList.

https://reviewboard.mozilla.org/r/69436/#review68928

r=me on part 12 (just a straight code-move/refactor)

(Though, as I noted for part 9, please do make sure that my earlier review comments are still preserved through this code-move -- since some of the code that's moved in this patch is code that I've suggested changes to above.)
Attachment #8778051 - Flags: review?(dholbert) → review+
Comment on attachment 8778052 [details]
Bug 1216843 - Part 13: Implement color accumulation in filter property.

https://reviewboard.mozilla.org/r/69438/#review68930

r=me with the following nit addressed:

::: layout/style/StyleAnimationValue.cpp
(Diff revision 5)
>      }
>      case eUnit_Shadow: {
>        UniquePtr<nsCSSValueList> result =
> -        AddWeightedShadowList(aCoeff1,
> -                              aValue1.GetCSSValueListValue(),
> +        AddWeightedShadowList(aCoeff1, aValue1.GetCSSValueListValue(),
> +                              aCoeff2, aValue2.GetCSSValueListValue(),
> -                              aCoeff2,

This change (rewrapping the args in a AddWeightedShadowList call) is non-functional & isn't relevant to this patch.   Please revert this change.

(If you really do want these lines to end up rewrapped like this, please make that change in whichever earlier patch split these args into multiple lines, rather than in this patch -- or make it in a whitespace-only fixup patch at the end of this patch-queue, if you like.)
Attachment #8778052 - Flags: review?(dholbert) → review+
Depends on: 1294614
Depends on: 1295107
Attachment #8778055 - Attachment is obsolete: true
Comment on attachment 8778047 [details]
Bug 1216843 - Part 8: Make AddShadowItems() return UniquePtr and rename it to AddWeightedShadowItems().

https://reviewboard.mozilla.org/r/69428/#review68910

> This 8-line chunk is repeated twice in this patch (twice in this same function, actually), and the logic is subtle, so it'd be nice to factor it into a helper-function.  Perhaps something like this (haven't tested this to see if it compiles, but it's hopefully pretty close):
> 
> // Appends a value to a nsCSSValueList.
> // aHead is the head of the list (and may be null, if we're "appending" the
> // first entry).
> // *aTail is the (current) final nsCSSValueList (or null if aHead is null).
> // This function will update *aTail to the (appended) new final entry, so
> // the caller can keep track of the tail.
> void
> AppendToCSSValueList(UniquePtr<nsCSSValueList>& aHead,
>                      UniquePtr<nsCSSValueList>&& aValToAppend,
>                      nsCSSValueList** aTail)
> {
>   MOZ_ASSERT(!aHead == !*aTail,
>              "Can't have head w/o tail, & vice versa");
> 
>   if (!aHead) {
>     aHead = Move(aToAppend);
>     *aTail = aHead->mNext;
>   } else {
>     (*aTail) = (*aTail)->mNext = aValToAppend.release();
>   }
> }
> 
> ...and then you'd call this like so, replacing each of your "Append the result" chunks in this patch:
>   // Append the result.
>   AppendToCSSValueList(result, Move(shadowValue), &tail);

Thanks! This looks really nice.
Comment on attachment 8778052 [details]
Bug 1216843 - Part 13: Implement color accumulation in filter property.

Hmm, MozReview something mistook about part 14. The patch already got r+ by Brian.  Sorry for bothering.
Attachment #8778052 - Flags: review?(bbirtles) → review+
Firstly: sorry for the wait here!

(In reply to Hiroyuki Ikezoe (:hiro) from comment #140)
> Hmm, MozReview something mistook about part 14. The patch already got r+ by
> Brian.  Sorry for bothering.

Darn -- it actually looks like MozReview got confused after you removed Part 6 & uploaded new versions, and it's lined up Parts 7 through 14 with the old Parts 6 through 13, or something like that. :-/  And that completely breaks its r+ state as well as its interdiffing.

e.g. if you visit https://reviewboard.mozilla.org/r/69424/ and scroll to the bottom (to "Review request changed" for "Revision 5"), you'll see that MozReview thinks the old Part 6 has morphed into Part 7! Similarly, it thinks part 7 has morphed into Part 8, as you can see if you scroll to the bottom of https://reviewboard.mozilla.org/r/69426/

I'm pretty sure it got confused like that because you accidentally bumped the MozReview-Commit-ID somehow, in your latest versions of these patches. Part 7 used to have:
> Bug 1216843 - Part 7: Implement color accumulation. r?dholbert
> MozReview-Commit-ID: Ic7dIrZWvih
(visible at the bottom of https://reviewboard.mozilla.org/r/69426/)

...but the latest version now has:
> Bug 1216843 - Part 7: Implement color accumulation. r?dholbert
> MozReview-Commit-ID: A7E1TzjvMYm
https://reviewboard.mozilla.org/r/69424/

If that ID had stayed constant, MozReview would've been able to figure out that you were uploading a new version of Part 7 (rather than a new version of Part 6 which happens to have Part 7 in the commit message).  But some mercurial operation that you performed (perhaps regenerating the patch from scratch, or folding something into it, or something else) caused the MozReview-Commit-ID to be changed.

In future MozReview sessions, be careful to not change that ID -- particularly when a patch is added/removed in the middle of the series, which presents an opportunity for MozReview to get confused & lean heavily on the IDs for matching.  Also, consider taking a close look at the interdiffs when you upload new versions of patches, to be sure they look like you expect them to.  (In this case, the latest interdiff for what's currently labeled "Part 7" shows a ton of bogus changes -- part 6 being applied in reverse, basically -- which is a hint that the patch was matched incorrectly. That interdiff page is here: https://reviewboard.mozilla.org/r/69424/diff/4-5/ )
gps tells me that we **can get MozReview back to a sane state** here (with current Part 7 matched against old versions of Part 7, for example). And that would be worth doing, because the patch stack here is large & complex, and it'll make interdiffs actually useful again (if we just skip the intermediate version) and it'll make the review process here actually possible to follow, I think.

To do that, please:
 (1) Restore the original MozReview-Commit-IDs in your local patches/changesets.
   (Or just generate new MozReview-Commit-IDs for part 7 and everything after it)

 (2) Add a dummy version of "Part 6" which makes no changes (or dummy/trivial changes, e.g. a whitespace tweak somewhere)
 (3) With that fixed, push to review again. (with the same number of patches as you originally had)

  This should get the current patches matched up with the original patches (with a brief diversion in the middle of the history for each patch, which we can skip over).

 (4) Then, locally remove your "dummy" version of Part 6.
 (5) ...and finally, push to review again, making sure the MozReview-Commit-IDs haven't changed since your tweak to them from (1).

Steps 1-3 should get us back to a state where original-part-7 is matched up against latest-version-of-part-7. And then steps 4 and 5 should let us correctly remove part 6 in a way that don't throw off the patch matching (as long as the commit IDs don't change).

Does that make sense?
Flags: needinfo?(hiikezoe)
(I filed bug 1216843 on seeing what we can do to avoid getting into this state with MozReview.)
Firstly I should note that I've been using MQ instead bookmark.  So I guess it causes this annoying symptoms.  Also all of the patch files in my local patch queue directory do not have 'MozReview-Commit-ID' in commit log.  I guess the IDs were generated while pushing review request.  I saw MozReview-Commit-ID in each commit message while creating each patch before, so something has been changed on my local mercurial settings.

So,  now I picked MozReview-Commit-IDs from a previous request (e.g. https://reviewboard-hg.mozilla.org/gecko/rev/ee6c3a4df462) and put them in patch files by hand.  And now I am trying to push it to review (it's step (3) in comment 142), as far as I see interdiff between revision 4 and 6 on draft pages (or revision 5 and 7, patches following part 7 have revision 7 somehow, I don't know why),  it seems to work as expected.  I really hope this will work!
Flags: needinfo?(hiikezoe)
Something is broken on an interaction between review board and bugzilla.  r+ flag against patch 13 was cleared on bugzilla. Whereas it still has r+ on review board.

I saw a review request of part 16 to Brian on the draft page,  I guessed that's because the number of the patch set increased.
Oh..  Another interesting thing is that the dummy part 6 does not have 'r?' in the commit message but there is an request to Daniel..  
The 'MozReview-Commit-ID' of the part 6 was newly generated (actually the patch file on my local machine does not have the 'MozReview-Commit-ID' so it's maybe generated while pushing), so it should not track ex-part6.
(In reply to Daniel Holbert [:dholbert] from comment #112)
> Comment on attachment 8778045 [details]
> Bug 1216843 - Part 6: A dummy empty changeset.

I really don't understand what happened here.  Did MozReview change a past comment?
Attachment #8778045 - Attachment is obsolete: true
Attachment #8778045 - Flags: review?(dholbert)
I am not really sure that the process worked as we supposed. 
Interdiffs are still hard to understand, e.g. https://reviewboard.mozilla.org/r/69428/diff/5-8/ , I guess that's because there is a broken revision 6.
Attachment #8789623 - Flags: review?(bbirtles) → review+
Attachment #8778052 - Flags: review+
Comment on attachment 8789623 [details]
Bug 1216843 - Part 16: Fix bug number for implementation of keyframe composition.

https://reviewboard.mozilla.org/r/77780/#review76066

Looks good to me (although I wonder if we should fix bug 1216844 and bug 1291468 as one bug, if I understand them correctly).

::: testing/web-platform/meta/web-animations/interfaces/KeyframeEffect/setKeyframes.html.ini:19
(Diff revision 2)
> +  [Keyframes can be replaced with a one property one keyframe sequence]
> +    expected: FAIL
> +
> +  [Keyframes can be replaced with a single keyframe sequence with omitted offsets]
> +    expected: FAIL

We should probably add a bug number for these. Perhaps a dependent bug on bug 1216844 to produce the missing 0%/100% value using additive behavior.
Attachment #8789623 - Flags: review+
Comment on attachment 8778046 [details]
Bug 1216843 - Part 7: Implement color accumulation.

https://reviewboard.mozilla.org/r/69426/#review76404

This part isn't quite r+ yet because of my concerns about hsl/hsla & redundant work, below (sorry for not thinking of them sooner):

::: layout/style/StyleAnimationValue.cpp:1203
(Diff revision 8)
>    // To save some math, scale the alpha down to a 0-1 scale, but
>    // leave the color components on a 0-255 scale.
> -  double A1 = NS_GET_A(color1) * (1.0 / 255.0);
> -  double R1 = NS_GET_R(color1) * A1;
> -  double G1 = NS_GET_G(color1) * A1;
> -  double B1 = NS_GET_B(color1) * A1;
> +  double A1 = GET_COLOR_COMPONENT(Alpha, A)(aValue1) * (1.0 / 255.0);
> +  double R1 = GET_COLOR_COMPONENT(Comp1, R)(aValue1) * A1;
> +  double G1 = GET_COLOR_COMPONENT(Comp2, G)(aValue1) * A1;
> +  double B1 = GET_COLOR_COMPONENT(Comp3, B)(aValue1) * A1;

Hmm, so I'm just realizing that this will make us do some redudnant colorspace-conversion-work (repeating the same work 4 times), for hsl/hsla() colors.

Specifically:
 * In current mozilla-central, this code calls value.GetColorValue() once (up-front) and pull components out of that. (And that GetColorValue call involves a call to NS_HSL2RGB under the hood.)
 * But with this patch, we'll be calling value.GetColorValue() for *each component* (in the final chunk of GET_COLOR_COMPONENT), i.e. 4 times instead of once, and doing a redundant NS_HSL2RGB call in each one.

(This is only a problem for hsl/hsla, because those are the only values that will take the GetColorValue() codepath & where GetColorValue() has a non-trivial implementation.)

To avoid this redundant work, I think we need to call GetColorValue() to get an nscolor up-front (at least in the non-PercentageRGBAColor/PercentageRGBColor case) and pass that nscolor to GET_COLOR_COMPONENT for it to use, so it doesn't have to call GetColorValue() over and over...  This is a bit less elegant than what you've got, but I think we need to avoid the redundant work somehow. (maybe you can come up with something even better/cleaner, too.)

::: layout/style/StyleAnimationValue.cpp:1226
(Diff revision 8)
> -  uint8_t Gres = ClampColor((G1 * aCoeff1 + G2 * aCoeff2) * factor);
> -  uint8_t Bres = ClampColor((B1 * aCoeff1 + B2 * aCoeff2) * factor);
> -  aResult.SetColorValue(NS_RGBA(Rres, Gres, Bres, Ares));
> +  double Bres = (B1 * aCoeff1 + B2 * aCoeff2) * factor;
> +
> +  if (aAdditionType == ColorAdditionType::Clamped) {
> +    aResult.SetColorValue(
> +      NS_RGBA(ClampColor(Rres), ClampColor(Gres), ClampColor(Bres),
> +              ClampColor(NSToIntRound(Aresf * 255.0))));

We don't need the ClampColor for Aresf here, because the logic above already ensures that Aresf is in the [0.0, 1.0] range, if we get here.

And that means NSToIntRound(AResf * 255.0) shouldn't be able to produce anything outside of the 0-255 range, and therefore the ClampColor(...) is unnecessary.

::: layout/style/StyleAnimationValue.cpp:1230
(Diff revision 8)
> +      NS_RGBA(ClampColor(Rres), ClampColor(Gres), ClampColor(Bres),
> +              ClampColor(NSToIntRound(Aresf * 255.0))));
> +    return;
> +  }
> +
> +  Rres = Rres * (1.0 / 255);

s/255/255.0/ to make it clearer that we're doing float math here (note that we consistently use 255.0 elsewhere in this function)
Attachment #8778046 - Flags: review?(dholbert) → review-
Comment on attachment 8778047 [details]
Bug 1216843 - Part 8: Make AddShadowItems() return UniquePtr and rename it to AddWeightedShadowItems().

https://reviewboard.mozilla.org/r/69428/#review76420

r=me on this part
Attachment #8778047 - Flags: review?(dholbert) → review+
Comment on attachment 8778046 [details]
Bug 1216843 - Part 7: Implement color accumulation.

https://reviewboard.mozilla.org/r/69426/#review76404

> Hmm, so I'm just realizing that this will make us do some redudnant colorspace-conversion-work (repeating the same work 4 times), for hsl/hsla() colors.
> 
> Specifically:
>  * In current mozilla-central, this code calls value.GetColorValue() once (up-front) and pull components out of that. (And that GetColorValue call involves a call to NS_HSL2RGB under the hood.)
>  * But with this patch, we'll be calling value.GetColorValue() for *each component* (in the final chunk of GET_COLOR_COMPONENT), i.e. 4 times instead of once, and doing a redundant NS_HSL2RGB call in each one.
> 
> (This is only a problem for hsl/hsla, because those are the only values that will take the GetColorValue() codepath & where GetColorValue() has a non-trivial implementation.)
> 
> To avoid this redundant work, I think we need to call GetColorValue() to get an nscolor up-front (at least in the non-PercentageRGBAColor/PercentageRGBColor case) and pass that nscolor to GET_COLOR_COMPONENT for it to use, so it doesn't have to call GetColorValue() over and over...  This is a bit less elegant than what you've got, but I think we need to avoid the redundant work somehow. (maybe you can come up with something even better/cleaner, too.)

Thanks!  I did not overlook the fact at all.  In this revision I did wrote a new utility function to get all components of nscolor or nsCSSValueFloatColor as a Tuple<double red, double green, double blue, double alpha>.
Comment on attachment 8778046 [details]
Bug 1216843 - Part 7: Implement color accumulation.

https://reviewboard.mozilla.org/r/69426/#review76650

I feel like this could perhaps be split into 2 patches -- with the first part adding & using this new GetPremultipliedColor() function, but only doing the "Clamped" behavior (so as not to change behavior) -- and the second patch introducing the Clamped/Unclamped enum & distinction, to actually implement accumulation.

If you'd like to make that division, great!  But, I won't require it; it's probably fine all together like this too.

r=me [unless you split the patch up, in which case one more look-see would be good], with the following addressed:

::: layout/style/StyleAnimationValue.cpp:1166
(Diff revisions 8 - 9)
>    float result = (n1 - aInitialVal) * aCoeff1 + (n2 - aInitialVal) * aCoeff2;
>    aResult.SetFloatValue(RestrictValue(aValueRestrictions, result + aInitialVal),
>                          eCSSUnit_Number);
>  }
>  
> -// Color component value might be greater than 1.0 in case when the color
> +// Returns Tuple(Red, Green, Blue, Alpha).

Please add a comment hinting at the ranges of the returned values here (since it's different per-component, and the sources that you draw from have different ranges).

E.g. "Red, Green, and Blue are scaled to the [0,255] range, and Alpha is scaled to the [0,1] range (though values are allowed to fall outside of these ranges)."

::: layout/style/StyleAnimationValue.cpp:1168
(Diff revisions 8 - 9)
>                          eCSSUnit_Number);
>  }
>  
> -// Color component value might be greater than 1.0 in case when the color
> -// value is accumulated, so we can't use GetColorValue() here because
> -// that function clamps its values.
> +// Returns Tuple(Red, Green, Blue, Alpha).
> +static Tuple<double, double, double, double>
> +GetPremultipliedColor(const nsCSSValue& aValue)

Maybe name this "GetPremultipliedColorComponents" (adding "Components"), since that's the key thing that it does -- it separates out the components, regardless of the internal representation.

(This makes the name a bit longer, but it still seems to fit nicely at all of the callsites.)

::: layout/style/StyleAnimationValue.cpp:1179
(Diff revisions 8 - 9)
> -    }                                                       \
> -    return NS_GET_##capital_(value.GetColorValue());        \
> +  if (aValue.GetUnit() == eCSSUnit_PercentageRGBColor ||
> +      aValue.GetUnit() == eCSSUnit_PercentageRGBAColor) {
> +    nsCSSValueFloatColor* floatColor = aValue.GetFloatColorValue();
> +    double alpha = floatColor->Alpha();
> +    return MakeTuple(
> +        floatColor->Comp1() * 255.0 * alpha,

The newline + 4-space-indentation inside of "MakeTuple" here is kinda nice for readability, but it doesn't really match the coding style guide, and it's not necessary to avoid going over 80 characters.

So, let's just unwrap this to:
    return MakeTuple(floatColor->Comp1() * 255.0 * alpha,
                     floatColor->Comp2() * 255.0 * alpha,
etc.

::: layout/style/StyleAnimationValue.cpp:1188
(Diff revisions 8 - 9)
> +  }
> +
> +  nscolor color = aValue.GetColorValue();
> +  double alpha = NS_GET_A(color) * (1.0 / 255.0);
> +  return MakeTuple(
> +      NS_GET_R(color) * alpha,

Same here - please don't wrap inside of the function call here.
Attachment #8778046 - Flags: review?(dholbert) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf96c8cbfc3d

A new part 6 patch has been split from part 7 as Daniel suggested in comment 194.

I will land these patches to inbound because I am afraid that the part 6 patch will break something on reviewboard again.

Daniel, thank you for kindly reviewing lots of patches and bugs and taking time in your busy schedule!
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a5ed6ae7f47d9f7d46d0db3c04903a52dd7606f
Bug 1216843 - Part 1: Tests for effect iteration composition. r=birtles

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff3627ca174dc0c280578abd4819a15439a51735
Bug 1216843 - Part 2: Implement effect iteration composition. r=birtles, r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/a62b8b9b0a3ba72e43a9ab5fc19c7ae07c3ea2d2
Bug 1216843 - Part 3: Use nsCSSValue instead of nscolor to store over 1.0 color components as an accumulated value so that we can correctly calculate intermediate values in interpolation. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ad4796f6bb82e2734c26d048304b6f19651e16d
Bug 1216843 - Part 4: Pass nsCSSValue& to AddWeightedColors and DiluteColor. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b9e1beaf507734232082af1920380595fda0fcf
Bug 1216843 - Part 5: Reuse AddWeightedColors and DiluteColor in AddShadowItems(). r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c1631ef0ad9d31c59a02a0a6fc26968414ea376
Bug 1216843 - Part 6: Add GetPremultipliedColorComponents. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/8bef6bb7442c95e31ebe6217a15698c8c2f820ec
Bug 1216843 - Part 7: Implement color accumulation. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/9900b2f32860efbf5d4f6c9df1bb37e0e403e090
Bug 1216843 - Part 8: Make AddShadowItems() return UniquePtr and rename it to AddWeightedShadowItems(). r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce1f420b495f2b559fd2a7d1b0501c6d93d5bc23
Bug 1216843 - Part 9: Factor out AddWeightedShadowList. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/fcb4c57e5ede31174798913bc4ecc82fb1b9cb2a
Bug 1216843 - Part 10: Implement box-shadow/text-shadow color accumulation. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/1811a83244dddf5da42ff337c7af2da05591d378
Bug 1216843 - Part 11: Make AddFilterFunction and AddFilterFunctionImpl return UniquePtr and rename them to AddWeightedFilterXX. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1511cefd6cacc32f9d46c63bb2562231e30247b
Bug 1216843 - Part 12: Factor out AddWeightedFilterList. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/759650fae57e703b05f4cfb28d2770913e70b17f
Bug 1216843 - Part 13: Implement color accumulation in filter property. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/15ca08d807f60ac373dbcc58fbc80b8b41515f88
Bug 1216843 - Part 14: Reftest for iterationComposite. r=birtles

https://hg.mozilla.org/integration/mozilla-inbound/rev/7008223109637735009b7c15a02a225226ff8396
Bug 1216843 - Part 15: Update styles when current iteration changed. r=birtles

https://hg.mozilla.org/integration/mozilla-inbound/rev/65cfe177d3163ad5997b2566e66c350ffef1938c
Bug 1216843 - Part 16: Fix bug number for implementation of keyframe composition. r=birtles
Sure! Thanks for being patient with me. :)
hiro, I have several questions when I'm rewriting my patches for bug 1299741 regarding your changes here:

The condition used in AddWeightedShadowItems for calling DiluteColor is different from that in AddWeighted (the former is "aCoeff2 == 0.0 && aCoeff1 != 1.0" and the later is just "aCoeff2 == 0.0", is that intended?

The AddWeightedColors call in StyleAnimationValue::Accumulate doesn't have associated DiluteColor, is that also intended?

DiluteColor always clamp the color components, and consequently if aCoeff2 == 0.0 and aCoeff1 != 1.0, AddWeightedShadowItems would return clamped color, is that also intended?
Flags: needinfo?(hiikezoe)
(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #199)
> hiro, I have several questions when I'm rewriting my patches for bug 1299741
> regarding your changes here:
> 
> The condition used in AddWeightedShadowItems for calling DiluteColor is
> different from that in AddWeighted (the former is "aCoeff2 == 0.0 && aCoeff1
> != 1.0" and the later is just "aCoeff2 == 0.0", is that intended?

Ah, that was a mistake.  I forgot to drop the "aCoeff1 != 1.0" condition there.  Would you mind dropping it in bug 1299741?

> The AddWeightedColors call in StyleAnimationValue::Accumulate doesn't have
> associated DiluteColor, is that also intended?

Yes, it's intentional.  As far as I can tell Accumulate does not need DiluteColor.

> DiluteColor always clamp the color components, and consequently if aCoeff2
> == 0.0 and aCoeff1 != 1.0, AddWeightedShadowItems would return clamped
> color, is that also intended?

Yes for now.  But it will be changed once colors of SVG animation are clamped just before painting.  Please see bug 1294614 comment 16.

I am sorry for the superfluous work.
Flags: needinfo?(hiikezoe)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #200)
> (In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #199)
> > DiluteColor always clamp the color components, and consequently if aCoeff2
> > == 0.0 and aCoeff1 != 1.0, AddWeightedShadowItems would return clamped
> > color, is that also intended?
> 
> Yes for now.  But it will be changed once colors of SVG animation are
> clamped just before painting.  Please see bug 1294614 comment 16.

This doesn't sound quite sensible, because AddWeightColors may return unclamped result, but DiluteColor always does, which means you do not get consistent result in this case. Is that really what you want?

I suppose you want to either make DiluteColor respect the addition type, or only call it if addition type is clamped.

I had a patch in bug 1299741 to call DiluteColor inside AddWeightedColors (see [1]), so that we get consistent result when we adding colors without adding code to each callsite to distinguish the two cases. Do you think I should still do that? If so, what condition should that be? I suppose "aCoeff2 == 0 && aAdditionType == Clamped"? Or is there particular reason we should avoid calling DiluteColor in some cases?

[1] https://reviewboard.mozilla.org/r/75924/diff/2
Flags: needinfo?(hiikezoe)
(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #201)
> but DiluteColor always does

I meant, DiluteColor always returns clamped result.
(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #201)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #200)
> > (In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #199)
> > > DiluteColor always clamp the color components, and consequently if aCoeff2
> > > == 0.0 and aCoeff1 != 1.0, AddWeightedShadowItems would return clamped
> > > color, is that also intended?
> > 
> > Yes for now.  But it will be changed once colors of SVG animation are
> > clamped just before painting.  Please see bug 1294614 comment 16.
> 
> This doesn't sound quite sensible, because AddWeightColors may return
> unclamped result, but DiluteColor always does, which means you do not get
> consistent result in this case. Is that really what you want?
> 
> I suppose you want to either make DiluteColor respect the addition type, or
> only call it if addition type is clamped.

Yes, that's right. I am sorry current code is somewhat misleading.

> I had a patch in bug 1299741 to call DiluteColor inside AddWeightedColors
> (see [1]), so that we get consistent result when we adding colors without
> adding code to each callsite to distinguish the two cases. Do you think I
> should still do that? If so, what condition should that be? I suppose
> "aCoeff2 == 0 && aAdditionType == Clamped"? Or is there particular reason we
> should avoid calling DiluteColor in some cases?

Or, 
 if (aCoeff2 ==0) {
   MOZ_ASSERT(aAdditionType == Clamped);
 }

My intention is that we do never call DiluteColor() with Clamped.
Flags: needinfo?(hiikezoe)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #203)

> Or, 
>  if (aCoeff2 ==0) {
>    MOZ_ASSERT(aAdditionType == Clamped);
>  }
> 
> My intention is that we do never call DiluteColor() with Clamped.

with Unclamped.
The function DiluteColor looks weird:
> double Aresf = A * aDilutionRatio;
> double R = NS_GET_R(color) * A;
> double factor = 1.0 / Aresf;
> (return) ClampColor(R * aDilutionRatio * factor)

If you expand everything into the final formula, you would find that only NS_GET_R(color) is left after simplification, which indicates that the majority of computation in this function is actually useless.

Is it intended to write this way to match the structure of AddWeightedColors? Could we just put NS_GET_R(color) in the final result?
Flags: needinfo?(hiikezoe)
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #205)
> The function DiluteColor looks weird:
> > double Aresf = A * aDilutionRatio;
> > double R = NS_GET_R(color) * A;
> > double factor = 1.0 / Aresf;
> > (return) ClampColor(R * aDilutionRatio * factor)
> 
> If you expand everything into the final formula, you would find that only
> NS_GET_R(color) is left after simplification, which indicates that the
> majority of computation in this function is actually useless.
> 
> Is it intended to write this way to match the structure of
> AddWeightedColors? Could we just put NS_GET_R(color) in the final result?

Ah! Exactly!  I did not notice the fact, just modified the original.  Please fix it. Thanks.
Flags: needinfo?(hiikezoe)
Depends on: 1320655
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: