Bug 73489 – [chromium] Add fling animation support to ScrollAnimatorNone

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

Add fling animation support to ScrollAnimatorNone

Created attachment 117269 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 117269 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117269&action=review

It's quite surprising to see a patch that adds animation support to ScrollAnimatorNone. Isn't this class reason for existence to not do animation?

> Source/WebCore/platform/ScrollAnimatorNone.cpp:53
> -namespace WebCore {
> +namespace {

This should not be in anonymous namespace. Constants have internal linkage by default, and it's much easier (and consistent with other WebCore code) to mark the single function as static.

Created attachment 117493 [details]
Patch

[Alexey Proskuryakov]

Could you please comment on the question above?

(In reply to comment #4)
> Could you please comment on the question above?

ScrollAnimatorNone has an unfortunate name. It does not mean "a scroll animator that does not actually animate". In fact, it does quite a bit of animating (zoom animations, scroll animations, and now fling animations).

Created attachment 117499 [details]
Patch

> 
> > Source/WebCore/platform/ScrollAnimatorNone.cpp:53
> > -namespace WebCore {
> > +namespace {
> 
> This should not be in anonymous namespace. Constants have internal linkage by default, and it's much easier (and consistent with other WebCore code) to mark the single function as static.

Should be fixed with the latest patch.

[W. James MacLean]

(In reply to comment #7)
> > 
> > > Source/WebCore/platform/ScrollAnimatorNone.cpp:53
> > > -namespace WebCore {
> > > +namespace {
> > 
> > This should not be in anonymous namespace. Constants have internal linkage by default, and it's much easier (and consistent with other WebCore code) to mark the single function as static.
> 
> Should be fixed with the latest patch.

In general, looks good to me. One thing not obvious from the patch is how flings and smooth scrolls interact. For example, does initiation of a smooth scroll cancel any fling currently in flight? Vice versa? Or does the existing trajectory complete before the next starts? (I presume not ...)

(In reply to comment #8)
> (In reply to comment #7)
> > > 
> > > > Source/WebCore/platform/ScrollAnimatorNone.cpp:53
> > > > -namespace WebCore {
> > > > +namespace {
> > > 
> > > This should not be in anonymous namespace. Constants have internal linkage by default, and it's much easier (and consistent with other WebCore code) to mark the single function as static.
> > 
> > Should be fixed with the latest patch.
> 
> In general, looks good to me. One thing not obvious from the patch is how flings and smooth scrolls interact. For example, does initiation of a smooth scroll cancel any fling currently in flight? Vice versa? Or does the existing trajectory complete before the next starts? (I presume not ...)

The scroll / fling animations will interrupt one another (they will not be queued up in any way). The animation state is stored in the per axis data, and the PAD knows what sort of animation is happening (scroll vs fling).

[Kenneth Russell]

Comment on attachment 117499 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117499&action=review

The code changes look OK to me. One procedural issue and one minor nit.

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

This particular OOPS needs to be resolved. You really should add tests for this change. Talk with wjmaclean about how his recent ScrollAnimatorNone changes were tested in DumpRenderTree. If it's highly impractical to test these changes you can explain why and remove the OOPS, but I would discourage doing this.

> Source/WebCore/platform/ScrollAnimatorNone.h:3
> + *

Try to avoid spurious whitespace changes.

[Robert Kroeger]

View in context: https://bugs.webkit.org/attachment.cgi?id=117499&action=review

and you need tests. Really.

> Source/WebCore/ChangeLog:9
> +

this will break the commit queue. You need to change this to describe what you've added.

> Source/WebCore/platform/ScrollAnimatorNone.cpp:59
> +const double kDefaultFlingAcceleration = -4500.0; // pixels/second^2

people have complained that the default fling acceleration needs to vary with respect to the size of the page. Have you considered this feature? How hard would it be to add?

> Source/WebCore/platform/ScrollAnimatorNone.cpp:405
> +}

one blank?

> Source/WebCore/platform/ScrollAnimatorNone.cpp:591
> +    case PlatformGestureEvent::ScrollUpdateType: {

This case statement belongs in ScrollAnimator, not ScrollAnimatorNone -- PGEs that attempt to scroll the page still need to do the scroll even without hardware-accelerated scrolling. Forward the handling of ScrollUpdateType to there.

> Source/WebCore/platform/ScrollAnimatorNone.cpp:606
>      default:

It is correct webkit style to explicitly list all case statements.

> Source/WebCore/platform/ScrollAnimatorNone.h:-3
> - * 

nit: you could revert these lines that are only space changes.

Created attachment 118256 [details]
Patch

(In reply to comment #10)
> (From update of attachment 117499 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117499&action=review
> 
> The code changes look OK to me. One procedural issue and one minor nit.
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests. (OOPS!)
> 
> This particular OOPS needs to be resolved. You really should add tests for this change. Talk with wjmaclean about how his recent ScrollAnimatorNone changes were tested in DumpRenderTree. If it's highly impractical to test these changes you can explain why and remove the OOPS, but I would discourage doing this.

Fixed. I've added a unit test for fling to ScrollAnimatorNoneTest.cpp

> 
> > Source/WebCore/platform/ScrollAnimatorNone.h:3
> > + *
> 
> Try to avoid spurious whitespace changes.

Fixed.

[WebKit Review Bot]

Attachment 118256 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
From git://git.webkit.org/WebKit
 + f1b1893...46a47fc master     -> origin/master  (forced update)
	M	Source/JavaScriptCore/wtf/HashTraits.h
	M	Source/JavaScriptCore/ChangeLog
	M	Source/WebCore/dom/ChildListMutationScope.cpp
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/rendering/RenderLayer.cpp
	M	Source/WebCore/rendering/RenderLayer.h
r102267 = 67d4f2de4c2f4a2b6f1b3adfd20b50620fad3051 (refs/remotes/origin/master)
First, rewinding head to replay your work on top of it...
Applying: RenderLayer::updateZOrderLists(): Inline early-return condition.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 RenderLayer::updateZOrderLists(): Inline early-return condition.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 158.


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 118272 [details]
Patch

(In reply to comment #11)
> View in context: https://bugs.webkit.org/attachment.cgi?id=117499&action=review
> 
> and you need tests. Really.

Fixed. Added fling unit test to ScrollAnimatorNoneTest.cpp

> 
> > Source/WebCore/ChangeLog:9
> > +
> 
> this will break the commit queue. You need to change this to describe what you've added.

Done.

> 
> > Source/WebCore/platform/ScrollAnimatorNone.cpp:59
> > +const double kDefaultFlingAcceleration = -4500.0; // pixels/second^2
> 
> people have complained that the default fling acceleration needs to vary with respect to the size of the page. Have you considered this feature? How hard would it be to add?

No, this would not be hard to add. I would like to add this in a separate patch if/when this is needed.

> 
> > Source/WebCore/platform/ScrollAnimatorNone.cpp:405
> > +}
> 
> one blank?

Fixed.

> 
> > Source/WebCore/platform/ScrollAnimatorNone.cpp:591
> > +    case PlatformGestureEvent::ScrollUpdateType: {
> 
> This case statement belongs in ScrollAnimator, not ScrollAnimatorNone -- PGEs that attempt to scroll the page still need to do the scroll even without hardware-accelerated scrolling. Forward the handling of ScrollUpdateType to there.

Fixed. ScrollAnimatorNone passes the event on to it's base class if it doesn't handle the event. ScrollAnimator has such a switch, and handles the scroll update events since acceleration is not required.

> 
> > Source/WebCore/platform/ScrollAnimatorNone.cpp:606
> >      default:
> 
> It is correct webkit style to explicitly list all case statements.

Fixed.

> 
> > Source/WebCore/platform/ScrollAnimatorNone.h:-3
> > - * 
> 
> nit: you could revert these lines that are only space changes.

Done.

[WebKit Review Bot]

Comment on attachment 118272 [details]
Patch

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

New failing tests:
fast/events/touch/touch-gesture-scroll.html
fast/events/touch/gesture/gesture-scroll.html

[Robert Kroeger]

Comment on attachment 118272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118272&action=review

the broken layout tests are probably because you need https://bugs.webkit.org/show_bug.cgi?id=67822 to land. You might want to pester me about that and mark this as blocked on that bug or add an explicit expectation referencing that bug.

> Source/WebCore/platform/ScrollAnimator.cpp:133
> +void ScrollAnimator::handleGestureEvent(const PlatformGestureEvent& pge)

i've been dinged on reviews with use of abbreviations like this. I'd be good with it staying this way but the reviewer might still be sad.

> Source/WebCore/platform/ScrollAnimatorNone.cpp:606
> +        ScrollAnimator::handleGestureEvent(pge);

why not just call ScrollAnimator::handleGestureEvent explicitly inside the case? It would seem simpler.

(In reply to comment #18)
> (From update of attachment 118272 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118272&action=review
> 
> the broken layout tests are probably because you need https://bugs.webkit.org/show_bug.cgi?id=67822 to land. You might want to pester me about that and mark this as blocked on that bug or add an explicit expectation referencing that bug.

It's true. If I merge my changes with your patch, these layout tests pass. I've marked the bug as blocked on 67822.

> 
> > Source/WebCore/platform/ScrollAnimator.cpp:133
> > +void ScrollAnimator::handleGestureEvent(const PlatformGestureEvent& pge)
> 
> i've been dinged on reviews with use of abbreviations like this. I'd be good with it staying this way but the reviewer might still be sad.

Hopefully it's ok -- I was following the lead of other code in the file!

> 
> > Source/WebCore/platform/ScrollAnimatorNone.cpp:606
> > +        ScrollAnimator::handleGestureEvent(pge);
> 
> why not just call ScrollAnimator::handleGestureEvent explicitly inside the case? It would seem simpler.

It felt like a good idea to give ScrollAnimator a chance to handle the gesture event whenever ScrollAnimatorNone doesn't. ScrollAnimator happens to only care about scroll updates at the moment, but doing we're safe if ScrollAnimator decides to handle more gestures in the future.

[LFC]

> Source/WebCore/platform/ScrollAnimatorNone.cpp:535
> +    float velocity = orientation == VerticalScrollbar ? -pge.deltaY() : -pge.deltaX();

I don't think pge.deltaY() or page.deltaX() is velocity. You may have instance time divided to get a velocity.
Maybe we should add velocity as a field in PlatformGestureEvent so that when things like PlatformGestureRecognizer is detecting gesture from touch events, it can get velocity in each process.

[Robert Kroeger]

(In reply to comment #20)
> > Source/WebCore/platform/ScrollAnimatorNone.cpp:535
> > +    float velocity = orientation == VerticalScrollbar ? -pge.deltaY() : -pge.deltaX();
> 
> I don't think pge.deltaY() or page.deltaX() is velocity. You may have instance time divided to get a velocity.
> Maybe we should add velocity as a field in PlatformGestureEvent so that when things like PlatformGestureRecognizer is detecting gesture from touch events, it can get velocity in each process.

A few hopefully helpful points: 

AFAIK, all the code in WebKit & chromium that uses pge.deltaY, deltaX assumes that they represent velocities when the pge is a GestureScrollEnd. Code that does not make this assumption (WebKit2 say) ignores these fields and computes the velocity from the sequence of wheel events.

PlatformGestureRecognizer is gone from WebKit. It was only used in Chromium but the functionality has been moved into the Chrome browser.

[James Robinson]

Comment on attachment 118272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=118272&action=review

handleGestureEvent() is dead and has been for some time: http://trac.webkit.org/changeset/107823, so this patch needs some updating.

Are you sure you want to put the fling animation curves right into ScrollAnimatorNone?  On android, for instance, we'll be pulling the fling animation curves by calling into a system library, so it might be nice if the fling animation curves were easily extractable.

>>> Source/WebCore/platform/ScrollAnimator.cpp:133
>>> +void ScrollAnimator::handleGestureEvent(const PlatformGestureEvent& pge)
>> 
>> i've been dinged on reviews with use of abbreviations like this. I'd be good with it staying this way but the reviewer might still be sad.
> 
> Hopefully it's ok -- I was following the lead of other code in the file!

No, it's definitely not OK.  We pretty much never use abbreviations or acronyms in WebKit. http://www.webkit.org/coding/coding-style.html#names-full-words

> Source/WebCore/platform/ScrollAnimator.cpp:139
> +        float deltaX = (pge.deltaX() < 0.f) ? max(pge.deltaX(),  -float(maxForwardScrollDelta.width())) : min(pge.deltaX(), float(maxBackwardScrollDelta.width()));

don't append ".f" - http://www.webkit.org/coding/coding-style.html#float-suffixes

> Source/WebCore/platform/ScrollAnimatorNone.cpp:59
> +const double kDefaultFlingAcceleration = -2500.0; // pixels/second^2

the other constants are misnamed here too, but FYI we don't typically use the k prefix for constants, we just name them like normal variables.

> Source/WebCore/platform/ScrollAnimatorNone.cpp:61
> +static double getFlingPosition(double initialPosition, double initialVelocity, double acceleration, double t)

we very rarely use one-letter variable or parameter names. i think this would be better named "time"

> Source/WebCore/platform/ScrollAnimatorNone.cpp:63
> +    double maxT = -initialVelocity / acceleration;

again, maxTime would be a better name - although it's not incredibly clear why this is a cap here. a comment might help

> Source/WebCore/platform/ScrollAnimatorNone.cpp:66
> +    return initialPosition + t * (initialVelocity + 0.5f * acceleration * t);

no trailing "f"

> Source/WebCore/platform/ScrollAnimatorNone.cpp:346
> +    m_startTime = m_lastAnimationTime = WTF::monotonicallyIncreasingTime();

don't put the "WTF::" here. <wtf/CurrentTime.h> puts this function into the global namespace with a using statement. this is the general pattern WTF uses, it's very rare to see an explicit WTF:: unless there's an ambiguity

> Source/WebCore/platform/ScrollAnimatorNone.cpp:528
> +void ScrollAnimatorNone::fling(const PlatformGestureEvent& pge)

pge needs to be renamed

> Source/WebKit/chromium/tests/ScrollAnimatorNoneTest.cpp:994
> +    double const maxT = 10.0; // a scroll better not take longer than 10 seconds.

no trailing ".0"

this variable name isn't ideal

> Source/WebKit/chromium/tests/ScrollAnimatorNoneTest.cpp:997
> +    double t;

don't use one-letter variable names

[Robert Kroeger]

Different approach taken.