Bug 64591

Summary: Use requestAnimationFrame callbacks to pump CSS animations
Product: WebKit Reporter: Alex Russell <alex@dojotoolkit.org>
Component: CSSAssignee: Chris Marrin <cmarrin@apple.com>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth@webkit.org, cmarrin@apple.com, dglazkov@chromium.org, dino@apple.com, efidler@rim.com, jamesr@chromium.org, jchaffraix@webkit.org, jgw@chromium.org, mihnea@adobe.com, nduca@chromium.org, ojan@chromium.org, peter@chromium.org, simon.fraser@apple.com, tonikitoo@webkit.org, vangelis@chromium.org, webkit-bug-importer@group.apple.com, webkit.review.bot@gmail.com
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 67171    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Simple marquee using CSS animation on the "left" property
none
Simple marquee driven by rAF javascript animating the "left" style property
none
First pass at fixing this without breaking animation events
none
Fixed broken animation events
none
Removed old FIXME comment; fixed build by adding missing #if guard
none
Renamed a few things for clarity. Updated become-overlay-composited-layer test to not be flaky. Updated test expectations in anticipation of rebaselining.
none
Fixing pedantic presumbit check on (doubleValue == 0) expression.
none
Rebasing on top of expectation changes.
none
Fixing review-by line.
none
Removing non-chromium test-expectations changes. none

Description From 2011-07-15 03:55:46 PST
Ports which currently use the default AnimationController have their CSS animations pumped by a timer that runs every 25ms, meaning they top out at ~40fps. Going faster is problematic as we recalc style on every frame (potentially). ENABLE(REQUEST_ANIMATION_FRAME) provides an out.

Initial patch forthcoming.
------- Comment #1 From 2011-07-15 09:55:16 PST -------
Created an attachment (id=100998) [details]
Patch
------- Comment #2 From 2011-07-15 10:20:12 PST -------
(From update of attachment 100998 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=100998&action=review

> Source/WebCore/page/animation/AnimationController.h:69
> +    void pumpAnimations();

I don't really like "pump".  serviceAnimations()?

> Source/WebCore/page/animation/AnimationControllerPrivate.h:75
> +    void callbackFired();

This needs a more specific name than "callback".
------- Comment #3 From 2011-07-15 11:03:25 PST -------
State transition events (animationstart, animationend, animationiteration) still need to fire for non-visible tabs, even though requestAnimationFrame callbacks aren't invoked.
------- Comment #4 From 2011-07-15 13:00:10 PST -------
(From update of attachment 100998 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=100998&action=review

>> Source/WebCore/page/animation/AnimationControllerPrivate.h:75
>> +    void callbackFired();
> 
> This needs a more specific name than "callback".

scheduleAnimationsIfNecessary()? or something like that.
------- Comment #5 From 2011-07-16 04:42:36 PST -------
(From update of attachment 100998 [details])
Attachment 100998 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9093717

New failing tests:
animations/animation-drt-api.html
animations/animation-end-event-short-iterations.html
fast/selectors/unqualified-hover-strict.html
animations/play-state.html
animations/change-keyframes-name.html
animations/transition-and-animation-3.html
animations/animation-hit-test.html
animations/animation-drt-api-multiple-keyframes.html
animations/animation-end-event-destroy-renderer.html
animations/animation-start-event-destroy-renderer.html
animations/longhand-timing-function.html
fast/forms/input-spinbutton-capturing.html
fast/forms/input-number-large-padding.html
animations/animation-hit-test-transform.html
fast/forms/input-number-events.html
animations/unanimated-style.html
animations/suspend-transform-animation.html
fast/forms/implicit-submission.html
animations/state-at-end-event.html
------- Comment #6 From 2011-07-27 18:37:14 PST -------
(From update of attachment 100998 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=100998&action=review

R- for naming nits and because I don't think this handles state transition events (animationstart, animationend, animationiteration) correctly for background tabs.  I believe that they should all still fire at the expected times even when requestAnimationFrame isn't pumping at all.  To do this you might need to set some explicit Timers in addition to the requestAnimationFrame mechanism just for animation state transitions, and use requestAnimationFrame() just for intermediate ticks.

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

You'll need to edit or remove this line
------- Comment #7 From 2011-11-11 15:16:56 PST -------
(In reply to comment #3)
> State transition events (animationstart, animationend, animationiteration) still need to fire for non-visible tabs, even though requestAnimationFrame callbacks aren't invoked.

Right. For hardware animations we still use the Timer to fire these events. It's pretty efficient because the Timer only fires when needed rather than every frame. So we can use  that same mechanism and have the Timer fire the events, while rAF drives the actual animation.

(as you might be able to tell, I'm working on this now…)
------- Comment #8 From 2012-01-10 10:59:20 PST -------
<rdar://problem/10240395>
------- Comment #9 From 2012-01-10 11:11:53 PST -------
I've put this bug aside for now. I've gotten disappointing results, the animation is not smooth. I've tried printing lots of info during the animation, but it looks like we are often missing a cycle in rAF, possibly due to the expense of style resolution, etc. So I'm not sure it will be possible to use rAF to get smooth animation.

It might still be a good idea to do it. It would make it possible to run software animations a bit faster (60hz vs 40hz) and animations would automatically suspend using the rAF suspend logic. But we already suspend software animations using other mechanisms on some platforms, so I'm not sure doing this work is very compelling I would be happy to be proved wrong
------- Comment #10 From 2012-01-10 11:16:27 PST -------
A marquee example that makes the problem pretty obvious:
  http://www.page.ca/~wes/bugs/jerky-marquee/
------- Comment #11 From 2012-01-11 08:47:06 PST -------
(In reply to comment #10)
> A marquee example that makes the problem pretty obvious:
>   http://www.page.ca/~wes/bugs/jerky-marquee/

I see that the example is jerky. But do you think that's due to the current animation timer implementation? From what I have seen with the rAF based implementation the same jerkiness is there. That leads me to believe it might be dues to layout or style resolution and rAF wouldn't help that.
------- Comment #12 From 2012-01-11 08:55:33 PST -------
(In reply to comment #11)
> (In reply to comment #10)
> > A marquee example that makes the problem pretty obvious:
> >   http://www.page.ca/~wes/bugs/jerky-marquee/
> 
> I see that the example is jerky. But do you think that's due to the current animation timer implementation? From what I have seen with the rAF based implementation the same jerkiness is there. That leads me to believe it might be dues to layout or style resolution and rAF wouldn't help that.

My patch for this is not quite ready (it's still failing some tests because of some corner-cases starting animations under some circumstances). But so far this sample (as well as others I've been testing with) is greatly smoothed out.

The strategy is fairly simple (not all that different from Alex's July patch, though it covers extra situations, like making sure events still fire when rAF() doesn't, e.g., for background tabs). The timer is still used to fire all js events, but the rAF callback is used to pump visible changes. This means that events won't necessarily be fired *precisely* when the visual change occurs, but my understanding is that this is already the case when CoreAnimation is used in Safari. Making this more precise is possible, but rather trickier.

Note that it's still possible to have jerky animations if layouts triggered by each step take too long. There's probably little that can be done about this for animating properties that trigger layout, but it shouldn't be a problem for animation of accelerated properties (transform, opacity).
------- Comment #13 From 2012-01-11 09:02:19 PST -------
(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > A marquee example that makes the problem pretty obvious:
> > >   http://www.page.ca/~wes/bugs/jerky-marquee/
> > 
> > I see that the example is jerky. But do you think that's due to the current animation timer implementation? From what I have seen with the rAF based implementation the same jerkiness is there. That leads me to believe it might be dues to layout or style resolution and rAF wouldn't help that.
> 
> My patch for this is not quite ready (it's still failing some tests because of some corner-cases starting animations under some circumstances). But so far this sample (as well as others I've been testing with) is greatly smoothed out.
> 
> The strategy is fairly simple (not all that different from Alex's July patch, though it covers extra situations, like making sure events still fire when rAF() doesn't, e.g., for background tabs). The timer is still used to fire all js events, but the rAF callback is used to pump visible changes. This means that events won't necessarily be fired *precisely* when the visual change occurs, but my understanding is that this is already the case when CoreAnimation is used in Safari. Making this more precise is possible, but rather trickier.
> 
> Note that it's still possible to have jerky animations if layouts triggered by each step take too long. There's probably little that can be done about this for animating properties that trigger layout, but it shouldn't be a problem for animation of accelerated properties (transform, opacity).

I'll be very interested in seeing the patch. My results showed that even simple animations were jerky. It would be great if your implementation smoothed everything out!
------- Comment #14 From 2012-01-11 09:04:11 PST -------
(In reply to comment #13)
> I'll be very interested in seeing the patch. My results showed that even simple animations were jerky. It would be great if your implementation smoothed everything out!

Hopefully I haven't missed anything important. I guess we'll know soon :)

I'm trying to get something up for review this week, barring unforeseen difficulties fixing some tests.
------- Comment #15 From 2012-01-11 12:25:33 PST -------
Created an attachment (id=122071) [details]
Simple marquee using CSS animation on the "left" property
------- Comment #16 From 2012-01-11 12:26:07 PST -------
Created an attachment (id=122073) [details]
Simple marquee driven by rAF javascript animating the "left" style property
------- Comment #17 From 2012-01-11 12:29:49 PST -------
For me, the rAF+javascript driven animation looks significantly smoother in Chrome.  This is not too surprising - the first marquee runs at 40fps and the second runs at 60fps. In a WebKit nightly, rAF seems very buggy but when it's working correctly it looks a lot better.
------- Comment #18 From 2012-01-11 13:05:38 PST -------
(In reply to comment #17)
> In a WebKit nightly, rAF seems very buggy
How so?
------- Comment #19 From 2012-01-11 14:12:22 PST -------
(In reply to comment #17)
> For me, the rAF+javascript driven animation looks significantly smoother in Chrome.  This is not too surprising - the first marquee runs at 40fps and the second runs at 60fps. In a WebKit nightly, rAF seems very buggy but when it's working correctly it looks a lot better.

I agree that it is obviously running at a higher framerate. But I still see significant jerkiness to the animation. That's what I was seeing in my implementation. But I agree that jerky animation at 60Hz is better than jerky animation at 40hz!
------- Comment #20 From 2012-01-11 14:36:32 PST -------
(In reply to comment #18)
> (In reply to comment #17)
> > In a WebKit nightly, rAF seems very buggy
> How so?

With r104705 it would sometimes just get stuck and stop ticking sometimes (but not often). I couldn't find a reliable way to repro.  Additionally it doesn't look as smooth as Chrome's rAF implementation. It should be possible to quantify this exactly with careful instrumentation.
------- Comment #21 From 2012-01-13 11:55:36 PST -------
Created an attachment (id=122477) [details]
First pass at fixing this without breaking animation events
------- Comment #22 From 2012-01-13 12:01:43 PST -------
(From update of attachment 122477 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=122477&action=review

Not a full review.

> Source/WebCore/page/animation/AnimationController.cpp:208
> +    double needsService = updateAnimations(true);

needsService sounds like a boolean. Why is it a double?

> Source/WebCore/page/animation/AnimationController.cpp:226
>      updateAnimationTimer(true);

Mysterious boolean argument.

> Source/WebCore/page/animation/AnimationControllerPrivate.h:60
> +    double updateAnimations(bool callSetChanged);

What does the return value mean? Maybe the method should be renamed
------- Comment #23 From 2012-01-13 12:09:32 PST -------
(In reply to comment #21)
> Created an attachment (id=122477) [details] [details]
> First pass at fixing this without breaking animation events

I believe this patch is on the right track. It continues to use the timer for all the "logical" work (firing events, dispatching node changes triggered by AnimationBase's state machine), but uses the rAF callback to trigger visible changes via node->setNeedsStyleRecalc(). Visually it appears to make animations nice and smooth, with no tearing or jerking (on my Ubuntu desktop, at least). I've also compared it to simply cranking the animation timer up to 60fps, and you can still tell the difference (tearing artifacts).

It's not quite ready to commit, though, as it appears to be causing some flakiness in layout tests. The really strange thing I'm seeing is that, when I do any significant work (such as setting a couple of nodes' NeedsStyleRecalc()) in the rAF callback, setTimeout() becomes unreliable in test_shell -- e.g., a 500ms timer might fire anywhere from 500-2000ms later. Oddly, this doesn't seem to affect Chrome (just test_shell), and it doesn't seem to be pegging the CPU (it's doing about 50% more work, because of the increase from 40-60fps, but that's expected). There's something odd and subtle going on here -- if anyone has seen behavior like this before, please feel free to chime in.
------- Comment #24 From 2012-01-13 12:10:52 PST -------
(In reply to comment #22)
> (From update of attachment 122477 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122477&action=review
> 
> Not a full review.
> 
> > Source/WebCore/page/animation/AnimationController.cpp:208
> > +    double needsService = updateAnimations(true);
> 
> needsService sounds like a boolean. Why is it a double?
> 
> > Source/WebCore/page/animation/AnimationController.cpp:226
> >      updateAnimationTimer(true);
> 
> Mysterious boolean argument.
> 
> > Source/WebCore/page/animation/AnimationControllerPrivate.h:60
> > +    double updateAnimations(bool callSetChanged);
> 
> What does the return value mean? Maybe the method should be renamed

Thanks, Simon. Once I figure out what's going on with the flaky timers I'll come back and clean this up -- all of the above weirdness is a result of me trying to disturb as little as possible with my change.
------- Comment #25 From 2012-01-13 13:00:06 PST -------
(From update of attachment 122477 [details])
Attachment 122477 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11157720
------- Comment #26 From 2012-01-13 14:08:24 PST -------
(From update of attachment 122477 [details])
Attachment 122477 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11149732

New failing tests:
animations/animation-drt-api.html
animations/animation-end-event-short-iterations.html
animations/3d/change-transform-in-end-event.html
compositing/repaint/become-overlay-composited-layer.html
animations/3d/transform-perspective.html
animations/3d/matrix-transform-type-animation.html
animations/animation-direction.html
animations/3d/transform-origin-vs-functions.html
animations/animation-drt-api-multiple-keyframes.html
compositing/animation/state-at-end-event-transform-layer.html
animations/animation-end-event-destroy-renderer.html
compositing/animation/animation-compositing.html
animations/animation-add-events-in-handler.html
animations/additive-transform-animations.html
animations/animation-hit-test-transform.html
animations/3d/replace-filling-transform.html
animations/3d/state-at-end-event-transform.html
animations/animation-direction-normal.html
animations/animation-hit-test.html
compositing/reflections/nested-reflection-animated.html
------- Comment #27 From 2012-01-17 14:28:50 PST -------
Created an attachment (id=122813) [details]
Fixed broken animation events
------- Comment #28 From 2012-01-17 14:38:59 PST -------
(From update of attachment 122813 [details])
Attachment 122813 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11267356
------- Comment #29 From 2012-01-17 14:52:21 PST -------
Created an attachment (id=122817) [details]
Removed old FIXME comment; fixed build by adding missing #if guard
------- Comment #30 From 2012-01-17 17:48:53 PST -------
(From update of attachment 122817 [details])
Attachment 122817 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11117437

New failing tests:
compositing/repaint/become-overlay-composited-layer.html
------- Comment #31 From 2012-01-20 13:01:01 PST -------
Possibly related bug 76730.
------- Comment #32 From 2012-01-20 14:21:39 PST -------
@simon.fraser:

I'm trying to track down a single failing test:
  become-overlay-composited-layer.html

If I'm reading it correctly, it looks like this test subtly depends upon the update-rate of animations -- the opacity-animating #fader in its expected.png is actually at rgb(38,38,38), which is very close to what you'd expect if it had been animated one tick at 40f/s. Given that this patch changes the rate at which animations are re-evaluated, it now produces a slightly different image, but I'm not sure it's going to be 100% stable, since it depends upon when rAF() fires.

If I understand the purpose of the test correctly, it seems to be ensuring that there's not repaint garbage when the box/fader are moved into their own layers. So removing the { document.getElementById("container").className = ""; } line, while making the test more stable, would seem to no longer test what you intended. Does that make sense, and can you think of another way to do this without making it dependent upon the animation frame rate?
------- Comment #33 From 2012-01-27 12:08:55 PST -------
Just to be clear, I tested this in Safari and it has no noticeable impact (either on tests or, afaict, the behavior of animation in the browser). I expected as much, but thought it worthwhile to be sure.
------- Comment #34 From 2012-01-27 12:17:45 PST -------
(From update of attachment 122817 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=122817&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

this will fail an svn presubmit check - for a patch like this that changes the implementation of already-tested code but doesn't change behavior, then the normal thing is to describe the change here and list the test(s) that apply. For this I think listing the directories of the animation layout tests that exercise this code is probably the right thing to do.

> Source/WebCore/page/animation/AnimationController.cpp:207
> +    double needsService = updateAnimations(true);

needsService sounds like the name of a bool, and it used like a bool, so why is it a double?

> Source/WebCore/page/animation/AnimationControllerPrivate.h:61
> +    double updateAnimations(bool callSetChanged = false);

we typically prefer using two-state enums for this to make the callsites more readable. it's a lot easier to see what:

updateAnimations(CallSetChanged)

means than

updateAnimations(true)

without having to go find and read the header where updateAnimations() is defined.

I see that updateAnimationTimer() below doesn't follow good style here, but I think it's worth doing for new code.
------- Comment #35 From 2012-01-27 12:53:23 PST -------
(In reply to comment #34)
> (From update of attachment 122817 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122817&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests. (OOPS!)
> 
> this will fail an svn presubmit check - for a patch like this that changes the implementation of already-tested code but doesn't change behavior, then the normal thing is to describe the change here and list the test(s) that apply. For this I think listing the directories of the animation layout tests that exercise this code is probably the right thing to do.

Thanks, done.

> > Source/WebCore/page/animation/AnimationController.cpp:207
> > +    double needsService = updateAnimations(true);
> 
> needsService sounds like the name of a bool, and it used like a bool, so why is it a double?

Yeah, that was the original obscure name. I've renamed it to 'timeToNextService' which is significantly more accurate.

> > Source/WebCore/page/animation/AnimationControllerPrivate.h:61
> > +    double updateAnimations(bool callSetChanged = false);
> 
> we typically prefer using two-state enums for this to make the callsites more readable. it's a lot easier to see what:
> 
> updateAnimations(CallSetChanged)
> 
> means than
> 
> updateAnimations(true)
> 
> without having to go find and read the header where updateAnimations() is defined.
> 
> I see that updateAnimationTimer() below doesn't follow good style here, but I think it's worth doing for new code.

Ok, done. Hard to come up with a reasonable name for this kind of thing, but the call sites are clearer now.


I haven't uploaded a new patch yet, because I still have to chase down that failing test. I'll try to bug Simon again on Monday to see if there's another way to write that test such that it's not so dependent upon precisely when the animation is updated. In the meantime, if you have any other ideas, please feel free to chime in.
------- Comment #36 From 2012-01-27 12:56:28 PST -------
> > > Source/WebCore/page/animation/AnimationController.cpp:207
> > > +    double needsService = updateAnimations(true);
> > 
> > needsService sounds like the name of a bool, and it used like a bool, so why is it a double?
> 
> Yeah, that was the original obscure name. I've renamed it to 'timeToNextService' which is significantly more accurate.

That name doesn't very well communicate what the method does though; it sounds like a simple getter, but the method does more than that.

> I haven't uploaded a new patch yet, because I still have to chase down that failing test. I'll try to bug Simon again on Monday to see if there's another way to write that test such that it's not so dependent upon precisely when the animation is updated. In the meantime, if you have any other ideas, please feel free to chime in.

I don't have any good ideas here, alas.
------- Comment #37 From 2012-01-27 13:05:32 PST -------
(In reply to comment #36)
> > > > Source/WebCore/page/animation/AnimationController.cpp:207
> > > > +    double needsService = updateAnimations(true);
> > > 
> > > needsService sounds like the name of a bool, and it used like a bool, so why is it a double?
> > 
> > Yeah, that was the original obscure name. I've renamed it to 'timeToNextService' which is significantly more accurate.
> 
> That name doesn't very well communicate what the method does though; it sounds like a simple getter, but the method does more than that.

I'm a little confused here -- we're talking about the local 'needsService' (which feels like a boolean, but isn't) vs. 'timeToNextService', right? I think this name's pretty clear -- it contains the time until the next animation needs to be serviced, or a negative value if none are upcoming. Am I completely misunderstanding your point?

> > I haven't uploaded a new patch yet, because I still have to chase down that failing test. I'll try to bug Simon again on Monday to see if there's another way to write that test such that it's not so dependent upon precisely when the animation is updated. In the meantime, if you have any other ideas, please feel free to chime in.
> 
> I don't have any good ideas here, alas.

No worries, I'll see what I can come up with. As I understand it, the salient issue is that the elements being animated switch from and to being backed by composited layers, and that that leave no garbage behind.

One thought: Might I be able to get a similar effect by just applying a 3D -webkit-transform to these elements? Not quite the same, but might be close enough.
------- Comment #38 From 2012-01-28 01:11:21 PST -------
(In reply to comment #37)
> I'm a little confused here -- we're talking about the local 'needsService' (which feels like a boolean, but isn't) vs. 'timeToNextService', right? I think this name's pretty clear -- it contains the time until the next animation needs to be serviced, or a negative value if none are upcoming. Am I completely misunderstanding your point?

Sorry, I was working from memory here. Now I realize this is local variable. The name is fine.
------- Comment #39 From 2012-01-31 13:20:36 PST -------
Created an attachment (id=124808) [details]
Renamed a few things for clarity. Updated become-overlay-composited-layer test to not be flaky. Updated test expectations in anticipation of rebaselining.
------- Comment #40 From 2012-01-31 13:22:45 PST -------
Attachment 124808 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/compositing/repaint/become-ove..." exit_code: 1

Source/WebCore/page/animation/AnimationController.cpp:99:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/page/animation/AnimationController.cpp:123:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 48, in <module>
    sys.exit(CheckWebKitStyle().main())
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/main.py", line 154, in main
    patch_checker.check(patch)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/patchreader.py", line 66, in check
    self._text_file_reader.process_file(file_path=path, line_numbers=line_numbers)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filereader.py", line 130, in process_file
    self._processor.process(lines, file_path, **kwargs)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 826, in process
    checker.check(lines)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/test_expectations.py", line 119, in check
    overrides=overrides)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/test_expectations.py", line 96, in check_test_expectations
    is_lint_mode=True, overrides=overrides)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py", line 726, in __init__
    self._add_skipped_tests(port.skipped_tests(tests))
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 376, in skipped_tests
    return self.skipped_layout_tests(test_list)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 372, in skipped_layout_tests
    tests_to_skip.update(self._skipped_tests_for_unsupported_features(test_list))
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 319, in _skipped_tests_for_unsupported_features
    if self._has_test_in_directories(self._missing_feature_to_skipped_tests().values(), test_list):
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 311, in _has_test_in_directories
    for directory, test in itertools.product(directories, test_list):
TypeError: 'NoneType' object is not iterable


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #41 From 2012-01-31 13:27:18 PST -------
Finally got around to updating this bug. I *believe* my change to the broken test should still cover the case it's intended to, but in a less flaky way (it references a rdar: bug, so it's a bit hard to be sure). This requires a change to the expectations -- I've added the chromium-linux expectations to the patch, and marked it PASS FAIL to avoid breaking builds.
------- Comment #42 From 2012-01-31 13:30:47 PST -------
Created an attachment (id=124809) [details]
Fixing pedantic presumbit check on (doubleValue == 0) expression.
------- Comment #43 From 2012-01-31 13:35:42 PST -------
Attachment 124809 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/compositing/repaint/become-ove..." exit_code: 1

Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 48, in <module>
    sys.exit(CheckWebKitStyle().main())
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/main.py", line 154, in main
    patch_checker.check(patch)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/patchreader.py", line 66, in check
    self._text_file_reader.process_file(file_path=path, line_numbers=line_numbers)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filereader.py", line 130, in process_file
    self._processor.process(lines, file_path, **kwargs)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 826, in process
    checker.check(lines)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/test_expectations.py", line 119, in check
    overrides=overrides)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/test_expectations.py", line 96, in check_test_expectations
    is_lint_mode=True, overrides=overrides)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py", line 726, in __init__
    self._add_skipped_tests(port.skipped_tests(tests))
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 376, in skipped_tests
    return self.skipped_layout_tests(test_list)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 372, in skipped_layout_tests
    tests_to_skip.update(self._skipped_tests_for_unsupported_features(test_list))
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 319, in _skipped_tests_for_unsupported_features
    if self._has_test_in_directories(self._missing_feature_to_skipped_tests().values(), test_list):
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/webkit.py", line 311, in _has_test_in_directories
    for directory, test in itertools.product(directories, test_list):
TypeError: 'NoneType' object is not iterable


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #44 From 2012-02-09 11:39:02 PST -------
(From update of attachment 124809 [details])
R=me this looks great.

To Simon's earlier concern, if it turns out that animations don't look better on some platforms's implementations of rAF then I think the way to address that is to add an ENABLE to control whether software-driven animations use rAF or a 40Hz timer.  I feel that we've done sufficient experimentation in chromium to know that we definitely prefer the rAF mechanism to a 40Hz timer.
------- Comment #45 From 2012-02-09 18:02:22 PST -------
(From update of attachment 124809 [details])
This patch broke the commit-queue.
------- Comment #46 From 2012-02-09 18:03:17 PST -------
I'm 100% sure what the problem is, but you're using the test_expecations.txt file for a bunch of ports that normally use Skipped lists (e.g., windows).
------- Comment #47 From 2012-02-09 18:37:43 PST -------
(In reply to comment #46)
> I'm 100% sure what the problem is, but you're using the test_expecations.txt file for a bunch of ports that normally use Skipped lists (e.g., windows).

Probably hitting bug 78260 (basically changing win test_expectations.txt is broken).
------- Comment #48 From 2012-02-10 11:15:55 PST -------
Created an attachment (id=126541) [details]
Rebasing on top of expectation changes.
------- Comment #49 From 2012-02-10 11:19:21 PST -------
Attachment 126541 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/compositing/repaint/become-ove..." exit_code: 1

Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 48, in <module>
    sys.exit(CheckWebKitStyle().main())
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/main.py", line 154, in main
    patch_checker.check(patch)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/patchreader.py", line 66, in check
    self._text_file_reader.process_file(file_path=path, line_numbers=line_numbers)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filereader.py", line 130, in process_file
    self._processor.process(lines, file_path, **kwargs)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 833, in process
    checker.check(lines)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/test_expectations.py", line 103, in check
    overrides = self._port_obj.test_expectations_overrides()
AttributeError: 'NoneType' object has no attribute 'test_expectations_overrides'


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #50 From 2012-02-10 12:15:26 PST -------
Created an attachment (id=126549) [details]
Fixing review-by line.
------- Comment #51 From 2012-02-10 12:19:19 PST -------
Attachment 126549 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/compositing/repaint/become-ove..." exit_code: 1

Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 48, in <module>
    sys.exit(CheckWebKitStyle().main())
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/main.py", line 154, in main
    patch_checker.check(patch)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/patchreader.py", line 66, in check
    self._text_file_reader.process_file(file_path=path, line_numbers=line_numbers)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filereader.py", line 130, in process_file
    self._processor.process(lines, file_path, **kwargs)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 833, in process
    checker.check(lines)
  File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/test_expectations.py", line 103, in check
    overrides = self._port_obj.test_expectations_overrides()
AttributeError: 'NoneType' object has no attribute 'test_expectations_overrides'


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #52 From 2012-02-10 12:32:06 PST -------
I think that editing test_expectations.txt for non-chromium ports is not gonna work, they just don't really work :/
------- Comment #53 From 2012-02-10 12:41:32 PST -------
Created an attachment (id=126559) [details]
Removing non-chromium test-expectations changes.
------- Comment #54 From 2012-02-13 08:39:53 PST -------
(From update of attachment 126559 [details])
Clearing flags on attachment: 126559

Committed r107575: <http://trac.webkit.org/changeset/107575>
------- Comment #55 From 2012-02-13 08:40:04 PST -------
All reviewed patches have been landed.  Closing bug.