Implement CSS: scroll-boundary-behavior

Implement CSS: scroll-boundary-behavior.

CSS: scroll-boundary-behavior allows developers to specify whether the scroll
should be propagated to its ancestors. This is different from scroll
latching, in the way that when latching is turned on, a gesture will be
limited to a single element, but scroll-boundary-behavior will affect
propagation when the latch is being targeted at the beginning of the gesture.

This patch implements scroll-boundary-behavior: none, by cutting off the
scroll chain when the scrollNode has the property, so the scroll will not be
propagated. After this patch lands, we will implement scroll-boundary-behavior:
contain, which controls overscroll UI affordance such as glow/bounce etc.

BUG=672921
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2769793002
Cr-Commit-Position: refs/heads/master@{#487702}
Committed: https://chromium.googlesource.com/chromium/src/+/55ed8ccd43eadef318b5e7a6bcf2519b078b1213

[sunyunjia, 2017-03-22 19:16:15]

Description was changed from

==========
First patch of scroll-boundary-behavior

BUG=
==========

to

==========
First patch of scroll-boundary-behavior

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[sunyunjia, 2017-03-22 19:16:15]

Description was changed from

==========
First patch of scroll-boundary-behavior

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
First patch of scroll-boundary-behavior

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[sunyunjia, 2017-03-22 20:00:37]

Description was changed from

==========
First patch of scroll-boundary-behavior

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Implement CSS: scroll-boundary-behavior.

CSS: scroll-boundary-behavior allows developers to specify whether the scroll
should be propagated to the parent scroller. This is different from scroll
latching, in the way that even when the scroller has reached its extend,
additional scroll should not be propagated to the parent scroller.

This patch implements scroll-boundary-behavior: contain, by cutting off the
scroll chain when the scrollNode has the property, so the scroll will not be
propagated. After overscroll-action is implemented, we will implement
scroll-boundary-behavior: none using overscroll-action's backend support. As
both of them are meant to disable the overscroll effects.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[sunyunjia, 2017-03-22 20:00:37]

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

[sunyunjia, 2017-03-22 20:00:48]

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

[commit-bot: I haz the power, 2017-03-22 20:01:42]

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

[sunyunjia, 2017-03-22 20:03:25]

Hi David,

Can you take a look at my first implementation of scroll-boundary-behavior?
Thanks!

[commit-bot: I haz the power, 2017-03-22 20:17:13]

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

[commit-bot: I haz the power, 2017-03-22 20:17:14]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[bokan, 2017-03-23 13:18:27]

Have a few comments.

https://codereview.chromium.org/2769793002/diff/20001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/2769793002/diff/20001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:84:
scroll_boundary_behavior_(SCROLL_BOUNDARY_BEHAVIOR_PROPAGATE) {
How does a LayerImpl get scroll_boundary_behavior_ set? I think you need to add
a line in Layer::PushPropertiesTo(LayerImpl*)

https://codereview.chromium.org/2769793002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2769793002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:3177: LayerImpl* layer =
Layers are eventually going away. Instead of propagating the boundary behavior
to the LayerImpl, you should set it directly on the ScrollNode in
PropertyTreeBuilder. See AddScrollNodeIfNeeded in property_tree_builder.cc. You
may need to keep a method in LayerImpl to match the one in Layer since the
function is generic for layer types but it doesn't need to do anything since the
boundary behavior will always come from cc::Layer only.

https://codereview.chromium.org/2769793002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2769793002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1084:
updateScrollBoundaryBehavior();
Hmm...I'm not sure CLM::updateGraphicsLayerGeometry will get called if you just
change the scrollBoundaryBehavior. Do you know how that happens? If not, I think
you'll need to add a call to CLM::setNeedsGraphicsLayerUpdate. This gets called
when the scroll offset is changed so it might work in most cases but we should
probably do it explicitly when the CSS value changes. Is there another CSS
feature that needs to communicate behavior to the compositor like this so we can
reuse its path?

https://codereview.chromium.org/2769793002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyleConstants.h (right):

https://codereview.chromium.org/2769793002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyleConstants.h:533: enum
ScrollBoundaryBehavior {
You should add checks in Source/web/AssertMatchingEnums.cpp to assert that the
values of each enum in CC match the values of this Blink side enum.

https://codereview.chromium.org/2769793002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebScrollBoundaryBehavior.h (right):

https://codereview.chromium.org/2769793002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebScrollBoundaryBehavior.h:12:
WebScrollBoundaryBehaviorPropagate,
Also check these in Source/web/AssertMatchingEnums.cpp

[sunyunjia, 2017-03-23 20:11:58]

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

[commit-bot: I haz the power, 2017-03-23 20:12:25]

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

[commit-bot: I haz the power, 2017-03-23 20:15:48]

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

[commit-bot: I haz the power, 2017-03-23 20:15:48]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sunyunjia, 2017-03-23 20:16:55]

Hi David,

I updated the patch, and please see the comments. Perhaps I should add a
RuntimeEnabledFeature and a test for this？

https://codereview.chromium.org/2769793002/diff/20001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/2769793002/diff/20001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:84:
scroll_boundary_behavior_(SCROLL_BOUNDARY_BEHAVIOR_PROPAGATE) {
On 2017/03/23 13:18:26, bokan wrote:
> How does a LayerImpl get scroll_boundary_behavior_ set? I think you need to
add
> a line in Layer::PushPropertiesTo(LayerImpl*)

Yeah, that's added.

https://codereview.chromium.org/2769793002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2769793002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:3177: LayerImpl* layer =
On 2017/03/23 13:18:26, bokan wrote:
> Layers are eventually going away. Instead of propagating the boundary behavior
> to the LayerImpl, you should set it directly on the ScrollNode in
> PropertyTreeBuilder. See AddScrollNodeIfNeeded in property_tree_builder.cc.
You
> may need to keep a method in LayerImpl to match the one in Layer since the
> function is generic for layer types but it doesn't need to do anything since
the
> boundary behavior will always come from cc::Layer only.

Done.

https://codereview.chromium.org/2769793002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2769793002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1084:
updateScrollBoundaryBehavior();
On 2017/03/23 13:18:26, bokan wrote:
> Hmm...I'm not sure CLM::updateGraphicsLayerGeometry will get called if you
just
> change the scrollBoundaryBehavior. Do you know how that happens? If not, I
think
> you'll need to add a call to CLM::setNeedsGraphicsLayerUpdate. This gets
called
> when the scroll offset is changed so it might work in most cases but we should
> probably do it explicitly when the CSS value changes. Is there another CSS
> feature that needs to communicate behavior to the compositor like this so we
can
> reuse its path?

I checked the call stack, and found both CLM::updateGraphicsLayerGeometry and
CLM::setNeedsGraphicsLayerUpdate are called from
FrameView::updateAllLifecyclePhases

https://codereview.chromium.org/2769793002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyleConstants.h (right):

https://codereview.chromium.org/2769793002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyleConstants.h:533: enum
ScrollBoundaryBehavior {
On 2017/03/23 13:18:26, bokan wrote:
> You should add checks in Source/web/AssertMatchingEnums.cpp to assert that the
> values of each enum in CC match the values of this Blink side enum.

Done.

https://codereview.chromium.org/2769793002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebScrollBoundaryBehavior.h (right):

https://codereview.chromium.org/2769793002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebScrollBoundaryBehavior.h:12:
WebScrollBoundaryBehaviorPropagate,
On 2017/03/23 13:18:26, bokan wrote:
> Also check these in Source/web/AssertMatchingEnums.cpp

Feels like AssertMatchingEnums.cpp is checking WebXXX vs XXX, both in blink
side. 

Now I added a check in web_layer_impl.cc, as those are checked at
render_widget_compositor.cc, is it the right place to do that?

[sunyunjia, 2017-03-23 20:35:54]

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

[commit-bot: I haz the power, 2017-03-23 20:36:45]

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

[commit-bot: I haz the power, 2017-03-23 21:45:27]

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

[commit-bot: I haz the power, 2017-03-23 21:45:28]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[bokan, 2017-03-23 21:52:19]

Yup, we'll need REF and test to land this.

In addition, have you tried this with animated scrolls (using mouse wheel)? That
goes through LTHI::ScrollAnimated which doesn't use DistributeScroll. You'll
probably need to modify that path too.

And what's the plan for main thread? It's ok to do it separately, just wondering
if you've thought about it.

+ajuma@ for the question in property_tree_builder and as a double check on
scroll tree generally.

https://codereview.chromium.org/2769793002/diff/20001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/2769793002/diff/20001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:84:
scroll_boundary_behavior_(SCROLL_BOUNDARY_BEHAVIOR_PROPAGATE) {
On 2017/03/23 20:16:54, sunyunjia wrote:
> On 2017/03/23 13:18:26, bokan wrote:
> > How does a LayerImpl get scroll_boundary_behavior_ set? I think you need to
> add
> > a line in Layer::PushPropertiesTo(LayerImpl*)
> 
> Yeah, that's added.

Sorry, actually, now that you're storing the bit in ScrollNode, you don't
actually need to propagate this to LayerImpl. I think we can just return the
default value here and not worry about it, maybe DCHECK that it's not called?
+ajuma@, this value is only needed (and non-default from Blink) on cc::Layer to
build the scroll tree but the BuildPropertyTree functions demand both kinds of
layers have the same interface. What's the right way to handle this?

https://codereview.chromium.org/2769793002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebScrollBoundaryBehavior.h (right):

https://codereview.chromium.org/2769793002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebScrollBoundaryBehavior.h:12:
WebScrollBoundaryBehaviorPropagate,
On 2017/03/23 20:16:55, sunyunjia wrote:
> On 2017/03/23 13:18:26, bokan wrote:
> > Also check these in Source/web/AssertMatchingEnums.cpp
> 
> Feels like AssertMatchingEnums.cpp is checking WebXXX vs XXX, both in blink
> side. 
> 
> Now I added a check in web_layer_impl.cc, as those are checked at
> render_widget_compositor.cc, is it the right place to do that?

Actually, I think you can just get rid of the WebXXX version altogether. See how
this is done for WebKit/Source/platform/scroll/MainThreadScrollingReason.h. Then
you can just static_assert those vs the one defined for Style.

[sunyunjia, 2017-05-03 16:14:43]

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

[sunyunjia, 2017-05-03 16:14:44]

Hi Majid,

Here is the patch for scroll-boundary-behavior FYI. I'm still working on the
comments.

[sunyunjia, 2017-05-12 23:01:43]

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

[commit-bot: I haz the power, 2017-05-12 23:03:04]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-13 00:20:06]

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

[commit-bot: I haz the power, 2017-05-13 00:20:07]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sunyunjia, 2017-05-15 13:59:04]

Patchset #5 (id:80001) has been deleted

[sunyunjia, 2017-05-15 13:59:20]

Patchset #5 (id:100001) has been deleted

[sunyunjia, 2017-05-15 14:00:32]

Hi David,

I've updated the patch with main thread working and layout tests. PTAL, thanks!

[sunyunjia, 2017-05-18 14:28:54]

Hi David,

I've added the part to separate x and y axis. The patch is ready for review now.
Thanks!

[sunyunjia, 2017-05-18 14:29:05]

Patchset #8 (id:180001) has been deleted

[bokan, 2017-05-19 19:35:57]

https://codereview.chromium.org/2769793002/diff/220001/cc/blink/web_layer_impl.h
File cc/blink/web_layer_impl.h (right):

https://codereview.chromium.org/2769793002/diff/220001/cc/blink/web_layer_imp...
cc/blink/web_layer_impl.h:130: void
SetScrollBoundaryBehavior(blink::WebScrollBoundaryBehavior) override;
pass by const-ref

https://codereview.chromium.org/2769793002/diff/220001/cc/input/scroll_bounda...
File cc/input/scroll_boundary_behavior.h (right):

https://codereview.chromium.org/2769793002/diff/220001/cc/input/scroll_bounda...
cc/input/scroll_boundary_behavior.h:14: kScrollBoundaryBehaviorTypeAuto = 0x0,
You can leave the values off since they're the defaults.

https://codereview.chromium.org/2769793002/diff/220001/cc/input/scroll_bounda...
cc/input/scroll_boundary_behavior.h:22:
ScrollBoundaryBehavior(ScrollBoundaryBehaviorType type) : x(type), y(type) {}
Make this explicit.

https://codereview.chromium.org/2769793002/diff/220001/cc/input/scroll_bounda...
cc/input/scroll_boundary_behavior.h:34: return (a.x != x) || (a.y != y);
You can reuse operator==: return !(*this == a)

https://codereview.chromium.org/2769793002/diff/220001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/2769793002/diff/220001/cc/layers/layer.h#newc...
cc/layers/layer.h:118: void SetScrollBoundaryBehavior(ScrollBoundaryBehavior
behavior);
pass by const-ref

https://codereview.chromium.org/2769793002/diff/220001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/2769793002/diff/220001/cc/layers/layer_impl.h...
cc/layers/layer_impl.h:299: void
SetScrollBoundaryBehavior(ScrollBoundaryBehavior);
pass by const-ref

https://codereview.chromium.org/2769793002/diff/220001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2769793002/diff/220001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:3240: if
(scroll_node->scroll_boundary_behavior.x !=
I believe you'll also need similar logic in LTHI::ScrollAnimated which is used
by smooth wheel scrolls and is a separate path from this.

https://codereview.chromium.org/2769793002/diff/220001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:3247: applied_delta.set_y(delta.y());
I think we should do this in DistributeScrollDelta where we calculate the scroll
chain.

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html
(right):

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:1:
<!DOCTYPE html>
Please add a copy of this test that uses event sender so we test the main thread
paths too.

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:13:
var test = async_test('scroll-boundary-behavior-y: none should only prevent
overscroll propagation on y axis');
overscroll typically refers to scrolling a fully scrolled viewport. This should
be "prevent scroll propagation"

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:25:
assert_less_than(window.scrollX, 100);
Make this assert_equals - if you're using less_than because some of the scroll
is first consumed by the container - that'll change once Sahel's latching work
completes. In that case, I'd first scroll the container to its extent, that way
the test will continue to work when latching is enabled.

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp (right):

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp:967: case
CSSPropertyScrollBoundaryBehaviorX:
Nit: place these below CSSPropertyScrollBehavior, these are "almost"
alphabetically sorted.

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:533: }
Nit: no braces

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:598: if
(GetComputedStyle()->ScrollBoundaryBehaviorX() !=
Similarly, this should be done at the time that we compute the scroll chain in
ScrollManager::RecomputeScrollChain

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/UseCounter.cpp:1091: case
CSSPropertyScrollBoundaryBehaviorY:
You'll need matching changes in histograms.xml

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/ScrollManager.cpp (right):

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/ScrollManager.cpp:9: #include
"core/dom/Node.h"
Unneeded leftovers?

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.h (right):

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.h:281: void
SetScrollBoundaryBehavior(WebScrollBoundaryBehavior);
pass by const ref

[bokan, 2017-05-19 19:39:51]

In commit message:

>This is different from scroll latching, in the way that even when the scroller
has reached its extend, additional scroll should not be propagated to the parent
scroller.

Typo: extend -> extent

Reword this as I'm not sure if it's referring to latching or
scroll-boundary-behavior. I would say that when latching is turned on, a gesture
will be limited to a single element, but scroll-boundary-behavior will affect
propagation when the latch is being targeted at the beginning of the gesture.

[bokan, 2017-05-19 19:40:10]

Also, please add the BUG= field

[sunyunjia, 2017-05-25 19:44:54]

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

[commit-bot: I haz the power, 2017-05-25 19:45:35]

Dry run: CQ is trying da patch.

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

[sunyunjia, 2017-05-25 20:00:21]

Description was changed from

==========
Implement CSS: scroll-boundary-behavior.

CSS: scroll-boundary-behavior allows developers to specify whether the scroll
should be propagated to the parent scroller. This is different from scroll
latching, in the way that even when the scroller has reached its extend,
additional scroll should not be propagated to the parent scroller.

This patch implements scroll-boundary-behavior: contain, by cutting off the
scroll chain when the scrollNode has the property, so the scroll will not be
propagated. After overscroll-action is implemented, we will implement
scroll-boundary-behavior: none using overscroll-action's backend support. As
both of them are meant to disable the overscroll effects.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Implement CSS: scroll-boundary-behavior.

CSS: scroll-boundary-behavior allows developers to specify whether the scroll
should be propagated to the parent scroller. This is different from scroll
latching, in the way that even when the scroller has reached its extend,
additional scroll should not be propagated to the parent scroller.

This patch implements scroll-boundary-behavior: none, by cutting off the
scroll chain when the scrollNode has the property, so the scroll will not be
propagated. After overscroll-action is implemented, we will implement
scroll-boundary-behavior: contain using overscroll-action's backend support, as
both of them are meant to disable the overscroll effects.

BUG=672921
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[sunyunjia, 2017-05-25 20:04:06]

Description was changed from

==========
Implement CSS: scroll-boundary-behavior.

CSS: scroll-boundary-behavior allows developers to specify whether the scroll
should be propagated to the parent scroller. This is different from scroll
latching, in the way that even when the scroller has reached its extend,
additional scroll should not be propagated to the parent scroller.

This patch implements scroll-boundary-behavior: none, by cutting off the
scroll chain when the scrollNode has the property, so the scroll will not be
propagated. After overscroll-action is implemented, we will implement
scroll-boundary-behavior: contain using overscroll-action's backend support, as
both of them are meant to disable the overscroll effects.

BUG=672921
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Implement CSS: scroll-boundary-behavior.

CSS: scroll-boundary-behavior allows developers to specify whether the scroll
should be propagated to the parent scroller. This is different from scroll
latching, in the way that wehn latching is turned on, a gesture will be
limited to a single element, but scroll-boundary-behavior will affect
propagation when the latch is being targeted at the beginning of the gesture.

This patch implements scroll-boundary-behavior: none, by cutting off the
scroll chain when the scrollNode has the property, so the scroll will not be
propagated. After overscroll-action is implemented, we will implement
scroll-boundary-behavior: contain using overscroll-action's backend support, as
both of them are meant to disable the overscroll effects.

BUG=672921
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[sunyunjia, 2017-05-25 20:07:11]

I moved the compositor thread test to virtual/threaded/, where the tests are run
with --enable-threaded-compositing and --enable-prefer-compositing-to-lcd-text.

PTAL, thanks!

https://codereview.chromium.org/2769793002/diff/220001/cc/blink/web_layer_impl.h
File cc/blink/web_layer_impl.h (right):

https://codereview.chromium.org/2769793002/diff/220001/cc/blink/web_layer_imp...
cc/blink/web_layer_impl.h:130: void
SetScrollBoundaryBehavior(blink::WebScrollBoundaryBehavior) override;
On 2017/05/19 19:35:57, bokan wrote:
> pass by const-ref

Done.

https://codereview.chromium.org/2769793002/diff/220001/cc/input/scroll_bounda...
File cc/input/scroll_boundary_behavior.h (right):

https://codereview.chromium.org/2769793002/diff/220001/cc/input/scroll_bounda...
cc/input/scroll_boundary_behavior.h:14: kScrollBoundaryBehaviorTypeAuto = 0x0,
On 2017/05/19 19:35:57, bokan wrote:
> You can leave the values off since they're the defaults.

Done.

https://codereview.chromium.org/2769793002/diff/220001/cc/input/scroll_bounda...
cc/input/scroll_boundary_behavior.h:22:
ScrollBoundaryBehavior(ScrollBoundaryBehaviorType type) : x(type), y(type) {}
On 2017/05/19 19:35:57, bokan wrote:
> Make this explicit.

Done.

https://codereview.chromium.org/2769793002/diff/220001/cc/input/scroll_bounda...
cc/input/scroll_boundary_behavior.h:34: return (a.x != x) || (a.y != y);
On 2017/05/19 19:35:57, bokan wrote:
> You can reuse operator==: return !(*this == a)

Done.

https://codereview.chromium.org/2769793002/diff/220001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/2769793002/diff/220001/cc/layers/layer.h#newc...
cc/layers/layer.h:118: void SetScrollBoundaryBehavior(ScrollBoundaryBehavior
behavior);
On 2017/05/19 19:35:57, bokan wrote:
> pass by const-ref

Done.

https://codereview.chromium.org/2769793002/diff/220001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/2769793002/diff/220001/cc/layers/layer_impl.h...
cc/layers/layer_impl.h:299: void
SetScrollBoundaryBehavior(ScrollBoundaryBehavior);
On 2017/05/19 19:35:57, bokan wrote:
> pass by const-ref

Done.

https://codereview.chromium.org/2769793002/diff/220001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2769793002/diff/220001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:3240: if
(scroll_node->scroll_boundary_behavior.x !=
On 2017/05/19 19:35:57, bokan wrote:
> I believe you'll also need similar logic in LTHI::ScrollAnimated which is used
> by smooth wheel scrolls and is a separate path from this.

Done.

https://codereview.chromium.org/2769793002/diff/220001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:3247: applied_delta.set_y(delta.y());
On 2017/05/19 19:35:57, bokan wrote:
> I think we should do this in DistributeScrollDelta where we calculate the
scroll
> chain.

Done.

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html
(right):

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:1:
<!DOCTYPE html>
On 2017/05/19 19:35:57, bokan wrote:
> Please add a copy of this test that uses event sender so we test the main
thread
> paths too.

Done.

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:13:
var test = async_test('scroll-boundary-behavior-y: none should only prevent
overscroll propagation on y axis');
On 2017/05/19 19:35:57, bokan wrote:
> overscroll typically refers to scrolling a fully scrolled viewport. This
should
> be "prevent scroll propagation"

Done.

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:25:
assert_less_than(window.scrollX, 100);
On 2017/05/19 19:35:57, bokan wrote:
> Make this assert_equals - if you're using less_than because some of the scroll
> is first consumed by the container - that'll change once Sahel's latching work
> completes. In that case, I'd first scroll the container to its extent, that
way
> the test will continue to work when latching is enabled.

Done.

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp (right):

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp:967: case
CSSPropertyScrollBoundaryBehaviorX:
On 2017/05/19 19:35:57, bokan wrote:
> Nit: place these below CSSPropertyScrollBehavior, these are "almost"
> alphabetically sorted. 

Done.

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:533: }
On 2017/05/19 19:35:57, bokan wrote:
> Nit: no braces

Done.

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:598: if
(GetComputedStyle()->ScrollBoundaryBehaviorX() !=
On 2017/05/19 19:35:57, bokan wrote:
> Similarly, this should be done at the time that we compute the scroll chain in
> ScrollManager::RecomputeScrollChain

Done.

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/UseCounter.cpp:1091: case
CSSPropertyScrollBoundaryBehaviorY:
On 2017/05/19 19:35:57, bokan wrote:
> You'll need matching changes in histograms.xml

Done.

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/ScrollManager.cpp (right):

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/ScrollManager.cpp:9: #include
"core/dom/Node.h"
On 2017/05/19 19:35:57, bokan wrote:
> Unneeded leftovers?

Done.

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.h (right):

https://codereview.chromium.org/2769793002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.h:281: void
SetScrollBoundaryBehavior(WebScrollBoundaryBehavior);
On 2017/05/19 19:35:57, bokan wrote:
> pass by const ref

Done.

[sunyunjia, 2017-05-25 20:31:58]

Presubmit Warnings says "eventSender is deprecated, please use
chrome.gpuBenchmarking.pointerActionSequence instead". Does this apply to our
patch?

[commit-bot: I haz the power, 2017-05-25 21:08:56]

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

[commit-bot: I haz the power, 2017-05-25 21:08:58]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bokan, 2017-05-26 14:50:06]

On 2017/05/25 20:31:58, sunyunjia wrote:
> Presubmit Warnings says "eventSender is deprecated, please use
> chrome.gpuBenchmarking.pointerActionSequence instead". Does this apply to our
> patch?

hmm, you can ask lanwei@ since she added that warning. But you mentioned the
gpuBenchmarking version was hitting the main thread until you added it to the
virtual test suite. If you move
virtual/threaded/fast/scroll-behavior/scroll-boundary-behavior.html to
fast/scroll-behavior/scroll-boundary-behavior.html it should get run both as
part of the VTS and as part of normal tests so it should hit both code paths,
right? If you can confirm that, just remove the event sender version and I think
we should be good.

[sunyunjia, 2017-05-26 15:04:47]

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

[commit-bot: I haz the power, 2017-05-26 15:05:06]

Dry run: CQ is trying da patch.

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

[sunyunjia, 2017-05-26 15:49:20]

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

[commit-bot: I haz the power, 2017-05-26 15:49:36]

Dry run: CQ is trying da patch.

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

[sunyunjia, 2017-05-26 15:53:27]

On 2017/05/26 14:50:06, bokan wrote:
> On 2017/05/25 20:31:58, sunyunjia wrote:
> > Presubmit Warnings says "eventSender is deprecated, please use
> > chrome.gpuBenchmarking.pointerActionSequence instead". Does this apply to
our
> > patch?
> 
> hmm, you can ask lanwei@ since she added that warning. But you mentioned the
> gpuBenchmarking version was hitting the main thread until you added it to the
> virtual test suite. If you move
> virtual/threaded/fast/scroll-behavior/scroll-boundary-behavior.html to
> fast/scroll-behavior/scroll-boundary-behavior.html it should get run both as
> part of the VTS and as part of normal tests so it should hit both code paths,
> right? If you can confirm that, just remove the event sender version and I
think
> we should be good.

Just confirmed that with Dave. Now I have only the gpuBenchmarking test under
fast/scroll-behavior. It should run on both main and compositor thread.

[bokan, 2017-05-26 16:32:03]

Typo in commit message: "in the way that wehn latching is turned on": wehn ->
when

> Just confirmed that with Dave. Now I have only the gpuBenchmarking test under
> fast/scroll-behavior. It should run on both main and compositor thread.

Excellent, thanks and sorry for leading you down the deprecated path.

lgtm

https://codereview.chromium.org/2769793002/diff/280001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html
(right):

https://codereview.chromium.org/2769793002/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:20:
chrome.gpuBenchmarking.smoothDrag(100, 400, 100, 500,
Please file a bug to add wheel scrolling to gpuBenchmarking and add a matching
test that uses wheels. Currently, this wont exercise the ScrollAnimated path at
all - but I don't want to block this any more as I'm sure rebasing is getting
tedious. We should do that shortly after landing this though as wheel scrolling
is the primary mode on desktop so we need test coverage there.

Please make sure you try your changes manually using an mouse wheel that smooth
scrolls.

[commit-bot: I haz the power, 2017-05-26 16:58:39]

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

[commit-bot: I haz the power, 2017-05-26 16:58:40]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sunyunjia, 2017-05-28 20:55:18]

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

[commit-bot: I haz the power, 2017-05-28 20:55:28]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-28 23:27:33]

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

[commit-bot: I haz the power, 2017-05-28 23:27:35]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sunyunjia, 2017-05-29 01:20:04]

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

[commit-bot: I haz the power, 2017-05-29 01:20:16]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-29 04:00:03]

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

[commit-bot: I haz the power, 2017-05-29 04:00:04]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sunyunjia, 2017-05-29 14:26:42]

Description was changed from

==========
Implement CSS: scroll-boundary-behavior.

CSS: scroll-boundary-behavior allows developers to specify whether the scroll
should be propagated to the parent scroller. This is different from scroll
latching, in the way that wehn latching is turned on, a gesture will be
limited to a single element, but scroll-boundary-behavior will affect
propagation when the latch is being targeted at the beginning of the gesture.

This patch implements scroll-boundary-behavior: none, by cutting off the
scroll chain when the scrollNode has the property, so the scroll will not be
propagated. After overscroll-action is implemented, we will implement
scroll-boundary-behavior: contain using overscroll-action's backend support, as
both of them are meant to disable the overscroll effects.

BUG=672921
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Implement CSS: scroll-boundary-behavior.

CSS: scroll-boundary-behavior allows developers to specify whether the scroll
should be propagated to the parent scroller. This is different from scroll
latching, in the way that when latching is turned on, a gesture will be
limited to a single element, but scroll-boundary-behavior will affect
propagation when the latch is being targeted at the beginning of the gesture.

This patch implements scroll-boundary-behavior: none, by cutting off the
scroll chain when the scrollNode has the property, so the scroll will not be
propagated. After overscroll-action is implemented, we will implement
scroll-boundary-behavior: contain using overscroll-action's backend support, as
both of them are meant to disable the overscroll effects.

BUG=672921
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[sunyunjia, 2017-05-29 14:29:23]

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

[commit-bot: I haz the power, 2017-05-29 14:29:39]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-29 17:00:48]

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

[commit-bot: I haz the power, 2017-05-29 17:00:49]

Dry run: This issue passed the CQ dry run.

[sunyunjia, 2017-06-27 20:47:43]

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

[commit-bot: I haz the power, 2017-06-27 20:48:04]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-06-27 20:58:18]

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

[commit-bot: I haz the power, 2017-06-27 20:58:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sunyunjia, 2017-06-28 13:16:23]

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

[commit-bot: I haz the power, 2017-06-28 13:16:38]

Dry run: CQ is trying da patch.

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

[sunyunjia, 2017-06-28 13:35:00]

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

[sunyunjia, 2017-06-28 13:35:15]

Patchset #20 (id:440001) has been deleted

[commit-bot: I haz the power, 2017-06-28 13:35:15]

Dry run: CQ is trying da patch.

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

[sunyunjia, 2017-06-28 13:35:36]

Patchset #20 (id:440001) has been deleted

[sunyunjia, 2017-06-28 13:55:52]

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

[commit-bot: I haz the power, 2017-06-28 13:56:11]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-06-28 14:07:17]

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

[commit-bot: I haz the power, 2017-06-28 14:07:18]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sunyunjia, 2017-06-28 15:51:29]

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

[sunyunjia, 2017-06-28 15:51:38]

Patchset #21 (id:480001) has been deleted

[commit-bot: I haz the power, 2017-06-28 15:51:43]

Dry run: CQ is trying da patch.

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

[sunyunjia, 2017-06-28 15:53:08]

Patchset #20 (id:460001) has been deleted

[commit-bot: I haz the power, 2017-06-28 16:58:17]

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

[commit-bot: I haz the power, 2017-06-28 16:58:18]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sunyunjia, 2017-06-28 17:14:12]

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

[sunyunjia, 2017-06-28 17:14:26]

Patchset #20 (id:500001) has been deleted

[commit-bot: I haz the power, 2017-06-28 17:14:37]

Dry run: CQ is trying da patch.

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

[sunyunjia, 2017-06-28 17:32:07]

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

[commit-bot: I haz the power, 2017-06-28 17:32:34]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-06-28 19:16:06]

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

[commit-bot: I haz the power, 2017-06-28 19:16:08]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[sunyunjia, 2017-06-29 16:01:12]

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

[commit-bot: I haz the power, 2017-06-29 16:01:30]

Dry run: CQ is trying da patch.

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

[sunyunjia, 2017-06-29 18:02:19]

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

[commit-bot: I haz the power, 2017-06-29 18:02:37]

Dry run: CQ is trying da patch.

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

[sunyunjia, 2017-06-29 18:05:26]

Hi David,

There have been quite a lot of rebases since last review. However, the patch
only changes in layer_tree_host_impl.cc and ScrollManager.cpp. I also updated
the layout test with better format. PTAL, thanks!

[commit-bot: I haz the power, 2017-06-29 19:57:17]

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

[commit-bot: I haz the power, 2017-06-29 19:57:18]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sunyunjia, 2017-06-29 20:21:00]

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

[commit-bot: I haz the power, 2017-06-29 20:21:28]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-06-29 21:22:20]

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

[commit-bot: I haz the power, 2017-06-29 21:22:21]

Dry run: This issue passed the CQ dry run.

[bokan, 2017-06-30 15:44:10]

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

[bokan, 2017-06-30 15:44:12]

On 2017/06/29 18:05:26, sunyunjia wrote:
> Hi David,
> 
> There have been quite a lot of rebases since last review. However, the patch
> only changes in layer_tree_host_impl.cc and ScrollManager.cpp. I also updated
> the layout test with better format. PTAL, thanks!

Thanks, looks fine to me but I have a few nits and some suggestions in the test.
Also,

> Just confirmed that with Dave. Now I have only the gpuBenchmarking test under
> fast/scroll-behavior. It should run on both main and compositor thread.

I'm not sure that we've actually got main thread coverage with this test. It'll
run with and without the 

+sahel@ as FYI and to double-check the latching interaction in
ScrollManager/LTHI

https://codereview.chromium.org/2769793002/diff/600001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2769793002/diff/600001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:3058: bool x_dominated =
(std::abs(scroll_delta.x()) > std::abs(scroll_delta.y()));
nit: no need for outer parentheses

https://codereview.chromium.org/2769793002/diff/600001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:3382: (std::abs(scroll_state->delta_x()) >
std::abs(scroll_state->delta_y()));
ditto

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html
(right):

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:5:
<div id='container' style='height: 400px; width: 500px; overflow: scroll;
will-change: transform'>
Please remove will-change: transform. This forces the scroller to be composited.
I think it technically doesn't matter since you'll still test the main thread
path in the non-virtual test suite but the virtual/threaded/fast/scroll-behavior
tests run with --enable-prefer-compositing-to-lcd-text which should cause this
scroller to be composited anyway. It'd be better to let the normal test not
composite this. (If for some reason that doesn't work (ensure we're hitting both
main and impl paths) then I wouldn't worry about it.

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:7:
<div id='blue' style='height: 300px; background-color: blue; will-change:
transform'></div>
Nit: These inner <div>s don't need `will-change:transform`

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:29:
// We requestAnimationFrame either for 500 frames or until 20 frames with
If this is a callback that's called after the gesture is completed, why do we
have to wait?

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:57:
if (source_type == TOUCH_SOURCE_TYPE) {
make begin_type always start with WHEEL and move the `indexOf('Max')` check in
here so that it's more clear that we avoid the TOUCH test on Mac.

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:59:
test.done();
test.done() should be called first, no? (below too)

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:125:
chrome.gpuBenchmarking.smoothScrollBy(200, waitForAnimationEnd.bind(this,  
checkChangeAndFinish), 100, 400, source_type, "upleft", 4000);
nit: extra spaces between `this,   checkChangeAndFinish`

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/ScrollManager.cpp (right):

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/ScrollManager.cpp:97: bool x_dominated =
(std::abs(scroll_state.deltaXHint()) >
nit: no need for outer parenthesis

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/ScrollManager.cpp:130: break;
We end up railing the actual scroll updates to one axis or the other, right? So
if the GSB is diagonal but "mostly x", then we change direction and become
mostly y, we won't actually scroll y, right?

[sunyunjia, 2017-06-30 18:16:19]

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

[commit-bot: I haz the power, 2017-06-30 18:16:35]

Dry run: CQ is trying da patch.

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

[sunyunjia, 2017-06-30 18:26:54]

sunyunjia@chromium.org changed reviewers:
+ ajuma@chromium.org, rbyers@chromium.org

[sunyunjia, 2017-06-30 18:26:58]

David: I've updated the patch according to the comments. As I tested locally,
the test runs on main thread from fast/.., and runs on cc side from
virtual/threaded/fast/...

Ali: Could you please review the code under cc/ ?

Rick: Could you please review the code under third_party/WebKit/Source/platform/
and
third_party/WebKit/public/platform/ ?

Thanks!

https://codereview.chromium.org/2769793002/diff/600001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2769793002/diff/600001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:3058: bool x_dominated =
(std::abs(scroll_delta.x()) > std::abs(scroll_delta.y()));
On 2017/06/30 15:44:11, bokan wrote:
> nit: no need for outer parentheses

Done.

https://codereview.chromium.org/2769793002/diff/600001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:3382: (std::abs(scroll_state->delta_x()) >
std::abs(scroll_state->delta_y()));
On 2017/06/30 15:44:11, bokan wrote:
> ditto

Done.

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html
(right):

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:5:
<div id='container' style='height: 400px; width: 500px; overflow: scroll;
will-change: transform'>
On 2017/06/30 15:44:12, bokan wrote:
> Please remove will-change: transform. This forces the scroller to be
composited.
> I think it technically doesn't matter since you'll still test the main thread
> path in the non-virtual test suite but the
virtual/threaded/fast/scroll-behavior
> tests run with --enable-prefer-compositing-to-lcd-text which should cause this
> scroller to be composited anyway. It'd be better to let the normal test not
> composite this. (If for some reason that doesn't work (ensure we're hitting
both
> main and impl paths) then I wouldn't worry about it.

Done.

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:7:
<div id='blue' style='height: 300px; background-color: blue; will-change:
transform'></div>
On 2017/06/30 15:44:11, bokan wrote:
> Nit: These inner <div>s don't need `will-change:transform`

Done.

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:29:
// We requestAnimationFrame either for 500 frames or until 20 frames with
On 2017/06/30 15:44:12, bokan wrote:
> If this is a callback that's called after the gesture is completed, why do we
> have to wait?

We don't need to wait for touch, but need to wait for the wheel event to finish
the animation.

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:57:
if (source_type == TOUCH_SOURCE_TYPE) {
On 2017/06/30 15:44:11, bokan wrote:
> make begin_type always start with WHEEL and move the `indexOf('Max')` check in
> here so that it's more clear that we avoid the TOUCH test on Mac.

Done.

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:59:
test.done();
On 2017/06/30 15:44:12, bokan wrote:
> test.done() should be called first, no? (below too)

The later tests won't run if I call test.done() first.

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:125:
chrome.gpuBenchmarking.smoothScrollBy(200, waitForAnimationEnd.bind(this,  
checkChangeAndFinish), 100, 400, source_type, "upleft", 4000);
On 2017/06/30 15:44:11, bokan wrote:
> nit: extra spaces between `this,   checkChangeAndFinish`

Done.

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/ScrollManager.cpp (right):

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/ScrollManager.cpp:97: bool x_dominated =
(std::abs(scroll_state.deltaXHint()) >
On 2017/06/30 15:44:12, bokan wrote:
> nit: no need for outer parenthesis

Done.

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/ScrollManager.cpp:130: break;
On 2017/06/30 15:44:12, bokan wrote:
> We end up railing the actual scroll updates to one axis or the other, right?
So
> if the GSB is diagonal but "mostly x", then we change direction and become
> mostly y, we won't actually scroll y, right? 

That is right on blink side(ScrollManager). Correct me if I'm wrong. I think the
behavior is same with latching? i.e. once we latch to a scroller at scrollbegin,
we won't change the scrolling element no matter how we change the scroll
direction in GSU. Because RecomputeScrollChain is only called on GSB.

However, on cc side, we update the scroll chain with GSU, as
DistributedScrollDelta is called by both GSB and GSU.

[bokan, 2017-06-30 18:55:36]

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html
(right):

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:29:
// We requestAnimationFrame either for 500 frames or until 20 frames with
On 2017/06/30 18:26:58, sunyunjia wrote:
> On 2017/06/30 15:44:12, bokan wrote:
> > If this is a callback that's called after the gesture is completed, why do
we
> > have to wait?
> 
> We don't need to wait for touch, but need to wait for the wheel event to
finish
> the animation.

We should turn off wheel animations, they're unrelated to what we're trying to
test and can only add flakiness. I think you should be able to do that with
internals.settings.setScrollAnimatorEnabled

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/ScrollManager.cpp (right):

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/ScrollManager.cpp:130: break;
On 2017/06/30 18:26:58, sunyunjia wrote:
> On 2017/06/30 15:44:12, bokan wrote:
> > We end up railing the actual scroll updates to one axis or the other, right?
> So
> > if the GSB is diagonal but "mostly x", then we change direction and become
> > mostly y, we won't actually scroll y, right? 
> 
> That is right on blink side(ScrollManager). Correct me if I'm wrong. I think
the
> behavior is same with latching? i.e. once we latch to a scroller at
scrollbegin,
> we won't change the scrolling element no matter how we change the scroll
> direction in GSU. Because RecomputeScrollChain is only called on GSB.
> 
> However, on cc side, we update the scroll chain with GSU, as
> DistributedScrollDelta is called by both GSB and GSU.

Sahel can confirm. But my question wasn't so much about latching but about how
the GSUs will look like. My concern is that if you start by scrolling
horizontally, you might allow going up in the scrollchain because
scroll-boundary-behavior-y: none but x:auto. Then if you switch directions and
get GSUs with vertical scroll, you'll end up chaining scrolls vertically even
though s-b-b-y: none.

[commit-bot: I haz the power, 2017-06-30 20:42:35]

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

[commit-bot: I haz the power, 2017-06-30 20:42:36]

Dry run: This issue passed the CQ dry run.

[ajuma, 2017-06-30 20:46:49]

cc changes look good overall. Would it make sense to write a
LayerTreeHostScrollTest (or some other kind of cc unit test) for the logic added
to LayerTreeHostImpl?

https://codereview.chromium.org/2769793002/diff/620001/cc/input/scroll_bounda...
File cc/input/scroll_boundary_behavior.h (right):

https://codereview.chromium.org/2769793002/diff/620001/cc/input/scroll_bounda...
cc/input/scroll_boundary_behavior.h:16: kScrollBoundaryBehaviorTypeNone
Please add brief comments about what these values mean.

https://codereview.chromium.org/2769793002/diff/620001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2769793002/diff/620001/cc/layers/layer.cc#new...
cc/layers/layer.cc:314: inputs_.scroll_boundary_behavior = behavior;
This also needs to update the scroll node if there is one, and otherwise trigger
a property tree rebuild (see SetUserScrollable for an example of this).

https://codereview.chromium.org/2769793002/diff/620001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/2769793002/diff/620001/cc/layers/layer_impl.h...
cc/layers/layer_impl.h:297: return scroll_boundary_behavior_;
Does LayerImpl need to have a copy of this value, or is it enough for ScrollNode
to have it? If it's needed just for tests, it can be moved to
LayerImplTestProperties.

https://codereview.chromium.org/2769793002/diff/620001/cc/trees/property_tree...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/2769793002/diff/620001/cc/trees/property_tree...
cc/trees/property_tree_builder.cc:1053: node.scroll_boundary_behavior =
layer->scroll_boundary_behavior();
After removing scroll_boundary_behavior from LayerImpl, this will need to be
changed to use a helper function (e.g. like we have for
UserScrollableHorizontal).

[majidvp, 2017-07-05 21:30:03]

[I still need to review the layout tests but here are my
comments so far]

https://codereview.chromium.org/2769793002/diff/620001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2769793002/diff/620001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:3058: bool x_dominated =
std::abs(scroll_delta.x()) > std::abs(scroll_delta.y());
nit: x_dominant is more appropriate. Also we should add a comment
explaining why we look at the dominant axis when we have to decide
how to handle the propagation. It is not immediately obvious.

https://codereview.chromium.org/2769793002/diff/620001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:3145: break;
Please move this to its own function e.g., CanBubbleScroll(node, delta).

Should help readability, and also can be reused below.

https://codereview.chromium.org/2769793002/diff/620001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:3403:
current_scroll_chain.push_front(scroll_node);
Do I understand this correctly that the reason the previous early out
is removed is to allow the case where the node is not "scrollable"
but has a boundary behavior. 

If this is so, is this even possible?

https://codereview.chromium.org/2769793002/diff/620001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:3411: break;
ditto. This should be a function called from both places.

https://codereview.chromium.org/2769793002/diff/620001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2769793002/diff/620001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1156:
WebScrollBoundaryBehavior::ScrollBoundaryBehaviorType;
nit: I don't feel like this using declaration is helping much with
the readability of this code. It is just saving typing
'WebScrollBoundaryBehavior' twice below. Also the naming is confusing
as it removes the Type prefix. I suggest not having it and just specifying the
exact type below.

[majidvp, 2017-07-06 19:44:00]

https://codereview.chromium.org/2769793002/diff/620001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html
(right):

https://codereview.chromium.org/2769793002/diff/620001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:3:
<script src="../../resources/testharnessreport.js"></script>
nit: blank line

https://codereview.chromium.org/2769793002/diff/620001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:5:
<div id='container' style='height: 400px; width: 500px; overflow: scroll;'>
Best to move these styles into a <style> element.

Also it seems possible to dedup multiple of them with a common
class (e.g., heigh:300px; width:1000px)

https://codereview.chromium.org/2769793002/diff/620001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:7:
<div id='blue' style='height: 300px; background-color: blue;'></div>
What is the significant of these colors? Why even have two?

https://codereview.chromium.org/2769793002/diff/620001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:9:
<div id='space2' style='height: 1000px;'></div>
why do you need two 'space' div one before and one after?

https://codereview.chromium.org/2769793002/diff/620001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:71:
assert_equals(window.scrollX, 100);
The above 4 lines are just a setup and repeated for all tests. 
Please create a function e.g., setUp that does this and use that.

In fact, testharness.js has some way[1] to define a setup but I am
not sure if it works as expected for async tests. Feel free to 
use that if it works.

[1] http://web-platform-tests.org/writing-tests/testharness-api.html#setup

https://codereview.chromium.org/2769793002/diff/620001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:85:
test_x(TOUCH_SOURCE_TYPE);
I have difficulty reading and understanding what this test is trying to do. The
main problem I
encountered is that the actual test logics are intermixed with async control
flow to sequence
multiple tests. In particular, test_y triggers another test_y or test_x based on
what test_y
occurred before.

I suggest these two concerns be separated. A neat way to do this is to have the
test functions
return a promise and then start next test once the first test promise is
resolved e

test_y(wheel).then(()=> {
  test_y(touch).then( () => {... }
}

In fact, if you do it that way you can use |promise_test| facility from
test_harness which does
exactly what you need [1]. So you can just do: 
promise_test(test_y('wheel'));
promise_test(test_y('touch'));
promise_test(test_x('wheel'));
...

And your test function will return a promise instead for example:

test_y = function(type) {
  return new Promise(resolve => {
    // do async work that calls back to waitForAnimationEnd.bind(this,  resolve)
    // which resolves this promise.
  }).then(() => {
     // verify result by asserts 
  });
}



[1] http://web-platform-tests.org/writing-tests/testharness-api.html
"Unlike Asynchronous Tests, Promise Tests don’t start running until after
the previous Promise Test finishes."

https://codereview.chromium.org/2769793002/diff/620001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:104:
function test_scroll(source_type) {
The function names test_x, test_y, test_scroll do no match.
The first two are test cases for when scroll is large enough
to cause the boundary action to get triggered while last one
does not do so.

Perhaps: 
test_boundary_scroll_x
test_boundary_scroll_y
test_none_boundary_scroll

[sunyunjia, 2017-07-12 19:36:17]

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

[commit-bot: I haz the power, 2017-07-12 19:36:52]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-07-12 19:52:20]

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

[commit-bot: I haz the power, 2017-07-12 19:52:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sunyunjia, 2017-07-13 00:59:04]

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

[commit-bot: I haz the power, 2017-07-13 00:59:29]

Dry run: CQ is trying da patch.

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

[sunyunjia, 2017-07-13 01:13:55]

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

[commit-bot: I haz the power, 2017-07-13 01:14:12]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-07-13 01:29:58]

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

[commit-bot: I haz the power, 2017-07-13 01:30:00]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[sunyunjia, 2017-07-13 02:47:08]

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

[commit-bot: I haz the power, 2017-07-13 02:47:35]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-07-13 04:56:13]

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

[commit-bot: I haz the power, 2017-07-13 04:56:14]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[sunyunjia, 2017-07-13 14:32:57]

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

[commit-bot: I haz the power, 2017-07-13 14:33:12]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-07-13 16:08:24]

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

[commit-bot: I haz the power, 2017-07-13 16:08:26]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[sunyunjia, 2017-07-13 20:19:26]

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

[commit-bot: I haz the power, 2017-07-13 20:19:52]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-07-14 01:04:29]

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

[commit-bot: I haz the power, 2017-07-14 01:04:30]

Dry run: This issue passed the CQ dry run.

[sunyunjia, 2017-07-14 02:59:01]

I've fixed most of the comments. PTAL, thanks!

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html
(right):

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:29:
// We requestAnimationFrame either for 500 frames or until 20 frames with
On 2017/06/30 18:55:36, bokan wrote:
> On 2017/06/30 18:26:58, sunyunjia wrote:
> > On 2017/06/30 15:44:12, bokan wrote:
> > > If this is a callback that's called after the gesture is completed, why do
> we
> > > have to wait?
> > 
> > We don't need to wait for touch, but need to wait for the wheel event to
> finish
> > the animation.
> 
> We should turn off wheel animations, they're unrelated to what we're trying to
> test and can only add flakiness. I think you should be able to do that with
> internals.settings.setScrollAnimatorEnabled

I think it can help test the ScrollAnimated() branch?

I tried internals.settings.setScrollAnimatorEnabled(false) and removed the
animation, both touch_type and wheel_type passed on main thread but failed on
impl thread. I did some codesearch and noticed that internals.settings.XXX are
mainly used for eventSender. I guess this setting only works for main thread.

https://codereview.chromium.org/2769793002/diff/620001/cc/input/scroll_bounda...
File cc/input/scroll_boundary_behavior.h (right):

https://codereview.chromium.org/2769793002/diff/620001/cc/input/scroll_bounda...
cc/input/scroll_boundary_behavior.h:16: kScrollBoundaryBehaviorTypeNone
On 2017/06/30 20:46:49, ajuma wrote:
> Please add brief comments about what these values mean.

Done.

https://codereview.chromium.org/2769793002/diff/620001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2769793002/diff/620001/cc/layers/layer.cc#new...
cc/layers/layer.cc:314: inputs_.scroll_boundary_behavior = behavior;
On 2017/06/30 20:46:49, ajuma wrote:
> This also needs to update the scroll node if there is one, and otherwise
trigger
> a property tree rebuild (see SetUserScrollable for an example of this).

Done.

https://codereview.chromium.org/2769793002/diff/620001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/2769793002/diff/620001/cc/layers/layer_impl.h...
cc/layers/layer_impl.h:297: return scroll_boundary_behavior_;
On 2017/06/30 20:46:49, ajuma wrote:
> Does LayerImpl need to have a copy of this value, or is it enough for
ScrollNode
> to have it? If it's needed just for tests, it can be moved to
> LayerImplTestProperties.

Done.

https://codereview.chromium.org/2769793002/diff/620001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2769793002/diff/620001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:3058: bool x_dominated =
std::abs(scroll_delta.x()) > std::abs(scroll_delta.y());
On 2017/07/05 21:30:03, majidvp wrote:
> nit: x_dominant is more appropriate. Also we should add a comment
> explaining why we look at the dominant axis when we have to decide
> how to handle the propagation. It is not immediately obvious.

Done.

https://codereview.chromium.org/2769793002/diff/620001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:3145: break;
On 2017/07/05 21:30:03, majidvp wrote:
> Please move this to its own function e.g., CanBubbleScroll(node, delta).
> 
> Should help readability, and also can be reused below.
>  

Done.

https://codereview.chromium.org/2769793002/diff/620001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:3403:
current_scroll_chain.push_front(scroll_node);
On 2017/07/05 21:30:03, majidvp wrote:
> Do I understand this correctly that the reason the previous early out
> is removed is to allow the case where the node is not "scrollable"
> but has a boundary behavior. 
> 
> If this is so, is this even possible?

Done.

https://codereview.chromium.org/2769793002/diff/620001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:3411: break;
On 2017/07/05 21:30:03, majidvp wrote:
> ditto. This should be a function called from both places.

Done.

https://codereview.chromium.org/2769793002/diff/620001/cc/trees/property_tree...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/2769793002/diff/620001/cc/trees/property_tree...
cc/trees/property_tree_builder.cc:1053: node.scroll_boundary_behavior =
layer->scroll_boundary_behavior();
On 2017/06/30 20:46:49, ajuma wrote:
> After removing scroll_boundary_behavior from LayerImpl, this will need to be
> changed to use a helper function (e.g. like we have for
> UserScrollableHorizontal).

Done.

https://codereview.chromium.org/2769793002/diff/620001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html
(right):

https://codereview.chromium.org/2769793002/diff/620001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:3:
<script src="../../resources/testharnessreport.js"></script>
On 2017/07/06 19:43:59, majidvp wrote:
> nit: blank line

Done.

https://codereview.chromium.org/2769793002/diff/620001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:5:
<div id='container' style='height: 400px; width: 500px; overflow: scroll;'>
On 2017/07/06 19:43:59, majidvp wrote:
> Best to move these styles into a <style> element.
> 
> Also it seems possible to dedup multiple of them with a common
> class (e.g., heigh:300px; width:1000px)

Done.

https://codereview.chromium.org/2769793002/diff/620001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:7:
<div id='blue' style='height: 300px; background-color: blue;'></div>
On 2017/07/06 19:43:59, majidvp wrote:
> What is the significant of these colors? Why even have two?

Thanks for pointing this out. They were originally for manually debugging. I've
removed them.

https://codereview.chromium.org/2769793002/diff/620001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:9:
<div id='space2' style='height: 1000px;'></div>
On 2017/07/06 19:43:59, majidvp wrote:
> why do you need two 'space' div one before and one after?

ditto

https://codereview.chromium.org/2769793002/diff/620001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:71:
assert_equals(window.scrollX, 100);
On 2017/07/06 19:43:59, majidvp wrote:
> The above 4 lines are just a setup and repeated for all tests. 
> Please create a function e.g., setUp that does this and use that.
> 
> In fact, testharness.js has some way[1] to define a setup but I am
> not sure if it works as expected for async tests. Feel free to 
> use that if it works.
> 
> [1] http://web-platform-tests.org/writing-tests/testharness-api.html#setup

Done.

https://codereview.chromium.org/2769793002/diff/620001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:85:
test_x(TOUCH_SOURCE_TYPE);
On 2017/07/06 19:43:59, majidvp wrote:
> I have difficulty reading and understanding what this test is trying to do.
The
> main problem I
> encountered is that the actual test logics are intermixed with async control
> flow to sequence
> multiple tests. In particular, test_y triggers another test_y or test_x based
on
> what test_y
> occurred before.
> 
> I suggest these two concerns be separated. A neat way to do this is to have
the
> test functions
> return a promise and then start next test once the first test promise is
> resolved e
> 
> test_y(wheel).then(()=> {
>   test_y(touch).then( () => {... }
> }
> 
> In fact, if you do it that way you can use |promise_test| facility from
> test_harness which does
> exactly what you need [1]. So you can just do: 
> promise_test(test_y('wheel'));
> promise_test(test_y('touch'));
> promise_test(test_x('wheel'));
> ...
> 
> And your test function will return a promise instead for example:
> 
> test_y = function(type) {
>   return new Promise(resolve => {
>     // do async work that calls back to waitForAnimationEnd.bind(this, 
resolve)
>     // which resolves this promise.
>   }).then(() => {
>      // verify result by asserts 
>   });
> }
> 
> 
> 
> [1] http://web-platform-tests.org/writing-tests/testharness-api.html
> "Unlike Asynchronous Tests, Promise Tests don’t start running until after
> the previous Promise Test finishes."

Done.

https://codereview.chromium.org/2769793002/diff/620001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:104:
function test_scroll(source_type) {
On 2017/07/06 19:43:59, majidvp wrote:
> The function names test_x, test_y, test_scroll do no match.
> The first two are test cases for when scroll is large enough
> to cause the boundary action to get triggered while last one
> does not do so.
> 
> Perhaps: 
> test_boundary_scroll_x
> test_boundary_scroll_y
> test_none_boundary_scroll

Done.

https://codereview.chromium.org/2769793002/diff/620001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2769793002/diff/620001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:1156:
WebScrollBoundaryBehavior::ScrollBoundaryBehaviorType;
On 2017/07/05 21:30:03, majidvp wrote:
> nit: I don't feel like this using declaration is helping much with
> the readability of this code. It is just saving typing
> 'WebScrollBoundaryBehavior' twice below. Also the naming is confusing
> as it removes the Type prefix. I suggest not having it and just specifying the
> exact type below.

Done.

[majidvp, 2017-07-14 14:29:44]

The layout test looks much more readable! 
Thanks for clean up. :)

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html
(right):

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:29:
// We requestAnimationFrame either for 500 frames or until 20 frames with
On 2017/07/14 02:59:00, sunyunjia wrote:
> On 2017/06/30 18:55:36, bokan wrote:
> > On 2017/06/30 18:26:58, sunyunjia wrote:
> > > On 2017/06/30 15:44:12, bokan wrote:
> > > > If this is a callback that's called after the gesture is completed, why
do
> > we
> > > > have to wait?
> > > 
> > > We don't need to wait for touch, but need to wait for the wheel event to
> > finish
> > > the animation.
> > 
> > We should turn off wheel animations, they're unrelated to what we're trying
to
> > test and can only add flakiness. I think you should be able to do that with
> > internals.settings.setScrollAnimatorEnabled
> 
> I think it can help test the ScrollAnimated() branch?
> 
> I tried internals.settings.setScrollAnimatorEnabled(false) and removed the
> animation, both touch_type and wheel_type passed on main thread but failed on
> impl thread. I did some codesearch and noticed that internals.settings.XXX are
> mainly used for eventSender. I guess this setting only works for main thread.



Impl thread has its own internal flag that controls smooth scrolling 
InputHandlerProxy::smooth_scroll_enabled_. This is actually controlled by 
command line flags during initialization but I suspect that it should not be too
complex to set this impl flag from WebView anytime that
internals.settings.setScrollAnimatorEnable
is called.

https://codereview.chromium.org/2769793002/diff/720001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2769793002/diff/720001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:3386: 
nit: remove the unnecessary blank line.

https://codereview.chromium.org/2769793002/diff/720001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:3458: bool
LayerTreeHostImpl::CanPropagate(ScrollNode* scroll_node,
This function is not using any state in LayerTreeHostImpl so 
it does not need to be a member function.

https://codereview.chromium.org/2769793002/diff/720001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:3464: // should be scrolled.
The last sentence is not accurate. We don't decide which node should be
scrolled here. Rather we decide if scroll should be propagate from this node
given its relevant* ScrollBoundaryBehavior value.


* relevant here depends on the the dominant axis of scroll gesture.

https://codereview.chromium.org/2769793002/diff/720001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html
(right):

https://codereview.chromium.org/2769793002/diff/720001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:25:
const IS_MAC = navigator.platform.indexOf('Mac') == 0;
nit: probably best to move these values closer to where they 
are used. e.g., MAX_FRAME is only relevant inside waitForAnimationEnd.

https://codereview.chromium.org/2769793002/diff/720001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:26:
var container = document.getElementById('container');
nit: Since container value is never changed it can be a const.
Javascript const is just a const reference and does not require immutability.

https://codereview.chromium.org/2769793002/diff/720001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:58:
if (frames >= MAX_FRAME || frames - last_changed_frame > 20) {
20 frames seems a bit arbitrary, why do you need more than 1 frame?
Unnecessarily waiting around 20 frames will make this test too slow.

I think if there is not scroll in the last frame, we can deduct that
the animation curve delta was smaller than 1px. Given that and the
magnitude of initial scroll can we have a tighter bound than 20 
frames? 

I also echo bokan@'s sentiment that we should prefer to disable animations on
for this test. See my other comment on you you may be able to do so for impl
thread as well. However, if this turns out more complex than I anticipate then
it is fine to do that as a separate
patch. Just make sure to create a bug to follow up on it.

https://codereview.chromium.org/2769793002/diff/720001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/ScrollManager.cpp (right):

https://codereview.chromium.org/2769793002/diff/720001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/ScrollManager.cpp:160: const Element&
current_element) {
ditto. This function does not need to be a member function.

nit: The name |current_element| is not very appropriate here.
There is no other element so just calling it scrolling_element
or just element is better IMHO.

https://codereview.chromium.org/2769793002/diff/720001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/ScrollManager.cpp:164: // be scrolled.
ditto. the last sentence of the comment is not accurate.

[ajuma, 2017-07-14 15:04:48]

Thanks for adding the unit test. I just have a couple more nits.

https://codereview.chromium.org/2769793002/diff/720001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/2769793002/diff/720001/cc/layers/layer_impl.h...
cc/layers/layer_impl.h:569: ScrollBoundaryBehavior scroll_boundary_behavior_;
I think this can be removed now. (Also the #include added above.)

https://codereview.chromium.org/2769793002/diff/720001/cc/layers/layer_impl_t...
File cc/layers/layer_impl_test_properties.h (right):

https://codereview.chromium.org/2769793002/diff/720001/cc/layers/layer_impl_t...
cc/layers/layer_impl_test_properties.h:62: ScrollBoundaryBehavior
scroll_boundary_behavior;
This needs to be initialized.

[sunyunjia, 2017-07-14 20:46:14]

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

[commit-bot: I haz the power, 2017-07-14 20:46:34]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-07-14 20:50:22]

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

[commit-bot: I haz the power, 2017-07-14 20:50:24]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[sunyunjia, 2017-07-14 21:01:46]

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

[commit-bot: I haz the power, 2017-07-14 21:02:06]

Dry run: CQ is trying da patch.

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

[sunyunjia, 2017-07-14 22:07:24]

PTAL, thanks!

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html
(right):

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:29:
// We requestAnimationFrame either for 500 frames or until 20 frames with
On 2017/07/14 14:29:43, majidvp wrote:
> On 2017/07/14 02:59:00, sunyunjia wrote:
> > On 2017/06/30 18:55:36, bokan wrote:
> > > On 2017/06/30 18:26:58, sunyunjia wrote:
> > > > On 2017/06/30 15:44:12, bokan wrote:
> > > > > If this is a callback that's called after the gesture is completed,
why
> do
> > > we
> > > > > have to wait?
> > > > 
> > > > We don't need to wait for touch, but need to wait for the wheel event to
> > > finish
> > > > the animation.
> > > 
> > > We should turn off wheel animations, they're unrelated to what we're
trying
> to
> > > test and can only add flakiness. I think you should be able to do that
with
> > > internals.settings.setScrollAnimatorEnabled
> > 
> > I think it can help test the ScrollAnimated() branch?
> > 
> > I tried internals.settings.setScrollAnimatorEnabled(false) and removed the
> > animation, both touch_type and wheel_type passed on main thread but failed
on
> > impl thread. I did some codesearch and noticed that internals.settings.XXX
are
> > mainly used for eventSender. I guess this setting only works for main
thread.
> 
> 
> 
> Impl thread has its own internal flag that controls smooth scrolling 
> InputHandlerProxy::smooth_scroll_enabled_. This is actually controlled by 
> command line flags during initialization but I suspect that it should not be
too
> complex to set this impl flag from WebView anytime that
> internals.settings.setScrollAnimatorEnable
> is called.
Seems like internals.settings.setScrollAnimatorEnabled modifies page.settings by
a series of generated code. And the main_thread flag is set by looking at the
value in settings.

I consulted dtapuska@ about this and he said we could disable the animator it by
disabling the smooth_scroll runtime flag for all virtual/threaded tests, which
is not worth it. Instead, he suggests waiting for some frames as it simulates
the actual user behavior.

https://codereview.chromium.org/2769793002/diff/720001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/2769793002/diff/720001/cc/layers/layer_impl.h...
cc/layers/layer_impl.h:569: ScrollBoundaryBehavior scroll_boundary_behavior_;
On 2017/07/14 15:04:48, ajuma wrote:
> I think this can be removed now. (Also the #include added above.)

Done.

https://codereview.chromium.org/2769793002/diff/720001/cc/layers/layer_impl_t...
File cc/layers/layer_impl_test_properties.h (right):

https://codereview.chromium.org/2769793002/diff/720001/cc/layers/layer_impl_t...
cc/layers/layer_impl_test_properties.h:62: ScrollBoundaryBehavior
scroll_boundary_behavior;
On 2017/07/14 15:04:48, ajuma wrote:
> This needs to be initialized.

Done.

https://codereview.chromium.org/2769793002/diff/720001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/2769793002/diff/720001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:3386: 
On 2017/07/14 14:29:43, majidvp wrote:
> nit: remove the unnecessary blank line.

Done.

https://codereview.chromium.org/2769793002/diff/720001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:3458: bool
LayerTreeHostImpl::CanPropagate(ScrollNode* scroll_node,
On 2017/07/14 14:29:43, majidvp wrote:
> This function is not using any state in LayerTreeHostImpl so 
> it does not need to be a member function.

Done.

https://codereview.chromium.org/2769793002/diff/720001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:3464: // should be scrolled.
On 2017/07/14 14:29:43, majidvp wrote:
> The last sentence is not accurate. We don't decide which node should be
> scrolled here. Rather we decide if scroll should be propagate from this node
> given its relevant* ScrollBoundaryBehavior value.
> 
> 
> * relevant here depends on the the dominant axis of scroll gesture.

Done.

https://codereview.chromium.org/2769793002/diff/720001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html
(right):

https://codereview.chromium.org/2769793002/diff/720001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:25:
const IS_MAC = navigator.platform.indexOf('Mac') == 0;
On 2017/07/14 14:29:43, majidvp wrote:
> nit: probably best to move these values closer to where they 
> are used. e.g., MAX_FRAME is only relevant inside waitForAnimationEnd.

Done.

https://codereview.chromium.org/2769793002/diff/720001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:26:
var container = document.getElementById('container');
On 2017/07/14 14:29:43, majidvp wrote:
> nit: Since container value is never changed it can be a const.
> Javascript const is just a const reference and does not require immutability.

Done.

https://codereview.chromium.org/2769793002/diff/720001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:58:
if (frames >= MAX_FRAME || frames - last_changed_frame > 20) {
On 2017/07/14 14:29:43, majidvp wrote:
> 20 frames seems a bit arbitrary, why do you need more than 1 frame?
> Unnecessarily waiting around 20 frames will make this test too slow.
> 
> I think if there is not scroll in the last frame, we can deduct that
> the animation curve delta was smaller than 1px. Given that and the
> magnitude of initial scroll can we have a tighter bound than 20 
> frames? 
> 
> I also echo bokan@'s sentiment that we should prefer to disable animations on
> for this test. See my other comment on you you may be able to do so for impl
> thread as well. However, if this turns out more complex than I anticipate then
> it is fine to do that as a separate
> patch. Just make sure to create a bug to follow up on it.
> 
> 

I deducted to 5 now.

https://codereview.chromium.org/2769793002/diff/720001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/ScrollManager.cpp (right):

https://codereview.chromium.org/2769793002/diff/720001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/ScrollManager.cpp:160: const Element&
current_element) {
On 2017/07/14 14:29:44, majidvp wrote:
> ditto. This function does not need to be a member function.
> 
> nit: The name |current_element| is not very appropriate here.
> There is no other element so just calling it scrolling_element
> or just element is better IMHO. 

Done.

https://codereview.chromium.org/2769793002/diff/720001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/ScrollManager.cpp:164: // be scrolled.
On 2017/07/14 14:29:44, majidvp wrote:
> ditto. the last sentence of the comment is not accurate.

Done.

[ajuma, 2017-07-14 22:20:04]

Thanks! cc lgtm

[commit-bot: I haz the power, 2017-07-15 01:48:39]

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

[commit-bot: I haz the power, 2017-07-15 01:48:41]

Dry run: This issue passed the CQ dry run.

[majidvp, 2017-07-17 15:49:30]

lgtm but please update the description before landing.

For examples the descriptions mentions overscroll-action
which is no longer relevant and confusing.

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html
(right):

https://codereview.chromium.org/2769793002/diff/600001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/scroll-behavior/scroll-boundary-behavior.html:29:
// We requestAnimationFrame either for 500 frames or until 20 frames with
On 2017/07/14 22:07:23, sunyunjia wrote:
> On 2017/07/14 14:29:43, majidvp wrote:
> > On 2017/07/14 02:59:00, sunyunjia wrote:
> > > On 2017/06/30 18:55:36, bokan wrote:
> > > > On 2017/06/30 18:26:58, sunyunjia wrote:
> > > > > On 2017/06/30 15:44:12, bokan wrote:
> > > > > > If this is a callback that's called after the gesture is completed,
> why
> > do
> > > > we
> > > > > > have to wait?
> > > > > 
> > > > > We don't need to wait for touch, but need to wait for the wheel event
to
> > > > finish
> > > > > the animation.
> > > > 
> > > > We should turn off wheel animations, they're unrelated to what we're
> trying
> > to
> > > > test and can only add flakiness. I think you should be able to do that
> with
> > > > internals.settings.setScrollAnimatorEnabled
> > > 
> > > I think it can help test the ScrollAnimated() branch?
> > > 
> > > I tried internals.settings.setScrollAnimatorEnabled(false) and removed the
> > > animation, both touch_type and wheel_type passed on main thread but failed
> on
> > > impl thread. I did some codesearch and noticed that internals.settings.XXX
> are
> > > mainly used for eventSender. I guess this setting only works for main
> thread.
> > 
> > 
> > 
> > Impl thread has its own internal flag that controls smooth scrolling 
> > InputHandlerProxy::smooth_scroll_enabled_. This is actually controlled by 
> > command line flags during initialization but I suspect that it should not be
> too
> > complex to set this impl flag from WebView anytime that
> > internals.settings.setScrollAnimatorEnable
> > is called.
> Seems like internals.settings.setScrollAnimatorEnabled modifies page.settings
by
> a series of generated code. And the main_thread flag is set by looking at the
> value in settings.
> 
> I consulted dtapuska@ about this and he said we could disable the animator it
by
> disabling the smooth_scroll runtime flag for all virtual/threaded tests, which
> is not worth it. Instead, he suggests waiting for some frames as it simulates
> the actual user behavior.

This is fine. Following our offline discussion I filed a separate bug 
https://crbug.com/744666 to explore various option to disable/control 
threaded scroll animations in layout tests.

nit: You actually only need to wait for some frames for wheel! The touch
scrolling
does not do any animation which means by the time the callback is called it
should
be finished.

[sunyunjia, 2017-07-17 16:15:13]

Description was changed from

==========
Implement CSS: scroll-boundary-behavior.

CSS: scroll-boundary-behavior allows developers to specify whether the scroll
should be propagated to the parent scroller. This is different from scroll
latching, in the way that when latching is turned on, a gesture will be
limited to a single element, but scroll-boundary-behavior will affect
propagation when the latch is being targeted at the beginning of the gesture.

This patch implements scroll-boundary-behavior: none, by cutting off the
scroll chain when the scrollNode has the property, so the scroll will not be
propagated. After overscroll-action is implemented, we will implement
scroll-boundary-behavior: contain using overscroll-action's backend support, as
both of them are meant to disable the overscroll effects.

BUG=672921
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Implement CSS: scroll-boundary-behavior.

CSS: scroll-boundary-behavior allows developers to specify whether the scroll
should be propagated to the parent scroller. This is different from scroll
latching, in the way that when latching is turned on, a gesture will be
limited to a single element, but scroll-boundary-behavior will affect
propagation when the latch is being targeted at the beginning of the gesture.

This patch implements scroll-boundary-behavior: none, by cutting off the
scroll chain when the scrollNode has the property, so the scroll will not be
propagated. After this scroll-boundary-behavior: none is implemented, we will
implement scroll-boundary-behavior: contain using overscroll-action's backend
support, as both of them are meant to disable the overscroll effects.

BUG=672921
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[sunyunjia, 2017-07-17 16:17:47]

Hi Rick,

Could you please do the ownership review under
third_party/WebKit/Source/platform/ and
third_party/WebKit/public/platform/ ?

Thanks!

[sunyunjia, 2017-07-17 16:23:25]

Description was changed from

==========
Implement CSS: scroll-boundary-behavior.

CSS: scroll-boundary-behavior allows developers to specify whether the scroll
should be propagated to the parent scroller. This is different from scroll
latching, in the way that when latching is turned on, a gesture will be
limited to a single element, but scroll-boundary-behavior will affect
propagation when the latch is being targeted at the beginning of the gesture.

This patch implements scroll-boundary-behavior: none, by cutting off the
scroll chain when the scrollNode has the property, so the scroll will not be
propagated. After this scroll-boundary-behavior: none is implemented, we will
implement scroll-boundary-behavior: contain using overscroll-action's backend
support, as both of them are meant to disable the overscroll effects.

BUG=672921
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Implement CSS: scroll-boundary-behavior.

CSS: scroll-boundary-behavior allows developers to specify whether the scroll
should be propagated to its ancestors. This is different from scroll
latching, in the way that when latching is turned on, a gesture will be
limited to a single element, but scroll-boundary-behavior will affect
propagation when the latch is being targeted at the beginning of the gesture.

This patch implements scroll-boundary-behavior: none, by cutting off the
scroll chain when the scrollNode has the property, so the scroll will not be
propagated. After this patch lands, we will implement scroll-boundary-behavior:
contain, which controls overscroll UI affordance such as glow/bounce etc.

BUG=672921
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Rick Byers, 2017-07-18 18:44:26]

Source/platform and public/platform LGTM with nit about comments.

https://codereview.chromium.org/2769793002/diff/760001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebLayer.h (right):

https://codereview.chromium.org/2769793002/diff/760001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebLayer.h:216: virtual void
SetScrollBoundaryBehavior(const WebScrollBoundaryBehavior&) = 0;
nit: please add a comment describing at a high level the purpose of this API. 
We try to make the public API be fairly well documented (compared to other areas
of blink) - eg. there are chromium developers who will use public/ APIs but
never look at any of the code underneath it.

https://codereview.chromium.org/2769793002/diff/760001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebScrollBoundaryBehavior.h (right):

https://codereview.chromium.org/2769793002/diff/760001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebScrollBoundaryBehavior.h:14: struct
WebScrollBoundaryBehavior : public ScrollBoundaryBehavior {
also need API docs here

[Rick Byers, 2017-07-18 18:44:28]

Source/platform and public/platform LGTM with nit about comments.

[sunyunjia, 2017-07-18 19:51:13]

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

[commit-bot: I haz the power, 2017-07-18 19:51:38]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-07-18 21:51:49]

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

[commit-bot: I haz the power, 2017-07-18 21:51:51]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sunyunjia, 2017-07-18 22:13:30]

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

[commit-bot: I haz the power, 2017-07-18 22:13:42]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-07-18 23:58:59]

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

[commit-bot: I haz the power, 2017-07-18 23:59:01]

Dry run: This issue passed the CQ dry run.

[sunyunjia, 2017-07-19 01:06:49]

The CQ bit was checked by sunyunjia@chromium.org

[sunyunjia, 2017-07-19 01:06:50]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
ajuma@chromium.org, majidvp@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/2769793002/#ps780001
(title: "Add documentation")

[commit-bot: I haz the power, 2017-07-19 01:07:15]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-07-19 01:19:15]

CQ is committing da patch.

Bot data: {"patchset_id": 780001, "attempt_start_ts": 1500426409873930,
"parent_rev": "69fff5cffe928185a70e61b5c4cdeffb63a1756c", "commit_rev":
"55ed8ccd43eadef318b5e7a6bcf2519b078b1213"}

[commit-bot: I haz the power, 2017-07-19 01:19:31]

Description was changed from

==========
Implement CSS: scroll-boundary-behavior.

CSS: scroll-boundary-behavior allows developers to specify whether the scroll
should be propagated to its ancestors. This is different from scroll
latching, in the way that when latching is turned on, a gesture will be
limited to a single element, but scroll-boundary-behavior will affect
propagation when the latch is being targeted at the beginning of the gesture.

This patch implements scroll-boundary-behavior: none, by cutting off the
scroll chain when the scrollNode has the property, so the scroll will not be
propagated. After this patch lands, we will implement scroll-boundary-behavior:
contain, which controls overscroll UI affordance such as glow/bounce etc.

BUG=672921
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Implement CSS: scroll-boundary-behavior.

CSS: scroll-boundary-behavior allows developers to specify whether the scroll
should be propagated to its ancestors. This is different from scroll
latching, in the way that when latching is turned on, a gesture will be
limited to a single element, but scroll-boundary-behavior will affect
propagation when the latch is being targeted at the beginning of the gesture.

This patch implements scroll-boundary-behavior: none, by cutting off the
scroll chain when the scrollNode has the property, so the scroll will not be
propagated. After this patch lands, we will implement scroll-boundary-behavior:
contain, which controls overscroll UI affordance such as glow/bounce etc.

BUG=672921
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2769793002
Cr-Commit-Position: refs/heads/master@{#487702}
Committed:
https://chromium.googlesource.com/chromium/src/+/55ed8ccd43eadef318b5e7a6bcf2...
==========

[commit-bot: I haz the power, 2017-07-19 01:19:34]

Committed patchset #33 (id:780001) as
https://chromium.googlesource.com/chromium/src/+/55ed8ccd43eadef318b5e7a6bcf2...