Stop negating the overscroll velocity to enable the glow of main thread fling

Stop negating the overscroll velocity to enable the glow of main thread fling

The glow of the fling is calculated from the fling velocity, which may be
defined for gesture and scroll that are in opposite directions. The overscroll
glow logic expects to receive the fling scroll velocity as opposed to fling
gesture velocity. So we stop negating the value to enable the glow effect at
disable-threaded-scrolling.

BUG=597005
Committed: https://crrev.com/5b3f75d6275ec65b1db1116f58e462e729a782ac
Cr-Commit-Position: refs/heads/master@{#416137}

[sunyunjia, 2016-08-29 20:59:29]

Description was changed from

==========
Negate the velocity to enable the overscroll-glow when threaded scrolling is
disabled

BUG=597005
==========

to

==========
Negate the overscroll velocity to enable the glow of main thread fling

The glow of the fling is calculated from the overscroll velocity. In current
implementation, the overscroll velocity at disable-threaded-scrolling is the
negative value of the velocity at enable-threaded-scrolling, blocking the proper
glow effect. So we negate the value at disable-threaded-scrolling to enable the
glow effect.

BUG=597005
==========

[sunyunjia, 2016-08-29 20:59:30]

sunyunjia@chromium.org changed reviewers:
+ dtapuska@chromium.org

[sunyunjia, 2016-08-29 20:59:47]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[sunyunjia, 2016-08-29 21:00:56]

Description was changed from

==========
Negate the overscroll velocity to enable the glow of main thread fling

The glow of the fling is calculated from the overscroll velocity. In current
implementation, the overscroll velocity at disable-threaded-scrolling is the
negative value of the velocity at enable-threaded-scrolling, blocking the proper
glow effect. So we negate the value at disable-threaded-scrolling to enable the
glow effect.

BUG=597005
==========

to

==========
Negate the overscroll velocity to enable the glow of main thread fling

The glow of the fling is calculated from the overscroll velocity. In current
implementation, the overscroll velocity at disable-threaded-scrolling is the
negative value of the velocity at enable-threaded-scrolling, blocking the
proper glow effect. So we negate the value at disable-threaded-scrolling to
enable the glow effect.

BUG=597005
==========

[sunyunjia, 2016-08-29 21:03:19]

PTAL, Thanks!

[dtapuska, 2016-08-29 21:16:34]

dtapuska@chromium.org changed reviewers:
+ majidvp@chromium.org

[dtapuska, 2016-08-29 21:16:34]

majidvp@ do you know the status of the crbug quoted in this change...

https://codereview.chromium.org/2285423002/diff/1/content/renderer/input/rend...
File content/renderer/input/render_widget_input_handler.cc (right):

https://codereview.chromium.org/2285423002/diff/1/content/renderer/input/rend...
content/renderer/input/render_widget_input_handler.cc:517: // TODO(sataya.m):
don't negate velocity once http://crbug.com/499743 is
Do we need to remove this comment? Is this partially fixed now?

[commit-bot: I haz the power, 2016-08-29 21:23:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sunyunjia, 2016-08-30 01:24:29]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 02:03:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 03:50:33]

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

[commit-bot: I haz the power, 2016-08-30 03:50:33]

Dry run: This issue passed the CQ dry run.

[majidvp, 2016-08-31 19:39:41]

https://codereview.chromium.org/2285423002/diff/1/content/renderer/input/rend...
File content/renderer/input/render_widget_input_handler.cc (right):

https://codereview.chromium.org/2285423002/diff/1/content/renderer/input/rend...
content/renderer/input/render_widget_input_handler.cc:517: // TODO(sataya.m):
don't negate velocity once http://crbug.com/499743 is
On 2016/08/29 21:16:34, dtapuska wrote:
> Do we need to remove this comment? Is this partially fixed now?

Overscroll glow logic expects to receive delta and velocity of scroll which are
in the same direction.

In the past this velocity here was the actual *gesture* velocity which needed to
be negated to get scroll behaviour. I did some digging and I think this bokan@
changed the semantic of this velocity here which is probably the point where the
glow effect broke: https://codereview.chromium.org/1755773002

So yes, please remove this TODO and close that bug as well :).

[bokan, 2016-08-31 21:04:30]

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

[bokan, 2016-08-31 21:04:30]

https://codereview.chromium.org/2285423002/diff/1/content/renderer/input/rend...
File content/renderer/input/render_widget_input_handler.cc (right):

https://codereview.chromium.org/2285423002/diff/1/content/renderer/input/rend...
content/renderer/input/render_widget_input_handler.cc:517: // TODO(sataya.m):
don't negate velocity once http://crbug.com/499743 is
On 2016/08/31 19:39:40, majidvp wrote:
> On 2016/08/29 21:16:34, dtapuska wrote:
> > Do we need to remove this comment? Is this partially fixed now?
> 
> Overscroll glow logic expects to receive delta and velocity of scroll which
are
> in the same direction.
> 
> In the past this velocity here was the actual *gesture* velocity which needed
to
> be negated to get scroll behaviour. I did some digging and I think this bokan@
> changed the semantic of this velocity here which is probably the point where
the
> glow effect broke: https://codereview.chromium.org/1755773002
> 
> So yes, please remove this TODO and close that bug as well :).

Yes, I believe that's correct.

[sunyunjia, 2016-09-01 15:18:25]

On 2016/08/31 21:04:30, bokan wrote:
>
https://codereview.chromium.org/2285423002/diff/1/content/renderer/input/rend...
> File content/renderer/input/render_widget_input_handler.cc (right):
> 
>
https://codereview.chromium.org/2285423002/diff/1/content/renderer/input/rend...
> content/renderer/input/render_widget_input_handler.cc:517: // TODO(sataya.m):
> don't negate velocity once http://crbug.com/499743 is
> On 2016/08/31 19:39:40, majidvp wrote:
> > On 2016/08/29 21:16:34, dtapuska wrote:
> > > Do we need to remove this comment? Is this partially fixed now?
> > 
> > Overscroll glow logic expects to receive delta and velocity of scroll which
> are
> > in the same direction.
> > 
> > In the past this velocity here was the actual *gesture* velocity which
needed
> to
> > be negated to get scroll behaviour. I did some digging and I think this
bokan@
> > changed the semantic of this velocity here which is probably the point where
> the
> > glow effect broke: https://codereview.chromium.org/1755773002
> > 
> > So yes, please remove this TODO and close that bug as well :).
> 
> Yes, I believe that's correct.

TODO Removed. PTAL, Thanks!

[sunyunjia, 2016-09-01 15:18:33]

The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 15:18:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 16:18:20]

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

[commit-bot: I haz the power, 2016-09-01 16:18:21]

Dry run: This issue passed the CQ dry run.

[dtapuska, 2016-09-01 18:59:22]

lgtm

[sunyunjia, 2016-09-01 19:00:29]

The CQ bit was checked by sunyunjia@chromium.org

[commit-bot: I haz the power, 2016-09-01 19:01:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 19:07:26]

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

[commit-bot: I haz the power, 2016-09-01 19:07:27]

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

[bokan, 2016-09-01 19:16:39]

I would add to the description that the reason this was broken is because
https://codereview.chromium.org/1755773002 started reporting velocity relative
to the page, rather than relative to the screen. I would also phrase it as
"Stopped negating" :)

[bokan, 2016-09-01 19:16:50]

but lgtm in any case

[sunyunjia, 2016-09-01 19:27:27]

Description was changed from

==========
Negate the overscroll velocity to enable the glow of main thread fling

The glow of the fling is calculated from the overscroll velocity. In current
implementation, the overscroll velocity at disable-threaded-scrolling is the
negative value of the velocity at enable-threaded-scrolling, blocking the
proper glow effect. So we negate the value at disable-threaded-scrolling to
enable the glow effect.

BUG=597005
==========

to

==========
Stop negating the overscroll velocity to enable the glow of main thread fling

The glow of the fling is calculated from the overscroll velocity. In current
implementation, the overscroll velocity at disable-threaded-scrolling is the
negative value of the velocity at enable-threaded-scrolling, blocking the
proper glow effect. This was broken because
https://codereview.chromium.org/1755773002 started reporting velocity relative
to the page, rather than relative to the screen. So we stop negating the value
at disable-threaded-scrolling to enable the glow effect.

BUG=597005
==========

[sunyunjia, 2016-09-01 19:27:27]

sunyunjia@chromium.org changed reviewers:
+ esprehn@chromium.org

[sunyunjia, 2016-09-01 19:30:11]

esprehn@ could you please take a look at this patch? Thanks!

[majidvp, 2016-09-01 19:41:16]

nit: The description is a bit confusing. It talks about velocity at
"disable-threaded-scrolling" 
vs it at "enable-threaded-scrolling". This is not the right terminology.

I think you want to rephrease this to talk about fling velocity (I prefer fling
velocity as it matches
current naming and not overscroll velocity) which may be defined for gesture and
scroll that 
are in opposite directions. The overscroll glow logic expects to receive the
fling scroll velocity
as opposed to fling gesture velocity.

bokan@: 
> reporting velocity relative to the page, rather than relative to the screen

Is this really accurate? I mean do we consider screen and page to be at opposite
directions? I assumed we are taking about gesture velocity and scroll velocity
which
are in opposite directions.

[bokan, 2016-09-01 19:42:55]

> bokan@: 
> > reporting velocity relative to the page, rather than relative to the screen
> 
> Is this really accurate? I mean do we consider screen and page to be at
opposite
> directions? I assumed we are taking about gesture velocity and scroll velocity
> which
> are in opposite directions.

You're right, that's what I was trying to capture but scroll vs gesture is the
better way to phrase it.

[sunyunjia, 2016-09-01 21:31:20]

Description was changed from

==========
Stop negating the overscroll velocity to enable the glow of main thread fling

The glow of the fling is calculated from the overscroll velocity. In current
implementation, the overscroll velocity at disable-threaded-scrolling is the
negative value of the velocity at enable-threaded-scrolling, blocking the
proper glow effect. This was broken because
https://codereview.chromium.org/1755773002 started reporting velocity relative
to the page, rather than relative to the screen. So we stop negating the value
at disable-threaded-scrolling to enable the glow effect.

BUG=597005
==========

to

==========
Stop negating the overscroll velocity to enable the glow of main thread fling

The glow of the fling is calculated from the fling velocity, which may be
defined for gesture and scroll that are in opposite directions. The overscroll
glow logic expects to receive the fling scroll velocity as opposed to fling
gesture velocity. So we stop negating the value to enable the glow effect at
disable-threaded-scrolling.

BUG=597005
==========

[esprehn, 2016-09-01 23:41:15]

lgtm

[sunyunjia, 2016-09-02 00:05:55]

The CQ bit was checked by sunyunjia@chromium.org

[commit-bot: I haz the power, 2016-09-02 00:06:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 00:10:53]

Description was changed from

==========
Stop negating the overscroll velocity to enable the glow of main thread fling

The glow of the fling is calculated from the fling velocity, which may be
defined for gesture and scroll that are in opposite directions. The overscroll
glow logic expects to receive the fling scroll velocity as opposed to fling
gesture velocity. So we stop negating the value to enable the glow effect at
disable-threaded-scrolling.

BUG=597005
==========

to

==========
Stop negating the overscroll velocity to enable the glow of main thread fling

The glow of the fling is calculated from the fling velocity, which may be
defined for gesture and scroll that are in opposite directions. The overscroll
glow logic expects to receive the fling scroll velocity as opposed to fling
gesture velocity. So we stop negating the value to enable the glow effect at
disable-threaded-scrolling.

BUG=597005
==========

[commit-bot: I haz the power, 2016-09-02 00:10:55]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-02 00:14:42]

Description was changed from

==========
Stop negating the overscroll velocity to enable the glow of main thread fling

The glow of the fling is calculated from the fling velocity, which may be
defined for gesture and scroll that are in opposite directions. The overscroll
glow logic expects to receive the fling scroll velocity as opposed to fling
gesture velocity. So we stop negating the value to enable the glow effect at
disable-threaded-scrolling.

BUG=597005
==========

to

==========
Stop negating the overscroll velocity to enable the glow of main thread fling

The glow of the fling is calculated from the fling velocity, which may be
defined for gesture and scroll that are in opposite directions. The overscroll
glow logic expects to receive the fling scroll velocity as opposed to fling
gesture velocity. So we stop negating the value to enable the glow effect at
disable-threaded-scrolling.

BUG=597005

Committed: https://crrev.com/5b3f75d6275ec65b1db1116f58e462e729a782ac
Cr-Commit-Position: refs/heads/master@{#416137}
==========

[commit-bot: I haz the power, 2016-09-02 00:14:43]

Patchset 3 (id:??) landed as
https://crrev.com/5b3f75d6275ec65b1db1116f58e462e729a782ac
Cr-Commit-Position: refs/heads/master@{#416137}