Setting Accurate ScrollResult from Blink for Elastic Scroll.

Setting Accurate ScrollResult from Blink for Elastic Scroll.

Setting Accurate ScrollResult(overscroll information) from Blink for
Elastic Scroll. Currently unprocessed event is considered as
overscroll, where as processed event is considered as scroll.

Blink Changes @
https://codereview.chromium.org/1195803003/

BUG=442859
Committed: https://crrev.com/f405d0e2e64c89ae9ce2f7d60360a5816942e4cd
Cr-Commit-Position: refs/heads/master@{#337621}

[MuVen, 2015-06-23 15:27:50]

Patchset #1 (id:1) has been deleted

[ccameron, 2015-06-23 16:09:15]

ccameron@chromium.org changed reviewers:
+ ccameron@chromium.org

[ccameron, 2015-06-23 16:09:16]

https://codereview.chromium.org/1203693003/diff/20001/content/renderer/render...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1203693003/diff/20001/content/renderer/render...
content/renderer/render_widget.cc:2302: // http://crbug.com/442859
Even if event_processed is true, if you have a latest_overscroll_delta_ you
should still process that. So something like

scroll_result.did_scroll = event_processed;
scroll_result.did_overscroll_result = !latest_overscroll_delta_.isZero();
scroll_result.unused_scroll_delta = latest_overscroll_delta_;

or something to that effect.

[MuVen, 2015-06-25 09:48:14]

Patchset #4 (id:80001) has been deleted

[MuVen, 2015-06-25 09:48:24]

Patchset #4 (id:100001) has been deleted

[MuVen, 2015-06-25 09:48:30]

Patchset #3 (id:60001) has been deleted

[MuVen, 2015-06-25 09:48:38]

Patchset #2 (id:40001) has been deleted

[MuVen, 2015-06-25 12:56:01]

PTAL.

[ccameron, 2015-06-25 15:52:07]

The result computation looks good. The enabling I'll need to track carefully
(see the attached comment). The fact that the overscroll amount is accumulated
and then asynchronously consumed and reset is a bit hard to reason about (but
may be fine -- the input owners will have an opinion). I would suggest renaming
latest_overscroll_delta_ to something that suggests it's from the main thread.

https://codereview.chromium.org/1203693003/diff/120001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1203693003/diff/120001/content/renderer/rende...
content/renderer/render_view_impl.cc:1135:
settings->setElasticOverscrollEnabled(true);
This will want to share the logic used by
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
somehow.

[MuVen, 2015-06-25 17:31:21]

sataya.m@samsung.com changed reviewers:
+ jdduke@chromium.org

[MuVen, 2015-06-25 17:31:22]

Addressed as ccameron suggested.

Jared/ccameron, PTAL.

[jdduke (slow), 2015-06-25 23:55:45]

https://codereview.chromium.org/1203693003/diff/140001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1203693003/diff/140001/content/renderer/rende...
content/renderer/render_widget.cc:2301: if
(!compositor_deps_->IsElasticOverscrollEnabled())
With https://codereview.chromium.org/1209043002/, you should be able to pass a
DidOverscrollParams* (optional) value into this function using
|event_overscroll.get()|. Then you can populate the scroll results depending on
those values.

[MuVen, 2015-06-26 06:32:12]

Patchset #4 (id:160001) has been deleted

[MuVen, 2015-06-26 06:32:59]

Patchset #4 (id:180001) has been deleted

[MuVen, 2015-06-26 08:12:18]

Patchset #5 (id:220001) has been deleted

[MuVen, 2015-06-26 08:12:32]

Patchset #4 (id:200001) has been deleted

[MuVen, 2015-06-26 11:39:24]

addressed as mentioned. PTAL.

[jdduke (slow), 2015-06-26 15:15:45]

Thanks, I'll defer to ccameron@ for the remaining review.

https://codereview.chromium.org/1203693003/diff/260001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1203693003/diff/260001/content/renderer/rende...
content/renderer/render_widget.cc:2321: gfx::Vector2dF wheel_unused_delta;
Ah, I didn't realize we only use the "latest_overscroll_delta" value from the
params. I'd be OK passing that in directly as an arg (event_overscroll ?
event_overscroll->latest_overscroll_delta : gfx::Vector2df()).

https://codereview.chromium.org/1203693003/diff/260001/content/renderer/rende...
content/renderer/render_widget.cc:2328: ;
Looks like an extra ';'

[MuVen, 2015-06-29 13:29:52]

Ccameron, PTAL. Once 

https://codereview.chromium.org/1195803003/ is landed, shall fix the trybot
failures.

Thanks,

https://codereview.chromium.org/1203693003/diff/260001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1203693003/diff/260001/content/renderer/rende...
content/renderer/render_widget.cc:2321: gfx::Vector2dF wheel_unused_delta;
On 2015/06/26 15:15:45, jdduke wrote:
> Ah, I didn't realize we only use the "latest_overscroll_delta" value from the
> params. I'd be OK passing that in directly as an arg (event_overscroll ?
> event_overscroll->latest_overscroll_delta : gfx::Vector2df()).

Done.

https://codereview.chromium.org/1203693003/diff/260001/content/renderer/rende...
content/renderer/render_widget.cc:2328: ;
On 2015/06/26 15:15:45, jdduke wrote:
> Looks like an extra ';'

Done.

[ccameron, 2015-06-30 17:57:32]

lgtm

[MuVen, 2015-06-30 18:01:38]

The CQ bit was checked by sataya.m@samsung.com

[commit-bot: I haz the power, 2015-06-30 18:02:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1203693003/300001

[MuVen, 2015-06-30 18:12:38]

sataya.m@samsung.com changed reviewers:
+ piman@chromium.org

[MuVen, 2015-06-30 18:12:38]

Hi Piman,

Need owners approal. PTAL.

Thank,

[commit-bot: I haz the power, 2015-06-30 18:14:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-30 18:14:39]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[piman, 2015-06-30 21:30:00]

https://codereview.chromium.org/1203693003/diff/300001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1203693003/diff/300001/content/renderer/rende...
content/renderer/render_view_impl.cc:1140: ?
RenderViewImpl::FromWebView(web_view)->IsElasticOverscrollEnabled()
RenderViewImpl will disappear in the future (out-of-process iframes).
It sounds like all you need here is the compositor_deps_. Can you pass those
explicitly? The callers (where web_view is for a RenderViewImpl) have one -
either RenderViewImpl::OnUpdateWebPreferences or RenderViewImpl::Initialize.

For example you can add a CompositorDependencies* parameter to this function,
maybe renaming it ApplyWebPreferencesInternal, and adding a ApplyWebPreferences
that just calls this with a NULL CompositorDependencies for the other callers.

That way, you even avoid the map lookup, and no need for
IsElasticOverscrollEnabled on the RenderWidget.

[MuVen, 2015-07-01 10:59:32]

Patchset #8 (id:320001) has been deleted

[MuVen, 2015-07-01 11:21:16]

The CQ bit was checked by sataya.m@samsung.com

[MuVen, 2015-07-01 11:21:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
ccameron@chromium.org
Link to the patchset: https://codereview.chromium.org/1203693003/#ps340001
(title: "addressed piman review comments")

[commit-bot: I haz the power, 2015-07-01 11:21:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1203693003/340001

[commit-bot: I haz the power, 2015-07-01 11:28:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-01 11:28:58]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[MuVen, 2015-07-01 13:23:21]

Hi Piman,

PTAL.

Thanks,

https://codereview.chromium.org/1203693003/diff/300001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1203693003/diff/300001/content/renderer/rende...
content/renderer/render_view_impl.cc:1140: ?
RenderViewImpl::FromWebView(web_view)->IsElasticOverscrollEnabled()
On 2015/06/30 21:30:00, piman (Very slow to review) wrote:
> RenderViewImpl will disappear in the future (out-of-process iframes).
> It sounds like all you need here is the compositor_deps_. Can you pass those
> explicitly? The callers (where web_view is for a RenderViewImpl) have one -
> either RenderViewImpl::OnUpdateWebPreferences or RenderViewImpl::Initialize.
> 
> For example you can add a CompositorDependencies* parameter to this function,
> maybe renaming it ApplyWebPreferencesInternal, and adding a
ApplyWebPreferences
> that just calls this with a NULL CompositorDependencies for the other callers.
> 
> That way, you even avoid the map lookup, and no need for
> IsElasticOverscrollEnabled on the RenderWidget.

Done. PTAL.

[piman, 2015-07-01 16:08:05]

https://codereview.chromium.org/1203693003/diff/340001/content/public/rendere...
File content/public/renderer/render_view.h (right):

https://codereview.chromium.org/1203693003/diff/340001/content/public/rendere...
content/public/renderer/render_view.h:66: CompositorDependencies*
compositor_deps);
What I had in mind was to have ApplyWebPreferencesInternal be a (private?)
method on RenderViewImpl, and keep an ApplyWebPreferences on RenderView, with
the same signature as the current one.
The reason is that we don't want to expose CompositorDependencies in the public
content API since it's an implementation detail.

[MuVen, 2015-07-01 17:01:39]

PTAL.

https://codereview.chromium.org/1203693003/diff/340001/content/public/rendere...
File content/public/renderer/render_view.h (right):

https://codereview.chromium.org/1203693003/diff/340001/content/public/rendere...
content/public/renderer/render_view.h:66: CompositorDependencies*
compositor_deps);
On 2015/07/01 16:08:05, piman (Very slow to review) wrote:
> What I had in mind was to have ApplyWebPreferencesInternal be a (private?)
> method on RenderViewImpl, and keep an ApplyWebPreferences on RenderView, with
> the same signature as the current one.
> The reason is that we don't want to expose CompositorDependencies in the
public
> content API since it's an implementation detail.

Done.

[piman, 2015-07-01 20:30:46]

https://codereview.chromium.org/1203693003/diff/380001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1203693003/diff/380001/content/renderer/rende...
content/renderer/render_view_impl.cc:1513: compositor_deps ?
compositor_deps->IsElasticOverscrollEnabled() : false;
DCHECK(compositor_deps) instead. When called from a RenderViewImpl, it should
always be there.

https://codereview.chromium.org/1203693003/diff/380001/content/renderer/rende...
content/renderer/render_view_impl.cc:2661:
ApplyWebPreferencesInternal(webkit_preferences_, webview(), NULL);
compositor_deps_ instead of NULL.

[MuVen, 2015-07-02 09:50:52]

The CQ bit was checked by sataya.m@samsung.com

[MuVen, 2015-07-02 09:50:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
ccameron@chromium.org
Link to the patchset: https://codereview.chromium.org/1203693003/#ps400001
(title: "addressed comments by Piman")

[commit-bot: I haz the power, 2015-07-02 09:51:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1203693003/400001

[commit-bot: I haz the power, 2015-07-02 09:58:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-02 09:58:26]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[MuVen, 2015-07-02 13:52:48]

Hi Piman, PTAL. 

Thanks,

https://codereview.chromium.org/1203693003/diff/380001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1203693003/diff/380001/content/renderer/rende...
content/renderer/render_view_impl.cc:1513: compositor_deps ?
compositor_deps->IsElasticOverscrollEnabled() : false;
On 2015/07/01 20:30:46, piman (Very slow to review) wrote:
> DCHECK(compositor_deps) instead. When called from a RenderViewImpl, it should
> always be there.

Done.

https://codereview.chromium.org/1203693003/diff/380001/content/renderer/rende...
content/renderer/render_view_impl.cc:2661:
ApplyWebPreferencesInternal(webkit_preferences_, webview(), NULL);
On 2015/07/01 20:30:46, piman (Very slow to review) wrote:
> compositor_deps_ instead of NULL.

Done.

[MuVen, 2015-07-02 18:11:20]

PTAL.

https://codereview.chromium.org/1203693003/diff/380001/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/1203693003/diff/380001/content/renderer/rende...
content/renderer/render_view_impl.cc:2661:
ApplyWebPreferencesInternal(webkit_preferences_, webview(), NULL);
On 2015/07/02 13:52:47, MuVen wrote:
> On 2015/07/01 20:30:46, piman (Very slow to review) wrote:
> > compositor_deps_ instead of NULL.
> 
> Done.

Missed in the patch 11. Addressed this comment in patch 12.

[piman, 2015-07-07 15:01:29]

lgtm

[MuVen, 2015-07-07 15:03:53]

The CQ bit was checked by sataya.m@samsung.com

[MuVen, 2015-07-07 15:03:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
ccameron@chromium.org
Link to the patchset: https://codereview.chromium.org/1203693003/#ps420001
(title: "addressed piman comments")

[commit-bot: I haz the power, 2015-07-07 15:04:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1203693003/420001

[jdduke (slow), 2015-07-07 15:06:18]

This might be a good patch to apply the small delta filtering we discussed
earlier.

https://codereview.chromium.org/1203693003/diff/420001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1203693003/diff/420001/content/renderer/rende...
content/renderer/render_widget.cc:2321: scroll_result.did_overscroll_root =
!wheel_unused_delta.IsZero();
Can you make sure we're not getting small residual "unused" deltas? In cc/, the
unused delta can be ~.001, and I'm not sure we're filtering those out on the
Blink side.

[MuVen, 2015-07-07 15:07:04]

The CQ bit was unchecked by sataya.m@samsung.com

[MuVen, 2015-07-07 15:36:45]

PTAL.

https://codereview.chromium.org/1203693003/diff/420001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/1203693003/diff/420001/content/renderer/rende...
content/renderer/render_widget.cc:2321: scroll_result.did_overscroll_root =
!wheel_unused_delta.IsZero();
On 2015/07/07 15:06:17, jdduke wrote:
> Can you make sure we're not getting small residual "unused" deltas? In cc/,
the
> unused delta can be ~.001, and I'm not sure we're filtering those out on the
> Blink side.

Done.

[jdduke (slow), 2015-07-07 15:38:46]

On 2015/07/07 15:36:45, MuVen wrote:
> PTAL.
> 
>
https://codereview.chromium.org/1203693003/diff/420001/content/renderer/rende...
> File content/renderer/render_widget.cc (right):
> 
>
https://codereview.chromium.org/1203693003/diff/420001/content/renderer/rende...
> content/renderer/render_widget.cc:2321: scroll_result.did_overscroll_root =
> !wheel_unused_delta.IsZero();
> On 2015/07/07 15:06:17, jdduke wrote:
> > Can you make sure we're not getting small residual "unused" deltas? In cc/,
> the
> > unused delta can be ~.001, and I'm not sure we're filtering those out on the
> > Blink side.
> 
> Done.

Did you check? What kind of values are you seeing?

[MuVen, 2015-07-07 15:40:57]

On 2015/07/07 15:38:46, jdduke wrote:
> On 2015/07/07 15:36:45, MuVen wrote:
> > PTAL.
> > 
> >
>
https://codereview.chromium.org/1203693003/diff/420001/content/renderer/rende...
> > File content/renderer/render_widget.cc (right):
> > 
> >
>
https://codereview.chromium.org/1203693003/diff/420001/content/renderer/rende...
> > content/renderer/render_widget.cc:2321: scroll_result.did_overscroll_root =
> > !wheel_unused_delta.IsZero();
> > On 2015/07/07 15:06:17, jdduke wrote:
> > > Can you make sure we're not getting small residual "unused" deltas? In
cc/,
> > the
> > > unused delta can be ~.001, and I'm not sure we're filtering those out on
the
> > > Blink side.
> > 
> > Done.
> 
> Did you check? What kind of values are you seeing?

Actually i have seen values > 0.01 always, just added as a check as we suppress
in android, 
to ensure uniformity.

[jdduke (slow), 2015-07-07 15:44:05]

On 2015/07/07 15:40:57, MuVen wrote:
> On 2015/07/07 15:38:46, jdduke wrote:
> > On 2015/07/07 15:36:45, MuVen wrote:
> > > PTAL.
> > > 
> > >
> >
>
https://codereview.chromium.org/1203693003/diff/420001/content/renderer/rende...
> > > File content/renderer/render_widget.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1203693003/diff/420001/content/renderer/rende...
> > > content/renderer/render_widget.cc:2321: scroll_result.did_overscroll_root
=
> > > !wheel_unused_delta.IsZero();
> > > On 2015/07/07 15:06:17, jdduke wrote:
> > > > Can you make sure we're not getting small residual "unused" deltas? In
> cc/,
> > > the
> > > > unused delta can be ~.001, and I'm not sure we're filtering those out on
> the
> > > > Blink side.
> > > 
> > > Done.
> > 
> > Did you check? What kind of values are you seeing?
> 
> Actually i have seen values > 0.01 always, just added as a check as we
suppress
> in android, 
> to ensure uniformity.

So you're not seeing overscroll events after every scroll (as occurs in cc/ when
we don't explicitly suppress small residual overscroll deltas)? If you're not
seeing that, then your original change is fine and we shouldn't use ps#13
(that's why I wanted you to check =/).

[MuVen, 2015-07-07 15:46:23]

On 2015/07/07 15:44:05, jdduke wrote:
> On 2015/07/07 15:40:57, MuVen wrote:
> > On 2015/07/07 15:38:46, jdduke wrote:
> > > On 2015/07/07 15:36:45, MuVen wrote:
> > > > PTAL.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1203693003/diff/420001/content/renderer/rende...
> > > > File content/renderer/render_widget.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1203693003/diff/420001/content/renderer/rende...
> > > > content/renderer/render_widget.cc:2321:
scroll_result.did_overscroll_root
> =
> > > > !wheel_unused_delta.IsZero();
> > > > On 2015/07/07 15:06:17, jdduke wrote:
> > > > > Can you make sure we're not getting small residual "unused" deltas? In
> > cc/,
> > > > the
> > > > > unused delta can be ~.001, and I'm not sure we're filtering those out
on
> > the
> > > > > Blink side.
> > > > 
> > > > Done.
> > > 
> > > Did you check? What kind of values are you seeing?
> > 
> > Actually i have seen values > 0.01 always, just added as a check as we
> suppress
> > in android, 
> > to ensure uniformity.
> 
> So you're not seeing overscroll events after every scroll (as occurs in cc/
when
> we don't explicitly suppress small residual overscroll deltas)? If you're not
> seeing that, then your original change is fine and we shouldn't use ps#13
> (that's why I wanted you to check =/).

No, its not called for residual overscroll deltas. I have checked it, just added
for uniformity. I Shall land ps#13. Thanks for your patience.

[MuVen, 2015-07-07 15:49:46]

The CQ bit was checked by sataya.m@samsung.com

[MuVen, 2015-07-07 15:49:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
ccameron@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1203693003/#ps460001
(title: " ")

[commit-bot: I haz the power, 2015-07-07 15:50:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1203693003/460001

[commit-bot: I haz the power, 2015-07-07 16:51:19]

Committed patchset #14 (id:460001)

[commit-bot: I haz the power, 2015-07-07 16:52:26]

Patchset 14 (id:??) landed as
https://crrev.com/f405d0e2e64c89ae9ce2f7d60360a5816942e4cd
Cr-Commit-Position: refs/heads/master@{#337621}