Showing Gloweffect correctly during fling on mainthread.

Showing Gloweffect correctly during fling on mainthread.

Showning Gloweffect correctly during fling on mainthread. Fling velocity
should be incremented so that gloweffect is correctly shown during
fling. Currently on flinging down on mainthread, on overscroll
gloweffect is shown top and viceversa.

BUG=499163
Committed: https://crrev.com/a5cc13ac54aaf04efcc25d660c0106f7af3d997c
Cr-Commit-Position: refs/heads/master@{#334339}

[MuVen, 2015-06-11 12:58:50]

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

[MuVen, 2015-06-11 12:59:12]

PTAL.

Thanks,

[MuVen, 2015-06-11 12:59:32]

sataya.m@samsung.com changed reviewers:
+ siva.gunturi@samsung.com

[jdduke (slow), 2015-06-11 15:28:10]

On 2015/06/11 12:59:12, MuVen wrote:
> PTAL.
> 
> Thanks,

Looks fine, I've never liked how our velocities are in the opposite direction of
the deltas they produce, so we need to do weird stuff like this. Probably an
artifact from the old WebKit days?

I have no preference as to whether we do the coordinate fixing here or in
RenderWidget. It might be a bit nicer to do in RenderWidget as here there's not
much context for the negation, and that might give us more flexibility if we
decide to change how we interpret velocity directions (as done in
InputHandlerProxy and by the overscroll glow code), but I don't really have a
strong preference and I'll defer to those that reviewed the Blink patch earlier
(majidvp).

[MuVen, 2015-06-11 15:35:02]

On 2015/06/11 15:28:10, jdduke wrote:
> On 2015/06/11 12:59:12, MuVen wrote:
> > PTAL.
> > 
> > Thanks,
> 
> Looks fine, I've never liked how our velocities are in the opposite direction
of
> the deltas they produce, so we need to do weird stuff like this. Probably an
> artifact from the old WebKit days?
> 
> I have no preference as to whether we do the coordinate fixing here or in
> RenderWidget. It might be a bit nicer to do in RenderWidget as here there's
not
> much context for the negation, and that might give us more flexibility if we
> decide to change how we interpret velocity directions (as done in
> InputHandlerProxy and by the overscroll glow code), but I don't really have a
> strong preference and I'll defer to those that reviewed the Blink patch
earlier
> (majidvp).

Small inference: if i move to RenderWidget, in that case test-case which i have
included might fail. In that case do we need test case ?

[jdduke (slow), 2015-06-11 15:43:42]

On 2015/06/11 15:35:02, MuVen wrote:
> Small inference: if i move to RenderWidget, in that case test-case which i
have
> included might fail. In that case do we need test case ?

I'm OK without a test case for this, even this new test won't guarantee that the
browser interprets the velocity correctly =/. Eventually we'll be able to
consolidate compositor/main interpretation of velocities. It's possible the
right thing to do is simply negate the velocity checks in
OverscrollGlow::Absorb, then we wouldn't need the negation in the first place,
but let's fix this for now =/. If you do move the change to RenderWidget, I
would just put a note about how the negation keeps the velocity direction
consistent with that returned on the compositor thread.

[majidvp, 2015-06-11 15:52:37]

On 2015/06/11 15:43:42, jdduke wrote:
> On 2015/06/11 15:35:02, MuVen wrote:
> > Small inference: if i move to RenderWidget, in that case test-case which i
> have
> > included might fail. In that case do we need test case ?
> 
> I'm OK without a test case for this, even this new test won't guarantee that
the
> browser interprets the velocity correctly =/. Eventually we'll be able to
> consolidate compositor/main interpretation of velocities. It's possible the
> right thing to do is simply negate the velocity checks in
> OverscrollGlow::Absorb, then we wouldn't need the negation in the first place,
> but let's fix this for now =/. If you do move the change to RenderWidget, I
> would just put a note about how the negation keeps the velocity direction
> consistent with that returned on the compositor thread.

I agree with jdduke@. It is more appropriate to reverse the velocity in 
RenderWidget where there is more context. The method looks out of place
in the event handler here.

[MuVen, 2015-06-12 10:19:46]

Patchset #3 (id:40001) has been deleted

[MuVen, 2015-06-12 11:14:03]

Patchset #3 (id:60001) has been deleted

[MuVen, 2015-06-12 12:37:42]

PTAL.

[jdduke (slow), 2015-06-12 14:53:17]

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

https://codereview.chromium.org/1176353002/diff/80001/content/renderer/render...
content/renderer/render_widget.cc:170: gfx::Vector2dF
ToClientScrollIncrement(const blink::WebFloatSize& increment) {
Let's just inline this function and the comment, I don't think we want it to be
used by code other than overscroll.

[MuVen, 2015-06-12 15:08:04]

PTAL.

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

https://codereview.chromium.org/1176353002/diff/80001/content/renderer/render...
content/renderer/render_widget.cc:170: gfx::Vector2dF
ToClientScrollIncrement(const blink::WebFloatSize& increment) {
On 2015/06/12 14:53:16, jdduke wrote:
> Let's just inline this function and the comment, I don't think we want it to
be
> used by code other than overscroll.

Done.

[jdduke (slow), 2015-06-12 15:57:38]

jdduke@chromium.org changed reviewers:
+ sievers@chromium.org

[jdduke (slow), 2015-06-12 15:57:38]

lgtm, +sievers for owner approval.

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

https://codereview.chromium.org/1176353002/diff/100001/content/renderer/rende...
content/renderer/render_widget.cc:2231: // TODO(sataya.m): ratify velocity once
http://crbug.com/499743 is fixed.
Nit: "Ratify", thought it's not quite clear what ratify means in this context
=/.

[no sievers, 2015-06-12 16:32:01]

lgtm

[MuVen, 2015-06-12 17:31:49]

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

[commit-bot: I haz the power, 2015-06-12 17:33:59]

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

[MuVen, 2015-06-12 17:37:51]

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

[MuVen, 2015-06-13 09:08:47]

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

[MuVen, 2015-06-13 09:08:47]

The patchset sent to the CQ was uploaded after l-g-t-m from jdduke@chromium.org,
sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1176353002/#ps140001
(title: "addressing nits")

[commit-bot: I haz the power, 2015-06-13 09:08:57]

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

[commit-bot: I haz the power, 2015-06-13 11:06:10]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2015-06-13 11:06:58]

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