Summary: | Implement reverse animation direction | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dean Jackson <dino> | ||||||||||
Component: | CSS | Assignee: | Dean Jackson <dino> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | cmarrin, dbaron, dglazkov, igor.oliveira, macpherson, menard, noam, rafael.lobo, simon.fraser, webkit-bug-importer, webkit.review.bot | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Dean Jackson
2011-05-09 18:22:00 PDT
David, Do you have opinions on this? We think it is a good idea but I don't want to implement it if Mozilla don't agree. Sounds fine to me. Created attachment 125534 [details]
Patch
Proposed patch.
Comment on attachment 125534 [details] Patch Attachment 125534 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11428136 Comment on attachment 125534 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125534&action=review Some nitpicks to the TextureMapper part. > Source/WebCore/platform/graphics/texmap/TextureMapperAnimation.cpp:29 > +static double normalizedAnimationValue(double runningTime, double duration, unsigned direction) Why not just pass the AnimationDirection enum? > Source/WebCore/platform/graphics/texmap/TextureMapperAnimation.cpp:42 > + if ((direction == Animation::AnimationDirectionAlternate) && (loopCount & 1) > + || ((direction == Animation::AnimationDirectionAlternateReverse) && !(loopCount & 1)) > + || direction == Animation::AnimationDirectionReverse) > + normalized = 1 - normalized; this part is a bit verbose... can be better in a static function that gets AnimationDirection and loopCount and returns if reverse or not. Comment on attachment 125534 [details] Patch Attachment 125534 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11428139 Comment on attachment 125534 [details]
Patch
r- based on Noam's comments.
Created attachment 125673 [details]
Patch
Proposed patch v2.
The last patch addresses Noam suggestions. (In reply to comment #8) > Created an attachment (id=125673) [details] > Patch > > Proposed patch v2. Comment on attachment 125673 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125673&action=review Apart from the naming nitpick, this is an r+ for me on the TextureMapper part. smfr/others, are you ok with the common part? > Source/WebCore/platform/graphics/texmap/TextureMapperAnimation.cpp:30 > +static bool animationValueIsReverse(Animation::AnimationDirection direction, int loopCount) <nitpicks/> name -> shouldReverseAnimationValue Created attachment 125880 [details]
Patch
Fix nitpick.
Comment on attachment 125880 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125880&action=review I think we need a test for "reverse" as well. I could only see one for "alternate-reverse". r- for the missing test, but otherwise looks good. NOTE: Apple will need to raise a followup bug because our hardware animation won't support this directly. > LayoutTests/animations/animation-direction-alternate-reverse.html:53 > +<!-- Test animation-direction: alternate --> Nit: it's now alternate-reverse While I think of it, it would be nice to add tests of this in combination with fill-mode too. To make sure things extend in the right way. Created attachment 126129 [details]
Patch
Updated patch. Add new tests.
Comment on attachment 126129 [details]
Patch
great!
Comment on attachment 126129 [details] Patch Rejecting attachment 126129 [details] from commit-queue. New failing tests: platform/chromium/compositing/layout-width-change.html Full output: http://queues.webkit.org/results/11460584 Comment on attachment 126129 [details]
Patch
Trying again, this failing test does not looks related with this patch
Comment on attachment 126129 [details] Patch Clearing flags on attachment: 126129 Committed r107162: <http://trac.webkit.org/changeset/107162> All reviewed patches have been landed. Closing bug. |