Make touch events uncancelable during fling when they are on the current active scroll layer

Make touch events uncancelable during fling when they are on the current active scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to check if the
touch events which we will make uncancelable when there is an active fling are on the
same layer as the current active scroll layer or its descendants.

Now, we add the scroll layer check here to minimize the breaking cases and also match with other
browsers' behavior, such as Safaris and Firefox.

This code has two parts:
1. Delete the is_fling flag we used on https://codereview.chromium.org/1923973002/
to indicate if the actice fling is happening.
2. Add check to the touch start if it hits on the same layer as the current active scroll layer.

BUG=595327
Committed: https://crrev.com/53c12368f78425a27d592f47f814029cf2edaae7
Cr-Commit-Position: refs/heads/master@{#434917}

[lanwei, 2016-11-01 18:12:33]

Description was changed from

==========
fling layer


 fling layer

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

to

==========
fling layer


 fling layer

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[lanwei, 2016-11-01 19:17:10]

Description was changed from

==========
fling layer


 fling layer

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Make touch events uncancelable during fling when they are on the current active
scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to
check if the touch events
which we will make uncancelable when there is an active fling are on the same
layer as the current active scroll layer
or its descendants.

BUG=595327
==========

[lanwei, 2016-11-01 19:17:10]

lanwei@chromium.org changed reviewers:
+ bokan@chromium.org, dtapuska@chromium.org, tdresser@chromium.org

[lanwei, 2016-11-01 19:19:01]

Description was changed from

==========
Make touch events uncancelable during fling when they are on the current active
scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to
check if the touch events
which we will make uncancelable when there is an active fling are on the same
layer as the current active scroll layer
or its descendants.

BUG=595327
==========

to

==========
Make touch events uncancelable during fling when they are on the current active
scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to
check if
the touch events which we will make uncancelable when there is an active fling
are on the
same layer as the current active scroll layer or its descendants.

BUG=595327
==========

[lanwei, 2016-11-01 20:05:48]

Description was changed from

==========
Make touch events uncancelable during fling when they are on the current active
scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to
check if
the touch events which we will make uncancelable when there is an active fling
are on the
same layer as the current active scroll layer or its descendants.

BUG=595327
==========

to

==========
Make touch events uncancelable during fling when they are on the current active
scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to
check if the touch events which we will
make uncancelable when there is an active fling are on the same layer as the
current active scroll layer or its descendants.
Now, we add the scroll layer check here to minimize the breaking cases and also
match with other browsers' behavior,
such as Safaris and Firefox.

BUG=595327
==========

[lanwei, 2016-11-01 20:06:16]

Description was changed from

==========
Make touch events uncancelable during fling when they are on the current active
scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to
check if the touch events which we will
make uncancelable when there is an active fling are on the same layer as the
current active scroll layer or its descendants.
Now, we add the scroll layer check here to minimize the breaking cases and also
match with other browsers' behavior,
such as Safaris and Firefox.

BUG=595327
==========

to

==========
Make touch events uncancelable during fling when they are on the current active
scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to
check if the touch events
which we will make uncancelable when there is an active fling are on the same
layer as the current active scroll layer
or its descendants.

Now, we add the scroll layer check here to minimize the breaking cases and also
match with other browsers' behavior,
such as Safaris and Firefox.

BUG=595327
==========

[lanwei, 2016-11-01 20:06:44]

Description was changed from

==========
Make touch events uncancelable during fling when they are on the current active
scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to
check if the touch events
which we will make uncancelable when there is an active fling are on the same
layer as the current active scroll layer
or its descendants.

Now, we add the scroll layer check here to minimize the breaking cases and also
match with other browsers' behavior,
such as Safaris and Firefox.

BUG=595327
==========

to

==========
Make touch events uncancelable during fling when they are on the current active
scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to
check if the touch events
which we will make uncancelable when there is an active fling are on the same
layer as the current
active scroll layer or its descendants.

Now, we add the scroll layer check here to minimize the breaking cases and also
match with other browsers' behavior,
such as Safaris and Firefox.

BUG=595327
==========

[lanwei, 2016-11-01 20:07:14]

Description was changed from

==========
Make touch events uncancelable during fling when they are on the current active
scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to
check if the touch events
which we will make uncancelable when there is an active fling are on the same
layer as the current
active scroll layer or its descendants.

Now, we add the scroll layer check here to minimize the breaking cases and also
match with other browsers' behavior,
such as Safaris and Firefox.

BUG=595327
==========

to

==========
Make touch events uncancelable during fling when they are on the current active
scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to
check if the
touch events which we will make uncancelable when there is an active fling are
on the same layer as the current
active scroll layer or its descendants.

Now, we add the scroll layer check here to minimize the breaking cases and also
match with other
browsers' behavior, such as Safaris and Firefox.

BUG=595327
==========

[lanwei, 2016-11-01 20:07:37]

Description was changed from

==========
Make touch events uncancelable during fling when they are on the current active
scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to
check if the
touch events which we will make uncancelable when there is an active fling are
on the same layer as the current
active scroll layer or its descendants.

Now, we add the scroll layer check here to minimize the breaking cases and also
match with other
browsers' behavior, such as Safaris and Firefox.

BUG=595327
==========

to

==========
Make touch events uncancelable during fling when they are on the current active
scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to
check if the
touch events which we will make uncancelable when there is an active fling are
on the
same layer as the current active scroll layer or its descendants.

Now, we add the scroll layer check here to minimize the breaking cases and also
match with other
browsers' behavior, such as Safaris and Firefox.

BUG=595327
==========

[lanwei, 2016-11-03 21:35:21]

Description was changed from

==========
Make touch events uncancelable during fling when they are on the current active
scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to
check if the
touch events which we will make uncancelable when there is an active fling are
on the
same layer as the current active scroll layer or its descendants.

Now, we add the scroll layer check here to minimize the breaking cases and also
match with other
browsers' behavior, such as Safaris and Firefox.

BUG=595327
==========

to

==========
Make touch events uncancelable during fling when they are on the current active
scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to
check if the
touch events which we will make uncancelable when there is an active fling are
on the
same layer as the current active scroll layer or its descendants.

Now, we add the scroll layer check here to minimize the breaking cases and also
match with other
browsers' behavior, such as Safaris and Firefox.

BUG=595327
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[lanwei, 2016-11-03 21:35:28]

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

[commit-bot: I haz the power, 2016-11-03 21:36:18]

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

[commit-bot: I haz the power, 2016-11-03 21:54:19]

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

[commit-bot: I haz the power, 2016-11-03 21:54:19]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lanwei, 2016-11-04 01:45:30]

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

[commit-bot: I haz the power, 2016-11-04 01:46: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, 2016-11-04 02:10:17]

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

[commit-bot: I haz the power, 2016-11-04 02:10:18]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[lanwei, 2016-11-04 03:17:41]

Patchset #2 (id:20001) has been deleted

[lanwei, 2016-11-04 04:10:34]

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

[commit-bot: I haz the power, 2016-11-04 04:11:18]

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

[lanwei, 2016-11-04 04:13:00]

Patchset #2 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-11-04 04:28:14]

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

[commit-bot: I haz the power, 2016-11-04 04:28:15]

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

[lanwei, 2016-11-04 12:41:24]

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

[commit-bot: I haz the power, 2016-11-04 12:41:35]

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

[lanwei, 2016-11-04 12:42:36]

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

[commit-bot: I haz the power, 2016-11-04 12:42:44]

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

[commit-bot: I haz the power, 2016-11-04 13:37:38]

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

[commit-bot: I haz the power, 2016-11-04 13:37:39]

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_...)

[lanwei, 2016-11-04 13:53:21]

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

[commit-bot: I haz the power, 2016-11-04 13:53:33]

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

[lanwei, 2016-11-04 14:10:09]

Patchset #1 (id:1) has been deleted

[lanwei, 2016-11-04 14:10:24]

Patchset #1 (id:60001) has been deleted

[lanwei, 2016-11-04 14:10:37]

Patchset #1 (id:80001) has been deleted

[lanwei, 2016-11-04 14:11:32]

Could you please take a look, while I am adding a test to test layer check?

Thank you.

[lanwei, 2016-11-04 15:09:45]

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

[commit-bot: I haz the power, 2016-11-04 15:10:00]

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

[lanwei, 2016-11-04 15:12:11]

Description was changed from

==========
Make touch events uncancelable during fling when they are on the current active
scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to
check if the
touch events which we will make uncancelable when there is an active fling are
on the
same layer as the current active scroll layer or its descendants.

Now, we add the scroll layer check here to minimize the breaking cases and also
match with other
browsers' behavior, such as Safaris and Firefox.

BUG=595327
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Make touch events uncancelable during fling when they are on the current active
scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to
check if the
touch events which we will make uncancelable when there is an active fling are
on the
same layer as the current active scroll layer or its descendants.

Now, we add the scroll layer check here to minimize the breaking cases and also
match with other
browsers' behavior, such as Safaris and Firefox.

This code has two parts:
1. Delete the is_fling flag we used on
https://codereview.chromium.org/1923973002/ to indicate if the actice fling is
happening.
2. Add check to the touch start if it hits on the same layer as the current
actice scroll layer.

BUG=595327
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[lanwei, 2016-11-04 15:12:28]

Description was changed from

==========
Make touch events uncancelable during fling when they are on the current active
scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to
check if the
touch events which we will make uncancelable when there is an active fling are
on the
same layer as the current active scroll layer or its descendants.

Now, we add the scroll layer check here to minimize the breaking cases and also
match with other
browsers' behavior, such as Safaris and Firefox.

This code has two parts:
1. Delete the is_fling flag we used on
https://codereview.chromium.org/1923973002/ to indicate if the actice fling is
happening.
2. Add check to the touch start if it hits on the same layer as the current
actice scroll layer.

BUG=595327
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Make touch events uncancelable during fling when they are on the current active
scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to
check if the
touch events which we will make uncancelable when there is an active fling are
on the
same layer as the current active scroll layer or its descendants.

Now, we add the scroll layer check here to minimize the breaking cases and also
match with other
browsers' behavior, such as Safaris and Firefox.

This code has two parts:
1. Delete the is_fling flag we used on
https://codereview.chromium.org/1923973002/
to indicate if the actice fling is happening.
2. Add check to the touch start if it hits on the same layer as the current
actice scroll layer.

BUG=595327
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[lanwei, 2016-11-04 15:14:04]

bokan@, could you please take a look at cc/trees/layer_tree_host_impl* to see of
the layer check is correct.

[commit-bot: I haz the power, 2016-11-04 15:20:41]

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

[commit-bot: I haz the power, 2016-11-04 15:20:42]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[lanwei, 2016-11-04 15:33:34]

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

[commit-bot: I haz the power, 2016-11-04 15:33:46]

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

[lanwei, 2016-11-04 15:34:39]

Patchset #2 (id:120001) has been deleted

[tdresser, 2016-11-04 17:15:20]

You mention the code has two parts.

Can we land them in separate patches?

I've only given a bit of initial feedback.

https://codereview.chromium.org/2471523002/diff/140001/cc/input/input_handler.h
File cc/input/input_handler.h (right):

https://codereview.chromium.org/2471523002/diff/140001/cc/input/input_handler...
cc/input/input_handler.h:108: enum TouchStartHitResult { HANDLER, SAME_LAYER,
DIFFERENT_LAYER };
These names aren't clear to me. Same layer as what? They need to make sense in
the context of: |DoTouchEventsBlockScrollAt| (or we could rename that method).

Perhaps:
DoTouchHandlersBlockScrollAt(...)
NO_HANDLER
HANDLER
HANDLER_ON_FLINGING_LAYER

Maybe get a second opinion on this - I'm not too fond of the naming here.

https://codereview.chromium.org/2471523002/diff/140001/cc/input/input_handler...
cc/input/input_handler.h:194: // consuming touch events that started at
|viewport_point|.
Update this comment.

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

https://codereview.chromium.org/2471523002/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:624: return TouchStartHitResult::SAME_LAYER;
I don't understand this, but I might just be confused by the naming.

[commit-bot: I haz the power, 2016-11-04 17:19:20]

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

[commit-bot: I haz the power, 2016-11-04 17:19:21]

Dry run: This issue passed the CQ dry run.

[bokan, 2016-11-04 19:32:55]

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

https://codereview.chromium.org/2471523002/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:166: for (; scroll_tree.parent(scroll_node);
This will avoid comparing against the root layer. I would change this to just
check that scroll_node is non-null

https://codereview.chromium.org/2471523002/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:624: return TouchStartHitResult::SAME_LAYER;
On 2016/11/04 17:15:20, tdresser wrote:
> I don't understand this, but I might just be confused by the naming.

+1, I don't know what these returns mean.

https://codereview.chromium.org/2471523002/diff/140001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:627: HasScrollAncestor(layer_impl,
active_tree_->CurrentlyScrollingLayer());
What should happen if layer_impl == CurrentlyScrollingLayer? HasScrollAncestor
will return false but maybe that's by design?

[lanwei, 2016-11-06 22:01:06]

Description was changed from

==========
Make touch events uncancelable during fling when they are on the current active
scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to
check if the
touch events which we will make uncancelable when there is an active fling are
on the
same layer as the current active scroll layer or its descendants.

Now, we add the scroll layer check here to minimize the breaking cases and also
match with other
browsers' behavior, such as Safaris and Firefox.

This code has two parts:
1. Delete the is_fling flag we used on
https://codereview.chromium.org/1923973002/
to indicate if the actice fling is happening.
2. Add check to the touch start if it hits on the same layer as the current
actice scroll layer.

BUG=595327
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Make touch events uncancelable during fling when they are on the current active
scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to
check if the
touch events which we will make uncancelable when there is an active fling are
on the
same layer as the current active scroll layer or its descendants.

Now, we add the scroll layer check here to minimize the breaking cases and also
match with other
browsers' behavior, such as Safaris and Firefox.

This code has two parts:
1. Delete the is_fling flag we used on
https://codereview.chromium.org/1923973002/
to indicate if the actice fling is happening.
2. Add check to the touch start if it hits on the same layer as the current
active scroll layer.

BUG=595327
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[lanwei, 2016-11-10 03:12:08]

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

[commit-bot: I haz the power, 2016-11-10 03:12:32]

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

[commit-bot: I haz the power, 2016-11-10 03:14:50]

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

[commit-bot: I haz the power, 2016-11-10 03:14:51]

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

[lanwei, 2016-11-11 17:13:03]

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

[commit-bot: I haz the power, 2016-11-11 17:13:20]

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

[lanwei, 2016-11-11 17:29:35]

Patchset #3 (id:150001) has been deleted

[lanwei, 2016-11-11 17:44:23]

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

[commit-bot: I haz the power, 2016-11-11 17:44:39]

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

[lanwei, 2016-11-11 18:11:54]

Patchset #3 (id:170001) has been deleted

[lanwei, 2016-11-11 18:22:45]

https://codereview.chromium.org/2471523002/diff/140001/cc/input/input_handler.h
File cc/input/input_handler.h (right):

https://codereview.chromium.org/2471523002/diff/140001/cc/input/input_handler...
cc/input/input_handler.h:194: // consuming touch events that started at
|viewport_point|.
On 2016/11/04 17:15:20, tdresser wrote:
> Update this comment.

Done.

[commit-bot: I haz the power, 2016-11-11 19:14:29]

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

[commit-bot: I haz the power, 2016-11-11 19:14:30]

Dry run: This issue passed the CQ dry run.

[bokan, 2016-11-11 20:41:53]

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

https://codereview.chromium.org/2471523002/diff/190001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:159: bool isAncestor(LayerImpl* child,
LayerImpl* scroll_ancestor) {
Lets replace HasScrollAncestor with this function.

https://codereview.chromium.org/2471523002/diff/190001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:167: ScrollNode* scroll_node =
scroll_tree.Node(child->scroll_tree_index());
Put this the for loop initializer below:

for(ScrollNode* scroll_node = ...; )

https://codereview.chromium.org/2471523002/diff/190001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:168: for (; scroll_tree.parent(scroll_node);
This is still only looping if we have a parent which will miss the root node.
E.g:

A <- B <- C

If child = C and scroll_ancestor = A:

Iteration 1: Compare C and A, set scroll_node = B (side note: we've already
compared C and A above, you can remove the child == scroll_ancestor check above)
Iteration 2: Compare B and A, set scroll_node = A

But now scroll_tree.parent(A) == nullptr so we won't iterate again. We should be
looping while scroll_node is non-null.

[lanwei, 2016-11-12 02:13:27]

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

[commit-bot: I haz the power, 2016-11-12 02:13:45]

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

[lanwei, 2016-11-12 02:17:14]

lanwei@chromium.org changed reviewers:
+ weiliangc@chromium.org

[lanwei, 2016-11-12 02:17:15]

weiliangc@chromium.org: Please review changes in 

cc/

do you think we should replace the HasScrollAncestor function with the new
function isAncestor?

Thank you.

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

https://codereview.chromium.org/2471523002/diff/190001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:167: ScrollNode* scroll_node =
scroll_tree.Node(child->scroll_tree_index());
On 2016/11/11 20:41:53, bokan wrote:
> Put this the for loop initializer below:
> 
> for(ScrollNode* scroll_node = ...; )

Done.

https://codereview.chromium.org/2471523002/diff/190001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:168: for (; scroll_tree.parent(scroll_node);
On 2016/11/11 20:41:53, bokan wrote:
> This is still only looping if we have a parent which will miss the root node.
> E.g:
> 
> A <- B <- C
> 
> If child = C and scroll_ancestor = A:
> 
> Iteration 1: Compare C and A, set scroll_node = B (side note: we've already
> compared C and A above, you can remove the child == scroll_ancestor check
above)
> Iteration 2: Compare B and A, set scroll_node = A
> 
> But now scroll_tree.parent(A) == nullptr so we won't iterate again. We should
be
> looping while scroll_node is non-null.

Done.

[lanwei, 2016-11-12 02:26:25]

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

[commit-bot: I haz the power, 2016-11-12 02:26:47]

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

[lanwei, 2016-11-12 02:26:49]

Patchset #4 (id:210001) has been deleted

[commit-bot: I haz the power, 2016-11-12 03:35:40]

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

[commit-bot: I haz the power, 2016-11-12 03:35:41]

Dry run: This issue passed the CQ dry run.

[bokan, 2016-11-14 18:32:49]

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

https://codereview.chromium.org/2471523002/diff/230001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:158: bool isAncestor(LayerImpl* child,
LayerImpl* ancestor) {
This should be called isScrollingAncestor to make clear that it's looking along
the scroll tree.

This leaves us with isScrollingAncestor and hasScrollingAncestor, the latter of
which checks all the way up to - but not including - the root. weiliangc@, is
the behavior in hasScrollingAncestor intentionally skipping the root scroll
node? If so, we should rename it to make it clear. Otherwise, we should remove
it and replace uses with this function.

[weiliangc, 2016-11-14 19:02:55]

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

https://codereview.chromium.org/2471523002/diff/230001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:158: bool isAncestor(LayerImpl* child,
LayerImpl* ancestor) {
On 2016/11/14 at 18:32:48, bokan wrote:
> This should be called isScrollingAncestor to make clear that it's looking
along the scroll tree.
> 
> This leaves us with isScrollingAncestor and hasScrollingAncestor, the latter
of which checks all the way up to - but not including - the root. weiliangc@, is
the behavior in hasScrollingAncestor intentionally skipping the root scroll
node? If so, we should rename it to make it clear. Otherwise, we should remove
it and replace uses with this function.

It is an unfortunate implementation detail of Property Tree that the node 0 is
actually a dummy node that contains no real information. This implementation
would be able to reach the root node we care about.

[bokan, 2016-11-14 19:04:32]

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

https://codereview.chromium.org/2471523002/diff/230001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:158: bool isAncestor(LayerImpl* child,
LayerImpl* ancestor) {
On 2016/11/14 19:02:55, weiliangc wrote:
> On 2016/11/14 at 18:32:48, bokan wrote:
> > This should be called isScrollingAncestor to make clear that it's looking
> along the scroll tree.
> > 
> > This leaves us with isScrollingAncestor and hasScrollingAncestor, the latter
> of which checks all the way up to - but not including - the root. weiliangc@,
is
> the behavior in hasScrollingAncestor intentionally skipping the root scroll
> node? If so, we should rename it to make it clear. Otherwise, we should remove
> it and replace uses with this function.
> 
> It is an unfortunate implementation detail of Property Tree that the node 0 is
> actually a dummy node that contains no real information. This implementation
> would be able to reach the root node we care about.

But in that case, there shouldn't be a LayerImpl to match the scroll tree root
node right? So looping all the way up to the root should be ok.

[weiliangc, 2016-11-14 20:57:21]

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

https://codereview.chromium.org/2471523002/diff/230001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:158: bool isAncestor(LayerImpl* child,
LayerImpl* ancestor) {
On 2016/11/14 at 19:04:32, bokan wrote:
> On 2016/11/14 19:02:55, weiliangc wrote:
> > On 2016/11/14 at 18:32:48, bokan wrote:
> > > This should be called isScrollingAncestor to make clear that it's looking
> > along the scroll tree.
> > > 
> > > This leaves us with isScrollingAncestor and hasScrollingAncestor, the
latter
> > of which checks all the way up to - but not including - the root.
weiliangc@, is
> > the behavior in hasScrollingAncestor intentionally skipping the root scroll
> > node? If so, we should rename it to make it clear. Otherwise, we should
remove
> > it and replace uses with this function.
> > 
> > It is an unfortunate implementation detail of Property Tree that the node 0
is
> > actually a dummy node that contains no real information. This implementation
> > would be able to reach the root node we care about.
> 
> But in that case, there shouldn't be a LayerImpl to match the scroll tree root
node right? So looping all the way up to the root should be ok.

Yeah, so only one of the two function would be fine.

[lanwei, 2016-11-14 22:47:45]

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

https://codereview.chromium.org/2471523002/diff/230001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:158: bool isAncestor(LayerImpl* child,
LayerImpl* ancestor) {
On 2016/11/14 20:57:20, weiliangc wrote:
> On 2016/11/14 at 19:04:32, bokan wrote:
> > On 2016/11/14 19:02:55, weiliangc wrote:
> > > On 2016/11/14 at 18:32:48, bokan wrote:
> > > > This should be called isScrollingAncestor to make clear that it's
looking
> > > along the scroll tree.
> > > > 
> > > > This leaves us with isScrollingAncestor and hasScrollingAncestor, the
> latter
> > > of which checks all the way up to - but not including - the root.
> weiliangc@, is
> > > the behavior in hasScrollingAncestor intentionally skipping the root
scroll
> > > node? If so, we should rename it to make it clear. Otherwise, we should
> remove
> > > it and replace uses with this function.
> > > 
> > > It is an unfortunate implementation detail of Property Tree that the node
0
> is
> > > actually a dummy node that contains no real information. This
implementation
> > > would be able to reach the root node we care about.
> > 
> > But in that case, there shouldn't be a LayerImpl to match the scroll tree
root
> node right? So looping all the way up to the root should be ok.
> 
> Yeah, so only one of the two function would be fine.

https://codesearch.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc...
One more different part is 
if (scroll_node->scrollable)
  return scroll_node->owner_id == scroll_ancestor->id();

if the scroll_node is scrollable, and scroll_node is a ancestor of
scroll_ancestor, but not equal, this will return false. But we want the loop to
be continue and check its parents...

weiliangc@ I guess HasScrollAncestor want to compare with the first scroll layer
of child with scroll_ancestor?

[weiliangc, 2016-11-15 15:32:35]

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

https://codereview.chromium.org/2471523002/diff/230001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:158: bool isAncestor(LayerImpl* child,
LayerImpl* ancestor) {
On 2016/11/14 at 22:47:45, lanwei wrote:
> On 2016/11/14 20:57:20, weiliangc wrote:
> > On 2016/11/14 at 19:04:32, bokan wrote:
> > > On 2016/11/14 19:02:55, weiliangc wrote:
> > > > On 2016/11/14 at 18:32:48, bokan wrote:
> > > > > This should be called isScrollingAncestor to make clear that it's
looking
> > > > along the scroll tree.
> > > > > 
> > > > > This leaves us with isScrollingAncestor and hasScrollingAncestor, the
> > latter
> > > > of which checks all the way up to - but not including - the root.
> > weiliangc@, is
> > > > the behavior in hasScrollingAncestor intentionally skipping the root
scroll
> > > > node? If so, we should rename it to make it clear. Otherwise, we should
> > remove
> > > > it and replace uses with this function.
> > > > 
> > > > It is an unfortunate implementation detail of Property Tree that the
node 0
> > is
> > > > actually a dummy node that contains no real information. This
implementation
> > > > would be able to reach the root node we care about.
> > > 
> > > But in that case, there shouldn't be a LayerImpl to match the scroll tree
root
> > node right? So looping all the way up to the root should be ok.
> > 
> > Yeah, so only one of the two function would be fine.
> 
>
https://codesearch.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc...
> One more different part is 
> if (scroll_node->scrollable)
>   return scroll_node->owner_id == scroll_ancestor->id();
> 
> if the scroll_node is scrollable, and scroll_node is a ancestor of
scroll_ancestor, but not equal, this will return false. But we want the loop to
be continue and check its parents...
> 
> weiliangc@ I guess HasScrollAncestor want to compare with the first scroll
layer of child with scroll_ancestor?

How about rename the function to isScrolledBy? It would make it clear you are
only looking at the first scrollable ancestor.

Thanks!

[lanwei, 2016-11-15 18:22:47]

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

[commit-bot: I haz the power, 2016-11-15 18:23:16]

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

[lanwei, 2016-11-15 18:25:26]

bokan@, weiliangc@ could you please take another look, thanks?

[weiliangc, 2016-11-15 18:50:38]

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

https://codereview.chromium.org/2471523002/diff/250001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:167: if (scroll_node->owner_id ==
ancestor->id())
Another difference between this function and HasScrollAncestor is that the other
function checks for scollable. Is this intentional?

Probably a nit but I think it's very hard to tell the difference between this
function and HasScrollAncestor by the name.

[bokan, 2016-11-15 19:02:29]

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

https://codereview.chromium.org/2471523002/diff/250001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:167: if (scroll_node->owner_id ==
ancestor->id())
On 2016/11/15 18:50:38, weiliangc wrote:
> Another difference between this function and HasScrollAncestor is that the
other
> function checks for scollable. Is this intentional?
> 
> Probably a nit but I think it's very hard to tell the difference between this
> function and HasScrollAncestor by the name.

We should get rid of one of the functions because this would be quite confusing.
There's no point in checking scrollable in the loop, we should just check if the
returned layer's scroll node is scrollable at the call sites of
HasScrollingAncestor.

Also, nit: Chromium style functions start with capital letter.

[weiliangc, 2016-11-15 19:13:47]

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

https://codereview.chromium.org/2471523002/diff/250001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:167: if (scroll_node->owner_id ==
ancestor->id())
On 2016/11/15 19:02:29, bokan wrote:
> On 2016/11/15 18:50:38, weiliangc wrote:
> > Another difference between this function and HasScrollAncestor is that the
> other
> > function checks for scollable. Is this intentional?
> > 
> > Probably a nit but I think it's very hard to tell the difference between
this
> > function and HasScrollAncestor by the name.
> 
> We should get rid of one of the functions because this would be quite
confusing.
> There's no point in checking scrollable in the loop, we should just check if
the
> returned layer's scroll node is scrollable at the call sites of
> HasScrollingAncestor.
> 
> Also, nit: Chromium style functions start with capital letter.

We could break out of the loop and check if scrollable still inside the
function? It might be hard to reinforce checking for scrollable at all the call
sites. Unless there are use cases for both checking and not checking scrollable?

[bokan, 2016-11-15 19:17:34]

On 2016/11/15 19:13:47, weiliangc wrote:
>
https://codereview.chromium.org/2471523002/diff/250001/cc/trees/layer_tree_ho...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/2471523002/diff/250001/cc/trees/layer_tree_ho...
> cc/trees/layer_tree_host_impl.cc:167: if (scroll_node->owner_id ==
> ancestor->id())
> On 2016/11/15 19:02:29, bokan wrote:
> > On 2016/11/15 18:50:38, weiliangc wrote:
> > > Another difference between this function and HasScrollAncestor is that the
> > other
> > > function checks for scollable. Is this intentional?
> > > 
> > > Probably a nit but I think it's very hard to tell the difference between
> this
> > > function and HasScrollAncestor by the name.
> > 
> > We should get rid of one of the functions because this would be quite
> confusing.
> > There's no point in checking scrollable in the loop, we should just check if
> the
> > returned layer's scroll node is scrollable at the call sites of
> > HasScrollingAncestor.
> > 
> > Also, nit: Chromium style functions start with capital letter.
> 
> We could break out of the loop and check if scrollable still inside the
> function? It might be hard to reinforce checking for scrollable at all the
call
> sites. Unless there are use cases for both checking and not checking
scrollable?

I'm not sure if the use case in this patch requires or doesn't require it but
that could be a case of not needing to check it. HasScrollAncestor has only one
caller so convenience isn't an issue. Since we already have the layer we can
check if it's scroll node is scrollable without any lookup so IMO it doesn't
make sense in this function. The call site will be more clear as to what's
happening and the helper function more general.

[weiliangc, 2016-11-15 19:43:47]

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

https://codereview.chromium.org/2471523002/diff/250001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:167: if (scroll_node->owner_id ==
ancestor->id())
On 2016/11/15 at 19:13:46, weiliangc wrote:
> On 2016/11/15 19:02:29, bokan wrote:
> > On 2016/11/15 18:50:38, weiliangc wrote:
> > > Another difference between this function and HasScrollAncestor is that the
> > other
> > > function checks for scollable. Is this intentional?
> > > 
> > > Probably a nit but I think it's very hard to tell the difference between
this
> > > function and HasScrollAncestor by the name.
> > 
> > We should get rid of one of the functions because this would be quite
confusing.
> > There's no point in checking scrollable in the loop, we should just check if
the
> > returned layer's scroll node is scrollable at the call sites of
> > HasScrollingAncestor.
> > 
> > Also, nit: Chromium style functions start with capital letter.
> 
> We could break out of the loop and check if scrollable still inside the
function? It might be hard to reinforce checking for scrollable at all the call
sites. Unless there are use cases for both checking and not checking scrollable?

Oops now paid more attention to the code, I think the two functions are doing
different things.

Function here is "child IsCurrentlyScrolledBy ancestor" where the ancestor is
assumed to be scrollable, since it is the active scrolling layer. And the
function returns true if ancestor is anywhere between child and root (including
child). Maybe add a DCHECK for ancestor's scroll_node is scrollable?

Function HasScrollAncestor is probably more "child's ClosestScrollAncestorIs
ancestor" since the loop is broken out as soon as it finds a scrollable node.

So we would need both functions, just need to rename thing to distinguish them.

[bokan, 2016-11-15 19:47:33]

On 2016/11/15 19:43:47, weiliangc wrote:
>
https://codereview.chromium.org/2471523002/diff/250001/cc/trees/layer_tree_ho...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/2471523002/diff/250001/cc/trees/layer_tree_ho...
> cc/trees/layer_tree_host_impl.cc:167: if (scroll_node->owner_id ==
> ancestor->id())
> On 2016/11/15 at 19:13:46, weiliangc wrote:
> > On 2016/11/15 19:02:29, bokan wrote:
> > > On 2016/11/15 18:50:38, weiliangc wrote:
> > > > Another difference between this function and HasScrollAncestor is that
the
> > > other
> > > > function checks for scollable. Is this intentional?
> > > > 
> > > > Probably a nit but I think it's very hard to tell the difference between
> this
> > > > function and HasScrollAncestor by the name.
> > > 
> > > We should get rid of one of the functions because this would be quite
> confusing.
> > > There's no point in checking scrollable in the loop, we should just check
if
> the
> > > returned layer's scroll node is scrollable at the call sites of
> > > HasScrollingAncestor.
> > > 
> > > Also, nit: Chromium style functions start with capital letter.
> > 
> > We could break out of the loop and check if scrollable still inside the
> function? It might be hard to reinforce checking for scrollable at all the
call
> sites. Unless there are use cases for both checking and not checking
scrollable?
> 
> Oops now paid more attention to the code, I think the two functions are doing
> different things.
> 
> Function here is "child IsCurrentlyScrolledBy ancestor" where the ancestor is
> assumed to be scrollable, since it is the active scrolling layer. And the
> function returns true if ancestor is anywhere between child and root
(including
> child). Maybe add a DCHECK for ancestor's scroll_node is scrollable?
> 
> Function HasScrollAncestor is probably more "child's ClosestScrollAncestorIs
> ancestor" since the loop is broken out as soon as it finds a scrollable node.
> 
> So we would need both functions, just need to rename thing to distinguish
them.

Good catch, I missed that too.

[lanwei, 2016-11-15 20:24:29]

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

[commit-bot: I haz the power, 2016-11-15 20:25:14]

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

[lanwei, 2016-11-15 20:29:08]

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

[lanwei, 2016-11-15 20:29:38]

Patchset #6 (id:270001) has been deleted

[commit-bot: I haz the power, 2016-11-15 20:29:51]

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

[commit-bot: I haz the power, 2016-11-15 20:45:13]

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

[commit-bot: I haz the power, 2016-11-15 20:45: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...)

[weiliangc, 2016-11-15 21:12:24]

cc/ LGTM

[bokan, 2016-11-15 22:42:40]

Took a first pass through the rest of the patch

https://codereview.chromium.org/2471523002/diff/290001/cc/input/input_handler.h
File cc/input/input_handler.h (right):

https://codereview.chromium.org/2471523002/diff/290001/cc/input/input_handler...
cc/input/input_handler.h:200: virtual EventListenerProperties
DoTouchHandlersBlockScrollAt(
Do____ implies the method returns a bool. I would rename this to
TouchHandlerTypeAt. The comment could also be improved: it returns the type of
event listener at the given viewport point.

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

https://codereview.chromium.org/2471523002/diff/290001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:619: return is_ancestor ?
EventListenerProperties::kBlockingAndPassiveDueToFling
It's only a fling if we're currently in a touch start. If we don't have a bit
somewhere that keeps track of whether we're in a fling or not, the name of the
method should imply that it should only be called from a TouchStart. e.g.
EventListenerTypeForTouchStartAt()

https://codereview.chromium.org/2471523002/diff/290001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:2606: // Similar to LayerImpl::HasAncestor, but
walks up the scroll parents.
It's Layer::HasAncestor and the difference is significant because of the if
(scroll_node->scrollable) so I'd just remove this comment.

https://codereview.chromium.org/2471523002/diff/290001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2471523002/diff/290001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:9937: gfx::Size inner_size(50, 50);
Please use CreateBasicVirtualViewportLayers to initialize the layer tree. It
makes sure we're everything is properly initialized to look and behave like the
compositor does when Chrome is running in the wild. For an example, see:

TEST_F(LayerTreeHostImplTest, ScrollChildBeyondLimit)

Then you can add your child/grand-child layers and avoid a lot of boilerplate.

https://codereview.chromium.org/2471523002/diff/290001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:9964: LayerImpl* child =
host_impl_->active_tree()
We've already saved the child and grand_child pointers above, no need to get
them again.

https://codereview.chromium.org/2471523002/diff/290001/ui/events/blink/input_...
File ui/events/blink/input_handler_proxy.cc (right):

https://codereview.chromium.org/2471523002/diff/290001/ui/events/blink/input_...
ui/events/blink/input_handler_proxy.cc:850: ? true
No need for ? true : false

[lanwei, 2016-11-16 19:24:56]

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

[commit-bot: I haz the power, 2016-11-16 19:25: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, 2016-11-16 19:29:02]

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

[commit-bot: I haz the power, 2016-11-16 19:29:04]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[lanwei, 2016-11-16 19:59:22]

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

[commit-bot: I haz the power, 2016-11-16 20:00:33]

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

[commit-bot: I haz the power, 2016-11-16 20:23:22]

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

[commit-bot: I haz the power, 2016-11-16 20:23:23]

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

[lanwei, 2016-11-16 20:31:44]

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

[lanwei, 2016-11-16 20:32:11]

Patchset #7 (id:310001) has been deleted

[lanwei, 2016-11-16 20:32:27]

Patchset #7 (id:330001) has been deleted

[commit-bot: I haz the power, 2016-11-16 20:33:02]

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

[lanwei, 2016-11-16 20:38:11]

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

[commit-bot: I haz the power, 2016-11-16 20:38:59]

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

[lanwei, 2016-11-16 20:46:43]

Patchset #7 (id:350001) has been deleted

[lanwei, 2016-11-16 21:13:29]

https://codereview.chromium.org/2471523002/diff/290001/cc/input/input_handler.h
File cc/input/input_handler.h (right):

https://codereview.chromium.org/2471523002/diff/290001/cc/input/input_handler...
cc/input/input_handler.h:200: virtual EventListenerProperties
DoTouchHandlersBlockScrollAt(
On 2016/11/15 22:42:40, bokan wrote:
> Do____ implies the method returns a bool. I would rename this to
> TouchHandlerTypeAt. The comment could also be improved: it returns the type of
> event listener at the given viewport point.

Done.

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

https://codereview.chromium.org/2471523002/diff/290001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:619: return is_ancestor ?
EventListenerProperties::kBlockingAndPassiveDueToFling
On 2016/11/15 22:42:40, bokan wrote:
> It's only a fling if we're currently in a touch start. If we don't have a bit
> somewhere that keeps track of whether we're in a fling or not, the name of the
> method should imply that it should only be called from a TouchStart. e.g.
> EventListenerTypeForTouchStartAt()

Done.

https://codereview.chromium.org/2471523002/diff/290001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:2606: // Similar to LayerImpl::HasAncestor, but
walks up the scroll parents.
On 2016/11/15 22:42:40, bokan wrote:
> It's Layer::HasAncestor and the difference is significant because of the if
> (scroll_node->scrollable) so I'd just remove this comment.

Done.

https://codereview.chromium.org/2471523002/diff/290001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2471523002/diff/290001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:9937: gfx::Size inner_size(50, 50);
On 2016/11/15 22:42:40, bokan wrote:
> Please use CreateBasicVirtualViewportLayers to initialize the layer tree. It
> makes sure we're everything is properly initialized to look and behave like
the
> compositor does when Chrome is running in the wild. For an example, see:
> 
> TEST_F(LayerTreeHostImplTest, ScrollChildBeyondLimit)
> 
> Then you can add your child/grand-child layers and avoid a lot of boilerplate.

Done.

https://codereview.chromium.org/2471523002/diff/290001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:9964: LayerImpl* child =
host_impl_->active_tree()
On 2016/11/15 22:42:40, bokan wrote:
> We've already saved the child and grand_child pointers above, no need to get
> them again.

Done.

https://codereview.chromium.org/2471523002/diff/290001/ui/events/blink/input_...
File ui/events/blink/input_handler_proxy.cc (right):

https://codereview.chromium.org/2471523002/diff/290001/ui/events/blink/input_...
ui/events/blink/input_handler_proxy.cc:850: ? true
On 2016/11/15 22:42:40, bokan wrote:
> No need for ? true : false

Done.

[commit-bot: I haz the power, 2016-11-16 22:29:58]

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

[commit-bot: I haz the power, 2016-11-16 22:29:59]

Dry run: This issue passed the CQ dry run.

[bokan, 2016-11-17 15:39:29]

Looks good, mostly just some polish on tests.

https://codereview.chromium.org/2471523002/diff/370001/cc/input/input_handler.h
File cc/input/input_handler.h (right):

https://codereview.chromium.org/2471523002/diff/370001/cc/input/input_handler...
cc/input/input_handler.h:108: enum TouchEventDisposition {
This looks unused?

https://codereview.chromium.org/2471523002/diff/370001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2471523002/diff/370001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:812:
EventListenerProperties::kBlocking);
EXPECT_EQ's arguments are (expected, actual), which means this is backwards.

https://codereview.chromium.org/2471523002/diff/370001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:822:
EXPECT_EQ(host_impl_->EventListenerTypeForTouchStartAt(gfx::Point(10, 30)),
Ditto to these ones.

https://codereview.chromium.org/2471523002/diff/370001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:9935: TEST_F(LayerTreeHostImplTest,
TouchInsideOrOutsideFlingLayer) {
Please add a short 1-2 sentence comment above describing what this test is
checking for.

https://codereview.chromium.org/2471523002/diff/370001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:9940:
CreateBasicVirtualViewportLayers(surface_size, surface_size);
I think the viewports should be the inner_size (we resize the content root to
surface_size/2 in CreateScrollableLayer anyway so this will make things more
logical).

https://codereview.chromium.org/2471523002/diff/370001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:9946: CreateScrollableLayer(27,
inner_size, root);
I think the clip for the grand_child should be child instead of root. I have a
hard time visualizing how this look as it is (though I know it technically works
as other tests do this). If it still works, change this to use child and make
the size surface_size*2. That'll simulate 2 nested scrolling divs.

https://codereview.chromium.org/2471523002/diff/370001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:9964:
host_impl_->SetViewportSize(surface_size);
No need for this, it's called as part of CreateBasicVirtualViewportLayers.

https://codereview.chromium.org/2471523002/diff/370001/ui/events/blink/input_...
File ui/events/blink/input_handler_proxy_unittest.cc (right):

https://codereview.chromium.org/2471523002/diff/370001/ui/events/blink/input_...
ui/events/blink/input_handler_proxy_unittest.cc:1946:
.WillOnce(testing::Return(cc::EventListenerProperties::kNone));
I believe this expects to be called twice so I think (I'm not a gMock pro) it
should be:

EXPECT_CALL(...)
    .Times(2)
    .WillRepeatedly(testing::Return(cc::EventListenerProperties::kNone));

In fact, since we really want it to return kNone for all cases, you can remove
the EXPECT_CALL below and use Times(3).

https://codereview.chromium.org/2471523002/diff/370001/ui/events/blink/input_...
ui/events/blink/input_handler_proxy_unittest.cc:1996: // to the main thread.
Is this comment wrong? It says "sent to main thread" but the disposition says
DID_HANDLE which I think means handled on impl, right? Or am I misunderstanding
the disposition meaning?

[lanwei, 2016-11-18 00:17:22]

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

[lanwei, 2016-11-18 00:17:39]

Patchset #8 (id:390001) has been deleted

[commit-bot: I haz the power, 2016-11-18 00:19:06]

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

[lanwei, 2016-11-18 00:54:08]

https://codereview.chromium.org/2471523002/diff/370001/cc/input/input_handler.h
File cc/input/input_handler.h (right):

https://codereview.chromium.org/2471523002/diff/370001/cc/input/input_handler...
cc/input/input_handler.h:108: enum TouchEventDisposition {
On 2016/11/17 15:39:28, bokan wrote:
> This looks unused?

Acknowledged.

https://codereview.chromium.org/2471523002/diff/370001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2471523002/diff/370001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:812:
EventListenerProperties::kBlocking);
On 2016/11/17 15:39:28, bokan wrote:
> EXPECT_EQ's arguments are (expected, actual), which means this is backwards.

Acknowledged.

https://codereview.chromium.org/2471523002/diff/370001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:822:
EXPECT_EQ(host_impl_->EventListenerTypeForTouchStartAt(gfx::Point(10, 30)),
On 2016/11/17 15:39:28, bokan wrote:
> Ditto to these ones.

Done.

https://codereview.chromium.org/2471523002/diff/370001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:9935: TEST_F(LayerTreeHostImplTest,
TouchInsideOrOutsideFlingLayer) {
On 2016/11/17 15:39:28, bokan wrote:
> Please add a short 1-2 sentence comment above describing what this test is
> checking for.

Done.

https://codereview.chromium.org/2471523002/diff/370001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:9940:
CreateBasicVirtualViewportLayers(surface_size, surface_size);
On 2016/11/17 15:39:28, bokan wrote:
> I think the viewports should be the inner_size (we resize the content root to
> surface_size/2 in CreateScrollableLayer anyway so this will make things more
> logical).

Done.

https://codereview.chromium.org/2471523002/diff/370001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:9946: CreateScrollableLayer(27,
inner_size, root);
On 2016/11/17 15:39:28, bokan wrote:
> I think the clip for the grand_child should be child instead of root. I have a
> hard time visualizing how this look as it is (though I know it technically
works
> as other tests do this). If it still works, change this to use child and make
> the size surface_size*2. That'll simulate 2 nested scrolling divs.

Sorry, I tried, but they do not work. I want to be able touch the child layer,
so the grand_child layer size is smaller than child. If I make  child as the
clip for the grand_child, then I could not touch the child layer.

https://codereview.chromium.org/2471523002/diff/370001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:9964:
host_impl_->SetViewportSize(surface_size);
On 2016/11/17 15:39:28, bokan wrote:
> No need for this, it's called as part of CreateBasicVirtualViewportLayers.

Done.

https://codereview.chromium.org/2471523002/diff/370001/ui/events/blink/input_...
File ui/events/blink/input_handler_proxy_unittest.cc (right):

https://codereview.chromium.org/2471523002/diff/370001/ui/events/blink/input_...
ui/events/blink/input_handler_proxy_unittest.cc:1946:
.WillOnce(testing::Return(cc::EventListenerProperties::kNone));
On 2016/11/17 15:39:28, bokan wrote:
> I believe this expects to be called twice so I think (I'm not a gMock pro) it
> should be:
> 
> EXPECT_CALL(...)
>     .Times(2)
>     .WillRepeatedly(testing::Return(cc::EventListenerProperties::kNone));
> 
> In fact, since we really want it to return kNone for all cases, you can remove
> the EXPECT_CALL below and use Times(3).
I changed the test, it is called twice, because it is only called when it is a
press.

https://codereview.chromium.org/2471523002/diff/370001/ui/events/blink/input_...
ui/events/blink/input_handler_proxy_unittest.cc:1996: // to the main thread.
On 2016/11/17 15:39:28, bokan wrote:
> Is this comment wrong? It says "sent to main thread" but the disposition says
> DID_HANDLE which I think means handled on impl, right? Or am I
misunderstanding
> the disposition meaning?

Done.

[commit-bot: I haz the power, 2016-11-18 03:13:41]

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

[commit-bot: I haz the power, 2016-11-18 03:13:42]

Dry run: This issue passed the CQ dry run.

[bokan, 2016-11-18 18:41:36]

lgtm

https://codereview.chromium.org/2471523002/diff/370001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2471523002/diff/370001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:9946: CreateScrollableLayer(27,
inner_size, root);
On 2016/11/18 00:54:07, lanwei wrote:
> On 2016/11/17 15:39:28, bokan wrote:
> > I think the clip for the grand_child should be child instead of root. I have
a
> > hard time visualizing how this look as it is (though I know it technically
> works
> > as other tests do this). If it still works, change this to use child and
make
> > the size surface_size*2. That'll simulate 2 nested scrolling divs.
> 
> Sorry, I tried, but they do not work. I want to be able touch the child layer,
> so the grand_child layer size is smaller than child. If I make  child as the
> clip for the grand_child, then I could not touch the child layer.

Ok, thanks for trying.

https://codereview.chromium.org/2471523002/diff/370001/ui/events/blink/input_...
File ui/events/blink/input_handler_proxy_unittest.cc (right):

https://codereview.chromium.org/2471523002/diff/370001/ui/events/blink/input_...
ui/events/blink/input_handler_proxy_unittest.cc:1946:
.WillOnce(testing::Return(cc::EventListenerProperties::kNone));
On 2016/11/18 00:54:07, lanwei wrote:
> On 2016/11/17 15:39:28, bokan wrote:
> > I believe this expects to be called twice so I think (I'm not a gMock pro)
it
> > should be:
> > 
> > EXPECT_CALL(...)
> >     .Times(2)
> >     .WillRepeatedly(testing::Return(cc::EventListenerProperties::kNone));
> > 
> > In fact, since we really want it to return kNone for all cases, you can
remove
> > the EXPECT_CALL below and use Times(3).
> I changed the test, it is called twice, because it is only called when it is a
> press.

Ah! You're right, that makes more sense :)

https://codereview.chromium.org/2471523002/diff/410001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2471523002/diff/410001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:9935: // We will force the touch event
handler to be passive, only if we touch on a
Nit: "passive, only if we touch" -> "passive if we touch"

[dtapuska, 2016-11-21 19:19:30]

https://codereview.chromium.org/2471523002/diff/410001/cc/input/event_listene...
File cc/input/event_listener_properties.h (left):

https://codereview.chromium.org/2471523002/diff/410001/cc/input/event_listene...
cc/input/event_listener_properties.h:24: kBlockingAndPassive,
I have a problem with this enum not matching the blink enum now.

https://cs.chromium.org/chromium/src/content/renderer/gpu/render_widget_compo...

The new method doesn't ever return kPassive. 

I wonder if we really should just return a special enum for this method that you
added; it could have very similar constants to this. But using the enum is
dangerous as it is statically cast and checked between blink and CC. (although
it appears to be missing a static size check for kMax which would have caught
this discrepency).

[lanwei, 2016-11-23 21:00:24]

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

[commit-bot: I haz the power, 2016-11-23 21:01: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, 2016-11-23 21:04:40]

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

[commit-bot: I haz the power, 2016-11-23 21:04:41]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[lanwei, 2016-11-23 21:12:10]

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

[commit-bot: I haz the power, 2016-11-23 21:12: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, 2016-11-23 21:15:47]

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

[commit-bot: I haz the power, 2016-11-23 21:15:48]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[lanwei, 2016-11-23 21:20:48]

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

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

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

[lanwei, 2016-11-23 21:23:22]

Patchset #9 (id:430001) has been deleted

[lanwei, 2016-11-23 21:23:31]

Patchset #9 (id:450001) has been deleted

[commit-bot: I haz the power, 2016-11-23 21:24:27]

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

[commit-bot: I haz the power, 2016-11-23 21:24:28]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[lanwei, 2016-11-23 21:33:12]

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

[commit-bot: I haz the power, 2016-11-23 21:33:47]

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

[lanwei, 2016-11-23 21:35:16]

Patchset #9 (id:470001) has been deleted

[commit-bot: I haz the power, 2016-11-23 21:36:41]

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

[commit-bot: I haz the power, 2016-11-23 21:36:42]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[lanwei, 2016-11-23 21:57:38]

https://codereview.chromium.org/2471523002/diff/410001/cc/input/event_listene...
File cc/input/event_listener_properties.h (left):

https://codereview.chromium.org/2471523002/diff/410001/cc/input/event_listene...
cc/input/event_listener_properties.h:24: kBlockingAndPassive,
On 2016/11/21 19:19:30, dtapuska wrote:
> I have a problem with this enum not matching the blink enum now.
> 
>
https://cs.chromium.org/chromium/src/content/renderer/gpu/render_widget_compo...
> 
> The new method doesn't ever return kPassive. 
> 
> I wonder if we really should just return a special enum for this method that
you
> added; it could have very similar constants to this. But using the enum is
> dangerous as it is statically cast and checked between blink and CC. (although
> it appears to be missing a static size check for kMax which would have caught
> this discrepency).

Done.

https://codereview.chromium.org/2471523002/diff/410001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2471523002/diff/410001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl_unittest.cc:9935: // We will force the touch event
handler to be passive, only if we touch on a
On 2016/11/18 18:41:36, bokan wrote:
> Nit: "passive, only if we touch" -> "passive if we touch"

Done.

[dtapuska, 2016-11-23 22:01:08]

On 2016/11/23 21:57:38, lanwei wrote:
>
https://codereview.chromium.org/2471523002/diff/410001/cc/input/event_listene...
> File cc/input/event_listener_properties.h (left):
> 
>
https://codereview.chromium.org/2471523002/diff/410001/cc/input/event_listene...
> cc/input/event_listener_properties.h:24: kBlockingAndPassive,
> On 2016/11/21 19:19:30, dtapuska wrote:
> > I have a problem with this enum not matching the blink enum now.
> > 
> >
>
https://cs.chromium.org/chromium/src/content/renderer/gpu/render_widget_compo...
> > 
> > The new method doesn't ever return kPassive. 
> > 
> > I wonder if we really should just return a special enum for this method that
> you
> > added; it could have very similar constants to this. But using the enum is
> > dangerous as it is statically cast and checked between blink and CC.
(although
> > it appears to be missing a static size check for kMax which would have
caught
> > this discrepency).
> 
> Done.
> 
>
https://codereview.chromium.org/2471523002/diff/410001/cc/trees/layer_tree_ho...
> File cc/trees/layer_tree_host_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/2471523002/diff/410001/cc/trees/layer_tree_ho...
> cc/trees/layer_tree_host_impl_unittest.cc:9935: // We will force the touch
event
> handler to be passive, only if we touch on a
> On 2016/11/18 18:41:36, bokan wrote:
> > Nit: "passive, only if we touch" -> "passive if we touch"
> 
> Done.

lgtm

[tdresser, 2016-11-24 19:53:41]

https://codereview.chromium.org/2471523002/diff/490001/ui/events/blink/input_...
File ui/events/blink/input_handler_proxy.cc (right):

https://codereview.chromium.org/2471523002/diff/490001/ui/events/blink/input_...
ui/events/blink/input_handler_proxy.cc:938: bool maybe_passive_due_to_fling =
false;
What does this bool actually mean? Should this be |touching_scrolling_layer| or
something like that?

[lanwei, 2016-11-24 20:22:34]

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

[commit-bot: I haz the power, 2016-11-24 20:22:50]

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

[commit-bot: I haz the power, 2016-11-24 20:24:06]

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

[commit-bot: I haz the power, 2016-11-24 20:24:07]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[lanwei, 2016-11-24 20:28:01]

https://codereview.chromium.org/2471523002/diff/490001/ui/events/blink/input_...
File ui/events/blink/input_handler_proxy.cc (right):

https://codereview.chromium.org/2471523002/diff/490001/ui/events/blink/input_...
ui/events/blink/input_handler_proxy.cc:938: bool maybe_passive_due_to_fling =
false;
On 2016/11/24 19:53:41, tdresser wrote:
> What does this bool actually mean? Should this be |touching_scrolling_layer|
or
> something like that?

Done.

[tdresser, 2016-11-24 20:33:38]

https://codereview.chromium.org/2471523002/diff/510001/cc/input/input_handler.h
File cc/input/input_handler.h (right):

https://codereview.chromium.org/2471523002/diff/510001/cc/input/input_handler...
cc/input/input_handler.h:110: HANDLER_ON_FLINGING_LAYER
Same as previous comment - shouldn't this just be HANDLER_ON_SCROLLING_LAYER?

[lanwei, 2016-11-24 20:45:05]

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

[commit-bot: I haz the power, 2016-11-24 20:45:23]

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

[commit-bot: I haz the power, 2016-11-24 20:46:28]

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

[commit-bot: I haz the power, 2016-11-24 20:46:29]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[lanwei, 2016-11-28 05:39:33]

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

[commit-bot: I haz the power, 2016-11-28 05:39:40]

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

[commit-bot: I haz the power, 2016-11-28 05:40:29]

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

[commit-bot: I haz the power, 2016-11-28 05:40:30]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[lanwei, 2016-11-28 06:20:23]

https://codereview.chromium.org/2471523002/diff/510001/cc/input/input_handler.h
File cc/input/input_handler.h (right):

https://codereview.chromium.org/2471523002/diff/510001/cc/input/input_handler...
cc/input/input_handler.h:110: HANDLER_ON_FLINGING_LAYER
On 2016/11/24 20:33:38, tdresser wrote:
> Same as previous comment - shouldn't this just be HANDLER_ON_SCROLLING_LAYER?

Done.

[tdresser, 2016-11-28 14:18:48]

LGTM % comment fix.

https://codereview.chromium.org/2471523002/diff/530001/cc/input/input_handler.h
File cc/input/input_handler.h (right):

https://codereview.chromium.org/2471523002/diff/530001/cc/input/input_handler...
cc/input/input_handler.h:199: // |viewport_point|.
This comment is confusing.

This doesn't indicate whether the page "should" suppress scrolling, only if the
page should have the opportunity to suppress scrolling.

Whether the page has a touch handler or not is equivalent to whether it has the
opportunity to prevent scrolling, and this comment phrases it as though this is
separate information.

How is this comment different from the previous comment?

[lanwei, 2016-11-28 21:59:38]

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

[commit-bot: I haz the power, 2016-11-28 22:00:59]

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

[commit-bot: I haz the power, 2016-11-28 22:09:35]

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

[commit-bot: I haz the power, 2016-11-28 22:09:37]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[lanwei, 2016-11-28 23:01:43]

The CQ bit was checked by lanwei@chromium.org

[lanwei, 2016-11-28 23:01:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
weiliangc@chromium.org, bokan@chromium.org, dtapuska@chromium.org,
tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/2471523002/#ps550001
(title: "Reword the comments")

[commit-bot: I haz the power, 2016-11-28 23:03:08]

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

[commit-bot: I haz the power, 2016-11-28 23:09:46]

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

[commit-bot: I haz the power, 2016-11-28 23:09:48]

Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[lanwei, 2016-11-29 04:30:03]

The CQ bit was checked by lanwei@chromium.org

[commit-bot: I haz the power, 2016-11-29 04:30:28]

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

[commit-bot: I haz the power, 2016-11-29 04:31:48]

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

[commit-bot: I haz the power, 2016-11-29 04:31:51]

Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[lanwei, 2016-11-29 05:03:46]

The CQ bit was checked by lanwei@chromium.org

[lanwei, 2016-11-29 05:03:47]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
weiliangc@chromium.org, dtapuska@chromium.org, tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/2471523002/#ps570001
(title: "fling layer")

[commit-bot: I haz the power, 2016-11-29 05:03:59]

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

[commit-bot: I haz the power, 2016-11-29 05:04:58]

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

[commit-bot: I haz the power, 2016-11-29 05:05:00]

Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[lanwei, 2016-11-29 05:16:02]

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

[commit-bot: I haz the power, 2016-11-29 05:16: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, 2016-11-29 05:17:16]

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

[commit-bot: I haz the power, 2016-11-29 05:17:17]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, no build URL)

[lanwei, 2016-11-29 05:28:37]

Description was changed from

==========
Make touch events uncancelable during fling when they are on the current active
scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to
check if the
touch events which we will make uncancelable when there is an active fling are
on the
same layer as the current active scroll layer or its descendants.

Now, we add the scroll layer check here to minimize the breaking cases and also
match with other
browsers' behavior, such as Safaris and Firefox.

This code has two parts:
1. Delete the is_fling flag we used on
https://codereview.chromium.org/1923973002/
to indicate if the actice fling is happening.
2. Add check to the touch start if it hits on the same layer as the current
active scroll layer.

BUG=595327
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Make touch events uncancelable during fling when they are on the current active
scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to
check if the
touch events which we will make uncancelable when there is an active fling are
on the
same layer as the current active scroll layer or its descendants.

Now, we add the scroll layer check here to minimize the breaking cases and also
match with other
browsers' behavior, such as Safaris and Firefox.

This code has two parts:
1. Delete the is_fling flag we used on
https://codereview.chromium.org/1923973002/
to indicate if the actice fling is happening.
2. Add check to the touch start if it hits on the same layer as the current
active scroll layer.

BUG=595327
==========

[lanwei, 2016-11-29 05:29:08]

The CQ bit was checked by lanwei@chromium.org

[commit-bot: I haz the power, 2016-11-29 05:29:23]

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

[commit-bot: I haz the power, 2016-11-29 06:51:22]

CQ is committing da patch.

Bot data: {"patchset_id": 570001, "attempt_start_ts": 1480397348740560,
"parent_rev": "41cd7c95eb829d845b43ade63a29db08100cf4f2", "commit_rev":
"ad4202b79387f43ada35c680ed0cc216802976ba"}

[commit-bot: I haz the power, 2016-11-29 06:51:57]

Description was changed from

==========
Make touch events uncancelable during fling when they are on the current active
scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to
check if the
touch events which we will make uncancelable when there is an active fling are
on the
same layer as the current active scroll layer or its descendants.

Now, we add the scroll layer check here to minimize the breaking cases and also
match with other
browsers' behavior, such as Safaris and Firefox.

This code has two parts:
1. Delete the is_fling flag we used on
https://codereview.chromium.org/1923973002/
to indicate if the actice fling is happening.
2. Add check to the touch start if it hits on the same layer as the current
active scroll layer.

BUG=595327
==========

to

==========
Make touch events uncancelable during fling when they are on the current active
scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to
check if the
touch events which we will make uncancelable when there is an active fling are
on the
same layer as the current active scroll layer or its descendants.

Now, we add the scroll layer check here to minimize the breaking cases and also
match with other
browsers' behavior, such as Safaris and Firefox.

This code has two parts:
1. Delete the is_fling flag we used on
https://codereview.chromium.org/1923973002/
to indicate if the actice fling is happening.
2. Add check to the touch start if it hits on the same layer as the current
active scroll layer.

BUG=595327
==========

[commit-bot: I haz the power, 2016-11-29 06:51:59]

Committed patchset #13 (id:570001)

[commit-bot: I haz the power, 2016-11-29 06:54:21]

Description was changed from

==========
Make touch events uncancelable during fling when they are on the current active
scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to
check if the
touch events which we will make uncancelable when there is an active fling are
on the
same layer as the current active scroll layer or its descendants.

Now, we add the scroll layer check here to minimize the breaking cases and also
match with other
browsers' behavior, such as Safaris and Firefox.

This code has two parts:
1. Delete the is_fling flag we used on
https://codereview.chromium.org/1923973002/
to indicate if the actice fling is happening.
2. Add check to the touch start if it hits on the same layer as the current
active scroll layer.

BUG=595327
==========

to

==========
Make touch events uncancelable during fling when they are on the current active
scroll layer

In the previous patch https://codereview.chromium.org/2233543002/, we missed to
check if the
touch events which we will make uncancelable when there is an active fling are
on the
same layer as the current active scroll layer or its descendants.

Now, we add the scroll layer check here to minimize the breaking cases and also
match with other
browsers' behavior, such as Safaris and Firefox.

This code has two parts:
1. Delete the is_fling flag we used on
https://codereview.chromium.org/1923973002/
to indicate if the actice fling is happening.
2. Add check to the touch start if it hits on the same layer as the current
active scroll layer.

BUG=595327

Committed: https://crrev.com/53c12368f78425a27d592f47f814029cf2edaae7
Cr-Commit-Position: refs/heads/master@{#434917}
==========

[commit-bot: I haz the power, 2016-11-29 06:54:24]

Patchset 13 (id:??) landed as
https://crrev.com/53c12368f78425a27d592f47f814029cf2edaae7
Cr-Commit-Position: refs/heads/master@{#434917}

[lanwei, 2016-12-01 22:44:18]

https://codereview.chromium.org/2471523002/diff/530001/cc/input/input_handler.h
File cc/input/input_handler.h (right):

https://codereview.chromium.org/2471523002/diff/530001/cc/input/input_handler...
cc/input/input_handler.h:199: // |viewport_point|.
On 2016/11/28 14:18:47, tdresser wrote:
> This comment is confusing.
> 
> This doesn't indicate whether the page "should" suppress scrolling, only if
the
> page should have the opportunity to suppress scrolling.
> 
> Whether the page has a touch handler or not is equivalent to whether it has
the
> opportunity to prevent scrolling, and this comment phrases it as though this
is
> separate information.
> 
> How is this comment different from the previous comment?

Done.