Bug 60525 - Implement reverse animation direction
: Implement reverse animation direction
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
: InRadar
:
:
  Show dependency treegraph
 
Reported: 2011-05-09 18:22 PST by
Modified: 2012-08-29 05:33 PST (History)


Attachments
Patch (12.61 KB, patch)
2012-02-05 13:47 PST, Igor Trindade Oliveira
simon.fraser: review-
webkit-ews: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch (12.89 KB, patch)
2012-02-06 11:19 PST, Igor Trindade Oliveira
no flags Review Patch | Details | Formatted Diff | Diff
Patch (12.90 KB, patch)
2012-02-07 11:23 PST, Igor Trindade Oliveira
dino: review-
dino: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch (21.74 KB, patch)
2012-02-08 12:02 PST, Igor Trindade Oliveira
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


------- Comment #1 From 2011-05-09 18:23:51 PST -------
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.
------- Comment #2 From 2011-05-09 18:38:32 PST -------
Sounds fine to me.
------- Comment #3 From 2012-02-05 13:47:02 PST -------
Created an attachment (id=125534) [details]
Patch

Proposed patch.
------- Comment #4 From 2012-02-05 13:58:46 PST -------
(From update of attachment 125534 [details])
Attachment 125534 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11428136
------- Comment #5 From 2012-02-05 14:10:13 PST -------
(From update of attachment 125534 [details])
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 #6 From 2012-02-05 14:16:11 PST -------
(From update of attachment 125534 [details])
Attachment 125534 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11428139
------- Comment #7 From 2012-02-06 00:17:24 PST -------
(From update of attachment 125534 [details])
r- based on Noam's comments.
------- Comment #8 From 2012-02-06 11:19:00 PST -------
Created an attachment (id=125673) [details]
Patch

Proposed patch v2.
------- Comment #9 From 2012-02-06 17:42:14 PST -------
The last patch addresses Noam suggestions.

(In reply to comment #8)
> Created an attachment (id=125673) [details] [details]
> Patch
> 
> Proposed patch v2.
------- Comment #10 From 2012-02-06 19:52:40 PST -------
(From update of attachment 125673 [details])
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
------- Comment #11 From 2012-02-07 11:23:17 PST -------
Created an attachment (id=125880) [details]
Patch

Fix nitpick.
------- Comment #12 From 2012-02-07 14:14:29 PST -------
(From update of attachment 125880 [details])
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
------- Comment #13 From 2012-02-07 14:17:54 PST -------
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.
------- Comment #14 From 2012-02-07 14:19:05 PST -------
<rdar://problem/10822885>
------- Comment #15 From 2012-02-08 12:02:03 PST -------
Created an attachment (id=126129) [details]
Patch

Updated patch. Add new tests.
------- Comment #16 From 2012-02-08 13:08:57 PST -------
(From update of attachment 126129 [details])
great!
------- Comment #17 From 2012-02-08 14:48:40 PST -------
(From update of attachment 126129 [details])
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 #18 From 2012-02-08 14:53:59 PST -------
(From update of attachment 126129 [details])
Trying again, this failing test does not looks related with this patch
------- Comment #19 From 2012-02-08 18:07:29 PST -------
(From update of attachment 126129 [details])
Clearing flags on attachment: 126129

Committed r107162: <http://trac.webkit.org/changeset/107162>
------- Comment #20 From 2012-02-08 18:07:35 PST -------
All reviewed patches have been landed.  Closing bug.