Smooth scrolling for Chromium

• Component: New Bugs
• Status: RESOLVED FIXED

[Scott Byer]

Smooth scrolling for Chromium

[Scott Byer]

Created attachment 95645 [details]
Patch

[Scott Byer]

This is a start; it adds the plumbing for run-time enablement and an initial implementation (that leaves lots of tuning to do). Currently doesn't affect the standalone WebKit build at all. The initial focus is Linux/Chromium OS, but eventually this code wants to be refactored with the existing but turned off Windows smooth scrolling code into something more cross platform. Some of this will also dovetail with the platform gestures work.

[Eric Seidel (no email)]

Comment on attachment 95645 [details]
Patch

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

I'm very confused by this.  Why would we want to use anything other than the platform scroll behavior?

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

This will cause the cq to fail.  We should really make the style-bot warn about this.  This should be replaced with a list of tests or xplaination of why testing is impossible/impractical/not-needed.

[Scott Byer]

> I'm very confused by this.  Why would we want to use anything other than the platform scroll behavior?

When the platform doesn't have a smooth scrolling implementation on it's own that is reasonable (e.g., Linux, Windows). When the device doesn't have smooth scrolling on any platform in Chromium (e.g., keyboard, mouse wheel) - though on the Mac it may make more sense to plumb through the platform scrolling behavior instead. And to animate flick gestures for devices and platforms that don't provide it (e.g., trackpad on anything other than a Mac).


>> Source/WebCore/ChangeLog:11
>> +        No new tests. (OOPS!)
>
>This will cause the cq to fail.  We should really make the style-bot warn about
>this.  This should be replaced with a list of tests or xplaination of why
>testing is impossible/impractical/not-needed.

I'll update the ChangeLogs. Currently, testing is manual only, and only on Chromium when it's compiled in. I expect to have some automated testing as I figure out how to measure when things go wrong.

[Scott Byer]

Created attachment 95781 [details]
Patch

[Adam Barth]

Comment on attachment 95781 [details]
Patch

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

> Source/WebCore/platform/chromium/ScrollAnimatorSettings.cpp:42
> +static ScrollAnimatorSettings& globalScrollAnimatorSettings()
> +{
> +    DEFINE_STATIC_LOCAL(ScrollAnimatorSettings, settings, ());
> +    return settings;
> +}

Why is this a static?  Should this be part of WebCore::Settings?  It seems like a per-Page concept.

[Vangelis Kokkevis]

Comment on attachment 95781 [details]
Patch

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

This looks pretty good to me (I'm not a WK reviewer, I'm afraid).  Some comments inline.  I think ideally we'd want to make this available on windows too by switching the gyp file to include this version of the scroll animator instead of scrollAnimatorWin.  Also, why not have it compiled in by default and rely on the settings flag alone to turn it on or off?

> Source/WebCore/page/Settings.cpp:189
> +    , m_enableScrollAnimator(false)

This variable isn't used.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.cpp:57
> +    Settings initial = { false, 0.25, Linear, 0.01, Linear, 0.01, false, false, Linear, 0, 0 };

Better to use ScrollAnimatorSettings::Settings to make it clear this isn't the WebCore::Settings class.  Here and below.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.cpp:402
> +        multiplier *= 1.5;

Magic values here and below should probably be constants somewhere, although not necessarily for this patch.

[Peter Kasting]

(In reply to comment #7)
> I think ideally we'd want to make this available on windows too by switching the gyp file to include this version of the scroll animator instead of scrollAnimatorWin.

scrollAnimatorWin was written by me for Chromium.  AFAIK no one actually uses the algorithm in it.  If we're not going to be using that algorithm either, then that file should be pretty much completely eliminated, rather than left in as dead code.

I think Scott sees things along the same lines here given comment 2.

[Scott Byer]

(In reply to comment #6)
> (From update of attachment 95781 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95781&action=review
> 
> > Source/WebCore/platform/chromium/ScrollAnimatorSettings.cpp:42
> > +static ScrollAnimatorSettings& globalScrollAnimatorSettings()
> > +{
> > +    DEFINE_STATIC_LOCAL(ScrollAnimatorSettings, settings, ());
> > +    return settings;
> > +}
> 
> Why is this a static?  Should this be part of WebCore::Settings?  It seems like a per-Page concept.

Looking through things, I'm thinking that the right way to do that is for ScrollableArea to have a routine which gets the settings, which FrameView overrides to provide them from the Page. Does that make sense?

[Adam Barth]

(In reply to comment #9)
> (In reply to comment #6)
> > (From update of attachment 95781 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=95781&action=review
> > 
> > > Source/WebCore/platform/chromium/ScrollAnimatorSettings.cpp:42
> > > +static ScrollAnimatorSettings& globalScrollAnimatorSettings()
> > > +{
> > > +    DEFINE_STATIC_LOCAL(ScrollAnimatorSettings, settings, ());
> > > +    return settings;
> > > +}
> > 
> > Why is this a static?  Should this be part of WebCore::Settings?  It seems like a per-Page concept.
> 
> Looking through things, I'm thinking that the right way to do that is for ScrollableArea to have a routine which gets the settings, which FrameView overrides to provide them from the Page. Does that make sense?

That's a reasonable design.  I don't know enough context to understand the issue fully, but the state should be attached to the object graph somewhere.  :)

[Scott Byer]

Created attachment 95843 [details]
Patch

[Scott Byer]

(In reply to comment #10)

> That's a reasonable design.  I don't know enough context to understand the issue fully, but the state should be attached to the object graph somewhere.  :)

Everything should be in a better place now - no bloat to the Settings when not compiled for Chromium smooth scrolling, but that's where the info is placed when it is.

[Adam Barth]

Comment on attachment 95843 [details]
Patch

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

The structure of this patch makes very little sense.  I recommend splitting off the settings part because that's going to require a very different kind of review than the guts of the algorithm.

> Source/WebCore/page/FrameView.h:340
> +    virtual const ScrollAnimatorSettings* scrollAnimatorSettings() const;

const ScrollAnimatorSettings* => ScrollAnimatorSettings const*  right?

> Source/WebCore/platform/ScrollAnimatorSettings.h:44
> +class ScrollAnimatorSettings {
> +public:
> +    static PassOwnPtr<ScrollAnimatorSettings> create();
> +
> +    virtual void setEnableScrollAnimator(bool flag) { }
> +    virtual bool scrollAnimatorEnabled() const { return false; }
> +};

This seems like way overkill.  Is this supposed to be a client?  This isn't how we do settings.

[Vangelis Kokkevis]

Comment on attachment 95843 [details]
Patch

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

The Settings design confuses me too.  What functionality are you going for? Wouldn't things be a lot simpler if ScrollAnimatorSettingsChromium was a class nested in ScrollAnimatorChrome?  Are you trying to expose the scroll animator settings so that they can be modified from outside this code (i.e. via a UI or flags)?

> Source/WebCore/page/FrameView.cpp:2248
> +        return m_page->settings()->scrollAnimatorSettings();

I'm missing the point here.  Why go through the FrameView to get settings that are global to the Page?

>> Source/WebCore/platform/ScrollAnimatorSettings.h:44
>> +};
> 
> This seems like way overkill.  Is this supposed to be a client?  This isn't how we do settings.

I agree with Adam.  Even if you do end up with a separate ScrolAnimatorSettings class, I think the enable/disable still fits as a boolean in WebCore::Settings .

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.cpp:382
> +    const ScrollAnimatorSettingsChromium::Settings *input_settings;

input_settings -> inputSettings

> Source/WebCore/platform/chromium/ScrollAnimatorSettingsChromium.cpp:56
> +    for (int i = 0; i < 5; ++i)

Better to augment the Type enum with a last value entry and replace the 5 here by the entry's name.

[Adam Barth]

> The Settings design confuses me too.

The design is very simple.  There are two ways to control the behavior of WebCore:

1) You can push a bool (or an string, something simple) into WebCore per Page using a WebCore::Setting.  Not all code can find the Setting object, but those than can are able to read that simple type.

2) For more complicated things, or things that need to be more fine-grained than a Page, you can add a client interface, which is a pure virtual callback to the WebKit layer.  There are a number of these interfaces that you can extend, or (in rare cases) you can create a new one.

[Scott Byer]

(In reply to comment #14)
> (From update of attachment 95843 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95843&action=review
> 
> The Settings design confuses me too.  What functionality are you going for? Wouldn't things be a lot simpler if ScrollAnimatorSettingsChromium was a class nested in ScrollAnimatorChrome?  Are you trying to expose the scroll animator settings so that they can be modified from outside this code (i.e. via a UI or flags)?

Yes, I think eventually some of the settings will want to be modified base on values that the Browser knows (e.g., keyboard repeat rate). In addition, when we got down to the nitty-gritty of fine tuning things, I wanted to have a WebUI where I could fiddle with everything and see traces. For something like smooth scrolling, modify-compile-run is too slow.



> I agree with Adam.  Even if you do end up with a separate ScrolAnimatorSettings class, I think the enable/disable still fits as a boolean in WebCore::Settings .

That makes sense.

> 
> > Source/WebCore/platform/chromium/ScrollAnimatorChromium.cpp:382
> > +    const ScrollAnimatorSettingsChromium::Settings *input_settings;
> 
> input_settings -> inputSettings

Good catch. Not a fan of CamelCase (years of Microsoft Word auto-correcting Photoshop incorrectly), but I'm learning to cope.

> 
> > Source/WebCore/platform/chromium/ScrollAnimatorSettingsChromium.cpp:56
> > +    for (int i = 0; i < 5; ++i)
> 
> Better to augment the Type enum with a last value entry and replace the 5 here by the entry's name.

Will do.

[Scott Byer]

(In reply to comment #15)
> > The Settings design confuses me too.
> 
> The design is very simple.  There are two ways to control the behavior of WebCore:
> 
> 1) You can push a bool (or an string, something simple) into WebCore per Page using a WebCore::Setting.  Not all code can find the Setting object, but those than can are able to read that simple type.
> 
> 2) For more complicated things, or things that need to be more fine-grained than a Page, you can add a client interface, which is a pure virtual callback to the WebKit layer.  There are a number of these interfaces that you can extend, or (in rare cases) you can create a new one.

This definitely falls into the more complicated category. It's why I ended up with the static data in the first place - the settings are bulky and don't need to be per page.

If you could give me a quick pointer at an example you think would fit, and I'll get things cleaned up. I'm going to be working in WebKit more moving forward, so now is a good time to teach me.

[Adam Barth]

> If you could give me a quick pointer at an example you think would fit, and I'll get things cleaned up. I'm going to be working in WebKit more moving forward, so now is a good time to teach me.

I don't really understand what this code is all about, so it's difficult to give you good advice.  ChromeClient.h might make sense given that ChromeClient deals with UI stuff.  (Note the "Chrome" in ChromeClient refers to "browser chrome" not "the browser named Chrome.")

Really, you should find a reviewer who both understands how WebKit fits together and understands what you're trying to do.

[Vangelis Kokkevis]

(In reply to comment #18)
> > If you could give me a quick pointer at an example you think would fit, and I'll get things cleaned up. I'm going to be working in WebKit more moving forward, so now is a good time to teach me.
> 
> I don't really understand what this code is all about, so it's difficult to give you good advice.  ChromeClient.h might make sense given that ChromeClient deals with UI stuff.  (Note the "Chrome" in ChromeClient refers to "browser chrome" not "the browser named Chrome.")
> 
> Really, you should find a reviewer who both understands how WebKit fits together and understands what you're trying to do.

Using ChromeClient seems like a good idea.  We already use it for some other settings related to compositing. 

In the interest of making this check-in more manageable and since the current scroll settings are hardwired, would it make sense to embed the settings in the ScrollAnimatorChromium class and separate them out in a subsequent CL when we figure out who needs to adjust them an how?

[Adam Barth]

> In the interest of making this check-in more manageable and since the current scroll settings are hardwired, would it make sense to embed the settings in the ScrollAnimatorChromium class and separate them out in a subsequent CL when we figure out who needs to adjust them an how?

That sounds like a great idea.  I feel bad delaying your work with these settings / configuration issues.

[Scott Byer]

(In reply to comment #20)
> > In the interest of making this check-in more manageable and since the current scroll settings are hardwired, would it make sense to embed the settings in the ScrollAnimatorChromium class and separate them out in a subsequent CL when we figure out who needs to adjust them an how?
> 
> That sounds like a great idea.  I feel bad delaying your work with these settings / configuration issues.

Don't. I need to learn. That's what code reviews are for, and I appreciate the time.

[Scott Byer]

Created attachment 95985 [details]
Patch

[Scott Byer]

(In reply to comment #19)

> In the interest of making this check-in more manageable and since the current scroll settings are hardwired, would it make sense to embed the settings in the ScrollAnimatorChromium class and separate them out in a subsequent CL when we figure out who needs to adjust them an how?

Done. That simplified things quite a bit.

[James Robinson]

Comment on attachment 95985 [details]
Patch

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

> Source/WebCore/platform/ScrollableArea.h:142
> +    virtual bool scrollAnimatorEnabled() const { return false; }

this doesn't seem like a property of a ScrollableArea.  Do we ever intend to have the scroll animator on for some ScrollableAreas but off for others?

[Adam Barth]

Comment on attachment 95985 [details]
Patch

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

(Again, I don't have any understanding of the actual content of this patch, the below are mostly just style nits.)

Three high-level points:

1) This code appears to be untested.  As a general rule, the project doesn't accept code without tests.  Sometimes it's difficult to have a testing plan in the initial stages of bringing up a feature.  You discuss this some in your ChangeLog.  Is there a plan for automated testing in the future?  How can folks work on this code without immediately breaking it?

2) None of this patch seems to be specific to Chromium.  Instead, this looks like a generic implementation that anyone could use if they're running on a platform without a platform-native implementation.  Rather than siloing this code away in a directory named "chromium," it's probably better to put it in the main platform directory.  In the past, we've used suffixes like "None" to indicate that a particular implementation is a generic version that you can use if your platform or library suite doesn't include this sort of functionality.

3) A bunch of the code around the settings is still really strange.  I don't fully understand what's going on there, but it doesn't look the way any other code in the project looks, which leads me to believe something isn't quite right there.  Rather than continuing to post on this bug, it might be helpful to find me or someone else on #webkit and we can try to understand what you're trying to accomplish there.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.cpp:54
> +    ScrollAnimatorSettings::Settings initial =
> +            { false, 0.25, Linear, 0.01, Linear, 0.01, false, false, Linear, 0, 0 };

Pls merge these lines.  There's no line length limit in WebKit.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.cpp:59
> +    ScrollAnimatorSettings::Settings keyboardLine =
> +            { true, 7 * kTickTime, Quadratic, 3 * kTickTime, Quadratic, 3 * kTickTime, false, false, Quadratic, 0, 0 };

Same for all these.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.cpp:121
> +double ScrollAnimatorChromium::PerAxisData::attackCurve(ScrollAnimatorSettings::Curve curve, double deltaT,
> +                                                        double curveT, double startPos, double attackPos)

These can all be one line too.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.cpp:137
> +    default:
> +        notImplemented();
> +        break;

You should list all the enum values explicitly.  That way the compiler will complain when this code evolves.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.cpp:218
> +                                                                 const ScrollAnimatorSettings::Settings* settings)

the const in "const ScrollAnimatorSettings::Settings*" is kind of meaningless.  That means the value of the pointer can't change, not that the object pointed to can't change.  I'd just drop he const.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.cpp:318
> +        double currentAttackPos = m_desiredVelocity ? attackCurve(m_attackCurve, deltaT, m_attackTime,
> +                                                              m_startPos, m_attackPos) : m_startPos;

I'd merge these lines.  They're already pretty tricky to read.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.cpp:321
> +        double newAttackPos = m_desiredVelocity ? attackCurve(m_attackCurve, deltaT, m_attackTime,
> +                                                          m_startPos, m_attackPos) : m_startPos;

ditto

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.cpp:346
> +        // attack is based on adding velocity

I'd remove this comment.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.cpp:348
> +        double newPos = attackCurve(m_attackCurve, deltaT, m_attackTime,
> +                                    m_startPos, m_attackPos) + m_attackOffset;

merge


newPos => newPosition (WebKit doesn't like to abbreviate works in variable names.)  I guess really all instances of "pos" should be "position" in this class.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.cpp:394
> +    // TODO(scottbyer): get the type passed in. MouseWheel could also be by line, but should still have different

TODO(scottbyer) => FIXME
(This is just a stylistic difference between Chromium and WebKit.)

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.cpp:396
> +    static ScrollAnimatorSettings settings;

If you need a non-trivial static, you need to use DECLARE_STATIC_LOCAL.  However, having this static here is very strange.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.h:64
> +    // Can be overspecified. animationTime takes priority over releaseTime takes priority over attackTime.
> +    // Missing: how to specify animation time accumulation (e.g., pure additive, asymptotic to a limit with a curve?)

"Can be overspecified." => Please use complete sentences in comments.  Should "Missing" be "FIXME" ?

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.h:84
> +    Settings settings[5];

5 => No magic numbers please.  Presumably this is LastEntry ?

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.h:86
> +    // Allow for per-platform initialization.

Please remove this comment.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.h:91
> +  public:

No indent for these qualifiers.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.h:99
> +    friend class ::ScrollAnimatorChromiumTest;

This class does not appear to exist.  Please remove.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.h:113
> +        static double attackCurve(ScrollAnimatorSettings::Curve, double deltaT, double curveT,
> +                                  double startPos, double attackPos);
> +        static double releaseCurve(ScrollAnimatorSettings::Curve, double deltaT, double curveT,
> +                                   double releasePos, double desiredPos);

Please merge these lines.

> Source/WebCore/platform/chromium/ScrollAnimatorChromium.h:149
> +    static const double animationTimerDelay;

Why static?  This is strange.

[Scott Byer]

(In reply to comment #24)
> (From update of attachment 95985 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95985&action=review
> 
> > Source/WebCore/platform/ScrollableArea.h:142
> > +    virtual bool scrollAnimatorEnabled() const { return false; }
> 
> this doesn't seem like a property of a ScrollableArea.  Do we ever intend to have the scroll animator on for some ScrollableAreas but off for others?

In the long run, no. This is for the short term while things are under development, in order to leave them behind a run-time flag. This was the simplest path for ScrollAnimator to reach up and get the flag from Settings. Is there a preferred way to do that?

[Vangelis Kokkevis]

Comment on attachment 95985 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:2243
> +bool FrameView::scrollAnimatorEnabled() const

Are the changes to the FrameView still needed?

>>> Source/WebCore/platform/ScrollableArea.h:142
>>> +    virtual bool scrollAnimatorEnabled() const { return false; }
>> 
>> this doesn't seem like a property of a ScrollableArea.  Do we ever intend to have the scroll animator on for some ScrollableAreas but off for others?
> 
> In the long run, no. This is for the short term while things are under development, in order to leave them behind a run-time flag. This was the simplest path for ScrollAnimator to reach up and get the flag from Settings. Is there a preferred way to do that?

I don't see an easy way for the ScrollableArea to reach to the page Settings either...

[Vangelis Kokkevis]

(In reply to comment #25)
> (From update of attachment 95985 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95985&action=review
> 
> (Again, I don't have any understanding of the actual content of this patch, the below are mostly just style nits.)
> 
> Three high-level points:
> 
> 1) This code appears to be untested.  As a general rule, the project doesn't accept code without tests.  Sometimes it's difficult to have a testing plan in the initial stages of bringing up a feature.  You discuss this some in your ChangeLog.  Is there a plan for automated testing in the future?  How can folks work on this code without immediately breaking it?

Would window.scrollTo() go through this code? 

> 
> 2) None of this patch seems to be specific to Chromium.  Instead, this looks like a generic implementation that anyone could use if they're running on a platform without a platform-native implementation.  Rather than siloing this code away in a directory named "chromium," it's probably better to put it in the main platform directory.  In the past, we've used suffixes like "None" to indicate that a particular implementation is a generic version that you can use if your platform or library suite doesn't include this sort of functionality.

While I do agree that this would be generally useful, it's still under early development.  If it does get checked in to the common directory then any port that enabled SMOOTH_SCROLLING will be affected by it. What if we leave it in platform/graphics/chromium for now and move it back when it's ready to be used by others as well?

[Adam Barth]

> Would window.scrollTo() go through this code? 

Dunno.

> While I do agree that this would be generally useful, it's still under early development.  If it does get checked in to the common directory then any port that enabled SMOOTH_SCROLLING will be affected by it. What if we leave it in platform/graphics/chromium for now and move it back when it's ready to be used by others as well?

Maybe it would be better to have a USE(SCROLL_ANIMATOR) define instead?  I'm skeptical the code will actually get moved when it's done.

[Peter Kasting]

(In reply to comment #29)
> Maybe it would be better to have a USE(SCROLL_ANIMATOR) define instead?

Even non-animating scrolls use the ScrollAnimator.

[Adam Barth]

As I said, I don't really understand what this code is doing.  My perspective is just that it looks non-Chromium specific so it shouldn't be in a Chromium-specific file and directory.

[Scott Byer]

Created attachment 97852 [details]
Patch

[Adam Barth]

Comment on attachment 97852 [details]
Patch

This looks great (from the perspective of someone ignorant of smooth scrolling)

[Adam Barth]

Comment on attachment 97852 [details]
Patch

Marking R+ based on Vangelis' comments above.  Thanks for iterating on the patch.  I very much appreciate it.

[Adam Barth]

Let's let the EWS bots munch on the patch for a bit before setting commit-queue+ (given that this patch contains a bunch of ifdefs).

[Adam Barth]

Comment on attachment 97852 [details]
Patch

We'll need to watch this one when it lands.  I'll should be in the channel to make sure its ok.

[Scott Byer]

(In reply to comment #36)
> (From update of attachment 97852 [details])
> We'll need to watch this one when it lands.  I'll should be in the channel to make sure its ok.

Ok, I'll stick around as long as I can as well. Thanks for all the help!

[WebKit Review Bot]

Comment on attachment 97852 [details]
Patch

Clearing flags on attachment: 97852

Committed r89305: <http://trac.webkit.org/changeset/89305>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.