Bug 90468 – [chromium] Use WebAnimation and related classes in GraphicsLayerChromium and AnimTranslationUtil

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

Currently, GraphicsLayerChromium creates CCActiveAnimation objects to pass along to the LayerChromium. It should instead pass WebAnimations to the WebLayer.

Created attachment 150636 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Created attachment 152311 [details]
Patch

Rebased patch.

[Adrienne Walker]

Comment on attachment 152311 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152311&action=review

This is all a pretty straightforward conversion, and looks good in general.  I have one small comment:

> Source/Platform/chromium/public/WebAnimation.h:49
> -        WebAnimationTransform = 1,
> -        WebAnimationOpacity
> +        TargetPropertyTransform = 1,
> +        TargetPropertyOpacity

Were these just not used for anything previously?

> Source/WebCore/platform/graphics/chromium/AnimationTranslationUtil.cpp:172
> -bool appendKeyframe<TransformAnimationValue, CCTransformKeyframe, CCKeyframedTransformAnimationCurve>(CCKeyframedTransformAnimationCurve& curve, double keyTime, const TransformAnimationValue* value, const TransformAnimationValue* lastValue, PassOwnPtr<CCTimingFunction> timingFunction, const FloatSize& boxSize)
> +bool appendKeyframe<TransformAnimationValue, WebTransformKeyframe, WebTransformAnimationCurve>(WebTransformAnimationCurve& curve, double keyTime, const TransformAnimationValue* value, const TransformAnimationValue* lastValue, bool isUsingCustomBezierTimingFunction, WebKit::WebAnimationCurve::TimingFunctionType timingFunctionType, double x1, double y1, double x2, double y2, const FloatSize& boxSize)

I have a personal style nit against functions that take boolean values and a lot of optional parameters that are only used when going down one path.  This kind of feels like you should have two functions, e.g. addCustomBezierKeyFrame and addWhateverNormalKeyFrame.

Created attachment 154089 [details]
Patch

(In reply to comment #4)
> (From update of attachment 152311 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152311&action=review
>
> This is all a pretty straightforward conversion, and looks good in general.  I have one small comment:
>
> > Source/Platform/chromium/public/WebAnimation.h:49
> > -        WebAnimationTransform = 1,
> > -        WebAnimationOpacity
> > +        TargetPropertyTransform = 1,
> > +        TargetPropertyOpacity
>
> Were these just not used for anything previously?
Yup, and I'd just noticed with this patch that their names didn't conform with the API standards.
>
> > Source/WebCore/platform/graphics/chromium/AnimationTranslationUtil.cpp:172
> > -bool appendKeyframe<TransformAnimationValue, CCTransformKeyframe, CCKeyframedTransformAnimationCurve>(CCKeyframedTransformAnimationCurve& curve, double keyTime, const TransformAnimationValue* value, const TransformAnimationValue* lastValue, PassOwnPtr<CCTimingFunction> timingFunction, const FloatSize& boxSize)
> > +bool appendKeyframe<TransformAnimationValue, WebTransformKeyframe, WebTransformAnimationCurve>(WebTransformAnimationCurve& curve, double keyTime, const TransformAnimationValue* value, const TransformAnimationValue* lastValue, bool isUsingCustomBezierTimingFunction, WebKit::WebAnimationCurve::TimingFunctionType timingFunctionType, double x1, double y1, double x2, double y2, const FloatSize& boxSize)
>
> I have a personal style nit against functions that take boolean values and a lot of optional parameters that are only used when going down one path.  This kind of feels like you should have two functions, e.g. addCustomBezierKeyFrame and addWhateverNormalKeyFrame.
Cool, fixed.

[James Robinson]

Comment on attachment 154089 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154089&action=review

R=me

> Source/Platform/chromium/public/WebAnimation.h:48
> +        TargetPropertyTransform = 1,

we normally start at 0, is there a specific reason these are different? are they matching some other enum?

> Source/Platform/chromium/public/WebLayer.h:148
> +    // Returns false if the anim cannot be added.

"anim"->"animation" to be consistent with other comments

> Source/WebCore/WebCore.gypi:6551
> +            'inspector/front-end/Images/searchPrev.png',

please revert this before landing (and fix your editor to be less overeager)

Created attachment 155545 [details]
Patch

(In reply to comment #6)
> (From update of attachment 154089 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154089&action=review
>
> R=me
>
> > Source/Platform/chromium/public/WebAnimation.h:48
> > +        TargetPropertyTransform = 1,
>
> we normally start at 0, is there a specific reason these are different? are they matching some other enum?
This was because it was used in a HashSet. Looking at HashTraits.h, we can make
these zero if we change the traits that we use, which I've done.
>
> > Source/Platform/chromium/public/WebLayer.h:148
> > +    // Returns false if the anim cannot be added.
>
> "anim"->"animation" to be consistent with other comments
Done.
>
> > Source/WebCore/WebCore.gypi:6551
> > +            'inspector/front-end/Images/searchPrev.png',
>
> please revert this before landing (and fix your editor to be less overeager)
Done.

[WebKit Review Bot]

Comment on attachment 155545 [details]
Patch

Attachment 155545 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13385985

Created attachment 155568 [details]
Patch

Added missing file.

[James Robinson]

(In reply to comment #9)
> Created an attachment (id=155568) [details]
> Patch
> 
> Added missing file.

Looks good on EWS, do you want r+ cq+ set?

(In reply to comment #10)
> (In reply to comment #9)
> > Created an attachment (id=155568) [details] [details]
> > Patch
> > 
> > Added missing file.
> 
> Looks good on EWS, do you want r+ cq+ set?

Yes, please :)

[WebKit Review Bot]

Comment on attachment 155568 [details]
Patch

Rejecting attachment 155568 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
uto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Add MIPS add32 function

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.

Full output: http://queues.webkit.org/results/13404141

[Adam Barth]

Comment on attachment 155568 [details]
Patch

Sorry, we're tweaking how the commit-queue actually lands patches and there's a rough edge in there that caught your patch.

[WebKit Review Bot]

Comment on attachment 155568 [details]
Patch

Clearing flags on attachment: 155568

Committed r124239: <http://trac.webkit.org/changeset/124239>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.