Bug 60525 – Implement reverse animation direction

• Component: CSS
• Status: RESOLVED FIXED

[Dean Jackson]

See http://lists.w3.org/Archives/Public/www-style/2011May/0090.html

[Dean Jackson]

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.

[L. David Baron]

Sounds fine to me.

[Igor Trindade Oliveira]

Created attachment 125534 [details]
Patch

Proposed patch.

[Early Warning System Bot]

Comment on attachment 125534 [details]
Patch

Attachment 125534 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11428136

[Noam Rosenthal]

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.

[WebKit Review Bot]

Comment on attachment 125534 [details]
Patch

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

[Simon Fraser (smfr)]

Comment on attachment 125534 [details]
Patch

r- based on Noam's comments.

[Igor Trindade Oliveira]

Created attachment 125673 [details]
Patch

Proposed patch v2.

[Igor Trindade Oliveira]

The last patch addresses Noam suggestions.

(In reply to comment #8)
> Created an attachment (id=125673) [details]
> Patch
> 
> Proposed patch v2.

[Noam Rosenthal]

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

[Igor Trindade Oliveira]

Created attachment 125880 [details]
Patch

Fix nitpick.

[Dean Jackson]

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

[Dean Jackson]

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.

[Radar WebKit Bug Importer]

<rdar://problem/10822885>

[Igor Trindade Oliveira]

Created attachment 126129 [details]
Patch

Updated patch. Add new tests.

[Dean Jackson]

Comment on attachment 126129 [details]
Patch

great!

[WebKit Review Bot]

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

[Igor Trindade Oliveira]

Comment on attachment 126129 [details]
Patch

Trying again, this failing test does not looks related with this patch

[WebKit Review Bot]

Comment on attachment 126129 [details]
Patch

Clearing flags on attachment: 126129

Committed r107162: <http://trac.webkit.org/changeset/107162>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.