[chromium] Zoom animator front-end [work-in-progress, not for review yet ...]
Created attachment 107241 [details] Patch
Comment on attachment 107241 [details] Patch Attachment 107241 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9657331
Comment on attachment 107241 [details] Patch Attachment 107241 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9655371 New failing tests: fast/events/wheelevent-in-horizontal-scrollbar-in-rtl.html fast/events/continuous-platform-wheelevent-in-scrolling-div.html fast/events/wheelevent-in-vertical-scrollbar-in-rtl.html fast/events/remove-child-onscroll.html fast/events/platform-wheelevent-in-scrolling-div.html fast/events/scroll-in-scaled-page-with-overflow-hidden.html
Comment on attachment 107241 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107241&action=review > Source/WebCore/platform/ScrollAnimatorNone.cpp:58 > +#if ENABLE(GESTURE_EVENTS) Is this a good idea? In particular what happens when we have gestures and non-accelerated pages? > Source/WebCore/platform/ScrollableArea.cpp:39 > +#if ENABLE(GESTURE_EVENTS) I'm not sure we want to to this either. > Source/WebKit/chromium/src/WebViewImpl.cpp:585 > +// PlatformWheelEventBuilder platformEvent(mainFrameImpl()->frameView(), event); Is this is test code? I think it ought not to get committed right? Because doubletap via gesture recognition has landed I think.
(In reply to comment #4) > (From update of attachment 107241 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=107241&action=review > > > Source/WebCore/platform/ScrollAnimatorNone.cpp:58 > > +#if ENABLE(GESTURE_EVENTS) > > Is this a good idea? In particular what happens when we have gestures and non-accelerated pages? I think I assumed all pages will be accelerated if we want zoom animation to work, but perhaps I'm wrong. > > Source/WebCore/platform/ScrollableArea.cpp:39 > > +#if ENABLE(GESTURE_EVENTS) > > I'm not sure we want to to this either. For sure ... I waffled as to whether we should include one header or both ... we can change this. > > Source/WebKit/chromium/src/WebViewImpl.cpp:585 > > +// PlatformWheelEventBuilder platformEvent(mainFrameImpl()->frameView(), event); > > Is this is test code? I think it ought not to get committed right? Because doubletap via gesture recognition has landed I think. This is just test code ... I left it in since this is a early draft of the patch, but I should have warned people about it. I use this to generate double tap events on my desktop (since I don't know how else to get them).
Created attachment 107333 [details] Patch
(In reply to comment #6) > Created an attachment (id=107333) [details] > Patch Removed GESTURE_EVENT specialisation in ScrollAnimator::Create() - you need to invoke with --enable-smooth-scrolling for the zoom animator to work. Wheel test code still in this version, so do not commit.
Comment on attachment 107333 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107333&action=review and you'll probably want me to have finished https://bugs.webkit.org/show_bug.cgi?id=66272 so you can write the layout tests. :-) Looks reasonable to me so far. > Source/WebCore/page/EventHandler.cpp:2233 > + case PlatformGestureEvent::DoubleTapType: this will need adjustment to align with https://bugs.webkit.org/show_bug.cgi?id=67822
Comment on attachment 107333 [details] Patch Attachment 107333 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9660606
Created attachment 107359 [details] Patch
(In reply to comment #10) > Created an attachment (id=107359) [details] > Patch Same patch, updated to ToT for fsamuel.
Comment on attachment 107359 [details] Patch Attachment 107359 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9660837
Comment on attachment 107359 [details] Patch Attachment 107359 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9658881 New failing tests: fast/events/wheelevent-in-horizontal-scrollbar-in-rtl.html fast/events/continuous-platform-wheelevent-in-scrolling-div.html fast/events/wheelevent-in-vertical-scrollbar-in-rtl.html fast/events/remove-child-onscroll.html fast/events/platform-wheelevent-in-scrolling-div.html fast/events/scroll-in-scaled-page-with-overflow-hidden.html
Created attachment 107522 [details] Patch
Comment on attachment 107522 [details] Patch Attachment 107522 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9705092 New failing tests: fast/events/wheelevent-in-horizontal-scrollbar-in-rtl.html fast/events/continuous-platform-wheelevent-in-scrolling-div.html fast/events/wheelevent-in-vertical-scrollbar-in-rtl.html fast/events/remove-child-onscroll.html fast/events/platform-wheelevent-in-scrolling-div.html fast/events/scroll-in-scaled-page-with-overflow-hidden.html
Comment on attachment 107522 [details] Patch Attachment 107522 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9705467
Created attachment 109176 [details] Patch
Comment on attachment 109176 [details] Patch Attachment 109176 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9893380
Comment on attachment 109176 [details] Patch Attachment 109176 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9897004
Created attachment 109201 [details] Patch
Comment on attachment 109201 [details] Patch Attachment 109201 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9899218
Comment on attachment 109201 [details] Patch Attachment 109201 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9893492
Created attachment 109232 [details] Patch
Comment on attachment 109232 [details] Patch Attachment 109232 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9888721
Created attachment 109291 [details] Patch
Comment on attachment 109291 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109291&action=review looks good to me. The testing stuff is cool. I want to learn more about that. :-) > Source/WebCore/page/FrameView.cpp:1223 > +void FrameView::zoomAnimatorTransformChanged(double scale, double x, double y, bool isFinished) if accelerated compositing is turned off, what happens to the layout tests? > Source/WebCore/platform/ScrollAnimatorNone.cpp:512 > + // TODO: add any other event types we should handle We had decided that any other gesture event should halt the zoom animation. This would be a different CL? Blocked on me?
(In reply to comment #26) > (From update of attachment 109291 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=109291&action=review > > looks good to me. The testing stuff is cool. I want to learn more about that. :-) > > > Source/WebCore/page/FrameView.cpp:1223 > > +void FrameView::zoomAnimatorTransformChanged(double scale, double x, double y, bool isFinished) > > if accelerated compositing is turned off, what happens to the layout tests? > > > Source/WebCore/platform/ScrollAnimatorNone.cpp:512 > > + // TODO: add any other event types we should handle > > We had decided that any other gesture event should halt the zoom animation. This would be a different CL? Blocked on me? Yes, a different CL, blocked not so much on you as on not wanting to make this patch any bigger than it already is :-)
Comment on attachment 109291 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109291&action=review > Source/WebCore/platform/ScrollAnimator.cpp:152 > +void ScrollAnimator::testZoomParameters(float scale, float x, float y) Seems an odd name for this routine. setZoomParameters instead? > Source/WebCore/platform/ScrollAnimatorNone.cpp:554 > + notifyZoomChanged(true); This looks like it will always call notifyZoomChanged while a scroll animation is in effect, even if no zoom is in flight.
(In reply to comment #28) > (From update of attachment 109291 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=109291&action=review > > > Source/WebCore/platform/ScrollAnimator.cpp:152 > > +void ScrollAnimator::testZoomParameters(float scale, float x, float y) > > Seems an odd name for this routine. setZoomParameters instead? > > > Source/WebCore/platform/ScrollAnimatorNone.cpp:554 > > + notifyZoomChanged(true); > > This looks like it will always call notifyZoomChanged while a scroll animation is in effect, even if no zoom is in flight. Yes, although that's something we'll change before this lands ... no sense in calling the scheduleLayerFlush() when not zooming. We will also need a follow-on patch to deal with interrupting ZAC if a scroll request comes in.
Created attachment 109482 [details] Patch
(In reply to comment #29) > > > > This looks like it will always call notifyZoomChanged while a scroll animation is in effect, even if no zoom is in flight. > I've added a check to prevent this.
(In reply to comment #31) > (In reply to comment #29) > > > > > > This looks like it will always call notifyZoomChanged while a scroll animation is in effect, even if no zoom is in flight. > > > > I've added a check to prevent this. BTW, I had a alternate idea for preventing the unnecessary calls to notifyZoomChanged(), namely only call when ScrollAnimator::m_currentZoomScale != 1. Since the final frame of the animation triggers a reset of this value to 1 this idea should also work, but I was worried it might be too confusing for others who read the code. But, maybe it's the more elegant solution?
Comment on attachment 109482 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=109482&action=review The overall structure looks OK to me but there are a few things that should be cleaned up. Please also delete the comments in the ChangeLog about its not being for review yet. > LayoutTests/platform/chromium/compositing/zoom-animator-scale-test2.html:29 > + // Check every 100 mS for animation completion. This comment is out of sync with the value being passed to setInterval. > LayoutTests/platform/chromium/compositing/zoom-animator-scale-test2.html:30 > + timer = setInterval("finishTest()", 250); Shouldn't you add some provision for the test completing within a certain period of time? > Source/WebCore/page/FrameView.h:173 > + virtual void zoomAnimatorTransformChanged(double, double, double, bool); In general WebKit style is to use enums instead of bool arguments so the call site is more self-explanatory. If you can find a good place to define one for this, that would be good. > Source/WebCore/platform/ScrollAnimator.h:93 > + virtual void testZoomParameters(float, float, float); This method name is unclear. It sounds like it is supposed to test the incoming values and return a bool. I think you should rename it to setZoomParametersForTesting if that's what it's for, or just call it setZoomParameters. > Source/WebCore/platform/ScrollAnimatorNone.cpp:370 > +ScrollAnimatorNone::ZoomData::ZoomData(WebCore::ScrollAnimatorNone* parent, float* scale, float* transX, float* transY) Passing these pointers is pretty gross. Since you're already passing the parent ScrollAnimatorNone (and ignoring it right now, which is poor), why not just make ZoomData a friend and set its fields directly? > Source/WebCore/platform/ScrollAnimatorNone.cpp:390 > + *m_currentScale = deltaTime / m_animationTime * (m_desiredScale - m_startScale) + m_startScale; Compute deltaTime / m_animationTime into a local variable for clarity. > Source/WebCore/platform/ScrollAnimatorNone.cpp:469 > + // FIXME(wjmaclean): modify this so we can start even if the timer is active. WebKit style doesn't put user names into FIXMEs. Here and below.
(In reply to comment #33) > (From update of attachment 109482 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=109482&action=review > > The overall structure looks OK to me but there are a few things that should be cleaned up. Please also delete the comments in the ChangeLog about its not being for review yet. Done. > > LayoutTests/platform/chromium/compositing/zoom-animator-scale-test2.html:29 > > + // Check every 100 mS for animation completion. > > This comment is out of sync with the value being passed to setInterval. > > > LayoutTests/platform/chromium/compositing/zoom-animator-scale-test2.html:30 > > + timer = setInterval("finishTest()", 250); > > Shouldn't you add some provision for the test completing within a certain period of time? I was figuring the test timeout would catch that, but have added a maximum number of intervals. Could this possibly lead to flake, say if a test bot runs slow enough sometimes to hit the max, but not everytime? > > Source/WebCore/page/FrameView.h:173 > > + virtual void zoomAnimatorTransformChanged(double, double, double, bool); > > In general WebKit style is to use enums instead of bool arguments so the call site is more self-explanatory. If you can find a good place to define one for this, that would be good. OK. There are two levels of calls here, so I hope two enums are ok (otherwise we need to include ScrollableArea.h in ScrollAnimator.h instead of just using a forward declaration for ScrollableArea.) > > Source/WebCore/platform/ScrollAnimator.h:93 > > + virtual void testZoomParameters(float, float, float); > > This method name is unclear. It sounds like it is supposed to test the incoming values and return a bool. I think you should rename it to setZoomParametersForTesting if that's what it's for, or just call it setZoomParameters. Fixed. > > Source/WebCore/platform/ScrollAnimatorNone.cpp:370 > > +ScrollAnimatorNone::ZoomData::ZoomData(WebCore::ScrollAnimatorNone* parent, float* scale, float* transX, float* transY) > > Passing these pointers is pretty gross. Since you're already passing the parent ScrollAnimatorNone (and ignoring it right now, which is poor), why not just make ZoomData a friend and set its fields directly? Fixed. Nice suggestion, thanks! > > Source/WebCore/platform/ScrollAnimatorNone.cpp:390 > > + *m_currentScale = deltaTime / m_animationTime * (m_desiredScale - m_startScale) + m_startScale; > > Compute deltaTime / m_animationTime into a local variable for clarity. Fixed. > > Source/WebCore/platform/ScrollAnimatorNone.cpp:469 > > + // FIXME(wjmaclean): modify this so we can start even if the timer is active. > > WebKit style doesn't put user names into FIXMEs. Here and below. Fixed.
Created attachment 110008 [details] Patch
(In reply to comment #35) > Created an attachment (id=110008) [details] > Patch Also adds some TRACE_EVENTS calls so we can verify latency on zoom animator events.
Comment on attachment 110008 [details] Patch Attachment 110008 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9968342
Comment on attachment 110008 [details] Patch Attachment 110008 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9978323
Created attachment 110016 [details] Patch
(In reply to comment #39) > Created an attachment (id=110016) [details] > Patch Fixed QT compilation issue, re-baselined new test expectation.
Comment on attachment 110016 [details] Patch Attachment 110016 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9979314
Comment on attachment 110016 [details] Patch Attachment 110016 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9968359
Comment on attachment 110016 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110016&action=review Thanks, looks better but a couple of remaining issues. > LayoutTests/platform/chromium/compositing/zoom-animator-scale-test2.html:27 > + clearInterval(timer); The layoutTestController won't be notified if this happens. There should be a clear indication that the test failed in this case. Please test by manually injecting a failure. > LayoutTests/platform/chromium/test_expectations.txt:2666 > +BUGWKxxxxx MAC WIN GPU GPU-CG : platform/chromium/compositing/zoom-animator-scale-test2.html = IMAGE Could you go ahead and file the follow-on bug so you can reference it here? (Why doesn't this test work on the other platforms?) > Source/WebCore/page/FrameView.cpp:70 > +#include "TraceEvent.h" This is failing the bots. If you're putting it here it needs #if PLATFORM(CHROMIUM). > Source/WebCore/page/FrameView.cpp:1236 > + TRACE_EVENT("FrameView::zoomAnimatorTransformChanged", this, 0); PLATFORM(CHROMIUM) needed here.
(In reply to comment #34) > I was figuring the test timeout would catch that, but have added a maximum number of intervals. Could this possibly lead to flake, say if a test bot runs slow enough sometimes to hit the max, but not everytime? Yes, it could be flaky. Given the nature of the test, I'm not sure how to fix it, though. You might ask on IRC. > OK. There are two levels of calls here, so I hope two enums are ok (otherwise we need to include ScrollableArea.h in ScrollAnimator.h instead of just using a forward declaration for ScrollableArea.) It's OK to me though it might be cleaner to just split it into its own header.
Created attachment 110142 [details] Patch
(In reply to comment #43) > (From update of attachment 110016 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=110016&action=review > > Thanks, looks better but a couple of remaining issues. > > > LayoutTests/platform/chromium/compositing/zoom-animator-scale-test2.html:27 > > + clearInterval(timer); > > The layoutTestController won't be notified if this happens. There should be a clear indication that the test failed in this case. Please test by manually injecting a failure. Done. I looked to see if there was some way in js to directly tell layoutTestController the test had failed, but couldn't find one, so I modified the test to turn the div red again and print "Fail!" in the div. > > LayoutTests/platform/chromium/test_expectations.txt:2666 > > +BUGWKxxxxx MAC WIN GPU GPU-CG : platform/chromium/compositing/zoom-animator-scale-test2.html = IMAGE > > Could you go ahead and file the follow-on bug so you can reference it here? Done. > (Why doesn't this test work on the other platforms?) It does, but I don't have easy access to all these dev platforms, so I plan on getting the new baselines off the bots. The line is test expectations is to give a heads up to anyone gardening, but I forgot to create the issue in advance and reference it accordingly. > > Source/WebCore/page/FrameView.cpp:70 > > +#include "TraceEvent.h" > > This is failing the bots. If you're putting it here it needs #if PLATFORM(CHROMIUM). Ahh, OK. Wasn't sure why it was still unhappy (although as yet hadn't had a chance to look further). > > Source/WebCore/page/FrameView.cpp:1236 > > + TRACE_EVENT("FrameView::zoomAnimatorTransformChanged", this, 0); > > PLATFORM(CHROMIUM) needed here.
(In reply to comment #44) > (In reply to comment #34) > > I was figuring the test timeout would catch that, but have added a maximum number of intervals. Could this possibly lead to flake, say if a test bot runs slow enough sometimes to hit the max, but not everytime? > > Yes, it could be flaky. Given the nature of the test, I'm not sure how to fix it, though. You might ask on IRC. OK, I'll follow up on this, and revise in a subsequent CL if a better solution can be found. > > OK. There are two levels of calls here, so I hope two enums are ok (otherwise we need to include ScrollableArea.h in ScrollAnimator.h instead of just using a forward declaration for ScrollableArea.) > > It's OK to me though it might be cleaner to just split it into its own header.
Comment on attachment 110142 [details] Patch Looks good to me. r=me
Comment on attachment 110142 [details] Patch Clearing flags on attachment: 110142 Committed r97034: <http://trac.webkit.org/changeset/97034>
All reviewed patches have been landed. Closing bug.
Comment on attachment 110142 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110142&action=review > Source/WebCore/testing/Internals.cpp:260 > +void Internals::setZoomAnimatorTransform(Document *document, float scale, float tx, float ty, ExceptionCode& ec) This breaks my build because scale, tx and ty are not used if GESTURE_EVENTS is disabled. > Source/WebCore/testing/Internals.cpp:283 > +void Internals::setZoomParameters(Document* document, float scale, float x, float y, ExceptionCode& ec) probably same problem here when SMOOTH_SCROLLING disabled.
I've posted a fix here: https://bugs.webkit.org/show_bug.cgi?id=69735 seems not many reviewers are online tonight though.
This patch broke the Leopard, Snow Leopard, and Lion builds: Leopard Intel Debug: <http://build.webkit.org/builders/Leopard%20Intel%20Debug%20%28Build%29/builds/40399/steps/compile-webkit/logs/stdio> Snow Leopard Intel Release: <http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Build%29/builds/34595/steps/compile-webkit/logs/stdio> Snow Leopard Intel Debug: <http://build.webkit.org/builders/SnowLeopard%20Intel%20Debug%20%28Build%29/builds/2753/steps/compile-webkit/logs/stdio> Lion Intel Debug: <http://build.webkit.org/builders/Lion%20Intel%20Debug%20%28Build%29/builds/2246/steps/compile-webkit/logs/stdio>
Committed build fix in changeset 97040 <http://trac.webkit.org/changeset/97040> (bug #69735). For the "implicit conversion shortens 64-bit value into a 32-bit value" warnings I added some casts and committed this in changeset 97041 <http://trac.webkit.org/changeset/97041> and changeset 97042 <http://trac.webkit.org/changeset/97042>. See bug #69739 for more details. As remarked in <https://bugs.webkit.org/show_bug.cgi?id=69739#c3>: From reading the patch (attachment #110142 [details]) the only caller of zoomAnimatorTransformChanged() passes floats for the arguments. So, it seems sufficient to modify the prototype of zoomAnimatorTransformChanged() to take arguments of type float instead of double. Then we don't need any type casts. Is there a reason that zoomAnimatorTransformChanged() takes arguments of type double?
(In reply to comment #54) > Committed build fix in changeset 97040 <http://trac.webkit.org/changeset/97040> (bug #69735). > > For the "implicit conversion shortens 64-bit value into a 32-bit value" warnings I added some casts and committed this in changeset 97041 <http://trac.webkit.org/changeset/97041> and changeset 97042 <http://trac.webkit.org/changeset/97042>. See bug #69739 for more details. > > As remarked in <https://bugs.webkit.org/show_bug.cgi?id=69739#c3>: > > From reading the patch (attachment #110142 [details]) the only caller of zoomAnimatorTransformChanged() passes floats for the arguments. So, it seems sufficient to modify the prototype of zoomAnimatorTransformChanged() to take arguments of type float instead of double. Then we don't need any type casts. Is there a reason that zoomAnimatorTransformChanged() takes arguments of type double? The parameters for zoomAnimationTransformChanged can be changed to float without problem as fas as I know, but I can look into this to verify.