Use scroll customization primitives for touch scrolling (behind REF).

Use scroll customization primitives for touch scrolling (behind REF).

With the ScrollCustomization runtime enabled feature on, use scrolling
built on top of applyScroll, distributeScroll, and the ScrollState
object.

This is part of landing https://codereview.chromium.org/850443002/.

A follow up patch will expose the applyScroll and distributeScroll
methods to JS.

BUG=410974
TEST=scroll_customization virtual test suite.

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

[tdresser, 2015-03-06 20:38:11]

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

https://codereview.chromium.org/988823003/diff/1/Source/core/page/EventHandle...
Source/core/page/EventHandler.cpp:2586: // FIXME - check if its an element.
This needs to be done.

https://codereview.chromium.org/988823003/diff/1/Source/core/page/scrolling/S...
File Source/core/page/scrolling/ScrollState.h (right):

https://codereview.chromium.org/988823003/diff/1/Source/core/page/scrolling/S...
Source/core/page/scrolling/ScrollState.h:22: bool isBeginning = false, bool
isEnding = false,
This needs to be updated in the layout tests.

[tdresser, 2015-03-09 17:02:56]

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

https://codereview.chromium.org/988823003/diff/1/Source/core/page/EventHandle...
Source/core/page/EventHandler.cpp:2586: // FIXME - check if its an element.
On 2015/03/06 20:38:11, tdresser wrote:
> This needs to be done.

Done.

https://codereview.chromium.org/988823003/diff/1/Source/core/page/scrolling/S...
File Source/core/page/scrolling/ScrollState.h (right):

https://codereview.chromium.org/988823003/diff/1/Source/core/page/scrolling/S...
Source/core/page/scrolling/ScrollState.h:22: bool isBeginning = false, bool
isEnding = false,
On 2015/03/06 20:38:11, tdresser wrote:
> This needs to be updated in the layout tests.

Done.

[tdresser, 2015-03-09 20:48:55]

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

[tdresser, 2015-03-09 20:48:55]

PTAL.

The patch after this will expose applyScroll and distributeScroll to JS.

[Rick Byers, 2015-03-11 02:22:19]

/cc skobes and bokan incase they're interested

Looks reasonable, thanks!  Do you think the existing gesture scroll layout test
coverage is sufficient?  Eg. did failures there in the virtual test suite help
you catch bugs during development?  Where there are new special cases, we should
probably add generic tests that'll be run both with and without scroll
customization.

https://codereview.chromium.org/988823003/diff/60001/LayoutTests/fast/scroll-...
File
LayoutTests/fast/scroll-behavior/scroll-customization/scrollstate-basic.html
(right):

https://codereview.chromium.org/988823003/diff/60001/LayoutTests/fast/scroll-...
LayoutTests/fast/scroll-behavior/scroll-customization/scrollstate-basic.html:49:
assert_equals(scrollState.isBeginning, isBeginning);
There should be -expected.txt files checked in and updated in this CL.

https://codereview.chromium.org/988823003/diff/60001/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/988823003/diff/60001/Source/core/dom/Element....
Source/core/dom/Element.cpp:494: // an element other than this one, don't
scroll.
this comment says what (i.e. repeats the code below), but not why.  Why?

https://codereview.chromium.org/988823003/diff/60001/Source/core/dom/Element....
Source/core/dom/Element.cpp:496: && scrollState.deltaConsumedForScrollSequence()
Can you add a comment to the declaration of deltaConsumedForScrollSequence
describing exactly what it means?  It's not entirely obvious to me.

https://codereview.chromium.org/988823003/diff/60001/Source/core/dom/Element....
Source/core/dom/Element.cpp:544:
scrollState.consumeDeltaNative(scrollState.deltaX(), scrollState.deltaY());
If curBox->scroll(ScrollLeft) returned true, but curBox->scroll(ScrollUp) return
false, do we really want to consume the deltaY?  Seems like that would break
diagonal scrolling of nested scrollers which can each scroll in only a single
axis (a good test case to add!).

Perhaps update scrolledX/scrolledY values above and pass them here?  Perhaps
also worth adding a FIXME above saying that native elements should really only
consume the amount they move, rather than always consuming the entire thing?

https://codereview.chromium.org/988823003/diff/60001/Source/core/frame/LocalF...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/988823003/diff/60001/Source/core/frame/LocalF...
Source/core/frame/LocalFrame.cpp:772: return scrolledHorizontal ||
scrolledVertical;
again, don't you need to handle these two cases separately?  Eg. what about a
vertically scrollable iframe nested inside of a horizontally scrollable
document?

perhaps 'bool LocalFrame::applyScrollDelta' should just be 'void
LocalFrame::applyScroll(ScrollState&)'?

Ah, I see - this function is used in both custom and standard scroll cases. 
Maybe just mention that in a comment, with a FIXME if you think you can simplify
it once scroll customization is enabled by default.

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (left):

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2198: m_previousGestureScrolledNode = nullptr;
why remove this?

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:209: while (curBox && (rootLayerScrolls ||
!curBox->isLayoutView())) {
I think this deserves a comment describing why we're doing this.  Maybe just
"scrolling propagates along the containing block chain" or some such.  

Also I think this code is Shadow-DOM correct, but you should probably add at
least one test (eg. a scroller as a template argument to a custom element,
instantiated inside some other scroller inside the custom element - the chain
should reflect the instantiation).

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:211: curBox = curBox->containingBlock();
What if the first curBox is scrollable itself?  It won't get added to the scroll
chain since we walk up first before doing the below logic.

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:220: if (!addedDocumentElement)
Do we want to do this even when rootLayerScrolls is false?

Can't we simplify the loop by always stopping once we hit the documentElement or
RenderView, and then always (or at least whenver rootLayerScrolls is true)
adding the document element?  It's really special in that many properties /
operations on it are redirected to the RenderView.

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:991: // FIXME: enable scroll customization in
this case.
you might want to reference your scroll customization bug number - then we can
do a search for that URL in the code before calling your bug fixed.  I assume
this will be the first of many such FIXMEs ;-)

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2495: return false;
this doesn't make sense to me.  If the subframe handles the GSE then we report
NOT handled and terminate?  The original code made sure to clear any scrolling
state across the entire frame chain involved.  Your code doesn't, does it?

Do you need some more iframe tests?

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2498: 0, 0, 0, 0, 0, gestureEvent.inertial(),
false, true, true);
Blink coding style says: "Prefer enums to bools on function parameters if
callers are likely to be passing constants"
(http://www.chromium.org/blink/coding-style#TOC-Names) precisely because this
"false, true, true" is hard to understand.  I'd probably also accept comments in
front of each of these 3 args giving its name.

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2532: if
(!RuntimeEnabledFeatures::scrollCustomizationEnabled()) {
this would be slightly clearer to me as a if(enabled)/else block (with a single
return true at the end.  WDYT?

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2541: 0, 0, 0, 0, 0, false, true, false,
true);
ditto

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2580: if (m_previousGestureScrolledNode) {
So does this field now represent just native scrolls (doesn't include custom
scrolled nodes)?  Perhaps update it's comment or maybe even it's name.  I'm not
fully following (although I haven't finished looking at all code yet) why you
need this, and 'currentNativeScrollingElement', perhaps some more comments would
help.

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2597: | scroll(ScrollUp, granularity, node,
&stopNode, delta.height());
This is a little subtle (I could imagine someone replacing | with || and
breaking things).  Maybe a comment or just return to the more explicit form
(scrolled = horizontalScroll || verticalScroll).

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
File Source/core/page/EventHandler.h (right):

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.h:405: // scroll sequence. Only used with
ScrollCustomization.
consumed within this frame, or by this frame any any child frames (which have
their own EventHandler instance)?

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/scrolli...
File Source/core/page/scrolling/ScrollState.h (right):

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/scrolli...
Source/core/page/scrolling/ScrollState.h:79: return !m_deltaX && !m_deltaY &&
!m_isEnding && !m_isBeginning;
worth a comment perhaps - something like 'scroll begin and end must bubble to
all nodes to ensure state is updated'.

[tdresser, 2015-03-20 18:00:38]

Failures in the virtual test suite were extremely useful during development. I
think test coverage is fairly reasonable - we'll also be filling in the test
suite more as we align main thread scrolling behavior with the impl thread
behavior.

The expectations for the testharness.js tests are here:
https://drive.google.com/folderview?id=0B7mjRvOU-oG-fl81OXUwdzR1ZEhjTGhobXZqY...

https://codereview.chromium.org/988823003/diff/60001/LayoutTests/fast/scroll-...
File
LayoutTests/fast/scroll-behavior/scroll-customization/scrollstate-basic.html
(right):

https://codereview.chromium.org/988823003/diff/60001/LayoutTests/fast/scroll-...
LayoutTests/fast/scroll-behavior/scroll-customization/scrollstate-basic.html:49:
assert_equals(scrollState.isBeginning, isBeginning);
On 2015/03/11 02:22:18, Rick Byers wrote:
> There should be -expected.txt files checked in and updated in this CL.

Not for testharness.js tests.

https://codereview.chromium.org/988823003/diff/60001/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/988823003/diff/60001/Source/core/dom/Element....
Source/core/dom/Element.cpp:494: // an element other than this one, don't
scroll.
On 2015/03/11 02:22:18, Rick Byers wrote:
> this comment says what (i.e. repeats the code below), but not why.  Why?

Done.

https://codereview.chromium.org/988823003/diff/60001/Source/core/dom/Element....
Source/core/dom/Element.cpp:496: && scrollState.deltaConsumedForScrollSequence()
On 2015/03/11 02:22:18, Rick Byers wrote:
> Can you add a comment to the declaration of deltaConsumedForScrollSequence
> describing exactly what it means?  It's not entirely obvious to me.

Can you clarify what about the comment doesn't make sense?
The comment currently states 
"Whether the scroll sequence has had any delta consumed."

Is it the term "scroll sequence" that's confusing?

https://codereview.chromium.org/988823003/diff/60001/Source/core/dom/Element....
Source/core/dom/Element.cpp:544:
scrollState.consumeDeltaNative(scrollState.deltaX(), scrollState.deltaY());
On 2015/03/11 02:22:18, Rick Byers wrote:
> If curBox->scroll(ScrollLeft) returned true, but curBox->scroll(ScrollUp)
return
> false, do we really want to consume the deltaY?  Seems like that would break
> diagonal scrolling of nested scrollers which can each scroll in only a single
> axis (a good test case to add!).
> 
> Perhaps update scrolledX/scrolledY values above and pass them here?  Perhaps
> also worth adding a FIXME above saying that native elements should really only
> consume the amount they move, rather than always consuming the entire thing?

Done.

https://codereview.chromium.org/988823003/diff/60001/Source/core/frame/LocalF...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/988823003/diff/60001/Source/core/frame/LocalF...
Source/core/frame/LocalFrame.cpp:772: return scrolledHorizontal ||
scrolledVertical;
On 2015/03/11 02:22:18, Rick Byers wrote:
> again, don't you need to handle these two cases separately?  Eg. what about a
> vertically scrollable iframe nested inside of a horizontally scrollable
> document?
> 
> perhaps 'bool LocalFrame::applyScrollDelta' should just be 'void
> LocalFrame::applyScroll(ScrollState&)'?
> 
> Ah, I see - this function is used in both custom and standard scroll cases. 
> Maybe just mention that in a comment, with a FIXME if you think you can
simplify
> it once scroll customization is enabled by default.

This doesn't change any behavior, see 
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....

Main thread scroll bubbling across iframes doesn't feel great.
I'll add a test for diagonal scrolling across iframes at the same time as I fix
it (crbug.com/466991).

I did have a version of this patch which passed a ScrollState here, and it was
quite pretty, especially the top controls part. For now, I've added the FIXME,
because that change does make it a pain to get the non-scroll customized world
working.

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (left):

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2198: m_previousGestureScrolledNode = nullptr;
On 2015/03/11 02:22:19, Rick Byers wrote:
> why remove this?

Looks wrong to me. Fixed.

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:209: while (curBox && (rootLayerScrolls ||
!curBox->isLayoutView())) {
On 2015/03/11 02:22:18, Rick Byers wrote:
> I think this deserves a comment describing why we're doing this.  Maybe just
> "scrolling propagates along the containing block chain" or some such.  
> 

Done.

> Also I think this code is Shadow-DOM correct, but you should probably add at
> least one test (eg. a scroller as a template argument to a custom element,
> instantiated inside some other scroller inside the custom element - the chain
> should reflect the instantiation).

I'll add a test for scroll customization with shadow dom in the patch which
enables javascript scroll customization.

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:211: curBox = curBox->containingBlock();
On 2015/03/11 02:22:18, Rick Byers wrote:
> What if the first curBox is scrollable itself?  It won't get added to the
scroll
> chain since we walk up first before doing the below logic.

We don't use curBox until the next iteration of the loop, so this is logically
correct. I've cleaned it up so it's a bit clearer.

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:220: if (!addedDocumentElement)
On 2015/03/11 02:22:19, Rick Byers wrote:
> Do we want to do this even when rootLayerScrolls is false?
> 
> Can't we simplify the loop by always stopping once we hit the documentElement
or
> RenderView, and then always (or at least whenver rootLayerScrolls is true)
> adding the document element?  It's really special in that many properties /
> operations on it are redirected to the RenderView.

I've simplified the loop a bit. Actually determining if adding the
documentElement is necessary won't be possible until JS scroll customization is
implemented, so I'm going to mark this with a FIXME for now.

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:991: // FIXME: enable scroll customization in
this case.
On 2015/03/11 02:22:18, Rick Byers wrote:
> you might want to reference your scroll customization bug number - then we can
> do a search for that URL in the code before calling your bug fixed.  I assume
> this will be the first of many such FIXMEs ;-)

Done.

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2495: return false;
On 2015/03/11 02:22:18, Rick Byers wrote:
> this doesn't make sense to me.  If the subframe handles the GSE then we report
> NOT handled and terminate?  The original code made sure to clear any scrolling
> state across the entire frame chain involved.  Your code doesn't, does it?
> 
> Do you need some more iframe tests?

This was correct before, but _extremely_ confusing.

Note that the return value here is the same as it was before - we only ever
return false (I believe this is because we need to ensure that all frames
receive the gesture end). The return value of passScrollGestureEventToWidget is
ignored.

The early out will never trigger, since we always return false in
handleGestureScrollEnd... I've gotten rid of it.


clearGestureScrollNodes() will always be called across all frames, and it clears
up all of the scrolling state.

There's a reasonable set of existing iframe tests - I'm planning to add some
more when I align the behavior of main thread iframe scroll bubbling with impl
thread iframe scroll bubbling, but until then I think the coverage is
acceptable. Are there specific tests you think are missing?

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2498: 0, 0, 0, 0, 0, gestureEvent.inertial(),
false, true, true);
On 2015/03/11 02:22:18, Rick Byers wrote:
> Blink coding style says: "Prefer enums to bools on function parameters if
> callers are likely to be passing constants"
> (http://www.chromium.org/blink/coding-style#TOC-Names) precisely because this
> "false, true, true" is hard to understand.  I'd probably also accept comments
in
> front of each of these 3 args giving its name.

Done.

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2532: if
(!RuntimeEnabledFeatures::scrollCustomizationEnabled()) {
On 2015/03/11 02:22:18, Rick Byers wrote:
> this would be slightly clearer to me as a if(enabled)/else block (with a
single
> return true at the end.  WDYT?

Done.

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2541: 0, 0, 0, 0, 0, false, true, false,
true);
On 2015/03/11 02:22:18, Rick Byers wrote:
> ditto

Done.

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2580: if (m_previousGestureScrolledNode) {
On 2015/03/11 02:22:18, Rick Byers wrote:
> So does this field now represent just native scrolls (doesn't include custom
> scrolled nodes)?  Perhaps update it's comment or maybe even it's name.  I'm
not
> fully following (although I haven't finished looking at all code yet) why you
> need this, and 'currentNativeScrollingElement', perhaps some more comments
would
> help.

Done.

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2597: | scroll(ScrollUp, granularity, node,
&stopNode, delta.height());
On 2015/03/11 02:22:19, Rick Byers wrote:
> This is a little subtle (I could imagine someone replacing | with || and
> breaking things).  Maybe a comment or just return to the more explicit form
> (scrolled = horizontalScroll || verticalScroll).

Done.

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
File Source/core/page/EventHandler.h (right):

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.h:405: // scroll sequence. Only used with
ScrollCustomization.
On 2015/03/11 02:22:19, Rick Byers wrote:
> consumed within this frame, or by this frame any any child frames (which have
> their own EventHandler instance)?

Within this frame or any child frames. I've added a comment explaining how this
is used.

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/scrolli...
File Source/core/page/scrolling/ScrollState.h (right):

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/scrolli...
Source/core/page/scrolling/ScrollState.h:79: return !m_deltaX && !m_deltaY &&
!m_isEnding && !m_isBeginning;
On 2015/03/11 02:22:19, Rick Byers wrote:
> worth a comment perhaps - something like 'scroll begin and end must bubble to
> all nodes to ensure state is updated'.

Done.

https://codereview.chromium.org/988823003/diff/80001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/988823003/diff/80001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2636: if (!gestureEvent.preventPropagation())
This fixes a bug in main thread scrolling, which
touch-gesture-scroll-div-propagated-diagonally.html exposed.

[Rick Byers, 2015-03-26 21:22:49]

LGTM with nit

https://codereview.chromium.org/988823003/diff/60001/Source/core/dom/Element.cpp
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/988823003/diff/60001/Source/core/dom/Element....
Source/core/dom/Element.cpp:496: && scrollState.deltaConsumedForScrollSequence()
On 2015/03/20 18:00:36, tdresser wrote:
> On 2015/03/11 02:22:18, Rick Byers wrote:
> > Can you add a comment to the declaration of deltaConsumedForScrollSequence
> > describing exactly what it means?  It's not entirely obvious to me.
> 
> Can you clarify what about the comment doesn't make sense?
> The comment currently states 
> "Whether the scroll sequence has had any delta consumed."
> 
> Is it the term "scroll sequence" that's confusing?

Ah, I was looking at the method declaration - didn't think to go look at the
field for a comment.  But that's fine.  I assume 'scroll sequence' means from
the time of scroll begin, right?  That's fine - thanks.

https://codereview.chromium.org/988823003/diff/60001/Source/core/frame/LocalF...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/988823003/diff/60001/Source/core/frame/LocalF...
Source/core/frame/LocalFrame.cpp:772: return scrolledHorizontal ||
scrolledVertical;
On 2015/03/20 18:00:36, tdresser wrote:
> On 2015/03/11 02:22:18, Rick Byers wrote:
> > again, don't you need to handle these two cases separately?  Eg. what about
a
> > vertically scrollable iframe nested inside of a horizontally scrollable
> > document?
> > 
> > perhaps 'bool LocalFrame::applyScrollDelta' should just be 'void
> > LocalFrame::applyScroll(ScrollState&)'?
> > 
> > Ah, I see - this function is used in both custom and standard scroll cases. 
> > Maybe just mention that in a comment, with a FIXME if you think you can
> simplify
> > it once scroll customization is enabled by default.
> 
> This doesn't change any behavior, see 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit....
> 
> Main thread scroll bubbling across iframes doesn't feel great.
> I'll add a test for diagonal scrolling across iframes at the same time as I
fix
> it (crbug.com/466991).
> 
> I did have a version of this patch which passed a ScrollState here, and it was
> quite pretty, especially the top controls part. For now, I've added the FIXME,
> because that change does make it a pain to get the non-scroll customized world
> working.

Ok, SGTM - thanks.

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:211: curBox = curBox->containingBlock();
On 2015/03/20 18:00:37, tdresser wrote:
> On 2015/03/11 02:22:18, Rick Byers wrote:
> > What if the first curBox is scrollable itself?  It won't get added to the
> scroll
> > chain since we walk up first before doing the below logic.
> 
> We don't use curBox until the next iteration of the loop, so this is logically
> correct. I've cleaned it up so it's a bit clearer.

Acknowledged.

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:220: if (!addedDocumentElement)
On 2015/03/20 18:00:36, tdresser wrote:
> On 2015/03/11 02:22:19, Rick Byers wrote:
> > Do we want to do this even when rootLayerScrolls is false?
> > 
> > Can't we simplify the loop by always stopping once we hit the
documentElement
> or
> > RenderView, and then always (or at least whenver rootLayerScrolls is true)
> > adding the document element?  It's really special in that many properties /
> > operations on it are redirected to the RenderView.
> 
> I've simplified the loop a bit. Actually determining if adding the
> documentElement is necessary won't be possible until JS scroll customization
is
> implemented, so I'm going to mark this with a FIXME for now.

Acknowledged.

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2495: return false;
On 2015/03/20 18:00:37, tdresser wrote:
> On 2015/03/11 02:22:18, Rick Byers wrote:
> > this doesn't make sense to me.  If the subframe handles the GSE then we
report
> > NOT handled and terminate?  The original code made sure to clear any
scrolling
> > state across the entire frame chain involved.  Your code doesn't, does it?
> > 
> > Do you need some more iframe tests?
> 
> This was correct before, but _extremely_ confusing.
> 
> Note that the return value here is the same as it was before - we only ever
> return false (I believe this is because we need to ensure that all frames
> receive the gesture end). The return value of passScrollGestureEventToWidget
is
> ignored.
> 
> The early out will never trigger, since we always return false in
> handleGestureScrollEnd... I've gotten rid of it.
> 
> 
> clearGestureScrollNodes() will always be called across all frames, and it
clears
> up all of the scrolling state.

Thanks, makes sense.

> There's a reasonable set of existing iframe tests - I'm planning to add some
> more when I align the behavior of main thread iframe scroll bubbling with impl
> thread iframe scroll bubbling, but until then I think the coverage is
> acceptable. Are there specific tests you think are missing?

No, that sounds fine.  Thanks.

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2532: if
(!RuntimeEnabledFeatures::scrollCustomizationEnabled()) {
On 2015/03/20 18:00:37, tdresser wrote:
> On 2015/03/11 02:22:18, Rick Byers wrote:
> > this would be slightly clearer to me as a if(enabled)/else block (with a
> single
> > return true at the end.  WDYT?
> 
> Done.

nit: I meants put the true case first to avoid a double-negative

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
File Source/core/page/EventHandler.h (right):

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.h:405: // scroll sequence. Only used with
ScrollCustomization.
On 2015/03/20 18:00:37, tdresser wrote:
> On 2015/03/11 02:22:19, Rick Byers wrote:
> > consumed within this frame, or by this frame any any child frames (which
have
> > their own EventHandler instance)?
> 
> Within this frame or any child frames. I've added a comment explaining how
this
> is used.

Thanks, this is great.

[tdresser, 2015-03-27 15:28:52]

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/988823003/diff/60001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2532: if
(!RuntimeEnabledFeatures::scrollCustomizationEnabled()) {
On 2015/03/26 21:22:49, Rick Byers wrote:
> On 2015/03/20 18:00:37, tdresser wrote:
> > On 2015/03/11 02:22:18, Rick Byers wrote:
> > > this would be slightly clearer to me as a if(enabled)/else block (with a
> > single
> > > return true at the end.  WDYT?
> > 
> > Done.
> 
> nit: I meants put the true case first to avoid a double-negative

Done.

[tdresser, 2015-03-27 15:45:41]

The CQ bit was checked by tdresser@chromium.org

[tdresser, 2015-03-27 15:45:42]

The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/988823003/#ps140001
(title: "Address rbyers' nit.")

[commit-bot: I haz the power, 2015-03-27 15:46:06]

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

[commit-bot: I haz the power, 2015-03-27 16:49:19]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192688