Bug 73489 - [chromium] Add fling animation support to ScrollAnimatorNone
Summary: [chromium] Add fling animation support to ScrollAnimatorNone
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: vollick
URL:
Keywords:
Depends on: 67822
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-30 13:57 PST by vollick
Modified: 2012-11-30 09:53 PST (History)
10 users (show)

See Also:


Attachments
Patch (10.32 KB, patch)
2011-11-30 13:58 PST, vollick
no flags Details | Formatted Diff | Diff
Patch (10.22 KB, patch)
2011-12-01 14:40 PST, vollick
no flags Details | Formatted Diff | Diff
Patch (10.24 KB, patch)
2011-12-01 15:12 PST, vollick
no flags Details | Formatted Diff | Diff
Patch (16.33 KB, patch)
2011-12-07 12:59 PST, vollick
no flags Details | Formatted Diff | Diff
Patch (16.54 KB, patch)
2011-12-07 13:54 PST, vollick
jamesr: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description vollick 2011-11-30 13:57:21 PST
Add fling animation support to ScrollAnimatorNone
Comment 1 vollick 2011-11-30 13:58:16 PST
Created attachment 117269 [details]
Patch
Comment 2 Alexey Proskuryakov 2011-11-30 20:32:32 PST
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.
Comment 3 vollick 2011-12-01 14:40:55 PST
Created attachment 117493 [details]
Patch
Comment 4 Alexey Proskuryakov 2011-12-01 14:56:21 PST
Could you please comment on the question above?
Comment 5 vollick 2011-12-01 15:01:42 PST
(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).
Comment 6 vollick 2011-12-01 15:12:00 PST
Created attachment 117499 [details]
Patch
Comment 7 vollick 2011-12-01 15:19:20 PST
> 
> > 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.
Comment 8 W. James MacLean 2011-12-02 12:26:45 PST
(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 ...)
Comment 9 vollick 2011-12-05 11:41:27 PST
(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).
Comment 10 Kenneth Russell 2011-12-05 12:19:45 PST
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.
Comment 11 Robert Kroeger 2011-12-05 12:32:51 PST
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.
Comment 12 vollick 2011-12-07 12:59:48 PST
Created attachment 118256 [details]
Patch
Comment 13 vollick 2011-12-07 13:00:44 PST
(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.
Comment 14 WebKit Review Bot 2011-12-07 13:04:09 PST
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.
Comment 15 vollick 2011-12-07 13:54:47 PST
Created attachment 118272 [details]
Patch
Comment 16 vollick 2011-12-07 13:59:55 PST
(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.
Comment 17 WebKit Review Bot 2011-12-07 18:31:33 PST
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
Comment 18 Robert Kroeger 2011-12-08 07:36:12 PST
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.
Comment 19 vollick 2011-12-09 07:11:28 PST
(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.
Comment 20 LFC 2012-02-07 23:31:12 PST
> 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.
Comment 21 Robert Kroeger 2012-02-08 07:16:01 PST
(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.
Comment 22 James Robinson 2012-02-21 20:35:43 PST
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
Comment 23 Robert Kroeger 2012-11-30 09:53:18 PST
Different approach taken.