Web Animations CSS: Snapshot missing start/end Keyframes and remove duplicates

Web Animations CSS: Snapshot missing start/end Keyframes and remove duplicates

Construction of start/end keyframes and normalization is required outside
KeyframeAnimationEffect since CSS Animations have constraints outside the 
Web Animations model.

BUG=258896
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155677

[Timothy Loh, 2013-08-05 03:44:59]

https://codereview.chromium.org/21779003/diff/4001/Source/core/css/resolver/S...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/21779003/diff/4001/Source/core/css/resolver/S...
Source/core/css/resolver/StyleResolver.cpp:802: unsigned duplicates = 0;
I think this should work:
    keyframes.shrink(std::unique(keyframes.begin(), keyframes.end()) -
keyframes.begin());

[dstockwell, 2013-08-05 05:52:18]

https://codereview.chromium.org/21779003/diff/4001/Source/core/css/resolver/S...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/21779003/diff/4001/Source/core/css/resolver/S...
Source/core/css/resolver/StyleResolver.cpp:802: unsigned duplicates = 0;
On 2013/08/05 03:44:59, Timothy Loh wrote:
> I think this should work:
>     keyframes.shrink(std::unique(keyframes.begin(), keyframes.end()) -
> keyframes.begin());

Tim and I discussed off thread, and while something like this should work (keep
the last as opposed to the first duplicate):

keyframes.remove(0, keyframes.rend() - std::unique(keyframes.rbegin(),
keyframes.rend(), equalOffsets))

It's not clear if it's a win, and still appears to be the first use of
std::unique in blink??

[Steve Block, 2013-08-06 06:30:20]

https://codereview.chromium.org/21779003/diff/4001/Source/core/animation/css/...
File Source/core/animation/css/CSSAnimations.cpp (right):

https://codereview.chromium.org/21779003/diff/4001/Source/core/animation/css/...
Source/core/animation/css/CSSAnimations.cpp:185: // FIXME: Keyframes are already
normalized, perhaps there should be a flag on KeyframeAnimationEffect to skip
normalization.
Sounds reasonable. Filed crbug.com/268791

https://codereview.chromium.org/21779003/diff/4001/Source/core/css/resolver/S...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/21779003/diff/4001/Source/core/css/resolver/S...
Source/core/css/resolver/StyleResolver.cpp:769: }
We already have this helper in KeyframeAnimationEffect. Maybe we should make it
a static member of Keyframe so it can be shared?

https://codereview.chromium.org/21779003/diff/4001/Source/core/css/resolver/S...
Source/core/css/resolver/StyleResolver.cpp:802: unsigned duplicates = 0;
Does unique() work with reverse iterators?

https://codereview.chromium.org/21779003/diff/4001/Source/core/css/resolver/S...
Source/core/css/resolver/StyleResolver.cpp:808: keyframes[i - duplicates] =
keyframes[i];
I think this would be easier to follow if you used a local 'size_t targetIndex'
or 'iterator targetSlot' to represent the slot in the array that the current
keyframe will be moved to.

https://codereview.chromium.org/21779003/diff/4001/Source/core/css/resolver/S...
Source/core/css/resolver/StyleResolver.cpp:813: bool missingStartKeyframe =
!keyframes[0]->offset;
I think you have this inverted.

Also, this local isn't a keyframe, so should probably be isStartKeyframeMissing

https://codereview.chromium.org/21779003/diff/4001/Source/core/css/resolver/S...
Source/core/css/resolver/StyleResolver.cpp:819: for (unsigned i = 0; i <
styleKeyframes.size(); ++i) {
I think you should use size_t in loops like this, as that what Vector::size()
and Vector::operator[] use.

[dstockwell, 2013-08-06 07:14:28]

https://codereview.chromium.org/21779003/diff/4001/Source/core/css/resolver/S...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/21779003/diff/4001/Source/core/css/resolver/S...
Source/core/css/resolver/StyleResolver.cpp:769: }
On 2013/08/06 06:30:20, Steve Block wrote:
> We already have this helper in KeyframeAnimationEffect. Maybe we should make
it
> a static member of Keyframe so it can be shared?

Done.

https://codereview.chromium.org/21779003/diff/4001/Source/core/css/resolver/S...
Source/core/css/resolver/StyleResolver.cpp:808: keyframes[i - duplicates] =
keyframes[i];
On 2013/08/06 06:30:20, Steve Block wrote:
> I think this would be easier to follow if you used a local 'size_t
targetIndex'
> or 'iterator targetSlot' to represent the slot in the array that the current
> keyframe will be moved to.

Done.

https://codereview.chromium.org/21779003/diff/4001/Source/core/css/resolver/S...
Source/core/css/resolver/StyleResolver.cpp:813: bool missingStartKeyframe =
!keyframes[0]->offset;
On 2013/08/06 06:30:20, Steve Block wrote:
> I think you have this inverted.

Fixed. Be nice if we could write != 0 :)

> Also, this local isn't a keyframe, so should probably be
isStartKeyframeMissing

Done.

https://codereview.chromium.org/21779003/diff/4001/Source/core/css/resolver/S...
Source/core/css/resolver/StyleResolver.cpp:819: for (unsigned i = 0; i <
styleKeyframes.size(); ++i) {
On 2013/08/06 06:30:20, Steve Block wrote:
> I think you should use size_t in loops like this, as that what Vector::size()
> and Vector::operator[] use.

Done.

[Steve Block, 2013-08-07 01:50:40]

lgtm

https://codereview.chromium.org/21779003/diff/10001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/21779003/diff/10001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:801: targetIndex = previousIndex;
Might be easier to read if you increment targetIndex if the offsets don't match,
rather than always incrementing it and then decrementing it when they do match.

https://codereview.chromium.org/21779003/diff/10001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:807: bool isMissingStartKeyframe =
keyframes[0]->offset;
Yeah, the lack of '== 0' and '!= 0' is my least favourite WebKit style rule.

My point about naming the local was to avoid ending with 'keyframe' to make
clear this isn't a keyframe.

[dstockwell, 2013-08-07 03:40:22]

https://codereview.chromium.org/21779003/diff/10001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/21779003/diff/10001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:801: targetIndex = previousIndex;
On 2013/08/07 01:50:45, Steve Block wrote:
> Might be easier to read if you increment targetIndex if the offsets don't
match,
> rather than always incrementing it and then decrementing it when they do
match.

Done.

https://codereview.chromium.org/21779003/diff/10001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:807: bool isMissingStartKeyframe =
keyframes[0]->offset;
On 2013/08/07 01:50:45, Steve Block wrote:
> Yeah, the lack of '== 0' and '!= 0' is my least favourite WebKit style rule.
> 
> My point about naming the local was to avoid ending with 'keyframe' to make
> clear this isn't a keyframe.

Done.

[commit-bot: I haz the power, 2013-08-07 03:41:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/21779003/17001

[commit-bot: I haz the power, 2013-08-07 03:41:07]

Failed to apply patch for Source/core/animation/css/CSSAnimations.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/animation/css/CSSAnimations.cpp
  Hunk #1 FAILED at 182.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/animation/css/CSSAnimations.cpp.rej

Patch:       Source/core/animation/css/CSSAnimations.cpp
Index: Source/core/animation/css/CSSAnimations.cpp
diff --git a/Source/core/animation/css/CSSAnimations.cpp
b/Source/core/animation/css/CSSAnimations.cpp
index
281fea9593df8aa436fe62b624b4f0408382af41..e67ad68dc93c192c9bfbd8bc6c67e0bc011af995
100644
--- a/Source/core/animation/css/CSSAnimations.cpp
+++ b/Source/core/animation/css/CSSAnimations.cpp
@@ -182,6 +182,7 @@ void CSSAnimations::update(Element* element, const
RenderStyle* style)
             if (!keyframes.isEmpty()) {
                 Timing timing;
                 timingFromAnimationData(animationData, timing);
+                // FIXME: Keyframes are already normalized, perhaps there
should be a flag on KeyframeAnimationEffect to skip normalization.
                 m_animations.set(animationName.impl(),
element->document()->timeline()->play(
                     Animation::create(element,
KeyframeAnimationEffect::create(keyframes), timing).get()).get());
             }

[commit-bot: I haz the power, 2013-08-07 03:47:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/21779003/8002

[commit-bot: I haz the power, 2013-08-07 04:07:16]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_blink_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-07 07:02:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/21779003/38001

[commit-bot: I haz the power, 2013-08-07 10:29:39]

Change committed as 155677