Bug 60525 - Implement reverse animation direction
: Implement reverse animation direction
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Dean Jackson
: InRadar
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-09 18:22 PDT by Dean Jackson
Modified: 2012-08-29 05:33 PDT (History)
11 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Dean Jackson 2011-05-09 18:23:51 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.
Comment 2 L. David Baron 2011-05-09 18:38:32 PDT
Sounds fine to me.
Comment 3 Igor Trindade Oliveira 2012-02-05 13:47:02 PST
Created attachment 125534 [details]
Patch

Proposed patch.
Comment 4 Early Warning System Bot 2012-02-05 13:58:46 PST
Comment on attachment 125534 [details]
Patch

Attachment 125534 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11428136
Comment 5 Noam Rosenthal 2012-02-05 14:10:13 PST
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 6 WebKit Review Bot 2012-02-05 14:16:11 PST
Comment on attachment 125534 [details]
Patch

Attachment 125534 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11428139
Comment 7 Simon Fraser (smfr) 2012-02-06 00:17:24 PST
Comment on attachment 125534 [details]
Patch

r- based on Noam's comments.
Comment 8 Igor Trindade Oliveira 2012-02-06 11:19:00 PST
Created attachment 125673 [details]
Patch

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

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

Fix nitpick.
Comment 12 Dean Jackson 2012-02-07 14:14:29 PST
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
Comment 13 Dean Jackson 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 Radar WebKit Bug Importer 2012-02-07 14:19:05 PST
<rdar://problem/10822885>
Comment 15 Igor Trindade Oliveira 2012-02-08 12:02:03 PST
Created attachment 126129 [details]
Patch

Updated patch. Add new tests.
Comment 16 Dean Jackson 2012-02-08 13:08:57 PST
Comment on attachment 126129 [details]
Patch

great!
Comment 17 WebKit Review Bot 2012-02-08 14:48:40 PST
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 18 Igor Trindade Oliveira 2012-02-08 14:53:59 PST
Comment on attachment 126129 [details]
Patch

Trying again, this failing test does not looks related with this patch
Comment 19 WebKit Review Bot 2012-02-08 18:07:29 PST
Comment on attachment 126129 [details]
Patch

Clearing flags on attachment: 126129

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