Bug 68035 – [chromium] Zoom animator front-end

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[W. James MacLean]

[chromium] Zoom animator front-end [work-in-progress, not for review yet ...]

[W. James MacLean]

Created attachment 107241 [details]
Patch

[Collabora GTK+ EWS bot]

Comment on attachment 107241 [details]
Patch

Attachment 107241 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9657331

[WebKit Review Bot]

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

[Robert Kroeger]

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.

[W. James MacLean]

(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).

[W. James MacLean]

Created attachment 107333 [details]
Patch

[W. James MacLean]

(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.

[Robert Kroeger]

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

[WebKit Review Bot]

Comment on attachment 107333 [details]
Patch

Attachment 107333 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9660606

[W. James MacLean]

Created attachment 107359 [details]
Patch

[W. James MacLean]

(In reply to comment #10)
> Created an attachment (id=107359) [details]
> Patch

Same patch, updated to ToT for fsamuel.

[Gustavo Noronha (kov)]

Comment on attachment 107359 [details]
Patch

Attachment 107359 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9660837

[WebKit Review Bot]

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

[W. James MacLean]

Created attachment 107522 [details]
Patch

[WebKit Review Bot]

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

[Collabora GTK+ EWS bot]

Comment on attachment 107522 [details]
Patch

Attachment 107522 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9705467

[W. James MacLean]

Created attachment 109176 [details]
Patch

[Collabora GTK+ EWS bot]

Comment on attachment 109176 [details]
Patch

Attachment 109176 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9893380

[Early Warning System Bot]

Comment on attachment 109176 [details]
Patch

Attachment 109176 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9897004

[W. James MacLean]

Created attachment 109201 [details]
Patch

[Early Warning System Bot]

Comment on attachment 109201 [details]
Patch

Attachment 109201 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9899218

[Gustavo Noronha (kov)]

Comment on attachment 109201 [details]
Patch

Attachment 109201 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9893492

[W. James MacLean]

Created attachment 109232 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 109232 [details]
Patch

Attachment 109232 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9888721

[W. James MacLean]

Created attachment 109291 [details]
Patch

[Robert Kroeger]

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?

[W. James MacLean]

(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 :-)

[Scott Byer]

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.

[W. James MacLean]

(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.

[W. James MacLean]

Created attachment 109482 [details]
Patch

[W. James MacLean]

(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.

[W. James MacLean]

(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?

[Kenneth Russell]

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.

[W. James MacLean]

(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.

[W. James MacLean]

Created attachment 110008 [details]
Patch

[W. James MacLean]

(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.

[Gyuyoung Kim]

Comment on attachment 110008 [details]
Patch

Attachment 110008 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9968342

[Early Warning System Bot]

Comment on attachment 110008 [details]
Patch

Attachment 110008 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9978323

[W. James MacLean]

Created attachment 110016 [details]
Patch

[W. James MacLean]

(In reply to comment #39)
> Created an attachment (id=110016) [details]
> Patch

Fixed QT compilation issue, re-baselined new test expectation.

[Gyuyoung Kim]

Comment on attachment 110016 [details]
Patch

Attachment 110016 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9979314

[Early Warning System Bot]

Comment on attachment 110016 [details]
Patch

Attachment 110016 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9968359

[Kenneth Russell]

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.

[Kenneth Russell]

(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.

[W. James MacLean]

Created attachment 110142 [details]
Patch

[W. James MacLean]

(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.

[W. James MacLean]

(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.

[Kenneth Russell]

Comment on attachment 110142 [details]
Patch

Looks good to me. r=me

[WebKit Review Bot]

Comment on attachment 110142 [details]
Patch

Clearing flags on attachment: 110142

Committed r97034: <http://trac.webkit.org/changeset/97034>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Luke Macpherson]

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.

[Luke Macpherson]

I've posted a fix here: https://bugs.webkit.org/show_bug.cgi?id=69735 seems not many reviewers are online tonight though.

[Daniel Bates]

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>

[Daniel Bates]

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?

[W. James MacLean]

(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.