Attempt to linearize the scroll deltas created in RenderWidgetHostViewBase

Attempt to linearize the scroll deltas created in RenderWidgetHostViewBase

We're currently shifting 10 pixels every time the smooth scroll gesture gets
serviced. If we get serviced at uneven intervals (which seems to happen), then
we get uneven scroll speeds. This patch attempts to force the scroll position
to be a linear function of time, though there may be jank if we generate the
events an uneven intervals.

BUG=162779
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171506

[Ian Vollick, 2012-11-30 15:39:45]

Glenn and I were looking at the smooth scroll gesture event generation this
morning, and I saw that we weren't adjusting to cope with uneven intervals
between when the gesture animations were serviced. I had assumed that this was
already there. I thought that this compensation would look something like this.
What do you think?

[hartmanng, 2012-11-30 16:26:13]

On 2012/11/30 15:39:45, vollick wrote:
> Glenn and I were looking at the smooth scroll gesture event generation this
> morning, and I saw that we weren't adjusting to cope with uneven intervals
> between when the gesture animations were serviced. I had assumed that this was
> already there. I thought that this compensation would look something like
this.
> What do you think?

After running a few tests with this patch and
https://codereview.chromium.org/11413199/ both applied, the trace graphs seem to
show much less of a "tail" in our SmoothScroll code. Ie, it looks like we stop
sending scroll events almost exactly when the page hits the bottom, whereas
before we could continue sending events up to 2 seconds after we were finished
scrolling. I have only tried this on linux so far, but it seems to be a good
indication that we're closer to the "actual" scroll speed of the page than
before.

[nduca, 2012-11-30 18:44:42]

Gaah we're shifting 10px every time its serviced? Its not interpolated by time?!
i had this before...   https://chromiumcodereview.appspot.com/11013043

[hartmanng, 2012-11-30 18:52:59]

On 2012/11/30 18:44:42, nduca wrote:
> Gaah we're shifting 10px every time its serviced? Its not interpolated by
time?!
> i had this before...   https://chromiumcodereview.appspot.com/11013043

I remember when that code was in there, but I also have a vague recollection
that it didn't actually work because "wheelTicks" didn't do what you thought it
did. I don't know why we took out the code instead of just using it on deltaY
instead of on wheelTicksY...

[nduca, 2012-11-30 23:33:06]

lgtm if you think it does better

[Ian Vollick, 2012-11-30 23:53:55]

+piman for OWNERS

On 2012/11/30 23:33:06, nduca wrote:
> lgtm if you think it does better

Yeah, I really think it does. I think this is much saner than scrolling 10px
each time we service the animation. We still have the issue of spamming the
system with events on linux, but I'd like to address that in a separate patch.

[piman, 2012-12-01 00:15:47]

+jam for public/
LGTM for my parts. I feel like I keep reviewing the same thing over and over
again :/

[nduca, 2012-12-01 01:45:30]

@vollick, maybe you guys can pull this gesture class out to a new file. Then I
can put in an OWNER for that one file.

[Ian Vollick, 2012-12-01 06:08:12]

On 2012/12/01 01:45:30, nduca wrote:
> @vollick, maybe you guys can pull this gesture class out to a new file. Then I
> can put in an OWNER for that one file.

SG. Done.

[piman, 2012-12-01 19:04:50]

lgtm

https://codereview.chromium.org/11416283/diff/7002/content/browser/renderer_h...
File content/browser/renderer_host/basic_mouse_wheel_smooth_scroll_gesture.h
(right):

https://codereview.chromium.org/11416283/diff/7002/content/browser/renderer_h...
content/browser/renderer_host/basic_mouse_wheel_smooth_scroll_gesture.h:33: 
nit: that's a lot of vertical whitespace!

[Ian Vollick, 2012-12-01 21:28:02]

https://codereview.chromium.org/11416283/diff/7002/content/browser/renderer_h...
File content/browser/renderer_host/basic_mouse_wheel_smooth_scroll_gesture.h
(right):

https://codereview.chromium.org/11416283/diff/7002/content/browser/renderer_h...
content/browser/renderer_host/basic_mouse_wheel_smooth_scroll_gesture.h:33: 
On 2012/12/01 19:04:50, piman wrote:
> nit: that's a lot of vertical whitespace!

Whoa! I don't know what happened there. Fixed.

[jam, 2012-12-02 23:47:39]

https://codereview.chromium.org/11416283/diff/11001/content/public/browser/re...
File content/public/browser/render_widget_host.h (right):

https://codereview.chromium.org/11416283/diff/11001/content/public/browser/re...
content/public/browser/render_widget_host.h:269: virtual int
SyntheticScrollMessageInterval() const = 0;
since this is just called from inside content (i.e. not by embedders), no need
to have it on the public interface. add it on RenderWidgetHostImpl and cast to
the impl when you need to call it

[Ian Vollick, 2012-12-03 03:21:25]

On 2012/12/02 23:47:39, John Abd-El-Malek wrote:
>
https://codereview.chromium.org/11416283/diff/11001/content/public/browser/re...
> File content/public/browser/render_widget_host.h (right):
> 
>
https://codereview.chromium.org/11416283/diff/11001/content/public/browser/re...
> content/public/browser/render_widget_host.h:269: virtual int
> SyntheticScrollMessageInterval() const = 0;
> since this is just called from inside content (i.e. not by embedders), no need
> to have it on the public interface. add it on RenderWidgetHostImpl and cast to
> the impl when you need to call it

Since RenderWidgetHost is a virtual base class of RenderWidgetHostImpl, I can't
use static_cast to do the downcast -- I have to use dynamic_cast, which the
style guide recommends I avoid. I've uploaded a patch that does this. Is this
preferable to mucking with the public API of RenderWidgetHost?

[jam, 2012-12-03 03:35:49]

On 2012/12/03 03:21:25, vollick wrote:
> On 2012/12/02 23:47:39, John Abd-El-Malek wrote:
> >
>
https://codereview.chromium.org/11416283/diff/11001/content/public/browser/re...
> > File content/public/browser/render_widget_host.h (right):
> > 
> >
>
https://codereview.chromium.org/11416283/diff/11001/content/public/browser/re...
> > content/public/browser/render_widget_host.h:269: virtual int
> > SyntheticScrollMessageInterval() const = 0;
> > since this is just called from inside content (i.e. not by embedders), no
need
> > to have it on the public interface. add it on RenderWidgetHostImpl and cast
to
> > the impl when you need to call it
> 
> Since RenderWidgetHost is a virtual base class of RenderWidgetHostImpl, I
can't
> use static_cast to do the downcast -- I have to use dynamic_cast, which the
> style guide recommends I avoid. I've uploaded a patch that does this. Is this
> preferable to mucking with the public API of RenderWidgetHost?

1) dynamic_cast won't work since we don't turn on RTTI
2) we use static_cast inside content to cast from interfaces exposed to the
embedder to internal implementations. we do this when there's only one
implementation of the interface. this is a common pattern because often the
embedder calls back content with an interface, and we need to call internal
methods. this is common with the webkit api implementation as well
3) looking deeper, turns out RenderWidgetHost is a bit special because of the
diamond inheritence. so use RenderWidgetHostImpl::From() method instead of
casting

[Ian Vollick, 2012-12-05 16:52:50]

On 2012/12/03 03:35:49, John Abd-El-Malek wrote:
> On 2012/12/03 03:21:25, vollick wrote:
> > On 2012/12/02 23:47:39, John Abd-El-Malek wrote:
> > >
> >
>
https://codereview.chromium.org/11416283/diff/11001/content/public/browser/re...
> > > File content/public/browser/render_widget_host.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/11416283/diff/11001/content/public/browser/re...
> > > content/public/browser/render_widget_host.h:269: virtual int
> > > SyntheticScrollMessageInterval() const = 0;
> > > since this is just called from inside content (i.e. not by embedders), no
> need
> > > to have it on the public interface. add it on RenderWidgetHostImpl and
cast
> to
> > > the impl when you need to call it
> > 
> > Since RenderWidgetHost is a virtual base class of RenderWidgetHostImpl, I
> can't
> > use static_cast to do the downcast -- I have to use dynamic_cast, which the
> > style guide recommends I avoid. I've uploaded a patch that does this. Is
this
> > preferable to mucking with the public API of RenderWidgetHost?
> 
> 1) dynamic_cast won't work since we don't turn on RTTI
> 2) we use static_cast inside content to cast from interfaces exposed to the
> embedder to internal implementations. we do this when there's only one
> implementation of the interface. this is a common pattern because often the
> embedder calls back content with an interface, and we need to call internal
> methods. this is common with the webkit api implementation as well
> 3) looking deeper, turns out RenderWidgetHost is a bit special because of the
> diamond inheritence. so use RenderWidgetHostImpl::From() method instead of
> casting

Thanks! Done.

[jam, 2012-12-05 19:50:21]

lgtm

[commit-bot: I haz the power, 2012-12-05 19:57:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vollick@chromium.org/11416283/14001

[commit-bot: I haz the power, 2012-12-05 19:57:07]

Failed to apply patch for content/content_browser.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/content_browser.gypi
  Hunk #1 FAILED at 555.
  1 out of 1 hunk FAILED -- saving rejects to file
content/content_browser.gypi.rej

Patch:       content/content_browser.gypi
Index: content/content_browser.gypi
diff --git a/content/content_browser.gypi b/content/content_browser.gypi
index
278c162a4c3f515e5eca5fe8d42d0f15745bf31a..7b5473255c97c20a53d4f8dc7edc9e60d2cc5767
100644
--- a/content/content_browser.gypi
+++ b/content/content_browser.gypi
@@ -555,6 +555,8 @@
     'browser/renderer_host/backing_store_manager.h',
     'browser/renderer_host/backing_store_win.cc',
     'browser/renderer_host/backing_store_win.h',
+    'browser/renderer_host/basic_mouse_wheel_smooth_scroll_gesture.cc',
+    'browser/renderer_host/basic_mouse_wheel_smooth_scroll_gesture.h',
     'browser/renderer_host/buffered_resource_handler.cc',
     'browser/renderer_host/buffered_resource_handler.h',
     'browser/renderer_host/clipboard_message_filter.cc',

[commit-bot: I haz the power, 2012-12-05 22:39:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vollick@chromium.org/11416283/24001

[commit-bot: I haz the power, 2012-12-05 23:01:27]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-06 00:21:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vollick@chromium.org/11416283/14002

[commit-bot: I haz the power, 2012-12-06 15:34:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vollick@chromium.org/11416283/14002

[commit-bot: I haz the power, 2012-12-06 16:03:14]

Change committed as 171506