Bug 90468 - [chromium] Use WebAnimation and related classes in GraphicsLayerChromium and AnimTranslationUtil
Summary: [chromium] Use WebAnimation and related classes in GraphicsLayerChromium and ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: vollick
URL:
Keywords:
Depends on: 90303
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-03 10:02 PDT by vollick
Modified: 2012-07-31 12:38 PDT (History)
8 users (show)

See Also:


Attachments
Patch (101.09 KB, patch)
2012-07-03 10:04 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (42.39 KB, patch)
2012-07-13 11:49 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (43.25 KB, patch)
2012-07-24 09:49 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (42.41 KB, patch)
2012-07-31 08:23 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (44.38 KB, patch)
2012-07-31 09:34 PDT, vollick
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description vollick 2012-07-03 10:02:16 PDT
Currently, GraphicsLayerChromium creates CCActiveAnimation objects to pass along to the LayerChromium. It should instead pass WebAnimations to the WebLayer.
Comment 1 vollick 2012-07-03 10:04:18 PDT
Created attachment 150636 [details]
Patch
Comment 2 WebKit Review Bot 2012-07-03 10:25:44 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 vollick 2012-07-13 11:49:57 PDT
Created attachment 152311 [details]
Patch

Rebased patch.
Comment 4 Adrienne Walker 2012-07-16 18:22:33 PDT
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.
Comment 5 vollick 2012-07-24 09:49:21 PDT
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.
Comment 6 James Robinson 2012-07-30 20:49:06 PDT
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)
Comment 7 vollick 2012-07-31 08:23:46 PDT
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.
Comment 8 WebKit Review Bot 2012-07-31 08:50:36 PDT
Comment on attachment 155545 [details]
Patch

Attachment 155545 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13385985
Comment 9 vollick 2012-07-31 09:34:21 PDT
Created attachment 155568 [details]
Patch

Added missing file.
Comment 10 James Robinson 2012-07-31 10:42:51 PDT
(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?
Comment 11 vollick 2012-07-31 10:55:59 PDT
(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 :)
Comment 12 WebKit Review Bot 2012-07-31 12:05:02 PDT
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
Comment 13 Adam Barth 2012-07-31 12:09:24 PDT
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.
Comment 14 WebKit Review Bot 2012-07-31 12:38:51 PDT
Comment on attachment 155568 [details]
Patch

Clearing flags on attachment: 155568

Committed r124239: <http://trac.webkit.org/changeset/124239>
Comment 15 WebKit Review Bot 2012-07-31 12:38:56 PDT
All reviewed patches have been landed.  Closing bug.