OverscrollGlow for mainThread-{BLINK CHANGES}

Over-scroll Glow Animation for main-thread.

Currently we support over-scroll glow animation only on compositor 
scroll. Current patch updates main-thread and calculates overscroll
params to report the overscroll but doesn't do the animation itself.

Chromium changes @ https://codereview.chromium.org/1102743002/
which passes on overscroll params to RWHVA.

BUG=420207
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196748

[MuVen, 2015-04-23 11:38:38]

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

[MuVen, 2015-04-23 11:41:59]

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

[MuVen, 2015-04-23 11:43:43]

Hi Jdduke,

PTAL of the changes, This is just a working prototype, Based on your comments i
shall modify further.

Thanks,

[majidvp, 2015-04-23 18:25:09]

https://codereview.chromium.org/1056983004/diff/1/Source/core/layout/LayoutBo...
File Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1056983004/diff/1/Source/core/layout/LayoutBo...
Source/core/layout/LayoutBox.cpp:760: 
unused scroll delta is already being calculated in |ScrollableArea::scroll|. 
It makes sense to re-use that instead of trying to recompute it here.
On option is to have  |ScrollableArea::scroll| also return
|ScrollResultOneDimensional| which contains unused delta.

https://codereview.chromium.org/1056983004/diff/1/Source/core/layout/LayoutBox.h
File Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/1056983004/diff/1/Source/core/layout/LayoutBo...
Source/core/layout/LayoutBox.h:833: IntPoint m_unusedDelta;
It does not make sense to have this information in LayoutBox. Unused scroll
delta needs to only be tracked for main frame and then only
during a single scroll event. I don't think it needs to be a member.

https://codereview.chromium.org/1056983004/diff/1/Source/core/page/EventHandl...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1056983004/diff/1/Source/core/page/EventHandl...
Source/core/page/EventHandler.cpp:960: *unusedDelta = cbUnusedDelta.x();
Overscroll should happen only if the document's main frame is scrolled and has
any scroll delta remaining. This logic overscrolls for all scrolling elements
which is not correct.

Also, this only covers the case where root layer scrolls. See my other comments
the other two cases where frame view maybe scrolled and thus generate unused
scroll delta.

https://codereview.chromium.org/1056983004/diff/1/Source/core/page/EventHandl...
Source/core/page/EventHandler.cpp:1018: m_unusedDelta =
m_accumulatedRootOverscroll = IntPoint();
Scroll deltas should use |FloatSize| and not |IntPoint|

https://codereview.chromium.org/1056983004/diff/1/Source/core/page/EventHandl...
Source/core/page/EventHandler.cpp:2728: scrolled = scrollState->deltaX() !=
gestureEvent.deltaX()
We should also handle the case where scroll customization is enabled.

https://codereview.chromium.org/1056983004/diff/1/Source/core/page/EventHandl...
Source/core/page/EventHandler.cpp:2759: if (m_frame->applyScrollDelta(delta,
false)) {
It is also possible to scroll main frame's frameview here and needs to be
covered.

https://codereview.chromium.org/1056983004/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1056983004/diff/1/Source/web/WebViewImpl.cpp#...
Source/web/WebViewImpl.cpp:740: }
Overscroll should be reported on both gesture and wheel scrolls.

[majidvp, 2015-04-23 18:59:03]

https://codereview.chromium.org/1056983004/diff/1/Source/core/page/EventHandl...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1056983004/diff/1/Source/core/page/EventHandl...
Source/core/page/EventHandler.cpp:2728: scrolled = scrollState->deltaX() !=
gestureEvent.deltaX()
On 2015/04/23 18:25:09, majidvp wrote:
> We should also handle the case where scroll customization is enabled.

Muven, I think you can ignore the Scroll customization case for now given that
it may be a while before it is fully enabled. :)

[jdduke (slow), 2015-04-23 20:51:01]

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

[jdduke (slow), 2015-04-23 20:51:01]

+aelias

[MuVen, 2015-04-27 12:50:18]

Patchset #2 (id:20001) has been deleted

[MuVen, 2015-04-27 12:53:46]

majidvp, PTAL and share your inputs.

https://codereview.chromium.org/1056983004/diff/1/Source/core/layout/LayoutBo...
File Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/1056983004/diff/1/Source/core/layout/LayoutBo...
Source/core/layout/LayoutBox.cpp:760: 
On 2015/04/23 18:25:08, majidvp wrote:
> unused scroll delta is already being calculated in |ScrollableArea::scroll|. 
> It makes sense to re-use that instead of trying to recompute it here.
> On option is to have  |ScrollableArea::scroll| also return
> |ScrollResultOneDimensional| which contains unused delta.

Done.

https://codereview.chromium.org/1056983004/diff/1/Source/core/layout/LayoutBox.h
File Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/1056983004/diff/1/Source/core/layout/LayoutBo...
Source/core/layout/LayoutBox.h:833: IntPoint m_unusedDelta;
On 2015/04/23 18:25:08, majidvp wrote:
> It does not make sense to have this information in LayoutBox. Unused scroll
> delta needs to only be tracked for main frame and then only
> during a single scroll event. I don't think it needs to be a member.

Done.

https://codereview.chromium.org/1056983004/diff/1/Source/core/page/EventHandl...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1056983004/diff/1/Source/core/page/EventHandl...
Source/core/page/EventHandler.cpp:960: *unusedDelta = cbUnusedDelta.x();
On 2015/04/23 18:25:08, majidvp wrote:
> Overscroll should happen only if the document's main frame is scrolled and has
> any scroll delta remaining. This logic overscrolls for all scrolling elements
> which is not correct.
> 
> Also, this only covers the case where root layer scrolls. See my other
comments
> the other two cases where frame view maybe scrolled and thus generate unused
> scroll delta.

if (m_frame->isMainFrame()) condition will accumulate delta only on mainframe.
Share your inputs.

https://codereview.chromium.org/1056983004/diff/1/Source/core/page/EventHandl...
Source/core/page/EventHandler.cpp:1018: m_unusedDelta =
m_accumulatedRootOverscroll = IntPoint();
On 2015/04/23 18:25:08, majidvp wrote:
> Scroll deltas should use |FloatSize| and not |IntPoint|

Done.

https://codereview.chromium.org/1056983004/diff/1/Source/core/page/EventHandl...
Source/core/page/EventHandler.cpp:2728: scrolled = scrollState->deltaX() !=
gestureEvent.deltaX()
On 2015/04/23 18:59:02, majidvp wrote:
> On 2015/04/23 18:25:09, majidvp wrote:
> > We should also handle the case where scroll customization is enabled.
> 
> Muven, I think you can ignore the Scroll customization case for now given that
> it may be a while before it is fully enabled. :)

Done.

[aelias_OOO_until_Jul13, 2015-04-27 22:52:34]

We've planning to get rid of these Blink ScrollableArea codepaths entirely as
part of this design:
https://docs.google.com/document/d/1qbIfgjC5D_dZT5nx9epEV7dSu5jtY4lT8Z0RVWDHY...
so I'm not sure it's worth adding this feature to it at this time.  Is this
urgently needed for anything?

[MuVen, 2015-04-28 05:36:00]

On 2015/04/27 22:52:34, aelias wrote:
> We've planning to get rid of these Blink ScrollableArea codepaths entirely as
> part of this design:
>
https://docs.google.com/document/d/1qbIfgjC5D_dZT5nx9epEV7dSu5jtY4lT8Z0RVWDHY...
> so I'm not sure it's worth adding this feature to it at this time.  Is this
> urgently needed for anything?

majidvp, has mailed me regarding implementation of this issue, as it was pending
on me
since 3 months. May be can we add this feature, ill support any
maintaince/reshuffling, 
down the lane. Share your inputs to proceed further.

[majidvp, 2015-04-28 13:45:54]

On 2015/04/28 05:36:00, MuVen wrote:
> On 2015/04/27 22:52:34, aelias wrote:
> > We've planning to get rid of these Blink ScrollableArea codepaths entirely
as
> > part of this design:
> >
>
https://docs.google.com/document/d/1qbIfgjC5D_dZT5nx9epEV7dSu5jtY4lT8Z0RVWDHY...
> > so I'm not sure it's worth adding this feature to it at this time.  Is this
> > urgently needed for anything?
> 
> majidvp, has mailed me regarding implementation of this issue, as it was
pending
> on me
> since 3 months. May be can we add this feature, ill support any
> maintaince/reshuffling, 
> down the lane. Share your inputs to proceed further.

The main drive for having main-thread overscroll was to be able to launch
'scroll-blocks-on'. So it all depends on the fact that if that is going to
launch soon or we can have it block on scroll unification work.

rbyers@ what do you think?

[majidvp, 2015-04-28 13:46:18]

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

[Rick Byers, 2015-04-29 18:20:16]

rbyers@chromium.org changed reviewers:
- rbyers@chromium.org

[Rick Byers, 2015-04-29 18:24:24]

On 2015/04/28 13:45:54, majidvp wrote:
> On 2015/04/28 05:36:00, MuVen wrote:
> > On 2015/04/27 22:52:34, aelias wrote:
> > > We've planning to get rid of these Blink ScrollableArea codepaths entirely
> as
> > > part of this design:
> > >
> >
>
https://docs.google.com/document/d/1qbIfgjC5D_dZT5nx9epEV7dSu5jtY4lT8Z0RVWDHY...
> > > so I'm not sure it's worth adding this feature to it at this time.  Is
this
> > > urgently needed for anything?
> > 
> > majidvp, has mailed me regarding implementation of this issue, as it was
> pending
> > on me
> > since 3 months. May be can we add this feature, ill support any
> > maintaince/reshuffling, 
> > down the lane. Share your inputs to proceed further.
> 
> The main drive for having main-thread overscroll was to be able to launch
> 'scroll-blocks-on'. So it all depends on the fact that if that is going to
> launch soon or we can have it block on scroll unification work.
> 
> rbyers@ what do you think?

Alex, we probably can't ship main-thread scrolling without overscroll glow,
right?  How long do you expect scroll unification to take?  Getting consensus on
the API and jank mitigation for shipping main thread scrolling will likely
continue to take awhile, so it's not imminent or anything (hopefully Q3 some
time).  If we think it'll save time, I'm OK delaying this overscroll glow work
until at least Q3 when scroll unification is further along if think that's best.

[aelias_OOO_until_Jul13, 2015-05-01 04:45:04]

OK, we can get in main-thread overscroll support earlier.  I wasn't aware we had
requested that sataya work on this.

As a first comment, please write a meaningful CL description that also links the
Chromium-side patch.

https://codereview.chromium.org/1056983004/diff/40001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1056983004/diff/40001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:734:
m_client->didOverScrollOnMainThread(unusedDelta.x(), unusedDelta.y(),
accumaltedRootOverScroll.x(), accumaltedRootOverScroll.y(),
platformEvent.position().x(), platformEvent.position().y(),
platformEvent.velocityX(), platformEvent.velocityY());
Can this be called directly from handleGestureScrollUpdate, so that the logic
isn't split across two gesture handling methods and shared implicitly using
member variables?  (It's fine if it needs to pass through a new WebViewImpl
plumbing method to do that.)

[MuVen, 2015-05-04 08:44:12]

Patchset #3 (id:60001) has been deleted

[MuVen, 2015-05-04 08:47:22]

Patchset #3 (id:80001) has been deleted

[MuVen, 2015-05-04 09:04:18]

PTAL.

https://codereview.chromium.org/1056983004/diff/40001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/1056983004/diff/40001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:734:
m_client->didOverScrollOnMainThread(unusedDelta.x(), unusedDelta.y(),
accumaltedRootOverScroll.x(), accumaltedRootOverScroll.y(),
platformEvent.position().x(), platformEvent.position().y(),
platformEvent.velocityX(), platformEvent.velocityY());
On 2015/05/01 04:45:04, aelias wrote:
> Can this be called directly from handleGestureScrollUpdate, so that the logic
> isn't split across two gesture handling methods and shared implicitly using
> member variables?  (It's fine if it needs to pass through a new WebViewImpl
> plumbing method to do that.)

Done.

[MuVen, 2015-05-05 15:01:18]

On 2015/05/04 09:04:18, MuVen wrote:
> PTAL.
> 
>
https://codereview.chromium.org/1056983004/diff/40001/Source/web/WebViewImpl.cpp
> File Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/1056983004/diff/40001/Source/web/WebViewImpl....
> Source/web/WebViewImpl.cpp:734:
> m_client->didOverScrollOnMainThread(unusedDelta.x(), unusedDelta.y(),
> accumaltedRootOverScroll.x(), accumaltedRootOverScroll.y(),
> platformEvent.position().x(), platformEvent.position().y(),
> platformEvent.velocityX(), platformEvent.velocityY());
> On 2015/05/01 04:45:04, aelias wrote:
> > Can this be called directly from handleGestureScrollUpdate, so that the
logic
> > isn't split across two gesture handling methods and shared implicitly using
> > member variables?  (It's fine if it needs to pass through a new WebViewImpl
> > plumbing method to do that.)
> 
> Done.

friendly ping :)

[majidvp, 2015-05-05 15:29:14]

My major comment is to add tests. 
Some minor nits about naming and type usage.

https://codereview.chromium.org/1056983004/diff/100001/Source/core/page/Chrom...
File Source/core/page/ChromeClient.h (right):

https://codereview.chromium.org/1056983004/diff/100001/Source/core/page/Chrom...
Source/core/page/ChromeClient.h:103: virtual void
didOverScrollOnMainThread(FloatPoint&, FloatPoint&, FloatPoint&, FloatPoint&) =
0;
Scroll delta should use FloatSize and not FloatPoint. FloatPoint should be used
only for points.  Same for velocity.

https://codereview.chromium.org/1056983004/diff/100001/Source/web/ChromeClien...
File Source/web/ChromeClientImpl.h (right):

https://codereview.chromium.org/1056983004/diff/100001/Source/web/ChromeClien...
Source/web/ChromeClientImpl.h:84: virtual void
didOverScrollOnMainThread(FloatPoint&, FloatPoint&, FloatPoint&, FloatPoint&)
override;
Please use 'Overscroll' for consistency with CC code.
Please remove OnMainThread as it does not add any information here. Main thread
is implied.

https://codereview.chromium.org/1056983004/diff/100001/public/web/WebWidgetCl...
File public/web/WebWidgetClient.h (right):

https://codereview.chromium.org/1056983004/diff/100001/public/web/WebWidgetCl...
public/web/WebWidgetClient.h:156: virtual void didOverScrollOnMainThread(const
float& unusedDeltaX, const float& unusedDeltaY, const float&
accumaltedRootOverScrollX, const float& accumaltedRootOverScrollY, const float&
positionX, const float& positionY, const float& velocityX, const float&
velocityY) { }
s/OverScroll/Overscroll/
drop OnMainThread

Also use appropriate types instead of plain floats i.e., WebFloatSize for deltas
and velocity and WebFloatPoint for points.

[MuVen, 2015-05-06 09:31:19]

Patchset #4 (id:120001) has been deleted

[MuVen, 2015-05-06 19:01:22]

Patchset #5 (id:150001) has been deleted

[MuVen, 2015-05-06 19:02:47]

PTAL. Added tests !!!

https://codereview.chromium.org/1056983004/diff/100001/Source/core/page/Chrom...
File Source/core/page/ChromeClient.h (right):

https://codereview.chromium.org/1056983004/diff/100001/Source/core/page/Chrom...
Source/core/page/ChromeClient.h:103: virtual void
didOverScrollOnMainThread(FloatPoint&, FloatPoint&, FloatPoint&, FloatPoint&) =
0;
On 2015/05/05 15:29:14, majidvp wrote:
> Scroll delta should use FloatSize and not FloatPoint. FloatPoint should be
used
> only for points.  Same for velocity.

Done.

https://codereview.chromium.org/1056983004/diff/100001/public/web/WebWidgetCl...
File public/web/WebWidgetClient.h (right):

https://codereview.chromium.org/1056983004/diff/100001/public/web/WebWidgetCl...
public/web/WebWidgetClient.h:156: virtual void didOverScrollOnMainThread(const
float& unusedDeltaX, const float& unusedDeltaY, const float&
accumaltedRootOverScrollX, const float& accumaltedRootOverScrollY, const float&
positionX, const float& positionY, const float& velocityX, const float&
velocityY) { }
On 2015/05/05 15:29:14, majidvp wrote:
> s/OverScroll/Overscroll/
> drop OnMainThread
> 
> Also use appropriate types instead of plain floats i.e., WebFloatSize for
deltas
> and velocity and WebFloatPoint for points.

Done.

[MuVen, 2015-05-07 14:49:03]

Patchset #5 (id:170001) has been deleted

[majidvp, 2015-05-07 16:35:30]

The overscroll logic is incorrect and does not match CC. In particular
overscroll should only accumulate for main frames root layer and should be reset
anytime we can scroll again.

I have more detailed comments inline on the logic, tests and some additional
nits as well.

https://codereview.chromium.org/1056983004/diff/190001/Source/core/page/Chrom...
File Source/core/page/ChromeClient.h (right):

https://codereview.chromium.org/1056983004/diff/190001/Source/core/page/Chrom...
Source/core/page/ChromeClient.h:103: virtual void didOverscroll(FloatSize&,
FloatSize&, FloatPoint&, FloatSize&) = 0;
Please use const& here.

https://codereview.chromium.org/1056983004/diff/190001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1056983004/diff/190001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:963: return result;
This no longer guarantee that we return 'true' here.
Note that when shouldStopBubbling is set we should always return true to avoid
scroll event to bubble up to next frame.

Please update to match the existing logic.

https://codereview.chromium.org/1056983004/diff/190001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2719:
m_unusedDelta.setWidth(result.unusedScrollDelta);
Compositor overscroll logic only collects overscroll when the root layer on of
the mainframe has unused delta. This logic collects unused delta from any scroll
even if it is not root layer that scrolls.

You should match CC behaviour. See here:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

Perhaps EventHandler::scroll shouldn't return any unused scroll delta by only
unused _root_ scroll delta. This makes it more consistent with CC
implementation.

Please make sure this behaviour is captured in the test.

https://codereview.chromium.org/1056983004/diff/190001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2731: if (m_frame->isMainFrame())
mainframe check here is redundant. For non mainframe the unused delta is 0.

https://codereview.chromium.org/1056983004/diff/190001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2734: if (m_frame->isMainFrame() &&
m_unusedDelta != FloatSize()) {
Again mainframe check is redundant.

https://codereview.chromium.org/1056983004/diff/190001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2764: m_accumulatedRootOverscroll =
FloatSize();
This clears accumulated root overscroll only at the end of scroll gesture which
is not enough.

The overscroll amount should also get reset every time we can successfully
scroll which could happen during the same gesture.

See:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

Please make sure this is captured in the test.

https://codereview.chromium.org/1056983004/diff/190001/Source/web/ChromeClien...
File Source/web/ChromeClientImpl.h (right):

https://codereview.chromium.org/1056983004/diff/190001/Source/web/ChromeClien...
Source/web/ChromeClientImpl.h:84: virtual void didOverscroll(FloatSize&,
FloatSize&, FloatPoint&, FloatSize&) override;
should use const&

https://codereview.chromium.org/1056983004/diff/190001/Source/web/tests/WebFr...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1056983004/diff/190001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7294: 
Why do you need this resize here?

https://codereview.chromium.org/1056983004/diff/190001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7297: 
Again resize does not make sense to me.

https://codereview.chromium.org/1056983004/diff/190001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7301:
webViewHelper.webViewImpl()->mainFrameImpl()->frame()->eventHandler().handleGestureEvent(gestureEventSB);
I think it is better to have webView handle the gesture instead of passing it
directly to main frame event handler. That allows you to test scrolls that
happen inside iframes as well.

See |TopControlsTest.cpp| for example of how you can do that.

https://codereview.chromium.org/1056983004/diff/190001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7321: EXPECT_EQ(-26,
accumulatedRootOverscroll.height());
This tests the simple case where we have overscroll. It does not test any of the
following:
- accumulated over scroll reset logic
   - within a single scroll gesture
   - between different scroll gesture

- Scroll delta being correctly calculated for both axis
- Scrolling inner scrollers
- Scrolling iframes
- Impact of scale on overscroll delta

In general you should try to match any of the overscroll related tests in CC
that makes sense here. Search 'overscroll' in |layer_tree_host_impl_unittest.cc|
to see them.

Also I think it is best to move all overscroll tests into their own Test class.

https://codereview.chromium.org/1056983004/diff/190001/Source/web/tests/data/...
File Source/web/tests/data/mainthread_scrolling.html (right):

https://codereview.chromium.org/1056983004/diff/190001/Source/web/tests/data/...
Source/web/tests/data/mainthread_scrolling.html:7: bottom: 0px;
This is incorrect. We should only get overscroll on body scrolling and not inner
scrollers.

[MuVen, 2015-05-13 16:49:16]

Patchset #8 (id:250001) has been deleted

[MuVen, 2015-05-13 16:55:46]

@majidvp, 

Modified major review comments. PTAL.

https://codereview.chromium.org/1056983004/diff/190001/Source/core/page/Chrom...
File Source/core/page/ChromeClient.h (right):

https://codereview.chromium.org/1056983004/diff/190001/Source/core/page/Chrom...
Source/core/page/ChromeClient.h:103: virtual void didOverscroll(FloatSize&,
FloatSize&, FloatPoint&, FloatSize&) = 0;
On 2015/05/07 16:35:29, majidvp wrote:
> Please use const& here.

Done.

https://codereview.chromium.org/1056983004/diff/190001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1056983004/diff/190001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:963: return result;
On 2015/05/07 16:35:30, majidvp wrote:
> This no longer guarantee that we return 'true' here.
> Note that when shouldStopBubbling is set we should always return true to avoid
> scroll event to bubble up to next frame.
> 
> Please update to match the existing logic.

Done.

https://codereview.chromium.org/1056983004/diff/190001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2719:
m_unusedDelta.setWidth(result.unusedScrollDelta);
On 2015/05/07 16:35:29, majidvp wrote:
> Compositor overscroll logic only collects overscroll when the root layer on of
> the mainframe has unused delta. This logic collects unused delta from any
scroll
> even if it is not root layer that scrolls.
> 
> You should match CC behaviour. See here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...
> 
> Perhaps EventHandler::scroll shouldn't return any unused scroll delta by only
> unused _root_ scroll delta. This makes it more consistent with CC
> implementation.
> 
> Please make sure this behaviour is captured in the test.

Done.

https://codereview.chromium.org/1056983004/diff/190001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2731: if (m_frame->isMainFrame())
On 2015/05/07 16:35:30, majidvp wrote:
> mainframe check here is redundant. For non mainframe the unused delta is 0.

Done.

https://codereview.chromium.org/1056983004/diff/190001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2734: if (m_frame->isMainFrame() &&
m_unusedDelta != FloatSize()) {
On 2015/05/07 16:35:30, majidvp wrote:
> Again mainframe check is redundant.

Done.

https://codereview.chromium.org/1056983004/diff/190001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2764: m_accumulatedRootOverscroll =
FloatSize();
On 2015/05/07 16:35:30, majidvp wrote:
> This clears accumulated root overscroll only at the end of scroll gesture
which
> is not enough.
> 
> The overscroll amount should also get reset every time we can successfully
> scroll which could happen during the same gesture.
> 
> See:
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...
> 
> Please make sure this is captured in the test.

Done.

https://codereview.chromium.org/1056983004/diff/190001/Source/web/ChromeClien...
File Source/web/ChromeClientImpl.h (right):

https://codereview.chromium.org/1056983004/diff/190001/Source/web/ChromeClien...
Source/web/ChromeClientImpl.h:84: virtual void didOverscroll(FloatSize&,
FloatSize&, FloatPoint&, FloatSize&) override;
On 2015/05/07 16:35:30, majidvp wrote:
> should use const&

Done.

[MuVen, 2015-05-14 12:34:21]

majidvp,

i have missed one point in applyScrollDelta API, i have modified the 
logic and tests. PTAL.

Thanks,

https://codereview.chromium.org/1056983004/diff/310001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1056983004/diff/310001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2746:
m_unusedDelta.setWidth(resultScrollDelta.unusedScrollDeltaX);
modified the previous logic,  
m_unusedDelta.setWidth(-resultScrollDelta.unusedScrollDeltaX);

As i missed to modify the code at applyScrollDelta API. The logic
is captured in the tests.

[majidvp, 2015-05-14 21:08:27]

On 2015/05/14 12:34:21, MuVen wrote:
> majidvp,
> 
> i have missed one point in applyScrollDelta API, i have modified the 
> logic and tests. PTAL.
> 
> Thanks,
> 
>
https://codereview.chromium.org/1056983004/diff/310001/Source/core/page/Event...
> File Source/core/page/EventHandler.cpp (right):
> 
>
https://codereview.chromium.org/1056983004/diff/310001/Source/core/page/Event...
> Source/core/page/EventHandler.cpp:2746:
> m_unusedDelta.setWidth(resultScrollDelta.unusedScrollDeltaX);
> modified the previous logic,  
> m_unusedDelta.setWidth(-resultScrollDelta.unusedScrollDeltaX);
> 
> As i missed to modify the code at applyScrollDelta API. The logic
> is captured in the tests.

The logic looks correct now with some minor nits.
The tests are still incomplete. See my original comments about this: 
https://codereview.chromium.org/1056983004/diff/190001/Source/web/tests/WebFr...

[majidvp, 2015-05-14 21:08:39]

https://codereview.chromium.org/1056983004/diff/310001/Source/core/frame/Loca...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1056983004/diff/310001/Source/core/frame/Loca...
Source/core/frame/LocalFrame.cpp:728: // Returns true if a scroll occurred.
Please update the comment to reflect change.

https://codereview.chromium.org/1056983004/diff/310001/Source/core/frame/Loca...
Source/core/frame/LocalFrame.cpp:753: return ScrollResult(remainingDelta !=
delta, remainingDelta.width(), remainingDelta.height());
This is more readable and avoids an extra construction:

result.didScroll = remainingDelta != delta;
return result;

https://codereview.chromium.org/1056983004/diff/310001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1056983004/diff/310001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:943: return ScrollResultOneDimensional(false);
Shouldn't this return the delta?

https://codereview.chromium.org/1056983004/diff/310001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:960: if (!result.didScroll)
The if statement is redundant.

https://codereview.chromium.org/1056983004/diff/310001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:968: return result;
Again, I feel this should return {false, delta} instead of result which is
{false, 0}.

I think moving result declaration inside the loop and returning
|ScrollResult(false, delta)| here makes the logic more clear.

https://codereview.chromium.org/1056983004/diff/310001/Source/web/tests/WebFr...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1056983004/diff/310001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:6252:
webViewImpl->settings()->setThreadedScrollingEnabled(false);
Do you really need this?

[MuVen, 2015-05-26 09:58:35]

majidvp,

sry for the delay. PTAL. addressed nits and review comments.
Let me know if few more test cases needed to be added.

Thanks,

https://codereview.chromium.org/1056983004/diff/310001/Source/core/frame/Loca...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1056983004/diff/310001/Source/core/frame/Loca...
Source/core/frame/LocalFrame.cpp:728: // Returns true if a scroll occurred.
On 2015/05/14 21:08:39, majidvp wrote:
> Please update the comment to reflect change.

Done.

https://codereview.chromium.org/1056983004/diff/310001/Source/core/frame/Loca...
Source/core/frame/LocalFrame.cpp:753: return ScrollResult(remainingDelta !=
delta, remainingDelta.width(), remainingDelta.height());
On 2015/05/14 21:08:39, majidvp wrote:
> This is more readable and avoids an extra construction:
> 
> result.didScroll = remainingDelta != delta;
> return result;

Done.

https://codereview.chromium.org/1056983004/diff/310001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1056983004/diff/310001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:943: return ScrollResultOneDimensional(false);
On 2015/05/14 21:08:39, majidvp wrote:
> Shouldn't this return the delta?

Done.

https://codereview.chromium.org/1056983004/diff/310001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:960: if (!result.didScroll)
On 2015/05/14 21:08:39, majidvp wrote:
> The if statement is redundant.

Done.

https://codereview.chromium.org/1056983004/diff/310001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:968: return result;
On 2015/05/14 21:08:39, majidvp wrote:
> Again, I feel this should return {false, delta} instead of result which is
> {false, 0}.
> 
> I think moving result declaration inside the loop and returning
> |ScrollResult(false, delta)| here makes the logic more clear.
> 

Done.

https://codereview.chromium.org/1056983004/diff/310001/Source/web/tests/WebFr...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1056983004/diff/310001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:6252:
webViewImpl->settings()->setThreadedScrollingEnabled(false);
On 2015/05/14 21:08:39, majidvp wrote:
> Do you really need this?

Not requried.

[majidvp, 2015-05-27 14:30:08]

On 2015/05/26 09:58:35, MuVen wrote:
> majidvp,
> 
> sry for the delay. PTAL. addressed nits and review comments.
> Let me know if few more test cases needed to be added.
> 
> Thanks,
> 
>
https://codereview.chromium.org/1056983004/diff/310001/Source/core/frame/Loca...
> File Source/core/frame/LocalFrame.cpp (right):
> 
>
https://codereview.chromium.org/1056983004/diff/310001/Source/core/frame/Loca...
> Source/core/frame/LocalFrame.cpp:728: // Returns true if a scroll occurred.
> On 2015/05/14 21:08:39, majidvp wrote:
> > Please update the comment to reflect change.
> 
> Done.
> 
>
https://codereview.chromium.org/1056983004/diff/310001/Source/core/frame/Loca...
> Source/core/frame/LocalFrame.cpp:753: return ScrollResult(remainingDelta !=
> delta, remainingDelta.width(), remainingDelta.height());
> On 2015/05/14 21:08:39, majidvp wrote:
> > This is more readable and avoids an extra construction:
> > 
> > result.didScroll = remainingDelta != delta;
> > return result;
> 
> Done.
> 
>
https://codereview.chromium.org/1056983004/diff/310001/Source/core/page/Event...
> File Source/core/page/EventHandler.cpp (right):
> 
>
https://codereview.chromium.org/1056983004/diff/310001/Source/core/page/Event...
> Source/core/page/EventHandler.cpp:943: return
ScrollResultOneDimensional(false);
> On 2015/05/14 21:08:39, majidvp wrote:
> > Shouldn't this return the delta?
> 
> Done.
> 
>
https://codereview.chromium.org/1056983004/diff/310001/Source/core/page/Event...
> Source/core/page/EventHandler.cpp:960: if (!result.didScroll)
> On 2015/05/14 21:08:39, majidvp wrote:
> > The if statement is redundant.
> 
> Done.
> 
>
https://codereview.chromium.org/1056983004/diff/310001/Source/core/page/Event...
> Source/core/page/EventHandler.cpp:968: return result;
> On 2015/05/14 21:08:39, majidvp wrote:
> > Again, I feel this should return {false, delta} instead of result which is
> > {false, 0}.
> > 
> > I think moving result declaration inside the loop and returning
> > |ScrollResult(false, delta)| here makes the logic more clear.
> > 
> 
> Done.
> 
>
https://codereview.chromium.org/1056983004/diff/310001/Source/web/tests/WebFr...
> File Source/web/tests/WebFrameTest.cpp (right):
> 
>
https://codereview.chromium.org/1056983004/diff/310001/Source/web/tests/WebFr...
> Source/web/tests/WebFrameTest.cpp:6252:
> webViewImpl->settings()->setThreadedScrollingEnabled(false);
> On 2015/05/14 21:08:39, majidvp wrote:
> > Do you really need this?
> 
> Not requried.

The changes look good but I think it needs few more test cases: 

- accumulated over scroll reset logic within a single scroll gesture (currently
only test end of gesture) 
- Overscroll should not be reported when main frame is not scrollable
- Scrolling inner scrollers should not report overscroll
- Scrolling iframe to its limit (without it bubbling) should not cause
overscroll
- Overscroll delta should not be impacted by page scale

[majidvp, 2015-05-27 14:37:23]

https://codereview.chromium.org/1056983004/diff/350001/Source/core/frame/Loca...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1056983004/diff/350001/Source/core/frame/Loca...
Source/core/frame/LocalFrame.cpp:728: // Returns ScrollResult.
nit: this comment no longer conveys any useful information. Please remove.

https://codereview.chromium.org/1056983004/diff/350001/Source/web/tests/data/...
File Source/web/tests/data/mainthread-inner-iframe-overscroll.html (right):

https://codereview.chromium.org/1056983004/diff/350001/Source/web/tests/data/...
Source/web/tests/data/mainthread-inner-iframe-overscroll.html:4: <iframe/>
The iframe is not scrollable. The test should try to scroll iframe to its limit
and ensure that (without bubbling) the main frame does not report overscroll.

Also please drop 'mainthread' from test filenames. It is not needed. If it makes
it easier you can group them in a 'overscroll' directory.

[MuVen, 2015-05-28 13:18:48]

Patchset #14 (id:390001) has been deleted

[MuVen, 2015-05-28 14:45:52]

Hi Majid,

- accumulated over scroll reset logic within a single scroll gesture (currently
only test end of gesture) => Covered in 
"AccumulatedRootOverscrollAndUnsedDeltaSetAndUnset".

- Scrolling inner scrollers should not report overscroll => Covered in 
"RootLayerOverscrolledOnInnerDivOverScroll".

- Scrolling iframe to its limit (without it bubbling) should not cause
overscroll => Covered in
"RootLayerOverscrolledOnInnerIFrameOverScroll".

- Overscroll delta should not be impacted by page scale
=> Covered in
"ScaledPageRootLayerOverscrolled".

- Overscroll should not be reported when main frame is not scrollable.
=> For this i tried to check scrollSize in the frameview. If ScrollSize
in the orientation is 0 then dont report overscroll.But its not working.
Any inputs on this ?

Please check the tests and let me know if any changes needs to be done.

Thanks,

[majidvp, 2015-05-28 20:30:46]

> - Overscroll should not be reported when main frame is not scrollable.
> => For this i tried to check scrollSize in the frameview. > If ScrollSize
> in the orientation is 0 then dont report overscroll.But its not working.
> Any inputs on this ?

I believe FramView::isScrollable can be used here.

https://codereview.chromium.org/1056983004/diff/410001/Source/web/tests/WebFr...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1056983004/diff/410001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7409:
CompareUnusedDeltaAndAccumulatedRootOverscroll(&webViewHelper, 20, 13, 20, 26);
To test reset logic in middle of a single gesture try scrolling the page down
here (e.g. |ScrollUpdate(&webViewHelper, -20, 13)| )

The expectation is for the page to scroll vertically but overscroll
horizontally. This means that the vertical overscroll amount should reset but
not the horizontal one.

The horizontal reset can also be tested similarly.

[MuVen, 2015-05-29 09:37:16]

FramView::isScrollable Api is returning false, due to which not able to
report overscroll on scrollable axes.can i take on this bug separately?

https://codereview.chromium.org/1056983004/diff/350001/Source/core/frame/Loca...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1056983004/diff/350001/Source/core/frame/Loca...
Source/core/frame/LocalFrame.cpp:728: // Returns ScrollResult.
On 2015/05/27 14:37:23, majidvp wrote:
> nit: this comment no longer conveys any useful information. Please remove.

Done.

https://codereview.chromium.org/1056983004/diff/350001/Source/web/tests/data/...
File Source/web/tests/data/mainthread-inner-iframe-overscroll.html (right):

https://codereview.chromium.org/1056983004/diff/350001/Source/web/tests/data/...
Source/web/tests/data/mainthread-inner-iframe-overscroll.html:4: <iframe/>
On 2015/05/27 14:37:23, majidvp wrote:
> The iframe is not scrollable. The test should try to scroll iframe to its
limit
> and ensure that (without bubbling) the main frame does not report overscroll.
> 
> Also please drop 'mainthread' from test filenames. It is not needed. If it
makes
> it easier you can group them in a 'overscroll' directory.

https://codereview.chromium.org/1056983004/diff/410001/Source/web/tests/WebFr...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1056983004/diff/410001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7409:
CompareUnusedDeltaAndAccumulatedRootOverscroll(&webViewHelper, 20, 13, 20, 26);
On 2015/05/28 20:30:46, majidvp wrote:
> To test reset logic in middle of a single gesture try scrolling the page down
> here (e.g. |ScrollUpdate(&webViewHelper, -20, 13)| )
> 
> The expectation is for the page to scroll vertically but overscroll
> horizontally. This means that the vertical overscroll amount should reset but
> not the horizontal one.
> 
> The horizontal reset can also be tested similarly.

Done.

[majidvp, 2015-05-29 19:14:19]

Muven, I am fine with non-scrollable frame issue to be fixed in a separate patch
if that is easier.

However, the current rest logic still has a bug which is maked by a broken test.
Once you fix that I think this patch is good to go.

https://codereview.chromium.org/1056983004/diff/450001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1056983004/diff/450001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2746: ScrollResult resultScrollDelta =
m_frame->applyScrollDelta(delta, false);
s/resultScrollDelta/scrollResult/

https://codereview.chromium.org/1056983004/diff/450001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2746: ScrollResult resultScrollDelta =
m_frame->applyScrollDelta(delta, false);
I think you need to have the rest logic for overscroll values here as well in
case the frame scrolls. Right now the rest logic works at mid-gesture only if an
inner node scrolls.

Once you fix the test you should notice that the reset logic is necessary here.

I believe you need to update ScrollResult to have separate didScrollX and
didScrollY to be able to independently reset horizontal and vertical
overscrolls. It should be possible to add a |ScrollResutl::didScroll()| member
function to make the change easier for its existing clients.

https://codereview.chromium.org/1056983004/diff/450001/Source/web/tests/WebFr...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1056983004/diff/450001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7378: EXPECT_FLOAT_EQ(unUsedWidth,
unUsedDelta.width());
Having the assertions in this function which is called from multiple places
makes the failure line number useless because they all point to this line
number. 

I think it is best to use inline assertions like other tests. You can use
EXPECT_FLOAT_POINT_EQ with helper function that return unused and accumulated
values as float points to cut the duplication.

https://codereview.chromium.org/1056983004/diff/450001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7412:
CompareUnusedDeltaAndAccumulatedRootOverscroll(&webViewHelper, 0, -26, 20, 0);
Why is the unused value not 0? If the frame is being scrolled vertically both
the unused and accumulated values should be 0 at the same time.

This looks incorrect to me. I think accumulated value is 0 here because you are
scrolling an amount equal to accumulated value in reverse. The correct reset
logic should reset the accumulated value regardless of how much it is being
scrolled.

https://codereview.chromium.org/1056983004/diff/450001/Source/web/tests/data/...
File Source/web/tests/data/overscroll/overscroll.html (right):

https://codereview.chromium.org/1056983004/diff/450001/Source/web/tests/data/...
Source/web/tests/data/overscroll/overscroll.html:3: <body>
The fact the this document is not even scrollable tells me that indeed the test
is not actually testing the reset logic.

[MuVen, 2015-06-01 13:24:05]

Patchset #17 (id:470001) has been deleted

[MuVen, 2015-06-01 13:29:26]

Hi Majid,

PTAL. I have addressed all the review comments, and modified tests.

Thanks

https://codereview.chromium.org/1056983004/diff/450001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1056983004/diff/450001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2746: ScrollResult resultScrollDelta =
m_frame->applyScrollDelta(delta, false);
On 2015/05/29 19:14:19, majidvp wrote:
> I think you need to have the rest logic for overscroll values here as well in
> case the frame scrolls. Right now the rest logic works at mid-gesture only if
an
> inner node scrolls.
> 
> Once you fix the test you should notice that the reset logic is necessary
here.
> 
> I believe you need to update ScrollResult to have separate didScrollX and
> didScrollY to be able to independently reset horizontal and vertical
> overscrolls. It should be possible to add a |ScrollResutl::didScroll()| member
> function to make the change easier for its existing clients.

Done.

https://codereview.chromium.org/1056983004/diff/450001/Source/web/tests/WebFr...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1056983004/diff/450001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7378: EXPECT_FLOAT_EQ(unUsedWidth,
unUsedDelta.width());
On 2015/05/29 19:14:19, majidvp wrote:
> Having the assertions in this function which is called from multiple places
> makes the failure line number useless because they all point to this line
> number. 
> 
> I think it is best to use inline assertions like other tests. You can use
> EXPECT_FLOAT_POINT_EQ with helper function that return unused and accumulated
> values as float points to cut the duplication.

Done.

https://codereview.chromium.org/1056983004/diff/450001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7412:
CompareUnusedDeltaAndAccumulatedRootOverscroll(&webViewHelper, 0, -26, 20, 0);
the current reset logic fixes the issue.

https://codereview.chromium.org/1056983004/diff/450001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7412:
CompareUnusedDeltaAndAccumulatedRootOverscroll(&webViewHelper, 0, -26, 20, 0);
current reset logic fixes this.

https://codereview.chromium.org/1056983004/diff/450001/Source/web/tests/data/...
File Source/web/tests/data/overscroll/overscroll.html (right):

https://codereview.chromium.org/1056983004/diff/450001/Source/web/tests/data/...
Source/web/tests/data/overscroll/overscroll.html:3: <body>
Current reset logic fixes this. I think this needs modification once 
no overflow on nonscrollable axes patch landes. I shall modify this
along that patch.

[majidvp, 2015-06-01 15:58:29]

https://codereview.chromium.org/1056983004/diff/450001/Source/web/tests/WebFr...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1056983004/diff/450001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7378: EXPECT_FLOAT_EQ(unUsedWidth,
unUsedDelta.width());
On 2015/06/01 13:29:26, MuVen wrote:
> On 2015/05/29 19:14:19, majidvp wrote:
> > Having the assertions in this function which is called from multiple places
> > makes the failure line number useless because they all point to this line
> > number. 
> > 
> > I think it is best to use inline assertions like other tests. You can use
> > EXPECT_FLOAT_POINT_EQ with helper function that return unused and
accumulated
> > values as float points to cut the duplication.
> 
> Done.

Thanks.

https://codereview.chromium.org/1056983004/diff/450001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7412:
CompareUnusedDeltaAndAccumulatedRootOverscroll(&webViewHelper, 0, -26, 20, 0);
On 2015/06/01 13:29:26, MuVen wrote:
> current reset logic fixes this.

Maybe the issue is fixed but this test is not verifying that (see my original
comment again). The correct test should be:

ScrollUpdate(&webViewHelper, 0, 1); // do NOT use 26 as it masks the issue
EXPECT_FLOAT_SIZE_EQ(FloatSize(0, 0), UnusedDelta(&webViewHelper));  // unused
value should be ZERO.

EXPECT_FLOAT_SIZE_EQ(FloatSize(20, 0), AccumulatedRootOverscroll(&webViewHel
      per));

https://codereview.chromium.org/1056983004/diff/450001/Source/web/tests/data/...
File Source/web/tests/data/overscroll/overscroll.html (right):

https://codereview.chromium.org/1056983004/diff/450001/Source/web/tests/data/...
Source/web/tests/data/overscroll/overscroll.html:3: <body>
On 2015/06/01 13:29:26, MuVen wrote:
> Current reset logic fixes this. I think this needs modification once 
> no overflow on nonscrollable axes patch landes. I shall modify this
> along that patch.

I don't think you can test the reset logic with this not being scrollable.

https://codereview.chromium.org/1056983004/diff/490001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1056983004/diff/490001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2759: 
Perhaps this can be factored into a small utility function to avoid repeating
it:
 
resetOverscroll(didScrollX, didScrollY)

https://codereview.chromium.org/1056983004/diff/490001/Source/platform/scroll...
File Source/platform/scroll/ScrollTypes.h (right):

https://codereview.chromium.org/1056983004/diff/490001/Source/platform/scroll...
Source/platform/scroll/ScrollTypes.h:196: explicit ScrollResult(bool didScroll)
The 'true' value for didScroll is ambiguous in terms of didScrollX and
didScrollY but 'false' is not. So I think this constructor should be replaced
with a default constructor that takes not argument and initializes didScrollX
and didScrollY both to false.

All call sites for this constructor pass false so they can be replaced easily.

https://codereview.chromium.org/1056983004/diff/490001/Source/platform/scroll...
Source/platform/scroll/ScrollTypes.h:200: ScrollResult(bool didScroll, float
unusedScrollDeltaX, float unusedScrollDeltaY)
Please get rid of this constructor.

https://codereview.chromium.org/1056983004/diff/490001/Source/platform/scroll...
Source/platform/scroll/ScrollTypes.h:210: bool didScroll;
We should not keep didScroll around anymore as it can lead to the object being
in an inconsistent internal state. Please get rid of it and replace with an
inline:

bool didScroll() { return didScrollX || didScrollY; }

https://codereview.chromium.org/1056983004/diff/490001/Source/web/tests/WebFr...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1056983004/diff/490001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7457: ScrollUpdate(&webViewHelper, 0, 50);
Should use a value smaller than 50 otherwise the zero may be due to |-50+50|
instead of the reset logic.

[MuVen, 2015-06-02 08:59:30]

Patchset #18 (id:480029) has been deleted

[MuVen, 2015-06-02 09:04:01]

majidvp, I have modified test case and changes mentioned. 
PTAL.

https://codereview.chromium.org/1056983004/diff/450001/Source/web/tests/WebFr...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1056983004/diff/450001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7412:
CompareUnusedDeltaAndAccumulatedRootOverscroll(&webViewHelper, 0, -26, 20, 0);
On 2015/06/01 15:58:29, majidvp wrote:
> On 2015/06/01 13:29:26, MuVen wrote:
> > current reset logic fixes this.
> 
> Maybe the issue is fixed but this test is not verifying that (see my original
> comment again). The correct test should be:
> 
> ScrollUpdate(&webViewHelper, 0, 1); // do NOT use 26 as it masks the issue
> EXPECT_FLOAT_SIZE_EQ(FloatSize(0, 0), UnusedDelta(&webViewHelper));  // unused
> value should be ZERO.
> 
> EXPECT_FLOAT_SIZE_EQ(FloatSize(20, 0), AccumulatedRootOverscroll(&webViewHel
>       per));

Done.

https://codereview.chromium.org/1056983004/diff/450001/Source/web/tests/data/...
File Source/web/tests/data/overscroll/overscroll.html (right):

https://codereview.chromium.org/1056983004/diff/450001/Source/web/tests/data/...
Source/web/tests/data/overscroll/overscroll.html:3: <body>
On 2015/06/01 15:58:29, majidvp wrote:
> On 2015/06/01 13:29:26, MuVen wrote:
> > Current reset logic fixes this. I think this needs modification once 
> > no overflow on nonscrollable axes patch landes. I shall modify this
> > along that patch.
> 
> I don't think you can test the reset logic with this not being scrollable.

modified, testcase. Done.

https://codereview.chromium.org/1056983004/diff/490001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1056983004/diff/490001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2759: 
On 2015/06/01 15:58:29, majidvp wrote:
> Perhaps this can be factored into a small utility function to avoid repeating
> it:
>  
> resetOverscroll(didScrollX, didScrollY)

Done.

https://codereview.chromium.org/1056983004/diff/490001/Source/platform/scroll...
File Source/platform/scroll/ScrollTypes.h (right):

https://codereview.chromium.org/1056983004/diff/490001/Source/platform/scroll...
Source/platform/scroll/ScrollTypes.h:196: explicit ScrollResult(bool didScroll)
On 2015/06/01 15:58:29, majidvp wrote:
> The 'true' value for didScroll is ambiguous in terms of didScrollX and
> didScrollY but 'false' is not. So I think this constructor should be replaced
> with a default constructor that takes not argument and initializes didScrollX
> and didScrollY both to false.
> 
> All call sites for this constructor pass false so they can be replaced easily.

Done.

https://codereview.chromium.org/1056983004/diff/490001/Source/platform/scroll...
Source/platform/scroll/ScrollTypes.h:200: ScrollResult(bool didScroll, float
unusedScrollDeltaX, float unusedScrollDeltaY)
On 2015/06/01 15:58:29, majidvp wrote:
> Please get rid of this constructor.

Done.

https://codereview.chromium.org/1056983004/diff/490001/Source/platform/scroll...
Source/platform/scroll/ScrollTypes.h:210: bool didScroll;
On 2015/06/01 15:58:29, majidvp wrote:
> We should not keep didScroll around anymore as it can lead to the object being
> in an inconsistent internal state. Please get rid of it and replace with an
> inline:
> 
> bool didScroll() { return didScrollX || didScrollY; }

Done.

https://codereview.chromium.org/1056983004/diff/520001/Source/core/frame/Loca...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1056983004/diff/520001/Source/core/frame/Loca...
Source/core/frame/LocalFrame.cpp:739: return ScrollResult(delta.width(),
delta.height(), 0.0f, 0.0f);
Even this returns didScrollX, didScrollY. ple let me know your input.

https://codereview.chromium.org/1056983004/diff/520001/Source/core/frame/Root...
File Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/1056983004/diff/520001/Source/core/frame/Root...
Source/core/frame/RootFrameViewport.cpp:123: return
ScrollResult(locationDelta.x()-usedLocationDelta.x(),
locationDelta.y()-usedLocationDelta.y(), -unusedLocationDelta.width(),
-unusedLocationDelta.height());
This shall return didScrollX and didScrollY scroll result. fine ?

[majidvp, 2015-06-02 17:00:31]

https://codereview.chromium.org/1056983004/diff/490001/Source/web/tests/WebFr...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1056983004/diff/490001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7457: ScrollUpdate(&webViewHelper, 0, 50);
On 2015/06/01 15:58:29, majidvp wrote:
> Should use a value smaller than 50 otherwise the zero may be due to |-50+50|
> instead of the reset logic.

What about this?

https://codereview.chromium.org/1056983004/diff/520001/Source/core/frame/Loca...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1056983004/diff/520001/Source/core/frame/Loca...
Source/core/frame/LocalFrame.cpp:739: return ScrollResult(delta.width(),
delta.height(), 0.0f, 0.0f);
On 2015/06/02 09:04:00, MuVen wrote:
> Even this returns didScrollX, didScrollY. ple let me know your input.

This looks reasonable.

https://codereview.chromium.org/1056983004/diff/520001/Source/core/frame/Loca...
Source/core/frame/LocalFrame.cpp:744: return result;
I believe this is no longer correct. Consider this situation where the top
control consumes vertical scroll and the view consumes horizontal scroll. The
final result should have didScroll true for both orientation which is not the
case with this logic.

You can get rid of this conditional and instead use the two computations below.

https://codereview.chromium.org/1056983004/diff/520001/Source/core/frame/Loca...
Source/core/frame/LocalFrame.cpp:747: result.didScrollY =
remainingDelta.height() != delta.height();
If you get rid of the conditional above and replace these with

result.didScrollX = result.didScrollX || (remainingDelta.width() !=
delta.width())
...

https://codereview.chromium.org/1056983004/diff/520001/Source/core/frame/Root...
File Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/1056983004/diff/520001/Source/core/frame/Root...
Source/core/frame/RootFrameViewport.cpp:123: return
ScrollResult(locationDelta.x()-usedLocationDelta.x(),
locationDelta.y()-usedLocationDelta.y(), -unusedLocationDelta.width(),
-unusedLocationDelta.height());
On 2015/06/02 09:04:00, MuVen wrote:
> This shall return didScrollX and didScrollY scroll result. fine ?

Yes but I don't think the calculation is incorrect here.
The result should be true if either layoutViewport or visualViewport has
scrolled i.e., 

didScrollX = viewScrollResult.didScrollX || usedLocationDelta.width()

didScrollY = ...

https://codereview.chromium.org/1056983004/diff/520001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1056983004/diff/520001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2658: if (didScrollX) {
nitp: fix indentation

[MuVen, 2015-06-03 05:47:50]

Patchset #20 (id:560001) has been deleted

[MuVen, 2015-06-03 09:32:18]

hi majidvp,

PTAL. all the review comments are addressed.
No overscroll on non scrollable axes is also implemented.
please check the tests for the same.

Thanks,

https://codereview.chromium.org/1056983004/diff/490001/Source/web/tests/WebFr...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1056983004/diff/490001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7457: ScrollUpdate(&webViewHelper, 0, 50);
On 2015/06/02 17:00:31, majidvp wrote:
> On 2015/06/01 15:58:29, majidvp wrote:
> > Should use a value smaller than 50 otherwise the zero may be due to |-50+50|
> > instead of the reset logic.
> 
> What about this?

Done.

https://codereview.chromium.org/1056983004/diff/520001/Source/core/frame/Loca...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1056983004/diff/520001/Source/core/frame/Loca...
Source/core/frame/LocalFrame.cpp:739: return ScrollResult(delta.width(),
delta.height(), 0.0f, 0.0f);
On 2015/06/02 17:00:31, majidvp wrote:
> On 2015/06/02 09:04:00, MuVen wrote:
> > Even this returns didScrollX, didScrollY. ple let me know your input.
> 
> This looks reasonable.

Done.

https://codereview.chromium.org/1056983004/diff/520001/Source/core/frame/Loca...
Source/core/frame/LocalFrame.cpp:744: return result;
On 2015/06/02 17:00:31, majidvp wrote:
> I believe this is no longer correct. Consider this situation where the top
> control consumes vertical scroll and the view consumes horizontal scroll. The
> final result should have didScroll true for both orientation which is not the
> case with this logic.
> 
> You can get rid of this conditional and instead use the two computations
below.
> 

Done.

https://codereview.chromium.org/1056983004/diff/520001/Source/core/frame/Loca...
Source/core/frame/LocalFrame.cpp:747: result.didScrollY =
remainingDelta.height() != delta.height();
On 2015/06/02 17:00:31, majidvp wrote:
> If you get rid of the conditional above and replace these with
> 
> result.didScrollX = result.didScrollX || (remainingDelta.width() !=
> delta.width())
> ...

Done.

https://codereview.chromium.org/1056983004/diff/520001/Source/core/frame/Root...
File Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/1056983004/diff/520001/Source/core/frame/Root...
Source/core/frame/RootFrameViewport.cpp:123: return
ScrollResult(locationDelta.x()-usedLocationDelta.x(),
locationDelta.y()-usedLocationDelta.y(), -unusedLocationDelta.width(),
-unusedLocationDelta.height());
On 2015/06/02 17:00:31, majidvp wrote:
> On 2015/06/02 09:04:00, MuVen wrote:
> > This shall return didScrollX and didScrollY scroll result. fine ?
> 
> Yes but I don't think the calculation is incorrect here.
> The result should be true if either layoutViewport or visualViewport has
> scrolled i.e., 
> 
> didScrollX = viewScrollResult.didScrollX || usedLocationDelta.width()
> 
> didScrollY = ...
> 

Done.

https://codereview.chromium.org/1056983004/diff/520001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1056983004/diff/520001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2658: if (didScrollX) {
On 2015/06/02 17:00:31, majidvp wrote:
> nitp: fix indentation

Done.

https://codereview.chromium.org/1056983004/diff/600001/Source/core/frame/Loca...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1056983004/diff/600001/Source/core/frame/Loca...
Source/core/frame/LocalFrame.cpp:720: return ScrollResult(horizontal.didScroll,
vertical.didScroll, view.scrollSize(HorizontalScrollbar) ?
horizontal.unusedScrollDelta : 0.0f, view.scrollSize(VerticalScrollbar) ?
vertical.unusedScrollDelta : 0.0f);
If scroll size is "0" unusedDelta is set to "0" so that overscroll
is not reported.

[MuVen, 2015-06-03 10:47:03]

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

[majidvp, 2015-06-03 12:09:46]

https://codereview.chromium.org/1056983004/diff/620001/Source/core/frame/Loca...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1056983004/diff/620001/Source/core/frame/Loca...
Source/core/frame/LocalFrame.cpp:720: return ScrollResult(horizontal.didScroll,
vertical.didScroll, view.scrollSize(HorizontalScrollbar) ?
horizontal.unusedScrollDelta : 0.0f, view.scrollSize(VerticalScrollbar) ?
vertical.unusedScrollDelta : 0.0f);
Using scrollSize to detect scrollability is reasonable but I don't think we
should be adjusting (mis-reporting) unused delta here to fix the overscroll
issue elsewhere. I prefer to keep the overscroll related logic local to
EventHandler.

https://codereview.chromium.org/1056983004/diff/620001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1056983004/diff/620001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2749: ScrollResult resultScrollDelta =
m_frame->applyScrollDelta(delta, false);
replace resultScrollDelta with scrollResult.

https://codereview.chromium.org/1056983004/diff/620001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2751:
m_unusedDelta.setHeight(resultScrollDelta.unusedScrollDeltaY);
Instead of mis-reporting unused delta in general I think it is better if we do
not set unusedDelta for overscroll on axis that are not scrollable. Something
like:

// Do not overscroll on axis that is not scrollable 
m_unusedDelta.height = m_frame.scrollSize(Horizontal) ?
scrollResult.unusedScrollDelta : 0;

Make sure to leave a comment like that to explain the reasoning.

BTW CC sometimes (i.e., when elastic overscroll is enabled) reports overscroll
even on non-scrollable axis. However that is not related to this patch so you
can ignore.

[MuVen, 2015-06-03 15:54:39]

majidvp,

PTAL. modified as per review comments.

Thanks,

https://codereview.chromium.org/1056983004/diff/620001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1056983004/diff/620001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2749: ScrollResult resultScrollDelta =
m_frame->applyScrollDelta(delta, false);
On 2015/06/03 12:09:46, majidvp wrote:
> replace resultScrollDelta with scrollResult.

Done.

https://codereview.chromium.org/1056983004/diff/620001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2751:
m_unusedDelta.setHeight(resultScrollDelta.unusedScrollDeltaY);
On 2015/06/03 12:09:46, majidvp wrote:
> Instead of mis-reporting unused delta in general I think it is better if we do
> not set unusedDelta for overscroll on axis that are not scrollable. Something
> like:
> 
> // Do not overscroll on axis that is not scrollable 
> m_unusedDelta.height = m_frame.scrollSize(Horizontal) ?
> scrollResult.unusedScrollDelta : 0;
> 
> Make sure to leave a comment like that to explain the reasoning.
> 
> BTW CC sometimes (i.e., when elastic overscroll is enabled) reports overscroll
> even on non-scrollable axis. However that is not related to this patch so you
> can ignore.
> 
> 

Done.

[majidvp, 2015-06-03 17:36:41]

The logic lgtm with some minor nits around testing.
The patch fails to apply so I cannot see trybot results. I think you need to
rebase.

bokan@ will be able to do owner reviews for core/frame.

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2753:
m_unusedDelta.setWidth(scrollablearea->scrollSize(HorizontalScrollbar) ?
scrollResult.unusedScrollDeltaX : 0.0f);
nitp: blink style frowns upon 0.0f. use 0 instead

https://www.chromium.org/blink/coding-style#TOC-Floating-point-literals

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
File Source/core/page/EventHandler.h (right):

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
Source/core/page/EventHandler.h:133: 
This is minor but I am not a big fan of exposing these "Testing" functions. I
feel a better testing approach would be to
use a mock of WebViewClient similar to how other tests are implemented.

Which is  This is not a major issue and can perhaps be addressed in a follow-up
patch. bokan@ wdyt?

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
Source/core/page/EventHandler.h:386: FloatSize m_unusedDelta;
If we get rid of the *Testing functions we can remove this data member as well.

https://codereview.chromium.org/1056983004/diff/640001/Source/platform/scroll...
File Source/platform/scroll/ScrollAnimator.cpp (right):

https://codereview.chromium.org/1056983004/diff/640001/Source/platform/scroll...
Source/platform/scroll/ScrollAnimator.cpp:57: 
nitp: extra blank line

[bokan, 2015-06-03 19:12:19]

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Chrom...
File Source/core/page/ChromeClient.h (right):

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Chrom...
Source/core/page/ChromeClient.h:107: // Pass on values for overscroll glow
animation on main thread.
This kind of comment is bound to become stale. If a comment is needed here, it
should describe what overscroll is, rather than what downstream code will do
with it.

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2719: } else {
Sorry if I missed it, but has any thought been given to how this will work with
scroll customization?

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2753:
m_unusedDelta.setWidth(scrollablearea->scrollSize(HorizontalScrollbar) ?
scrollResult.unusedScrollDeltaX : 0.0f);
This should be main frame only, right?

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2757: if (m_frame->isMainFrame() &&
m_unusedDelta != FloatSize()) {
Side note, we'll likely also want to hook this up for at least wheel events (Mac
in particular has elastic overscroll on the trackpad) but I'm fine with starting
with gesture scrolls only for now. With that in mind though, this code block
looks like it should be in a private helper method. e.g. 

if(m_frame->isMainFrame())
  handleOverscroll(ScrollResult)

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2772: void
EventHandler::clearGestureScrollNodes()
This method is superfluous, it's private and only called in one place. Please
remove it and move its content to the call site.

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
File Source/core/page/EventHandler.h (right):

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
Source/core/page/EventHandler.h:133: 
On 2015/06/03 17:36:41, majidvp wrote:
> This is minor but I am not a big fan of exposing these "Testing" functions. I
> feel a better testing approach would be to
> use a mock of WebViewClient similar to how other tests are implemented.
> 
> Which is  This is not a major issue and can perhaps be addressed in a
follow-up
> patch. bokan@ wdyt?

Agreed, this is an anti-pattern.

[majidvp, 2015-06-03 21:39:01]

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2719: } else {
On 2015/06/03 19:12:19, bokan wrote:
> Sorry if I missed it, but has any thought been given to how this will work
with
> scroll customization?

A month back after chatting with rbyers@ we decided it is too early to require
patches like this to work with Scroll Customization. See
https://codereview.chromium.org/1056983004/#msg5

To handle scroll customization, I think we can move overscroll logic to
FrameView and invoke it in FrameView::applyScrollDelta. The only bit that will
remain in EventHandler will be the reset logic that needs to be invoked if any
element is scrolled.

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2772: void
EventHandler::clearGestureScrollNodes()
On 2015/06/03 19:12:19, bokan wrote:
> This method is superfluous, it's private and only called in one place. Please
> remove it and move its content to the call site.

IMHO it helps with readability.

[bokan, 2015-06-03 21:41:16]

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2719: } else {
On 2015/06/03 21:39:01, majidvp wrote:
> On 2015/06/03 19:12:19, bokan wrote:
> > Sorry if I missed it, but has any thought been given to how this will work
> with
> > scroll customization?
> 
> A month back after chatting with rbyers@ we decided it is too early to require
> patches like this to work with Scroll Customization. See
> https://codereview.chromium.org/1056983004/#msg5
> 
> To handle scroll customization, I think we can move overscroll logic to
> FrameView and invoke it in FrameView::applyScrollDelta. The only bit that will
> remain in EventHandler will be the reset logic that needs to be invoked if any
> element is scrolled.

Ok, sgtm to punt on this until scroll customization matures a bit.

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2772: void
EventHandler::clearGestureScrollNodes()
On 2015/06/03 21:39:01, majidvp wrote:
> On 2015/06/03 19:12:19, bokan wrote:
> > This method is superfluous, it's private and only called in one place.
Please
> > remove it and move its content to the call site.
> 
> IMHO it helps with readability.

In that case, rename to clearGestureScrollState...it does more than it says.

[MuVen, 2015-06-04 08:04:29]

sataya.m@samsung.com changed reviewers:
+ pdr@chromium.org, skobes@chromium.org

[MuVen, 2015-06-04 11:19:59]

majid,

PTAL on the test cases modified based on the mock webviewClient.

Thanks,

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Chrom...
File Source/core/page/ChromeClient.h (right):

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Chrom...
Source/core/page/ChromeClient.h:107: // Pass on values for overscroll glow
animation on main thread.
On 2015/06/03 19:12:19, bokan wrote:
> This kind of comment is bound to become stale. If a comment is needed here, it
> should describe what overscroll is, rather than what downstream code will do
> with it.

removing comment, as API explains what it does.

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2719: } else {
On 2015/06/03 21:41:15, bokan wrote:
> On 2015/06/03 21:39:01, majidvp wrote:
> > On 2015/06/03 19:12:19, bokan wrote:
> > > Sorry if I missed it, but has any thought been given to how this will work
> > with
> > > scroll customization?
> > 
> > A month back after chatting with rbyers@ we decided it is too early to
require
> > patches like this to work with Scroll Customization. See
> > https://codereview.chromium.org/1056983004/#msg5
> > 
> > To handle scroll customization, I think we can move overscroll logic to
> > FrameView and invoke it in FrameView::applyScrollDelta. The only bit that
will
> > remain in EventHandler will be the reset logic that needs to be invoked if
any
> > element is scrolled.
> 
> Ok, sgtm to punt on this until scroll customization matures a bit.

Done.

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2753:
m_unusedDelta.setWidth(scrollablearea->scrollSize(HorizontalScrollbar) ?
scrollResult.unusedScrollDeltaX : 0.0f);
On 2015/06/03 17:36:41, majidvp wrote:
> nitp: blink style frowns upon 0.0f. use 0 instead
> 
> https://www.chromium.org/blink/coding-style#TOC-Floating-point-literals

Done.

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2753:
m_unusedDelta.setWidth(scrollablearea->scrollSize(HorizontalScrollbar) ?
scrollResult.unusedScrollDeltaX : 0.0f);
On 2015/06/03 19:12:19, bokan wrote:
> This should be main frame only, right?

entire code can be under mainframe.

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2757: if (m_frame->isMainFrame() &&
m_unusedDelta != FloatSize()) {
On 2015/06/03 19:12:19, bokan wrote:
> Side note, we'll likely also want to hook this up for at least wheel events
(Mac
> in particular has elastic overscroll on the trackpad) but I'm fine with
starting
> with gesture scrolls only for now. With that in mind though, this code block
> looks like it should be in a private helper method. e.g. 
> 
> if(m_frame->isMainFrame())
>   handleOverscroll(ScrollResult)

shall i raise a bug or this to track ? or mention in the bug ?  
i shall take up this activity down the lane.

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2772: void
EventHandler::clearGestureScrollNodes()
On 2015/06/03 21:41:15, bokan wrote:
> On 2015/06/03 21:39:01, majidvp wrote:
> > On 2015/06/03 19:12:19, bokan wrote:
> > > This method is superfluous, it's private and only called in one place.
> Please
> > > remove it and move its content to the call site.
> > 
> > IMHO it helps with readability.
> 
> In that case, rename to clearGestureScrollState...it does more than it says.

Done.

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
File Source/core/page/EventHandler.h (right):

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
Source/core/page/EventHandler.h:133: 
On 2015/06/03 19:12:19, bokan wrote:
> On 2015/06/03 17:36:41, majidvp wrote:
> > This is minor but I am not a big fan of exposing these "Testing" functions.
I
> > feel a better testing approach would be to
> > use a mock of WebViewClient similar to how other tests are implemented.
> > 
> > Which is  This is not a major issue and can perhaps be addressed in a
> follow-up
> > patch. bokan@ wdyt?
> 
> Agreed, this is an anti-pattern.

Done. But when using mock WebViewClient, we cant check the reset logic
of the unusedDelta and accumulatedRootOverscroll. As Overscroll is not reported
when unusedDelta/accumulatedRootOverscroll is reset. Please let me know your
inputs on this.

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
Source/core/page/EventHandler.h:386: FloatSize m_unusedDelta;
On 2015/06/03 17:36:41, majidvp wrote:
> If we get rid of the *Testing functions we can remove this data member as
well.

Done.

https://codereview.chromium.org/1056983004/diff/640001/Source/platform/scroll...
File Source/platform/scroll/ScrollAnimator.cpp (right):

https://codereview.chromium.org/1056983004/diff/640001/Source/platform/scroll...
Source/platform/scroll/ScrollAnimator.cpp:57: 
On 2015/06/03 17:36:41, majidvp wrote:
> nitp: extra blank line

Done.

[bokan, 2015-06-04 12:05:42]

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2757: if (m_frame->isMainFrame() &&
m_unusedDelta != FloatSize()) {
On 2015/06/04 11:19:58, MuVen wrote:
> On 2015/06/03 19:12:19, bokan wrote:
> > Side note, we'll likely also want to hook this up for at least wheel events
> (Mac
> > in particular has elastic overscroll on the trackpad) but I'm fine with
> starting
> > with gesture scrolls only for now. With that in mind though, this code block
> > looks like it should be in a private helper method. e.g. 
> > 
> > if(m_frame->isMainFrame())
> >   handleOverscroll(ScrollResult)
> 
> shall i raise a bug or this to track ? or mention in the bug ?  
> i shall take up this activity down the lane.

I think tracking it in a new bug is best. You should still put this code in a
helper now. It aids readability.

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
File Source/core/page/EventHandler.h (right):

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
Source/core/page/EventHandler.h:133: 
On 2015/06/04 11:19:59, MuVen wrote:
> On 2015/06/03 19:12:19, bokan wrote:
> > On 2015/06/03 17:36:41, majidvp wrote:
> > > This is minor but I am not a big fan of exposing these "Testing"
functions.
> I
> > > feel a better testing approach would be to
> > > use a mock of WebViewClient similar to how other tests are implemented.
> > > 
> > > Which is  This is not a major issue and can perhaps be addressed in a
> > follow-up
> > > patch. bokan@ wdyt?
> > 
> > Agreed, this is an anti-pattern.
> 
> Done. But when using mock WebViewClient, we cant check the reset logic
> of the unusedDelta and accumulatedRootOverscroll. As Overscroll is not
reported
> when unusedDelta/accumulatedRootOverscroll is reset. Please let me know your
> inputs on this.

The reset logic is an internal detail of EventSender so we shouldn't test it
directly. You should be testing the visible output. Another way of putting it:
if the reset logic is broken, what about the output is wrong? Find that and
setup the test so you check it.

In this case, it seems you should have someting like this: one gesture scroll
generate some overscroll, produce another scroll in the opposite direction that
should clear the accumulated delta, then try to overscroll again and confirm the
overscroll amount is what you'd expect if the middle scroll cleared the delta.

https://codereview.chromium.org/1056983004/diff/680001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1056983004/diff/680001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2653: void EventHandler::resetOverscroll(bool
didScrollX, bool didScrollY, FloatSize& unusedDelta)
I don't think you need to reset unusedDelta. The first call below it's always
(0, 0) so it's a no-op and in the second call, I'd argue that if we used part of
a gesture to scroll but still have unused delta, the rest *should* be applied to
overscroll. This is consistent with how the delta bubbling works in CC.

[MuVen, 2015-06-04 14:01:19]

bokan, PTAL.

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
File Source/core/page/EventHandler.h (right):

https://codereview.chromium.org/1056983004/diff/640001/Source/core/page/Event...
Source/core/page/EventHandler.h:133: 
On 2015/06/04 12:05:42, bokan wrote:
> On 2015/06/04 11:19:59, MuVen wrote:
> > On 2015/06/03 19:12:19, bokan wrote:
> > > On 2015/06/03 17:36:41, majidvp wrote:
> > > > This is minor but I am not a big fan of exposing these "Testing"
> functions.
> > I
> > > > feel a better testing approach would be to
> > > > use a mock of WebViewClient similar to how other tests are implemented.
> > > > 
> > > > Which is  This is not a major issue and can perhaps be addressed in a
> > > follow-up
> > > > patch. bokan@ wdyt?
> > > 
> > > Agreed, this is an anti-pattern.
> > 
> > Done. But when using mock WebViewClient, we cant check the reset logic
> > of the unusedDelta and accumulatedRootOverscroll. As Overscroll is not
> reported
> > when unusedDelta/accumulatedRootOverscroll is reset. Please let me know your
> > inputs on this.
> 
> The reset logic is an internal detail of EventSender so we shouldn't test it
> directly. You should be testing the visible output. Another way of putting it:
> if the reset logic is broken, what about the output is wrong? Find that and
> setup the test so you check it.
> 
> In this case, it seems you should have someting like this: one gesture scroll
> generate some overscroll, produce another scroll in the opposite direction
that
> should clear the accumulated delta, then try to overscroll again and confirm
the
> overscroll amount is what you'd expect if the middle scroll cleared the delta.


Done.

https://codereview.chromium.org/1056983004/diff/680001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1056983004/diff/680001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2653: void EventHandler::resetOverscroll(bool
didScrollX, bool didScrollY, FloatSize& unusedDelta)
On 2015/06/04 12:05:42, bokan wrote:
> I don't think you need to reset unusedDelta. The first call below it's always
> (0, 0) so it's a no-op and in the second call, I'd argue that if we used part
of
> a gesture to scroll but still have unused delta, the rest *should* be applied
to
> overscroll. This is consistent with how the delta bubbling works in CC.

Done.

[bokan, 2015-06-04 14:45:44]

Looking good, mostly suggest some improvements in tests.

https://codereview.chromium.org/1056983004/diff/700001/Source/core/frame/Root...
File Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/1056983004/diff/700001/Source/core/frame/Root...
Source/core/frame/RootFrameViewport.cpp:301: return
ScrollResultOneDimensional(false);
Please make sure in these cases where we return a didScroll = false, we also
include the delta in the unusedDelta field.

https://codereview.chromium.org/1056983004/diff/700001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1056983004/diff/700001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2665: // Set unusedDelta if axes is
scrollable, else set 0 to ensure overflow is not reported on non-scrollable
axes.
Nit: axes -> axis

https://codereview.chromium.org/1056983004/diff/700001/Source/web/tests/WebFr...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1056983004/diff/700001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7495: webViewImpl->layout();
Nit: I don't think you need these layout calls.

https://codereview.chromium.org/1056983004/diff/700001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7501: EXPECT_WEB_FLOAT_SIZE_EQ(WebFloatSize(0,
0), client.m_accumulatedRootOverScroll);
Shouldn't these be non-0 now that the unusedDelta isn't reset in
resetOverscroll()? Is it because the test page actually has exactly 308, 316
amount of scrolling? In that case, add style:

body,html { padding: 0px; margin: 0px }

To the test files as appropriate so that it's easy to tell at a glance how much
scrolling there is on the page.

https://codereview.chromium.org/1056983004/diff/700001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7512: // UnusedDelta and
AccumulatedRootOverscroll values.
We should use the GMock in these tests, it'll make the expectations more
explicit and easier to read IMO. See
https://code.google.com/p/googlemock/wiki/ForDummies for documentation. Don't
worry, it's actually surprisingly easy

In summary:

OverscrollWebViewClient's didOverscroll method should be replaced by

MOCK_METHOD4(didOverscroll, void(const WebFloatSize& unusedDelta, /*...etc*/));

And you can remove the members, GMock will keep track of the argument values.

Then, if your ScrollUpdate causes an overscroll of (1, 2), before the
ScrollUpdate call you'd add the expectation:

EXPECT_CALL(client, didOverscroll(1, 2))

Before the next ScrollUpdate you'll want to call
Mock::VerifyAndClearExpectations(&client) to check and clear that expectation,
otherwise in the next call it'll trigger that condition again, think it got
called twice, and fail.

To say "didOverscroll should not be called":

EXPECT_CALL(client, didOverscroll(_, _)).Times(0);

See PinchViewportTest for some examples.

https://codereview.chromium.org/1056983004/diff/700001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7520: 
I would add another overscroll here to make sure the accumulated value is
actually cleared.

https://codereview.chromium.org/1056983004/diff/700001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7544: EXPECT_WEB_FLOAT_SIZE_EQ(WebFloatSize(0,
50), client.m_accumulatedRootOverScroll);
This is tested in the below test. You should keep the test cases as specific as
possible so if the test fails, its easier to determine what's breaking. I'd
remove all the checks except the ones specific to checking that scrolling
between different axes works as expected (only test what's in the test name)

https://codereview.chromium.org/1056983004/diff/700001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7667: TEST_F(WebFrameOverscrollTest,
ScaledPageRootLayerOverscrolled)
Awesome, thanks for testing under page scale :)

[MuVen, 2015-06-04 17:58:57]

Bokan, PTAL.

Thanks,

https://codereview.chromium.org/1056983004/diff/700001/Source/core/frame/Root...
File Source/core/frame/RootFrameViewport.cpp (right):

https://codereview.chromium.org/1056983004/diff/700001/Source/core/frame/Root...
Source/core/frame/RootFrameViewport.cpp:301: return
ScrollResultOneDimensional(false);
On 2015/06/04 14:45:43, bokan wrote:
> Please make sure in these cases where we return a didScroll = false, we also
> include the delta in the unusedDelta field.

Done.

https://codereview.chromium.org/1056983004/diff/700001/Source/core/page/Event...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/1056983004/diff/700001/Source/core/page/Event...
Source/core/page/EventHandler.cpp:2665: // Set unusedDelta if axes is
scrollable, else set 0 to ensure overflow is not reported on non-scrollable
axes.
On 2015/06/04 14:45:43, bokan wrote:
> Nit: axes -> axis

Done.

https://codereview.chromium.org/1056983004/diff/700001/Source/web/tests/WebFr...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/1056983004/diff/700001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7495: webViewImpl->layout();
On 2015/06/04 14:45:44, bokan wrote:
> Nit: I don't think you need these layout calls.

Done.

https://codereview.chromium.org/1056983004/diff/700001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7501: EXPECT_WEB_FLOAT_SIZE_EQ(WebFloatSize(0,
0), client.m_accumulatedRootOverScroll);
On 2015/06/04 14:45:44, bokan wrote:
> Shouldn't these be non-0 now that the unusedDelta isn't reset in
> resetOverscroll()? Is it because the test page actually has exactly 308, 316
> amount of scrolling? In that case, add style:
> 
> body,html { padding: 0px; margin: 0px }
> 
> To the test files as appropriate so that it's easy to tell at a glance how
much
> scrolling there is on the page.

Done.

https://codereview.chromium.org/1056983004/diff/700001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7512: // UnusedDelta and
AccumulatedRootOverscroll values.
On 2015/06/04 14:45:44, bokan wrote:
> We should use the GMock in these tests, it'll make the expectations more
> explicit and easier to read IMO. See
> https://code.google.com/p/googlemock/wiki/ForDummies for documentation. Don't
> worry, it's actually surprisingly easy
> 
> In summary:
> 
> OverscrollWebViewClient's didOverscroll method should be replaced by
> 
> MOCK_METHOD4(didOverscroll, void(const WebFloatSize& unusedDelta,
/*...etc*/));
> 
> And you can remove the members, GMock will keep track of the argument values.
> 
> Then, if your ScrollUpdate causes an overscroll of (1, 2), before the
> ScrollUpdate call you'd add the expectation:
> 
> EXPECT_CALL(client, didOverscroll(1, 2))
> 
> Before the next ScrollUpdate you'll want to call
> Mock::VerifyAndClearExpectations(&client) to check and clear that expectation,
> otherwise in the next call it'll trigger that condition again, think it got
> called twice, and fail.
> 
> To say "didOverscroll should not be called":
> 
> EXPECT_CALL(client, didOverscroll(_, _)).Times(0);
> 
> See PinchViewportTest for some examples.

Done.

https://codereview.chromium.org/1056983004/diff/700001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7520: 
On 2015/06/04 14:45:44, bokan wrote:
> I would add another overscroll here to make sure the accumulated value is
> actually cleared.

Done.

https://codereview.chromium.org/1056983004/diff/700001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7544: EXPECT_WEB_FLOAT_SIZE_EQ(WebFloatSize(0,
50), client.m_accumulatedRootOverScroll);
On 2015/06/04 14:45:44, bokan wrote:
> This is tested in the below test. You should keep the test cases as specific
as
> possible so if the test fails, its easier to determine what's breaking. I'd
> remove all the checks except the ones specific to checking that scrolling
> between different axes works as expected (only test what's in the test name)

Done.

https://codereview.chromium.org/1056983004/diff/700001/Source/web/tests/WebFr...
Source/web/tests/WebFrameTest.cpp:7667: TEST_F(WebFrameOverscrollTest,
ScaledPageRootLayerOverscrolled)
On 2015/06/04 14:45:43, bokan wrote:
> Awesome, thanks for testing under page scale :)

Thanks to majidvp :)

[bokan, 2015-06-04 18:02:40]

LGTM, thanks!

[MuVen, 2015-06-04 18:04:33]

On 2015/06/04 18:02:40, bokan wrote:
> LGTM, thanks!

Thanks a lot (for your patient review, teaching me new things :))

Skobes/pdr,

Need owners review. PTAL.

Thanks,
~MuVen.

[skobes, 2015-06-04 23:47:14]

lgtm for core/layout

[MuVen, 2015-06-08 07:22:25]

Patchset #29 (id:760001) has been deleted

[MuVen, 2015-06-08 07:46:23]

Patchset #29 (id:780001) has been deleted

[majidvp, 2015-06-08 14:54:06]

On 2015/06/04 23:47:14, skobes wrote:
> lgtm for core/layout


I think the patch description should be updates to make it clear that
1- The patch updates main-thread to report the overscroll amount but it does not

do the animation itself.
2- The patch only add overscroll reporting for touch scrolls

[pdr., 2015-06-08 22:47:47]

On 2015/06/08 at 14:54:06, majidvp wrote:
> On 2015/06/04 23:47:14, skobes wrote:
> > lgtm for core/layout

LGTM for web

[MuVen, 2015-06-09 08:26:23]

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

[MuVen, 2015-06-09 08:26:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
majidvp@chromium.org, skobes@chromium.org, bokan@chromium.org, pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/1056983004/#ps820001
(title: "rebased to latest")

[commit-bot: I haz the power, 2015-06-09 08:26:35]

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

[commit-bot: I haz the power, 2015-06-09 09:32:50]

Committed patchset #30 (id:820001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196748