Pass TouchAction bits together with rect in SetTouchEventHandlerRegion

Pass TouchAction bits together with rect in SetTouchEventHandlerRegion

At this moment, the WebLayer::SetTouchEventHandlerRegion only passes
a vector of rects to CC. We would like to pass touch action bits
corresponding to each rect so for optimization purposes.

In this CL, I only changed the API SetTouchEventHandlerRegion, so that
both blink and cc sides can work in parallel.

This CL has no behavior change.

BUG=716668
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2856153003
Cr-Commit-Position: refs/heads/master@{#471943}
Committed: https://chromium.googlesource.com/chromium/src/+/28dd0bab32d0c43cba71995b99e2255d36e42b7e

[xidachen, 2017-05-03 19:03:16]

Description was changed from

==========
Pass TouchAction bits together with rect in SetTouchEventHandlerRegion

At this moment, the WebLayer::SetTouchEventHandlerRegion only passes
a vector of rects to CC. We would like to pass touch action bits
corresponding to each rect so for optimization purposes.

In this CL, I only changed the API SetTouchEventHandlerRegion, so that
both blink and cc sides can work in parallel.

This CL has no behavior change.

BUG=716668
==========

to

==========
Pass TouchAction bits together with rect in SetTouchEventHandlerRegion

At this moment, the WebLayer::SetTouchEventHandlerRegion only passes
a vector of rects to CC. We would like to pass touch action bits
corresponding to each rect so for optimization purposes.

In this CL, I only changed the API SetTouchEventHandlerRegion, so that
both blink and cc sides can work in parallel.

This CL has no behavior change.

BUG=716668
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[xidachen, 2017-05-03 19:07:05]

xidachen@chromium.org changed reviewers:
+ pdr@chromium.org, wkorman@chromium.org

[xidachen, 2017-05-03 19:07:06]

PTAL.

Note: the name "WebTouchActionRect" is up to debate.

[wkorman, 2017-05-03 20:02:14]

https://codereview.chromium.org/2856153003/diff/1/cc/blink/web_layer_impl.cc
File cc/blink/web_layer_impl.cc (right):

https://codereview.chromium.org/2856153003/diff/1/cc/blink/web_layer_impl.cc#...
cc/blink/web_layer_impl.cc:303: const WebVector<blink::WebTouchActionRect>&
touch_action_rect) {
I am hoping that as part of our work we can stop using WebXxx classes, whose
usage in this area is perhaps now legacy from pre-Blink/Chromium-repo-merge, but
am supportive of landing this until we can explore that further.

I think we don't need to use WebVector, WebRect, or WebXxx any more.

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

https://codereview.chromium.org/2856153003/diff/1/cc/blink/web_layer_impl.h#n...
cc/blink/web_layer_impl.h:109: const
blink::WebVector<blink::WebTouchActionRect>& touch_action_rect)
Suggest pluralizing param name (it wasn't before, and isn't in similar method
above, but IMO it should have been)

https://codereview.chromium.org/2856153003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
(right):

https://codereview.chromium.org/2856153003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:789:
WebVector<WebTouchActionRect> web_touch_action_rect(
I think I prefer WebTouchInfo for the class name, both because it's shorter, and
seems possible/likely it could grow add'l fields or field types could change
later.

I would also support just calling the local var here "touch" since it's shorter,
has no naming conflict, and the block of code is not long.

https://codereview.chromium.org/2856153003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:793:
web_touch_action_rect[i].touch_action =
Add TODO(wkorman) here noting that we should route the real value here.

https://codereview.chromium.org/2856153003/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebTouchActionRect.h (right):

https://codereview.chromium.org/2856153003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebTouchActionRect.h:12: // WebTouchInfo
stores a rect with its corresponding touch action, which will
WebTouchInfo -> WebTouchActionRect (if we keep the name as WebTouchActionRect)

https://codereview.chromium.org/2856153003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebTouchActionRect.h:13: // be passed to CC
through WebLayer::SetTouchEventHandlerRegion.
Would remove the bit about "which will be passed..." since the code path/methods
can change down the line and easy to forget to update comments. Generally prefer
comments to focus on behavior/functionality.

https://codereview.chromium.org/2856153003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebTouchActionRect.h:21: int touch_action;
I think we can use an enum here, following pattern we use for element_id.h and
CompositorElementId.h.

You can explore that if you'd like, or I am ok landing this in interim and you
can focus on the cc side logic, but if we do that let's update the comment to
note that this is temporary while we explore that.

To elaborate, I think to do it we'll have to do something like:
1. define yet another copy of the enum under cc/ dir somewhere. This will really
just be moving the existing Blink definition detail (more below).
2. define a new Blink-side TouchAction class under platform/graphics or similar
that uses 'using' to extend with a Blink-specific class name
3. delete WebTouchAction class and change all usages to use the class we defined
in (2) instead.

There may be some trickiness though because there is a bunch of core/style usage
of the 'enum TouchAction' defined in core/style/ComputedStyleConstants.h, and
I'd hope we can get rid of that enum and have core/style use the one we define
in (2) as well, but it's possible I'm not yet aware of some dependency issue
we'll have to resolve.

Crazily it appears there are at least three exact copies of the enum today:
a. content/common/input/touch_action.h
b. third_party/WebKit/Source/public/web/WebTouchAction.h
c. third_party/WebKit/Source/core/style/ComputedStyleConstants.h

and even more crazily, code in public/web and content just does a straight
static_cast from one enum type to another.

So given the complexity (I am amidst unwinding/understanding all of the above,
have been researching the code) I support going with this interim patch if you'd
prefer.

[xidachen, 2017-05-04 12:11:21]

https://codereview.chromium.org/2856153003/diff/1/cc/blink/web_layer_impl.cc
File cc/blink/web_layer_impl.cc (right):

https://codereview.chromium.org/2856153003/diff/1/cc/blink/web_layer_impl.cc#...
cc/blink/web_layer_impl.cc:303: const WebVector<blink::WebTouchActionRect>&
touch_action_rect) {
On 2017/05/03 20:02:14, wkorman wrote:
> I am hoping that as part of our work we can stop using WebXxx classes, whose
> usage in this area is perhaps now legacy from pre-Blink/Chromium-repo-merge,
but
> am supportive of landing this until we can explore that further.
> 
> I think we don't need to use WebVector, WebRect, or WebXxx any more.

I think there is a bit of complexity here, and please correct me if I
misunderstood things:
1). If we use IntRect or gfx::rect in the new class, then we would be doing
things that is very similar to WebRect class, which is:
#if INSIDE_BLINK
  using IntRect
#else
  using gfx::rect
I think that makes things more complicated.

2). If we do use IntRect or gfx::rect, then in here we need to include either
WebKit/Source/platform/ or ui/gfx. At this moment, cc/blink/ is only allowed to
use things under WebKit/Source/public/platform.

To solve this problem systematically, we should investigate this bug:
crbug.com/584551, which is to erase the entire dir of cc/blink/ and that is
basically one class: WebLayer. I am not sure what is the best time for this, now
or after SPv2.

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

https://codereview.chromium.org/2856153003/diff/1/cc/blink/web_layer_impl.h#n...
cc/blink/web_layer_impl.h:109: const
blink::WebVector<blink::WebTouchActionRect>& touch_action_rect)
On 2017/05/03 20:02:14, wkorman wrote:
> Suggest pluralizing param name (it wasn't before, and isn't in similar method
> above, but IMO it should have been)

Changed to touch_info.

https://codereview.chromium.org/2856153003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
(right):

https://codereview.chromium.org/2856153003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:789:
WebVector<WebTouchActionRect> web_touch_action_rect(
On 2017/05/03 20:02:14, wkorman wrote:
> I think I prefer WebTouchInfo for the class name, both because it's shorter,
and
> seems possible/likely it could grow add'l fields or field types could change
> later.
> 
> I would also support just calling the local var here "touch" since it's
shorter,
> has no naming conflict, and the block of code is not long.

Done.

https://codereview.chromium.org/2856153003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:793:
web_touch_action_rect[i].touch_action =
On 2017/05/03 20:02:14, wkorman wrote:
> Add TODO(wkorman) here noting that we should route the real value here.

Done.

https://codereview.chromium.org/2856153003/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebTouchActionRect.h (right):

https://codereview.chromium.org/2856153003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebTouchActionRect.h:12: // WebTouchInfo
stores a rect with its corresponding touch action, which will
On 2017/05/03 20:02:14, wkorman wrote:
> WebTouchInfo -> WebTouchActionRect (if we keep the name as WebTouchActionRect)

Changed to WebTouchInfo.

https://codereview.chromium.org/2856153003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebTouchActionRect.h:13: // be passed to CC
through WebLayer::SetTouchEventHandlerRegion.
On 2017/05/03 20:02:14, wkorman wrote:
> Would remove the bit about "which will be passed..." since the code
path/methods
> can change down the line and easy to forget to update comments. Generally
prefer
> comments to focus on behavior/functionality.

comments updated.

https://codereview.chromium.org/2856153003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebTouchActionRect.h:21: int touch_action;
On 2017/05/03 20:02:14, wkorman wrote:
> I think we can use an enum here, following pattern we use for element_id.h and
> CompositorElementId.h.
> 
> You can explore that if you'd like, or I am ok landing this in interim and you
> can focus on the cc side logic, but if we do that let's update the comment to
> note that this is temporary while we explore that.
> 
> To elaborate, I think to do it we'll have to do something like:
> 1. define yet another copy of the enum under cc/ dir somewhere. This will
really
> just be moving the existing Blink definition detail (more below).
> 2. define a new Blink-side TouchAction class under platform/graphics or
similar
> that uses 'using' to extend with a Blink-specific class name
> 3. delete WebTouchAction class and change all usages to use the class we
defined
> in (2) instead.
> 
> There may be some trickiness though because there is a bunch of core/style
usage
> of the 'enum TouchAction' defined in core/style/ComputedStyleConstants.h, and
> I'd hope we can get rid of that enum and have core/style use the one we define
> in (2) as well, but it's possible I'm not yet aware of some dependency issue
> we'll have to resolve.
> 
> Crazily it appears there are at least three exact copies of the enum today:
> a. content/common/input/touch_action.h
> b. third_party/WebKit/Source/public/web/WebTouchAction.h
> c. third_party/WebKit/Source/core/style/ComputedStyleConstants.h
> 
> and even more crazily, code in public/web and content just does a straight
> static_cast from one enum type to another.
> 
> So given the complexity (I am amidst unwinding/understanding all of the above,
> have been researching the code) I support going with this interim patch if
you'd
> prefer.

I think I would prefer landing a simple patch first and investigate on two
things:
1). Unify TouchAction classes.
2). Get rid of the WebLayer class as I mentioned before.

WDYT?

[wkorman, 2017-05-04 18:45:12]

Discussed offline, xida is pursuing work in http://crbug.com/718525 as next
step, hopefully we can unify TouchAction enum as first step.

We could look at the WebRect, Vector, Layer stuff separately. I only have rough
sense they are obsolete and no longer needed. It may be there are details. I am
pretty sure we've been writing new code under platform/graphics that avoids all
use of them.

[xidachen, 2017-05-12 14:41:25]

PS#4 is now ready, PTAL.

[wkorman, 2017-05-12 22:11:14]

lgtm

https://codereview.chromium.org/2856153003/diff/60001/cc/blink/web_layer_impl.cc
File cc/blink/web_layer_impl.cc (left):

https://codereview.chromium.org/2856153003/diff/60001/cc/blink/web_layer_impl...
cc/blink/web_layer_impl.cc:307: }
Is this where we'll want to do something with the touch_info.touch_action bits
eventually? If so perhaps add TODO noting that as well.

[pdr., 2017-05-13 02:39:33]

LGTM

[xidachen, 2017-05-13 13:14:22]

xidachen@chromium.org changed reviewers:
+ chrishtr@chromium.org

[xidachen, 2017-05-13 13:14:22]

+chrishtr@: cc/blink/

PTAL. Thanks.

[chrishtr, 2017-05-15 18:03:29]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2017-05-15 18:03:30]

lgtm

[chrishtr, 2017-05-15 18:03:30]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2856153003/#ps80001
(title: "rebase + comment")

[commit-bot: I haz the power, 2017-05-15 18:04:30]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-15 18:48:37]

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

[commit-bot: I haz the power, 2017-05-15 18:48:39]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[xidachen, 2017-05-15 20:31:52]

The CQ bit was checked by xidachen@chromium.org

[commit-bot: I haz the power, 2017-05-15 20:32:35]

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-15 23:35:10]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494880312909250,
"parent_rev": "37c4e81eb4dfa713a06164dbce721710b92b2ca9", "commit_rev":
"28dd0bab32d0c43cba71995b99e2255d36e42b7e"}

[commit-bot: I haz the power, 2017-05-15 23:35:33]

Description was changed from

==========
Pass TouchAction bits together with rect in SetTouchEventHandlerRegion

At this moment, the WebLayer::SetTouchEventHandlerRegion only passes
a vector of rects to CC. We would like to pass touch action bits
corresponding to each rect so for optimization purposes.

In this CL, I only changed the API SetTouchEventHandlerRegion, so that
both blink and cc sides can work in parallel.

This CL has no behavior change.

BUG=716668
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Pass TouchAction bits together with rect in SetTouchEventHandlerRegion

At this moment, the WebLayer::SetTouchEventHandlerRegion only passes
a vector of rects to CC. We would like to pass touch action bits
corresponding to each rect so for optimization purposes.

In this CL, I only changed the API SetTouchEventHandlerRegion, so that
both blink and cc sides can work in parallel.

This CL has no behavior change.

BUG=716668
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2856153003
Cr-Commit-Position: refs/heads/master@{#471943}
Committed:
https://chromium.googlesource.com/chromium/src/+/28dd0bab32d0c43cba71995b99e2...
==========

[commit-bot: I haz the power, 2017-05-15 23:35:34]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/28dd0bab32d0c43cba71995b99e2...