|
Description
Alex Russell
2011-07-15 03:55:46 PDT
Created attachment 100998 [details]
Patch
Comment on attachment 100998 [details] Patch 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". State transition events (animationstart, animationend, animationiteration) still need to fire for non-visible tabs, even though requestAnimationFrame callbacks aren't invoked. Comment on attachment 100998 [details] Patch 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 on attachment 100998 [details] Patch 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 on attachment 100998 [details] Patch 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 (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…) 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 A marquee example that makes the problem pretty obvious: http://www.page.ca/~wes/bugs/jerky-marquee/ (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. (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). (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! (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. Created attachment 122071 [details]
Simple marquee using CSS animation on the "left" property
Created attachment 122073 [details]
Simple marquee driven by rAF javascript animating the "left" style property
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. (In reply to comment #17) > In a WebKit nightly, rAF seems very buggy How so? (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! (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. Created attachment 122477 [details]
First pass at fixing this without breaking animation events
Comment on attachment 122477 [details] First pass at fixing this without breaking animation events 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 (In reply to comment #21) > Created an attachment (id=122477) [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. (In reply to comment #22) > (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 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 on attachment 122477 [details] First pass at fixing this without breaking animation events Attachment 122477 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11157720 Comment on attachment 122477 [details] First pass at fixing this without breaking animation events 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 Created attachment 122813 [details]
Fixed broken animation events
Comment on attachment 122813 [details] Fixed broken animation events Attachment 122813 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11267356 Created attachment 122817 [details]
Removed old FIXME comment; fixed build by adding missing #if guard
Comment on attachment 122817 [details] Removed old FIXME comment; fixed build by adding missing #if guard 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 Possibly related bug 76730. @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?
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 on attachment 122817 [details] Removed old FIXME comment; fixed build by adding missing #if guard 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. (In reply to comment #34) > (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. 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. > > > 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. (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. (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. Created attachment 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.
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.
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. Created attachment 124809 [details]
Fixing pedantic presumbit check on (doubleValue == 0) expression.
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 on attachment 124809 [details]
Fixing pedantic presumbit check on (doubleValue == 0) expression.
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 on attachment 124809 [details]
Fixing pedantic presumbit check on (doubleValue == 0) expression.
This patch broke the commit-queue.
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). (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). Created attachment 126541 [details]
Rebasing on top of expectation changes.
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.
Created attachment 126549 [details]
Fixing review-by line.
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.
I think that editing test_expectations.txt for non-chromium ports is not gonna work, they just don't really work :/ Created attachment 126559 [details]
Removing non-chromium test-expectations changes.
Comment on attachment 126559 [details] Removing non-chromium test-expectations changes. Clearing flags on attachment: 126559 Committed r107575: <http://trac.webkit.org/changeset/107575> All reviewed patches have been landed. Closing bug. This was rolled out in http://trac.webkit.org/r110262 (In reply to comment #56) > This was rolled out in http://trac.webkit.org/r110262 This patch was not completely rolled out. Almost all the changes do still exist in the code as there were submitted expect the change in the FrameView.cpp. http://trac.webkit.org/r110262 rolled out https://trac.webkit.org/changeset/108616 which was a fix for https://bugs.webkit.org/show_bug.cgi?id=38025. The rolling change of this bug is https://trac.webkit.org/changeset/110889 which rolls out the change in FrameView.cpp only. Unfortunately the ChangeLogs of https://trac.webkit.org/changeset/107575 and https://trac.webkit.org/changeset/110889 did not say a single word explaining what these patches were doing. (In reply to comment #57) > (In reply to comment #56) > > This was rolled out in http://trac.webkit.org/r110262 > > This patch was not completely rolled out. Almost all the changes do still > exist in the code as there were submitted expect the change in the > FrameView.cpp. > > http://trac.webkit.org/r110262 rolled out > https://trac.webkit.org/changeset/108616 which was a fix for > https://bugs.webkit.org/show_bug.cgi?id=38025. > > The rolling change of this bug is https://trac.webkit.org/changeset/110889 > which rolls out the change in FrameView.cpp only. > > Unfortunately the ChangeLogs of https://trac.webkit.org/changeset/107575 and > https://trac.webkit.org/changeset/110889 did not say a single word > explaining what these patches were doing. I was wrong about the rolling part in FrameView.cpp. This change was not rolled out either. So closing this bug as fixed although I do not think the patch did what the title is saying. |