Unify TouchAction classes

Unify TouchAction classes

There are three TouchAction classes in chromium codebase and they look
and function exact the same. This CL unifies them into one. We put a
canonical one under cc/input/, and then the one in blink is placed under
platform/graphics, the one in blink is just using cc's TouchAction. We also
create a WebTouchAction enum under public/platform/ which directly uses the 
TouchAction in platform/graphics/. The reason of adding WebTouchAction is that 
there are methods in WebWidgetClient.h that is overridden by class under content/.

This CL does pure refactor, it causes no behavior change at all.

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

Review-Url: https://codereview.chromium.org/2863693003
Cr-Commit-Position: refs/heads/master@{#470919}
Committed: https://chromium.googlesource.com/chromium/src/+/fa0199e7e18ee33c28356449b6ba48ab41c879e7

[xidachen, 2017-05-05 02:15:49]

Description was changed from

==========
Unify TouchAction classes

There are three TouchAction classes in chromium codebase and they look
and function exact the same. This CL unifies them into one. We put a
canonical one under cc/input/, and then the one in blink is placed under
platform/graphics, the one in blink is just using cc's TouchAction.

BUG=718525
==========

to

==========
Unify TouchAction classes

There are three TouchAction classes in chromium codebase and they look
and function exact the same. This CL unifies them into one. We put a
canonical one under cc/input/, and then the one in blink is placed under
platform/graphics, the one in blink is just using cc's TouchAction.

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

[xidachen, 2017-05-05 15:14:01]

Description was changed from

==========
Unify TouchAction classes

There are three TouchAction classes in chromium codebase and they look
and function exact the same. This CL unifies them into one. We put a
canonical one under cc/input/, and then the one in blink is placed under
platform/graphics, the one in blink is just using cc's TouchAction.

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

to

==========
WIP: Unify TouchAction classes

There are three TouchAction classes in chromium codebase and they look
and function exact the same. This CL unifies them into one. We put a
canonical one under cc/input/, and then the one in blink is placed under
platform/graphics, the one in blink is just using cc's TouchAction.

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

[xidachen, 2017-05-05 15:14:01]

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

[xidachen, 2017-05-05 15:17:52]

https://codereview.chromium.org/2863693003/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebWidgetClient.h (right):

https://codereview.chromium.org/2863693003/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebWidgetClient.h:145: virtual void
SetTouchAction(blink::TouchAction touch_action) {}
wkorman@, pdr@: I am getting close. Just one last bit here. Right now this patch
has a blink::TouchAction directly using the cc::TouchAction. This particular
function is over-written in content/, and content/ is only allowed to use cc/,
or "third_party/WebKit/public". So to make it work, it looks like we need to
create a WebTouchAction under public/web/ and make WebTouchAction directly using
blink::TouchAction. Does that make sense?

[wkorman, 2017-05-05 20:25:47]

https://codereview.chromium.org/2863693003/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebWidgetClient.h (right):

https://codereview.chromium.org/2863693003/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebWidgetClient.h:145: virtual void
SetTouchAction(blink::TouchAction touch_action) {}
On 2017/05/05 15:17:52, xidachen wrote:
> wkorman@, pdr@: I am getting close. Just one last bit here. Right now this
patch
> has a blink::TouchAction directly using the cc::TouchAction. This particular
> function is over-written in content/, and content/ is only allowed to use cc/,
> or "third_party/WebKit/public". So to make it work, it looks like we need to
> create a WebTouchAction under public/web/ and make WebTouchAction directly
using
> blink::TouchAction. Does that make sense?

I think I agree with you, with my (our?) newfound insight from email thread with
haraken@ on the current and future purpose of public/web and your exploration
here.

Let's make sure we add a comment to the WebTouchAction class noting this info.
Also, from rereading haraken@ email, it sounds like in short term public/web is
going away and in that situation WebTouchAction would instead be in public/core
or public/platform. I am not sure whether we can actually just place it there
now, perhaps you could try and see.

[xidachen, 2017-05-05 20:32:46]

https://codereview.chromium.org/2863693003/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebWidgetClient.h (right):

https://codereview.chromium.org/2863693003/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebWidgetClient.h:142: virtual void
SetTouchAction(blink::WebTouchAction touch_action) {}
So I tried this, and unfortunately it doesn't work. As you see, in the
WebTouchAction.h, in order to include "platform/graphics/TouchAction.h", I have
to use the "if BLINK_IMPLEMENTATION" thing. With that, compiler complains that
"no type named 'WebTouchAction' in namespace 'blink'" at this point. I am not
sure if there is a workaround on this.

[wkorman, 2017-05-05 20:39:15]

https://codereview.chromium.org/2863693003/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebTouchAction.h (right):

https://codereview.chromium.org/2863693003/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebTouchAction.h:9: #include
"platform/graphics/TouchAction.h"
What if we move WebTouchAction to public/platform? I see code in public/platform
referencing platform/graphics in #includes already, for example,
WebImageLayer.h.

[wkorman, 2017-05-05 20:40:52]

https://codereview.chromium.org/2863693003/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebTouchAction.h (right):

https://codereview.chromium.org/2863693003/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebTouchAction.h:9: #include
"platform/graphics/TouchAction.h"
On 2017/05/05 20:39:15, wkorman wrote:
> What if we move WebTouchAction to public/platform? I see code in
public/platform
> referencing platform/graphics in #includes already, for example,
> WebImageLayer.h.

Well, that class actually is doing:

#if INSIDE_BLINK
#include "platform/graphics/paint/PaintImage.h"
#else
#include "cc/paint/paint_image.h"
using PaintImage = cc::PaintImage;
#endif

but perhaps we could do similar, and reference cc's TouchAction when not
INSIDE_BLINK.

[xidachen, 2017-05-05 20:41:47]

https://codereview.chromium.org/2863693003/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebTouchAction.h (right):

https://codereview.chromium.org/2863693003/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebTouchAction.h:9: #include
"platform/graphics/TouchAction.h"
On 2017/05/05 20:40:52, wkorman wrote:
> On 2017/05/05 20:39:15, wkorman wrote:
> > What if we move WebTouchAction to public/platform? I see code in
> public/platform
> > referencing platform/graphics in #includes already, for example,
> > WebImageLayer.h.
> 
> Well, that class actually is doing:
> 
> #if INSIDE_BLINK
> #include "platform/graphics/paint/PaintImage.h"
> #else
> #include "cc/paint/paint_image.h"
> using PaintImage = cc::PaintImage;
> #endif
> 
> but perhaps we could do similar, and reference cc's TouchAction when not
> INSIDE_BLINK.

I think that should work. Let me give it a try. Thanks.

[xidachen, 2017-05-06 01:23:47]

Description was changed from

==========
WIP: Unify TouchAction classes

There are three TouchAction classes in chromium codebase and they look
and function exact the same. This CL unifies them into one. We put a
canonical one under cc/input/, and then the one in blink is placed under
platform/graphics, the one in blink is just using cc's TouchAction.

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

to

==========
WIP: Unify TouchAction classes

There are three TouchAction classes in chromium codebase and they look
and function exact the same. This CL unifies them into one. We put a
canonical one under cc/input/, and then the one in blink is placed under
platform/graphics, the one in blink is just using cc's TouchAction. We also
create a WebTouchAction enum under public/platform/ which directly uses the
TouchAction
in platform/graphics/. The reason of adding WebTouchAction is that there are
methods in WebWidgetClient.h that is overridden by class under content/.

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

[xidachen, 2017-05-06 01:28:45]

Description was changed from

==========
WIP: Unify TouchAction classes

There are three TouchAction classes in chromium codebase and they look
and function exact the same. This CL unifies them into one. We put a
canonical one under cc/input/, and then the one in blink is placed under
platform/graphics, the one in blink is just using cc's TouchAction. We also
create a WebTouchAction enum under public/platform/ which directly uses the
TouchAction
in platform/graphics/. The reason of adding WebTouchAction is that there are
methods in WebWidgetClient.h that is overridden by class under content/.

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

to

==========
WIP: Unify TouchAction classes

There are three TouchAction classes in chromium codebase and they look
and function exact the same. This CL unifies them into one. We put a
canonical one under cc/input/, and then the one in blink is placed under
platform/graphics, the one in blink is just using cc's TouchAction. We also
create a WebTouchAction enum under public/platform/ which directly uses the 
TouchAction in platform/graphics/. The reason of adding WebTouchAction is that 
there are methods in WebWidgetClient.h that is overridden by class under
content/.

This CL does pure refactor, it causes no behavior change at all.

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

[xidachen, 2017-05-08 00:11:04]

xidachen@chromium.org changed reviewers:
+ danakj@chromium.org, esprehn@chromium.org, pfeldman@chromium.org

[xidachen, 2017-05-08 00:11:05]

danakj@: cc/
pfeldman@: content/
esprehn@, pdr@: WebKit/

PTAL.

[xidachen, 2017-05-08 00:11:18]

Description was changed from

==========
WIP: Unify TouchAction classes

There are three TouchAction classes in chromium codebase and they look
and function exact the same. This CL unifies them into one. We put a
canonical one under cc/input/, and then the one in blink is placed under
platform/graphics, the one in blink is just using cc's TouchAction. We also
create a WebTouchAction enum under public/platform/ which directly uses the 
TouchAction in platform/graphics/. The reason of adding WebTouchAction is that 
there are methods in WebWidgetClient.h that is overridden by class under
content/.

This CL does pure refactor, it causes no behavior change at all.

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

to

==========
Unify TouchAction classes

There are three TouchAction classes in chromium codebase and they look
and function exact the same. This CL unifies them into one. We put a
canonical one under cc/input/, and then the one in blink is placed under
platform/graphics, the one in blink is just using cc's TouchAction. We also
create a WebTouchAction enum under public/platform/ which directly uses the 
TouchAction in platform/graphics/. The reason of adding WebTouchAction is that 
there are methods in WebWidgetClient.h that is overridden by class under
content/.

This CL does pure refactor, it causes no behavior change at all.

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

[xidachen, 2017-05-08 15:58:57]

sunxd@: FYI

[wkorman, 2017-05-08 19:03:01]

lgtm

https://codereview.chromium.org/2863693003/diff/160001/cc/input/touch_action.h
File cc/input/touch_action.h (right):

https://codereview.chromium.org/2863693003/diff/160001/cc/input/touch_action....
cc/input/touch_action.h:10: // The current touch action specifies what
accelerated browser operations
Could enhance comments here to add:

- this is intended to be the single canonical definition of the enum
- it's used elsewhere in both Blink and content since touch action logic spans
those subsystems

[xidachen, 2017-05-09 12:11:50]

xidachen@chromium.org changed reviewers:
+ jbroman@chromium.org
- esprehn@chromium.org

[xidachen, 2017-05-09 12:11:51]

-esprehn@
+jbroman@: Please take a look at core/ and platform/, thanks.

[danakj, 2017-05-09 14:29:46]

cc LGTM % wkorman

https://codereview.chromium.org/2863693003/diff/160001/cc/input/touch_action.h
File cc/input/touch_action.h (right):

https://codereview.chromium.org/2863693003/diff/160001/cc/input/touch_action....
cc/input/touch_action.h:13: const size_t kTouchActionBits = 6;
whitespace before enum defn

https://codereview.chromium.org/2863693003/diff/160001/cc/input/touch_action....
cc/input/touch_action.h:30: inline TouchAction operator|(TouchAction a,
TouchAction b) {
whitespace before/after functions

[jbroman, 2017-05-09 14:40:26]

cc seems slightly odd given there are apparently no uses in cc at all, but if
cc/OWNERS are okay with that, then I can live with it.

Blink changes seem pretty routine; lgtm aside from comments.

https://codereview.chromium.org/2863693003/diff/160001/cc/input/touch_action.h
File cc/input/touch_action.h (right):

https://codereview.chromium.org/2863693003/diff/160001/cc/input/touch_action....
cc/input/touch_action.h:14: enum TouchAction {
nit: You might consider enum class, so that you get names like:

TouchAction::kNone

rather than

TouchAction::kTouchActionNone

This may have some consequences (due to the stronger typing it adds), but the
overloaded operator| and operator& might mean that it's not a big deal.

https://codereview.chromium.org/2863693003/diff/160001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebTouchAction.h (right):

https://codereview.chromium.org/2863693003/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebTouchAction.h:19: #if BLINK_IMPLEMENTATION
Just unconditionally use the cc one. We already do this in a handful of other
public/platform headers. This looks very much like an ODR violation, and is
rather confusing.

An alias here is certainly OK. I might make the case that we might as well just
call it cc::TouchAction in WebWidgetClient.h, but so far we only do that in
public/platform/.

[danakj, 2017-05-09 14:56:06]

On Tue, May 9, 2017 at 10:40 AM, <jbroman@chromium.org> wrote:

> cc seems slightly odd given there are apparently no uses in cc at all, but
> if
> cc/OWNERS are okay with that, then I can live with it.
>

My own assumption was it would be used in the future in cc, but you're
right if not then that's odd. What's the story here?


>
> Blink changes seem pretty routine; lgtm aside from comments.
>
>
> https://codereview.chromium.org/2863693003/diff/160001/cc/
> input/touch_action.h
> File cc/input/touch_action.h (right):
>
> https://codereview.chromium.org/2863693003/diff/160001/cc/
> input/touch_action.h#newcode14
> cc/input/touch_action.h:14: enum TouchAction {
> nit: You might consider enum class, so that you get names like:
>
> TouchAction::kNone
>
> rather than
>
> TouchAction::kTouchActionNone
>
> This may have some consequences (due to the stronger typing it adds),
> but the overloaded operator| and operator& might mean that it's not a
> big deal.
>
> https://codereview.chromium.org/2863693003/diff/160001/
> third_party/WebKit/public/platform/WebTouchAction.h
> File third_party/WebKit/public/platform/WebTouchAction.h (right):
>
> https://codereview.chromium.org/2863693003/diff/160001/
> third_party/WebKit/public/platform/WebTouchAction.h#newcode19
> third_party/WebKit/public/platform/WebTouchAction.h:19: #if
> BLINK_IMPLEMENTATION
> Just unconditionally use the cc one. We already do this in a handful of
> other public/platform headers. This looks very much like an ODR
> violation, and is rather confusing.
>
> An alias here is certainly OK. I might make the case that we might as
> well just call it cc::TouchAction in WebWidgetClient.h, but so far we
> only do that in public/platform/.
>
> https://codereview.chromium.org/2863693003/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[xidachen, 2017-05-09 15:00:48]

Sorry that I forgot to mention the whole story. We are trying to pass the
TouchAction bits from blink to cc so that in the future cc can handle some of
the TouchActions. Given that TouchAction will be used in cc, our idea is to have
a canonical one under cc, and all others will be using the one under cc. Please
refer to my other CL here: https://codereview.chromium.org/2856153003/. That CL
is currently paused because I want to land this CL first.

danakj@: I'd also like your thoughts on change TouchAction::kTouchActionNone to
TouchAction::kNone.

> 
> My own assumption was it would be used in the future in cc, but you're
> right if not then that's odd. What's the story here?

[danakj, 2017-05-09 15:03:33]

On Tue, May 9, 2017 at 10:40 AM, <jbroman@chromium.org> wrote:

> cc seems slightly odd given there are apparently no uses in cc at all, but
> if
> cc/OWNERS are okay with that, then I can live with it.
>

My own assumption was it would be used in the future in cc, but you're
right if not then that's odd. What's the story here?


>
> Blink changes seem pretty routine; lgtm aside from comments.
>
>
> https://codereview.chromium.org/2863693003/diff/160001/cc/
> input/touch_action.h
> File cc/input/touch_action.h (right):
>
> https://codereview.chromium.org/2863693003/diff/160001/cc/
> input/touch_action.h#newcode14
> cc/input/touch_action.h:14: enum TouchAction {
> nit: You might consider enum class, so that you get names like:
>
> TouchAction::kNone
>
> rather than
>
> TouchAction::kTouchActionNone
>
> This may have some consequences (due to the stronger typing it adds),
> but the overloaded operator| and operator& might mean that it's not a
> big deal.
>
> https://codereview.chromium.org/2863693003/diff/160001/
> third_party/WebKit/public/platform/WebTouchAction.h
> File third_party/WebKit/public/platform/WebTouchAction.h (right):
>
> https://codereview.chromium.org/2863693003/diff/160001/
> third_party/WebKit/public/platform/WebTouchAction.h#newcode19
> third_party/WebKit/public/platform/WebTouchAction.h:19: #if
> BLINK_IMPLEMENTATION
> Just unconditionally use the cc one. We already do this in a handful of
> other public/platform headers. This looks very much like an ODR
> violation, and is rather confusing.
>
> An alias here is certainly OK. I might make the case that we might as
> well just call it cc::TouchAction in WebWidgetClient.h, but so far we
> only do that in public/platform/.
>
> https://codereview.chromium.org/2863693003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2017-05-09 15:08:22]

On Tue, May 9, 2017 at 11:00 AM, <xidachen@chromium.org> wrote:

> Sorry that I forgot to mention the whole story. We are trying to pass the
> TouchAction bits from blink to cc so that in the future cc can handle some
> of
> the TouchActions. Given that TouchAction will be used in cc, our idea is
> to have
> a canonical one under cc, and all others will be using the one under cc.
> Please
> refer to my other CL here: https://codereview.chromium.org/2856153003/.
> That CL
> is currently paused because I want to land this CL first.
>
> danakj@: I'd also like your thoughts on change
> TouchAction::kTouchActionNone to
> TouchAction::kNone.
>

TBH I was going to comment that enum doesn't seem like the right thing,
because enum is a set of values of which you can choose one, but that is
not what this is.

If I wrote this, maintaining that you want a type that you can perform
operations on, I would write it as

struct TouchAction {
  enum { kNone = ... };
  enum { kEtc = ... };

  operator|(const TouchAction& t) { value |= t.value; }
  ...

  int value;
};

It's using enum to avoid making storage for the constants, but they are not
treated like a set of values with a single enum. And it uses a struct to
provide the type instead, because in your CL the TouchAction type is an
enum but ends up holding values not inside the enum if things get or'd
together.

But to answer the original question, yes not having redundancy in the name
would be nice, I would go for that too.


>
>
> >
> > My own assumption was it would be used in the future in cc, but you're
> > right if not then that's odd. What's the story here?
>
>
> https://codereview.chromium.org/2863693003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2017-05-09 15:08:23]

On Tue, May 9, 2017 at 11:00 AM, <xidachen@chromium.org> wrote:

> Sorry that I forgot to mention the whole story. We are trying to pass the
> TouchAction bits from blink to cc so that in the future cc can handle some
> of
> the TouchActions. Given that TouchAction will be used in cc, our idea is
> to have
> a canonical one under cc, and all others will be using the one under cc.
> Please
> refer to my other CL here: https://codereview.chromium.org/2856153003/.
> That CL
> is currently paused because I want to land this CL first.
>
> danakj@: I'd also like your thoughts on change
> TouchAction::kTouchActionNone to
> TouchAction::kNone.
>

TBH I was going to comment that enum doesn't seem like the right thing,
because enum is a set of values of which you can choose one, but that is
not what this is.

If I wrote this, maintaining that you want a type that you can perform
operations on, I would write it as

struct TouchAction {
  enum { kNone = ... };
  enum { kEtc = ... };

  operator|(const TouchAction& t) { value |= t.value; }
  ...

  int value;
};

It's using enum to avoid making storage for the constants, but they are not
treated like a set of values with a single enum. And it uses a struct to
provide the type instead, because in your CL the TouchAction type is an
enum but ends up holding values not inside the enum if things get or'd
together.

But to answer the original question, yes not having redundancy in the name
would be nice, I would go for that too.


>
>
> >
> > My own assumption was it would be used in the future in cc, but you're
> > right if not then that's odd. What's the story here?
>
>
> https://codereview.chromium.org/2863693003/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[xidachen, 2017-05-09 15:37:19]

So besides changing TouchAction::kTouchActionNone to TouchAction::kNone, I
should probably change what the TouchAction looks like. What do you think about
this:

struct TouchAction {
  enum TouchActionBit : uint32_t {
    kNone = 0,
    kPanLeft = 1 << 0,
    kPanRight = 1 << 1,
    kPanX = kPanLeft | kPanRight,
    // ...
  };
  const uint32_t bits = 6;

  TouchAction(TouchActionBit);

  TouchAction operator|(TouchAction);
  TouchAction& operator|=(TouchAction);
  // ...  

  uint32_t value;
};

[danakj, 2017-05-09 15:52:12]

On Tue, May 9, 2017 at 11:37 AM, <xidachen@chromium.org> wrote:

> So besides changing TouchAction::kTouchActionNone to TouchAction::kNone, I
> should probably change what the TouchAction looks like. What do you think
> about
> this:
>
> struct TouchAction {
> enum TouchActionBit : uint32_t {
> kNone = 0,
> kPanLeft = 1 << 0,
> kPanRight = 1 << 1,
> kPanX = kPanLeft | kPanRight,
> // ...
> };
> const uint32_t bits = 6;
>
> TouchAction(TouchActionBit);
>
> TouchAction operator|(TouchAction);
> TouchAction& operator|=(TouchAction);
> // ...
>
> uint32_t value;
> };
>

The difference to me appears that you want to group all the bitflag
constants into a single enum, but I don't understand why you want to treat
the bitvalues as a single enum, nor why you'd pass the enum as the type to
the constructor instead of passing an int.


>
> https://codereview.chromium.org/2863693003/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[danakj, 2017-05-09 15:53:27]

On Tue, May 9, 2017 at 11:37 AM, <xidachen@chromium.org> wrote:

> So besides changing TouchAction::kTouchActionNone to TouchAction::kNone, I
> should probably change what the TouchAction looks like. What do you think
> about
> this:
>
> struct TouchAction {
> enum TouchActionBit : uint32_t {
> kNone = 0,
> kPanLeft = 1 << 0,
> kPanRight = 1 << 1,
> kPanX = kPanLeft | kPanRight,
> // ...
> };
> const uint32_t bits = 6;
>
> TouchAction(TouchActionBit);
>
> TouchAction operator|(TouchAction);
> TouchAction& operator|=(TouchAction);
> // ...
>
> uint32_t value;
> };
>

The difference to me appears that you want to group all the bitflag
constants into a single enum, but I don't understand why you want to treat
the bitvalues as a single enum, nor why you'd pass the enum as the type to
the constructor instead of passing an int.


>
> https://codereview.chromium.org/2863693003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[blink-reviews, 2017-05-09 16:06:20]

On Tue, May 9, 2017 at 11:45 AM <danakj@chromium.org> wrote:

> On Tue, May 9, 2017 at 11:37 AM, <xidachen@chromium.org> wrote:
>
>> So besides changing TouchAction::kTouchActionNone to TouchAction::kNone, I
>> should probably change what the TouchAction looks like. What do you think
>> about
>> this:
>>
>> struct TouchAction {
>> enum TouchActionBit : uint32_t {
>> kNone = 0,
>> kPanLeft = 1 << 0,
>> kPanRight = 1 << 1,
>> kPanX = kPanLeft | kPanRight,
>> // ...
>> };
>> const uint32_t bits = 6;
>>
>> TouchAction(TouchActionBit);
>>
>> TouchAction operator|(TouchAction);
>> TouchAction& operator|=(TouchAction);
>> // ...
>>
>> uint32_t value;
>> };
>>
>
> The difference to me appears that you want to group all the bitflag
> constants into a single enum, but I don't understand why you want to treat
> the bitvalues as a single enum,
>
At this moment, the kTouchActionBits is only used here
<https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/Sty...>
to indicate how many bits we need for that member. In my opinion, the
meaning of that value is different than the rest of the values like kPanX.


> nor why you'd pass the enum as the type to the constructor instead of
> passing an int.
>
I don't have a preference for this one. The only reason that to pass the
enum is that I thought it maybe more clear

>
>
>>
>> https://codereview.chromium.org/2863693003/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2017-05-09 16:06:21]

On Tue, May 9, 2017 at 11:45 AM <danakj@chromium.org> wrote:

> On Tue, May 9, 2017 at 11:37 AM, <xidachen@chromium.org> wrote:
>
>> So besides changing TouchAction::kTouchActionNone to TouchAction::kNone, I
>> should probably change what the TouchAction looks like. What do you think
>> about
>> this:
>>
>> struct TouchAction {
>> enum TouchActionBit : uint32_t {
>> kNone = 0,
>> kPanLeft = 1 << 0,
>> kPanRight = 1 << 1,
>> kPanX = kPanLeft | kPanRight,
>> // ...
>> };
>> const uint32_t bits = 6;
>>
>> TouchAction(TouchActionBit);
>>
>> TouchAction operator|(TouchAction);
>> TouchAction& operator|=(TouchAction);
>> // ...
>>
>> uint32_t value;
>> };
>>
>
> The difference to me appears that you want to group all the bitflag
> constants into a single enum, but I don't understand why you want to treat
> the bitvalues as a single enum,
>
At this moment, the kTouchActionBits is only used here
<https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/Sty...>
to indicate how many bits we need for that member. In my opinion, the
meaning of that value is different than the rest of the values like kPanX.


> nor why you'd pass the enum as the type to the constructor instead of
> passing an int.
>
I don't have a preference for this one. The only reason that to pass the
enum is that I thought it maybe more clear

>
>
>>
>> https://codereview.chromium.org/2863693003/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[wkorman, 2017-05-09 20:11:35]

I had a comment re: 'enum class' in draft but I removed it. I think the history
here is that the CSS values in Blink's ComputedStyleConstants.h all sort of
follow this pattern, something like, (1) figure out how many bits we need to
represent all possible states we need to be able to track per the w3c spec,
bit-packing, and (2) define it with an enum.

I thought sashab@ or suzyh@ had a comment around exploring reworking them to be
'enum class', but I couldn't find it. And then I decided that for a first step
with this change (to turn three re-defs of the exact same enum into one),
leaving it as-is was a sensible initial step.

So, I agree that an enum here seems strange, but I'd vote for exploring any
further rework as a separate subsequent patch.

jbroman@ as background we did seek feedback from esprehn@, were routed to
dglazkov@, and finally received some guidance from haraken@, re: where to place
the class and what the intent was re: platform/ directories. I will forward that
thread to you.

I had a related comment maybe on xida's earlier referenced patch, re: the
weirdness of the enum defining 6 bits of state, but actually only certain combos
ever exist, and that logic only seems to exist in the TouchActionFlagsToCSSValue
method:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/Compu...

[danakj, 2017-05-09 21:30:21]

On Tue, May 9, 2017 at 12:06 PM, Xida Chen <xidachen@google.com> wrote:

>
>
> On Tue, May 9, 2017 at 11:45 AM <danakj@chromium.org> wrote:
>
>> On Tue, May 9, 2017 at 11:37 AM, <xidachen@chromium.org> wrote:
>>
>>> So besides changing TouchAction::kTouchActionNone to TouchAction::kNone,
>>> I
>>> should probably change what the TouchAction looks like. What do you
>>> think about
>>> this:
>>>
>>> struct TouchAction {
>>> enum TouchActionBit : uint32_t {
>>> kNone = 0,
>>> kPanLeft = 1 << 0,
>>> kPanRight = 1 << 1,
>>> kPanX = kPanLeft | kPanRight,
>>> // ...
>>> };
>>> const uint32_t bits = 6;
>>>
>>> TouchAction(TouchActionBit);
>>>
>>> TouchAction operator|(TouchAction);
>>> TouchAction& operator|=(TouchAction);
>>> // ...
>>>
>>> uint32_t value;
>>> };
>>>
>>
>> The difference to me appears that you want to group all the bitflag
>> constants into a single enum, but I don't understand why you want to treat
>> the bitvalues as a single enum,
>>
> At this moment, the kTouchActionBits is only used here
>
<https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/Sty...>
> to indicate how many bits we need for that member. In my opinion, the
> meaning of that value is different than the rest of the values like kPanX.
>

When I say bitflags here, I am referring to every constant value defined in
TouchAction. My confusion is why you want them to be all part of a single
enum. For instance, you would not write a switch statement for that enum
with cases for each kPanLeft, kPanRight etc.


>
>
>> nor why you'd pass the enum as the type to the constructor instead of
>> passing an int.
>>
> I don't have a preference for this one. The only reason that to pass the
> enum is that I thought it maybe more clear
>

I think it is misleading. Enums are a collection single-and-disjoint
values. But you would be accepting a bitset of flags which have constant
names. An unsigned int or a std::bitset is a better representation of this
IMO.


>
>>
>>>
>>> https://codereview.chromium.org/2863693003/
>>>
>>
>>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[danakj, 2017-05-09 21:30:23]

On Tue, May 9, 2017 at 12:06 PM, Xida Chen <xidachen@google.com> wrote:

>
>
> On Tue, May 9, 2017 at 11:45 AM <danakj@chromium.org> wrote:
>
>> On Tue, May 9, 2017 at 11:37 AM, <xidachen@chromium.org> wrote:
>>
>>> So besides changing TouchAction::kTouchActionNone to TouchAction::kNone,
>>> I
>>> should probably change what the TouchAction looks like. What do you
>>> think about
>>> this:
>>>
>>> struct TouchAction {
>>> enum TouchActionBit : uint32_t {
>>> kNone = 0,
>>> kPanLeft = 1 << 0,
>>> kPanRight = 1 << 1,
>>> kPanX = kPanLeft | kPanRight,
>>> // ...
>>> };
>>> const uint32_t bits = 6;
>>>
>>> TouchAction(TouchActionBit);
>>>
>>> TouchAction operator|(TouchAction);
>>> TouchAction& operator|=(TouchAction);
>>> // ...
>>>
>>> uint32_t value;
>>> };
>>>
>>
>> The difference to me appears that you want to group all the bitflag
>> constants into a single enum, but I don't understand why you want to treat
>> the bitvalues as a single enum,
>>
> At this moment, the kTouchActionBits is only used here
>
<https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/Sty...>
> to indicate how many bits we need for that member. In my opinion, the
> meaning of that value is different than the rest of the values like kPanX.
>

When I say bitflags here, I am referring to every constant value defined in
TouchAction. My confusion is why you want them to be all part of a single
enum. For instance, you would not write a switch statement for that enum
with cases for each kPanLeft, kPanRight etc.


>
>
>> nor why you'd pass the enum as the type to the constructor instead of
>> passing an int.
>>
> I don't have a preference for this one. The only reason that to pass the
> enum is that I thought it maybe more clear
>

I think it is misleading. Enums are a collection single-and-disjoint
values. But you would be accepting a bitset of flags which have constant
names. An unsigned int or a std::bitset is a better representation of this
IMO.


>
>>
>>>
>>> https://codereview.chromium.org/2863693003/
>>>
>>
>>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[wkorman, 2017-05-09 22:19:22]

On 2017/05/09 21:30:23, danakj wrote:
> On Tue, May 9, 2017 at 12:06 PM, Xida Chen <mailto:xidachen@google.com> wrote:
> 
> >
> >
> > On Tue, May 9, 2017 at 11:45 AM <mailto:danakj@chromium.org> wrote:
> >
> >> On Tue, May 9, 2017 at 11:37 AM, <mailto:xidachen@chromium.org> wrote:
> >>
> >>> So besides changing TouchAction::kTouchActionNone to TouchAction::kNone,
> >>> I
> >>> should probably change what the TouchAction looks like. What do you
> >>> think about
> >>> this:
> >>>
> >>> struct TouchAction {
> >>> enum TouchActionBit : uint32_t {
> >>> kNone = 0,
> >>> kPanLeft = 1 << 0,
> >>> kPanRight = 1 << 1,
> >>> kPanX = kPanLeft | kPanRight,
> >>> // ...
> >>> };
> >>> const uint32_t bits = 6;
> >>>
> >>> TouchAction(TouchActionBit);
> >>>
> >>> TouchAction operator|(TouchAction);
> >>> TouchAction& operator|=(TouchAction);
> >>> // ...
> >>>
> >>> uint32_t value;
> >>> };
> >>>
> >>
> >> The difference to me appears that you want to group all the bitflag
> >> constants into a single enum, but I don't understand why you want to treat
> >> the bitvalues as a single enum,
> >>
> > At this moment, the kTouchActionBits is only used here
> >
>
<https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/style/Sty...>
> > to indicate how many bits we need for that member. In my opinion, the
> > meaning of that value is different than the rest of the values like kPanX.
> >
> 
> When I say bitflags here, I am referring to every constant value defined in
> TouchAction. My confusion is why you want them to be all part of a single
> enum. For instance, you would not write a switch statement for that enum
> with cases for each kPanLeft, kPanRight etc.
> 
> 
> >
> >
> >> nor why you'd pass the enum as the type to the constructor instead of
> >> passing an int.
> >>
> > I don't have a preference for this one. The only reason that to pass the
> > enum is that I thought it maybe more clear
> >
> 
> I think it is misleading. Enums are a collection single-and-disjoint
> values. But you would be accepting a bitset of flags which have constant
> names. An unsigned int or a std::bitset is a better representation of this
> IMO.

Yeah, I think this code came from Blink using enums as a convenience to define a
batch of related ints. I don't think Blink can use std::bitset yet. I also found
this enum def surprising, but this change is still a big improvement from seeing
the same confusing enum defined in three places.

I'd still pitch for keeping as is as interim step, and exploring rework to ints
or something else subsequently. Checking with sashab@ to hear what CSS/style is
thinking for similar cases probably makes sense to inform that.

[danakj, 2017-05-09 22:30:38]

On Tue, May 9, 2017 at 6:19 PM, <wkorman@chromium.org> wrote:

> On 2017/05/09 21:30:23, danakj wrote:
> > On Tue, May 9, 2017 at 12:06 PM, Xida Chen <mailto:xidachen@google.com>
> wrote:
> >
> > >
> > >
> > > On Tue, May 9, 2017 at 11:45 AM <mailto:danakj@chromium.org> wrote:
> > >
> > >> On Tue, May 9, 2017 at 11:37 AM, <mailto:xidachen@chromium.org>
> wrote:
> > >>
> > >>> So besides changing TouchAction::kTouchActionNone to
> TouchAction::kNone,
> > >>> I
> > >>> should probably change what the TouchAction looks like. What do you
> > >>> think about
> > >>> this:
> > >>>
> > >>> struct TouchAction {
> > >>> enum TouchActionBit : uint32_t {
> > >>> kNone = 0,
> > >>> kPanLeft = 1 << 0,
> > >>> kPanRight = 1 << 1,
> > >>> kPanX = kPanLeft | kPanRight,
> > >>> // ...
> > >>> };
> > >>> const uint32_t bits = 6;
> > >>>
> > >>> TouchAction(TouchActionBit);
> > >>>
> > >>> TouchAction operator|(TouchAction);
> > >>> TouchAction& operator|=(TouchAction);
> > >>> // ...
> > >>>
> > >>> uint32_t value;
> > >>> };
> > >>>
> > >>
> > >> The difference to me appears that you want to group all the bitflag
> > >> constants into a single enum, but I don't understand why you want to
> treat
> > >> the bitvalues as a single enum,
> > >>
> > > At this moment, the kTouchActionBits is only used here
> > >
> >
> <https://cs.chromium.org/chromium/src/third_party/
> WebKit/Source/core/style/StyleRareNonInheritedData.h?l=208&gt;
> > > to indicate how many bits we need for that member. In my opinion, the
> > > meaning of that value is different than the rest of the values like
> kPanX.
> > >
> >
> > When I say bitflags here, I am referring to every constant value defined
> in
> > TouchAction. My confusion is why you want them to be all part of a single
> > enum. For instance, you would not write a switch statement for that enum
> > with cases for each kPanLeft, kPanRight etc.
> >
> >
> > >
> > >
> > >> nor why you'd pass the enum as the type to the constructor instead of
> > >> passing an int.
> > >>
> > > I don't have a preference for this one. The only reason that to pass
> the
> > > enum is that I thought it maybe more clear
> > >
> >
> > I think it is misleading. Enums are a collection single-and-disjoint
> > values. But you would be accepting a bitset of flags which have constant
> > names. An unsigned int or a std::bitset is a better representation of
> this
> > IMO.
>
> Yeah, I think this code came from Blink using enums as a convenience to
> define a
> batch of related ints. I don't think Blink can use std::bitset yet. I also
> found
> this enum def surprising, but this change is still a big improvement from
> seeing
> the same confusing enum defined in three places.
>

Yep, I didn't really want to give a lot of friction which is why I didn't
bring it up in the first place, but since it happened I wanted to get it
all out :)


> I'd still pitch for keeping as is as interim step, and exploring rework to
> ints
> or something else subsequently.
>

I'll trust your judgement on where to go with it.


> Checking with sashab@ to hear what CSS/style is
> thinking for similar cases probably makes sense to inform that.
>
> https://codereview.chromium.org/2863693003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2017-05-09 22:35:51]

On Tue, May 9, 2017 at 6:19 PM, <wkorman@chromium.org> wrote:

> On 2017/05/09 21:30:23, danakj wrote:
> > On Tue, May 9, 2017 at 12:06 PM, Xida Chen <mailto:xidachen@google.com>
> wrote:
> >
> > >
> > >
> > > On Tue, May 9, 2017 at 11:45 AM <mailto:danakj@chromium.org> wrote:
> > >
> > >> On Tue, May 9, 2017 at 11:37 AM, <mailto:xidachen@chromium.org>
> wrote:
> > >>
> > >>> So besides changing TouchAction::kTouchActionNone to
> TouchAction::kNone,
> > >>> I
> > >>> should probably change what the TouchAction looks like. What do you
> > >>> think about
> > >>> this:
> > >>>
> > >>> struct TouchAction {
> > >>> enum TouchActionBit : uint32_t {
> > >>> kNone = 0,
> > >>> kPanLeft = 1 << 0,
> > >>> kPanRight = 1 << 1,
> > >>> kPanX = kPanLeft | kPanRight,
> > >>> // ...
> > >>> };
> > >>> const uint32_t bits = 6;
> > >>>
> > >>> TouchAction(TouchActionBit);
> > >>>
> > >>> TouchAction operator|(TouchAction);
> > >>> TouchAction& operator|=(TouchAction);
> > >>> // ...
> > >>>
> > >>> uint32_t value;
> > >>> };
> > >>>
> > >>
> > >> The difference to me appears that you want to group all the bitflag
> > >> constants into a single enum, but I don't understand why you want to
> treat
> > >> the bitvalues as a single enum,
> > >>
> > > At this moment, the kTouchActionBits is only used here
> > >
> >
> <https://cs.chromium.org/chromium/src/third_party/
> WebKit/Source/core/style/StyleRareNonInheritedData.h?l=208&gt;
> > > to indicate how many bits we need for that member. In my opinion, the
> > > meaning of that value is different than the rest of the values like
> kPanX.
> > >
> >
> > When I say bitflags here, I am referring to every constant value defined
> in
> > TouchAction. My confusion is why you want them to be all part of a single
> > enum. For instance, you would not write a switch statement for that enum
> > with cases for each kPanLeft, kPanRight etc.
> >
> >
> > >
> > >
> > >> nor why you'd pass the enum as the type to the constructor instead of
> > >> passing an int.
> > >>
> > > I don't have a preference for this one. The only reason that to pass
> the
> > > enum is that I thought it maybe more clear
> > >
> >
> > I think it is misleading. Enums are a collection single-and-disjoint
> > values. But you would be accepting a bitset of flags which have constant
> > names. An unsigned int or a std::bitset is a better representation of
> this
> > IMO.
>
> Yeah, I think this code came from Blink using enums as a convenience to
> define a
> batch of related ints. I don't think Blink can use std::bitset yet. I also
> found
> this enum def surprising, but this change is still a big improvement from
> seeing
> the same confusing enum defined in three places.
>

Yep, I didn't really want to give a lot of friction which is why I didn't
bring it up in the first place, but since it happened I wanted to get it
all out :)


> I'd still pitch for keeping as is as interim step, and exploring rework to
> ints
> or something else subsequently.
>

I'll trust your judgement on where to go with it.


> Checking with sashab@ to hear what CSS/style is
> thinking for similar cases probably makes sense to inform that.
>
> https://codereview.chromium.org/2863693003/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[xidachen, 2017-05-10 14:43:19]

Based on the comments, I only made some small changes to cc/input/touch_action.h
(adding more comments and some whitespace line).

pdr@, pfeldman@: gentle ping.

[xidachen, 2017-05-10 15:43:13]

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

[xidachen, 2017-05-10 15:43:15]

+tsepez@: content/common/. PTAL.

[Tom Sepez, 2017-05-10 16:31:16]

lgtm

[pdr., 2017-05-10 17:08:19]

On 2017/05/10 at 16:31:16, tsepez wrote:
> lgtm

Lgtm

[wkorman, 2017-05-10 17:56:56]

https://codereview.chromium.org/2863693003/diff/240001/cc/input/touch_action.h
File cc/input/touch_action.h (right):

https://codereview.chromium.org/2863693003/diff/240001/cc/input/touch_action....
cc/input/touch_action.h:15: // subsystems.
Suggest adding TODO, filing P3 bug and linking bug in TODO comment, cc: sashab@,
to explore rework of this per danakj@ and our discussion on this change.

[xidachen, 2017-05-10 18:31:06]

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

[xidachen, 2017-05-10 18:31:08]

+piman@: content/

PTAL, thank you.

[piman, 2017-05-10 18:33:52]

lgtm

[xidachen, 2017-05-10 18:39:07]

The CQ bit was checked by xidachen@chromium.org

[xidachen, 2017-05-10 18:39:08]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
jbroman@chromium.org, wkorman@chromium.org, tsepez@chromium.org,
pdr@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2863693003/#ps260001
(title: "link crbug for rework enum to enum class")

[commit-bot: I haz the power, 2017-05-10 18:40:06]

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-10 18:59:19]

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

[commit-bot: I haz the power, 2017-05-10 18:59:21]

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...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[xidachen, 2017-05-10 19:30:10]

The CQ bit was checked by xidachen@chromium.org

[xidachen, 2017-05-10 19:30:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbroman@chromium.org, piman@chromium.org, danakj@chromium.org,
tsepez@chromium.org, pdr@chromium.org, wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2863693003/#ps280001
(title: "add cstdlib")

[commit-bot: I haz the power, 2017-05-10 19:31: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, 2017-05-10 22:19:50]

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

[commit-bot: I haz the power, 2017-05-10 22:19:52]

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

[xidachen, 2017-05-11 01:30:22]

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

[commit-bot: I haz the power, 2017-05-11 01:33:04]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-11 06:41:25]

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

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

Dry run: This issue passed the CQ dry run.

[xidachen, 2017-05-11 11:25:22]

The CQ bit was checked by xidachen@chromium.org

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

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-11 11:34:30]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1494501922002990,
"parent_rev": "02b1d73344512180911bc5280438b840dd5105c8", "commit_rev":
"fa0199e7e18ee33c28356449b6ba48ab41c879e7"}

[commit-bot: I haz the power, 2017-05-11 11:34:42]

Description was changed from

==========
Unify TouchAction classes

There are three TouchAction classes in chromium codebase and they look
and function exact the same. This CL unifies them into one. We put a
canonical one under cc/input/, and then the one in blink is placed under
platform/graphics, the one in blink is just using cc's TouchAction. We also
create a WebTouchAction enum under public/platform/ which directly uses the 
TouchAction in platform/graphics/. The reason of adding WebTouchAction is that 
there are methods in WebWidgetClient.h that is overridden by class under
content/.

This CL does pure refactor, it causes no behavior change at all.

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

to

==========
Unify TouchAction classes

There are three TouchAction classes in chromium codebase and they look
and function exact the same. This CL unifies them into one. We put a
canonical one under cc/input/, and then the one in blink is placed under
platform/graphics, the one in blink is just using cc's TouchAction. We also
create a WebTouchAction enum under public/platform/ which directly uses the 
TouchAction in platform/graphics/. The reason of adding WebTouchAction is that 
there are methods in WebWidgetClient.h that is overridden by class under
content/.

This CL does pure refactor, it causes no behavior change at all.

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

Review-Url: https://codereview.chromium.org/2863693003
Cr-Commit-Position: refs/heads/master@{#470919}
Committed:
https://chromium.googlesource.com/chromium/src/+/fa0199e7e18ee33c28356449b6ba...
==========

[commit-bot: I haz the power, 2017-05-11 11:34:45]

Committed patchset #15 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/fa0199e7e18ee33c28356449b6ba...