Currently, GraphicsLayerChromium creates CCActiveAnimation objects to pass along to the LayerChromium. It should instead pass WebAnimations to the WebLayer.
Created attachment 150636 [details] Patch
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.
Created attachment 152311 [details] Patch Rebased patch.
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.
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.
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.
(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 :)
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 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 on attachment 155568 [details] Patch Clearing flags on attachment: 155568 Committed r124239: <http://trac.webkit.org/changeset/124239>
All reviewed patches have been landed. Closing bug.