Refactoring button field and its type

Refactoring button field and its type

- Remove m_button field from PlatformMouseEvent as
    there was one in WebPointerProperties.
- Remove duplicate PlatformMouseEvent::MouseButton
    and WebScrollbarBehavior::Button enums and use
    WebPointerProperties::Button enum instead.
- Making WebPointerProperties::Button an enum class.

- Also cleanup related target for pointerevents. The related target for all pointerevents (similar to mouseevent) should be null except for boundary events. So relatedTarget can be removed from some of the functions as they don't send boundary events.

BUG=635670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

TBR=pfeldman@chromium.org

Committed: https://crrev.com/3acb734ff34a77e76228425d06952317c10e252b
Cr-Commit-Position: refs/heads/master@{#412552}

[Navid Zolghadr, 2016-08-08 21:52:05]

nzolghadr@chromium.org changed reviewers:
+ dtapuska@chromium.org, mustaq@chromium.org

[Navid Zolghadr, 2016-08-08 21:55:27]

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

[commit-bot: I haz the power, 2016-08-08 21:56:08]

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-08-08 22:07:16]

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

[commit-bot: I haz the power, 2016-08-08 22:07:17]

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...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[mustaq, 2016-08-09 15:38:05]

Thanks for starting the cleanup.

This is closely related: may we please switch WebPointerProperties::button away
from a classical enum to a typed one? I mean an "enum class" as in
WebPointerProperties::PointerType? Implicit type-casting between button and
'int'
should be as few as possible.

[Navid Zolghadr, 2016-08-10 15:45:58]

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

[commit-bot: I haz the power, 2016-08-10 15:46:11]

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

[Navid Zolghadr, 2016-08-10 15:46:37]

The CQ bit was unchecked by nzolghadr@chromium.org

[Navid Zolghadr, 2016-08-10 15:57:28]

Description was changed from

==========
Remove button field from PlatformMouseEvent

Remove button field and MouseButton enum
from PlatformMouseEvent.
Also cleanup related target for pointerevents.

BUG=635670
==========

to

==========
Remove button field from PlatformMouseEvent

Remove button field and MouseButton enum
from PlatformMouseEvent.
Also cleanup related target for pointerevents.

BUG=635670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Navid Zolghadr, 2016-08-10 15:57:31]

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

[commit-bot: I haz the power, 2016-08-10 15:57: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-08-10 16:02:42]

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

[commit-bot: I haz the power, 2016-08-10 16:02:43]

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

[Navid Zolghadr, 2016-08-10 16:04:37]

ptal.

I changed that enum to enum class. I had to change the None name to NoButton as
"None" was a keyword that was #defined somewhere in x11 on Linux files.

[mustaq, 2016-08-10 16:24:47]

On 2016/08/10 16:04:37, Navid Zolghadr wrote:
> ptal.
> 
> I changed that enum to enum class. I had to change the None name to NoButton
as
> "None" was a keyword that was #defined somewhere in x11 on Linux files.

LGTM modulo remaining ref fixes. Thanks again for doing it for sake of code
health.

[Navid Zolghadr, 2016-08-10 16:27:40]

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

[commit-bot: I haz the power, 2016-08-10 16:28:07]

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-08-10 16:46:21]

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

[commit-bot: I haz the power, 2016-08-10 16:46:22]

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...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Navid Zolghadr, 2016-08-11 12:56:04]

nzolghadr@chromium.org changed reviewers:
+ pfeldman@chromium.org

[Navid Zolghadr, 2016-08-11 12:56:04]

pfeldman@, I had a quick question regarding this change of mine. There are quite
a few places that I needed to static_cast<short/int> the enum class I used here
to sometimes compare (== or !=), sometimes assign to another variable, and
sometimes passing to another function.

I was wondering if it is okay to leave all the casts there or do you think it is
worth to add the == and != operators with short/int into WebPointerProperties.h?
This solution will still leave behind some of those static_casts for other use
cases. What do you think is the better approach here?

[Navid Zolghadr, 2016-08-11 14:29:04]

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

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

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-08-11 14:32:32]

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

[commit-bot: I haz the power, 2016-08-11 14:32:33]

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

[Navid Zolghadr, 2016-08-11 14:51:27]

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

[commit-bot: I haz the power, 2016-08-11 14:51: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-08-11 15:06:29]

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

[commit-bot: I haz the power, 2016-08-11 15:06:30]

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

[Navid Zolghadr, 2016-08-11 18:31:04]

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

[commit-bot: I haz the power, 2016-08-11 18:32:12]

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

[Navid Zolghadr, 2016-08-11 18:51:59]

Description was changed from

==========
Remove button field from PlatformMouseEvent

Remove button field and MouseButton enum
from PlatformMouseEvent.
Also cleanup related target for pointerevents.

BUG=635670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Refactoring button field and its type

- Remove m_button field from PlatformMouseEvent as 
    there was one in WebPointerProperties.
- Remove PlatformMouseEvent::MouseButton and
    WebScrollbarBehavior::Button enums and use 
    WebPointerEvent::Button enum instead.
- Making WebPointerEvent::Button an enum class.

- Also cleanup related target for pointerevents.

BUG=635670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Navid Zolghadr, 2016-08-11 18:55:25]

Description was changed from

==========
Refactoring button field and its type

- Remove m_button field from PlatformMouseEvent as 
    there was one in WebPointerProperties.
- Remove PlatformMouseEvent::MouseButton and
    WebScrollbarBehavior::Button enums and use 
    WebPointerEvent::Button enum instead.
- Making WebPointerEvent::Button an enum class.

- Also cleanup related target for pointerevents.

BUG=635670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Refactoring button field and its type

- Remove m_button field from PlatformMouseEvent as 
    there was one in WebPointerProperties.
- Remove duplicate PlatformMouseEvent::MouseButton
    and WebScrollbarBehavior::Button enums and use 
    WebPointerProperties::Button enum instead.
- Making WebPointerProperties::Button an enum class.

- Also cleanup related target for pointerevents.

BUG=635670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-08-11 19:00:58]

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

[commit-bot: I haz the power, 2016-08-11 19:00:59]

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

[Navid Zolghadr, 2016-08-11 20:03:08]

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

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

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

[commit-bot: I haz the power, 2016-08-11 20:30:53]

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

[commit-bot: I haz the power, 2016-08-11 20:30:55]

Dry run: 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_...)

[Navid Zolghadr, 2016-08-11 20:52:43]

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

[commit-bot: I haz the power, 2016-08-11 20:53:22]

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-08-11 21:16:32]

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

[commit-bot: I haz the power, 2016-08-11 21:16:34]

Dry run: 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_...)

[Navid Zolghadr, 2016-08-11 22:58:19]

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

[commit-bot: I haz the power, 2016-08-11 22:59:31]

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-08-11 23:38:39]

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

[commit-bot: I haz the power, 2016-08-11 23:38:40]

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

[Navid Zolghadr, 2016-08-12 05:37:48]

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

[commit-bot: I haz the power, 2016-08-12 05:38:03]

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-08-12 07:13:34]

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

[commit-bot: I haz the power, 2016-08-12 07:13:35]

Dry run: This issue passed the CQ dry run.

[Navid Zolghadr, 2016-08-12 14:36:34]

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

[mustaq, 2016-08-12 15:22:35]

Awesome! The typed |button| field now makes our code safe from (unsafe) enum-int
type-casting.

Thanks again for taking care of the long ref fixes.

Still LGTM.

[bokan, 2016-08-12 16:25:16]

Source/core and Source/web lgtm

https://codereview.chromium.org/2227563003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (left):

https://codereview.chromium.org/2227563003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:148:
pointerEventInit.setRelatedTarget(relatedTarget);
Why is this no longer needed? Please elaborate in the CL description.

https://codereview.chromium.org/2227563003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/2227563003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:55: unsigned short
buttonToButtons(WebPointerProperties::Button button)
There's got to be a better name for this...:P

Perhaps buttonToButtonsBitfield?

https://codereview.chromium.org/2227563003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:146: } else { //
Only when pointerEventName == EventTypeNames::pointermove
Perhaps turn this comment into a DCHECK?

[Navid Zolghadr, 2016-08-12 16:42:54]

Description was changed from

==========
Refactoring button field and its type

- Remove m_button field from PlatformMouseEvent as 
    there was one in WebPointerProperties.
- Remove duplicate PlatformMouseEvent::MouseButton
    and WebScrollbarBehavior::Button enums and use 
    WebPointerProperties::Button enum instead.
- Making WebPointerProperties::Button an enum class.

- Also cleanup related target for pointerevents.

BUG=635670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Refactoring button field and its type

- Remove m_button field from PlatformMouseEvent as 
    there was one in WebPointerProperties.
- Remove duplicate PlatformMouseEvent::MouseButton
    and WebScrollbarBehavior::Button enums and use 
    WebPointerProperties::Button enum instead.
- Making WebPointerProperties::Button an enum class.

- Also cleanup related target for pointerevents. The related target for all
pointerevents (similar to mouseevent) should be null except for boundary events.
So relatedTarget can be removed from some of the functions as they don't send
boundary events.

BUG=635670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Navid Zolghadr, 2016-08-12 16:52:55]

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

[commit-bot: I haz the power, 2016-08-12 16:53:12]

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

[Navid Zolghadr, 2016-08-12 17:14:37]

https://codereview.chromium.org/2227563003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (left):

https://codereview.chromium.org/2227563003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:148:
pointerEventInit.setRelatedTarget(relatedTarget);
On 2016/08/12 16:25:16, bokan wrote:
> Why is this no longer needed? Please elaborate in the CL description.

The related target for all pointerevents (similar to mouseevent) should be null
except for boundary events. So relatedTarget can be removed from some of the
functions as they don't generate boundary events.
I added this to the description as well.

https://codereview.chromium.org/2227563003/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/2227563003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:55: unsigned short
buttonToButtons(WebPointerProperties::Button button)
On 2016/08/12 16:25:16, bokan wrote:
> There's got to be a better name for this...:P
> 
> Perhaps buttonToButtonsBitfield?

Sure :)

https://codereview.chromium.org/2227563003/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:146: } else { //
Only when pointerEventName == EventTypeNames::pointermove
On 2016/08/12 16:25:16, bokan wrote:
> Perhaps turn this comment into a DCHECK?

Done.

[commit-bot: I haz the power, 2016-08-12 18:51:26]

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

[commit-bot: I haz the power, 2016-08-12 18:51:27]

Dry run: This issue passed the CQ dry run.

[Navid Zolghadr, 2016-08-15 15:02:17]

nzolghadr@chromium.org changed reviewers:
+ pdr@chromium.org

[Navid Zolghadr, 2016-08-15 15:02:18]

pdr@, I had a quick question regarding this change of mine. There are quite
a few places that I needed to static_cast<short/int> the enum class I used here
to sometimes compare (== or !=), sometimes assign to another variable, and
sometimes passing to setters and other functions.

I was wondering if it is okay to leave all the casts there or do you think it is
worth to add the == and != operators with short/int into WebPointerProperties.h?
This solution will still leave behind some of those static_casts for other use
cases. What do you think is the better approach here?

[pdr., 2016-08-15 17:21:57]

pdr@chromium.org changed reviewers:
+ danakj@chromium.org

[pdr., 2016-08-15 17:34:04]

On 2016/08/15 at 15:02:18, nzolghadr wrote:
> pdr@, I had a quick question regarding this change of mine. There are quite
> a few places that I needed to static_cast<short/int> the enum class I used
here
> to sometimes compare (== or !=), sometimes assign to another variable, and
> sometimes passing to setters and other functions.
> 
> I was wondering if it is okay to leave all the casts there or do you think it
is
> worth to add the == and != operators with short/int into
WebPointerProperties.h?
> This solution will still leave behind some of those static_casts for other use
> cases. What do you think is the better approach here?

This is a tough one :/ The existing code is brittle and depends on various enums
happening to align. I think the code you've written is reasonable to land.

Instead of operator overloading, I wonder if you could introduce "short
WebPointerProperties::Button::asMouseButton()" to silo the casts behind a couple
functions?

Just in case you didn't see it, there was a recent chromium-dev thread about
enums
(https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/enum$2...)
and a case was made for using enum class for this situation (as you did) but
there wasn't much more beyond that.

+cc danakj in case they have an opinion.

[danakj, 2016-08-15 18:46:31]

On Mon, Aug 15, 2016 at 10:34 AM, <pdr@chromium.org> wrote:

> On 2016/08/15 at 15:02:18, nzolghadr wrote:
> > pdr@, I had a quick question regarding this change of mine. There are
> quite
> > a few places that I needed to static_cast<short/int> the enum class I
> used
> here
> > to sometimes compare (== or !=), sometimes assign to another variable,
> and
> > sometimes passing to setters and other functions.
> >
> > I was wondering if it is okay to leave all the casts there or do you
> think it
> is
> > worth to add the == and != operators with short/int into
> WebPointerProperties.h?
> > This solution will still leave behind some of those static_casts for
> other use
> > cases. What do you think is the better approach here?
>
> This is a tough one :/ The existing code is brittle and depends on various
> enums
> happening to align. I think the code you've written is reasonable to land.
>
> Instead of operator overloading, I wonder if you could introduce "short
> WebPointerProperties::Button::asMouseButton()" to silo the casts behind a
> couple
> functions?
>
> Just in case you didn't see it, there was a recent chromium-dev thread
> about
> enums
> (https://groups.google.com/a/chromium.org/forum/#!searchin/c
> hromium-dev/enum$20class/chromium-dev/Q5WmkAImanc/jOh6Whj3DY8J)
> and a case was made for using enum class for this situation (as you did)
> but
> there wasn't much more beyond that.
>
> +cc danakj in case they have an opinion.
>

I ran into this a bunch while changing enums in ComputedStyleConstants.h,
as I was trying to make them into enum class, but they are used as integers
all over the place for arrays and such. In the end I went with leaving them
as enums and prefixing the enum name into the value names to give them
scope again. Like:

enum OverflowAlignment {
    OverflowAlignmentDefault,
    OverflowAlignmentUnsafe,
    OverflowAlignmentSafe
};
So if you also need to use the enum values as integers, I'd personally
recommend doing that.

Other things we considered were static casts, or using an operator+() on
the enum class to convert to an int, and helper methods. If it's just in a
few instances tho, static_cast seems totally reasonable too!

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

-- 
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, 2016-08-15 18:52:44]

On Mon, Aug 15, 2016 at 10:34 AM, <pdr@chromium.org> wrote:

> On 2016/08/15 at 15:02:18, nzolghadr wrote:
> > pdr@, I had a quick question regarding this change of mine. There are
> quite
> > a few places that I needed to static_cast<short/int> the enum class I
> used
> here
> > to sometimes compare (== or !=), sometimes assign to another variable,
> and
> > sometimes passing to setters and other functions.
> >
> > I was wondering if it is okay to leave all the casts there or do you
> think it
> is
> > worth to add the == and != operators with short/int into
> WebPointerProperties.h?
> > This solution will still leave behind some of those static_casts for
> other use
> > cases. What do you think is the better approach here?
>
> This is a tough one :/ The existing code is brittle and depends on various
> enums
> happening to align. I think the code you've written is reasonable to land.
>
> Instead of operator overloading, I wonder if you could introduce "short
> WebPointerProperties::Button::asMouseButton()" to silo the casts behind a
> couple
> functions?
>
> Just in case you didn't see it, there was a recent chromium-dev thread
> about
> enums
> (https://groups.google.com/a/chromium.org/forum/#!searchin/c
> hromium-dev/enum$20class/chromium-dev/Q5WmkAImanc/jOh6Whj3DY8J)
> and a case was made for using enum class for this situation (as you did)
> but
> there wasn't much more beyond that.
>
> +cc danakj in case they have an opinion.
>

I ran into this a bunch while changing enums in ComputedStyleConstants.h,
as I was trying to make them into enum class, but they are used as integers
all over the place for arrays and such. In the end I went with leaving them
as enums and prefixing the enum name into the value names to give them
scope again. Like:

enum OverflowAlignment {
    OverflowAlignmentDefault,
    OverflowAlignmentUnsafe,
    OverflowAlignmentSafe
};
So if you also need to use the enum values as integers, I'd personally
recommend doing that.

Other things we considered were static casts, or using an operator+() on
the enum class to convert to an int, and helper methods. If it's just in a
few instances tho, static_cast seems totally reasonable too!

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

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

[Navid Zolghadr, 2016-08-16 20:45:55]

nzolghadr@chromium.org changed reviewers:
+ sky@chromium.org

[Navid Zolghadr, 2016-08-16 20:45:57]

sky@chromium.org: Please review changes in 

chrome/browser/*
mojo/*

[Navid Zolghadr, 2016-08-16 20:48:58]

nzolghadr@chromium.org changed reviewers:
+ thestig@chromium.org

[Navid Zolghadr, 2016-08-16 20:48:59]

thestig@chromium.org: Please review changes in 

components/printing/*

[Lei Zhang, 2016-08-16 20:51:25]

On 2016/08/16 20:48:59, Navid Zolghadr wrote:
> mailto:thestig@chromium.org: Please review changes in 
> 
> components/printing/*

lgtm

I'm also approving chrome/ so sky@ only has to review mojo/.

[Navid Zolghadr, 2016-08-16 20:54:48]

danakj@ I thought I replied while back but I guess I didn't press send.
I looked around and it is about 10 static_cast<short> and a few cast to int
exist for this. But I expect this number to go down as I do further refactoring.

pdr@, note that the function (e.g.
WebPointerProperties::Button::asMouseButton()) by itself seems to be longer that
just a static_cast and doesn't prevent people to static_cast anyway. So I
believe even if I change all current occurrences people might easily miss that
cast function and do their own cast. Is the number of occurrences low enough to
keep the static_casts? If so does the platform/* look good?

[pdr., 2016-08-16 21:23:14]

On 2016/08/16 at 20:54:48, nzolghadr wrote:
> danakj@ I thought I replied while back but I guess I didn't press send.
> I looked around and it is about 10 static_cast<short> and a few cast to int
exist for this. But I expect this number to go down as I do further refactoring.
> 
> pdr@, note that the function (e.g.
WebPointerProperties::Button::asMouseButton()) by itself seems to be longer that
just a static_cast and doesn't prevent people to static_cast anyway. So I
believe even if I change all current occurrences people might easily miss that
cast function and do their own cast. Is the number of occurrences low enough to
keep the static_casts? If so does the platform/* look good?

LGTM for platform as-is

WebPointerProperties::Button::asMouseButton() was just a suggestion and the
benefit is really small (if any). Thanks for cleaning up this gross code!

[sky, 2016-08-16 22:03:04]

Button::NoButton is a bit redundant. Personally I would have went with
Button::None, but I'm not an owner of that part of the patch.

One question below:

https://codereview.chromium.org/2227563003/diff/200001/mojo/converters/blink/...
File mojo/converters/blink/blink_input_events_type_converters.cc (right):

https://codereview.chromium.org/2227563003/diff/200001/mojo/converters/blink/...
mojo/converters/blink/blink_input_events_type_converters.cc:80:
web_event->button = blink::WebMouseEvent::Button::Middle;
How is WebMouseEvent suppose to handle multiple buttons being down?

[dtapuska, 2016-08-16 22:51:52]

On 2016/08/16 22:03:04, sky wrote:
> Button::NoButton is a bit redundant. Personally I would have went with
> Button::None, but I'm not an owner of that part of the patch.
> 
> One question below:
> 
>
https://codereview.chromium.org/2227563003/diff/200001/mojo/converters/blink/...
> File mojo/converters/blink/blink_input_events_type_converters.cc (right):
> 
>
https://codereview.chromium.org/2227563003/diff/200001/mojo/converters/blink/...
> mojo/converters/blink/blink_input_events_type_converters.cc:80:
> web_event->button = blink::WebMouseEvent::Button::Middle;
> How is WebMouseEvent suppose to handle multiple buttons being down?

None as a keyword is a problem on x11 cause of a #define. I've run into that in
the past.

[sky, 2016-08-16 22:56:09]

Ugh! See my earlier question though.

On Tue, Aug 16, 2016 at 3:51 PM,  <dtapuska@chromium.org> wrote:
> On 2016/08/16 22:03:04, sky wrote:
>> Button::NoButton is a bit redundant. Personally I would have went with
>> Button::None, but I'm not an owner of that part of the patch.
>>
>> One question below:
>>
>>
>
https://codereview.chromium.org/2227563003/diff/200001/mojo/converters/blink/...
>> File mojo/converters/blink/blink_input_events_type_converters.cc (right):
>>
>>
>
https://codereview.chromium.org/2227563003/diff/200001/mojo/converters/blink/...
>> mojo/converters/blink/blink_input_events_type_converters.cc:80:
>> web_event->button = blink::WebMouseEvent::Button::Middle;
>> How is WebMouseEvent suppose to handle multiple buttons being down?
>
> None as a keyword is a problem on x11 cause of a #define. I've run into that
> in
> the past.
>
> https://codereview.chromium.org/2227563003/

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

[sky, 2016-08-16 22:56:09]

Ugh! See my earlier question though.

On Tue, Aug 16, 2016 at 3:51 PM,  <dtapuska@chromium.org> wrote:
> On 2016/08/16 22:03:04, sky wrote:
>> Button::NoButton is a bit redundant. Personally I would have went with
>> Button::None, but I'm not an owner of that part of the patch.
>>
>> One question below:
>>
>>
>
https://codereview.chromium.org/2227563003/diff/200001/mojo/converters/blink/...
>> File mojo/converters/blink/blink_input_events_type_converters.cc (right):
>>
>>
>
https://codereview.chromium.org/2227563003/diff/200001/mojo/converters/blink/...
>> mojo/converters/blink/blink_input_events_type_converters.cc:80:
>> web_event->button = blink::WebMouseEvent::Button::Middle;
>> How is WebMouseEvent suppose to handle multiple buttons being down?
>
> None as a keyword is a problem on x11 cause of a #define. I've run into that
> in
> the past.
>
> https://codereview.chromium.org/2227563003/

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

[Navid Zolghadr, 2016-08-16 22:56:53]

Regarding the Button::None, as Dave mentioned it is #defined in X11.

https://codereview.chromium.org/2227563003/diff/200001/mojo/converters/blink/...
File mojo/converters/blink/blink_input_events_type_converters.cc (right):

https://codereview.chromium.org/2227563003/diff/200001/mojo/converters/blink/...
mojo/converters/blink/blink_input_events_type_converters.cc:80:
web_event->button = blink::WebMouseEvent::Button::Middle;
On 2016/08/16 22:03:04, sky wrote:
> How is WebMouseEvent suppose to handle multiple buttons being down?

There is a buttons field in WebPointerProperties which shows which buttons are
pressed. The button field shows what button caused the event particularly for up
and down.

[sky, 2016-08-16 22:59:42]

LGTM - thanks for the clarification. I'll file a bug that this code is wrong (I
believe).

[Navid Zolghadr, 2016-08-17 00:49:09]

The CQ bit was checked by nzolghadr@chromium.org

[Navid Zolghadr, 2016-08-17 00:49:10]

The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org,
bokan@chromium.org
Link to the patchset: https://codereview.chromium.org/2227563003/#ps200001
(title: "Better names/description")

[commit-bot: I haz the power, 2016-08-17 00:49:39]

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-08-17 00:52:53]

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

[commit-bot: I haz the power, 2016-08-17 00:52:56]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Navid Zolghadr, 2016-08-17 14:29:06]

The CQ bit was checked by nzolghadr@chromium.org

[Navid Zolghadr, 2016-08-17 14:29:07]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
thestig@chromium.org, bokan@chromium.org, pdr@chromium.org, mustaq@chromium.org
Link to the patchset: https://codereview.chromium.org/2227563003/#ps220001
(title: "Rebasing")

[commit-bot: I haz the power, 2016-08-17 14:29:20]

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-08-17 14:37:51]

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

[commit-bot: I haz the power, 2016-08-17 14:37:54]

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

[Navid Zolghadr, 2016-08-17 14:40:35]

Description was changed from

==========
Refactoring button field and its type

- Remove m_button field from PlatformMouseEvent as 
    there was one in WebPointerProperties.
- Remove duplicate PlatformMouseEvent::MouseButton
    and WebScrollbarBehavior::Button enums and use 
    WebPointerProperties::Button enum instead.
- Making WebPointerProperties::Button an enum class.

- Also cleanup related target for pointerevents. The related target for all
pointerevents (similar to mouseevent) should be null except for boundary events.
So relatedTarget can be removed from some of the functions as they don't send
boundary events.

BUG=635670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Refactoring button field and its type

- Remove m_button field from PlatformMouseEvent as 
    there was one in WebPointerProperties.
- Remove duplicate PlatformMouseEvent::MouseButton
    and WebScrollbarBehavior::Button enums and use 
    WebPointerProperties::Button enum instead.
- Making WebPointerProperties::Button an enum class.

- Also cleanup related target for pointerevents. The related target for all
pointerevents (similar to mouseevent) should be null except for boundary events.
So relatedTarget can be removed from some of the functions as they don't send
boundary events.

BUG=635670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

TBR=pfeldman@chromium.org
==========

[Navid Zolghadr, 2016-08-17 14:40:55]

The CQ bit was checked by nzolghadr@chromium.org

[commit-bot: I haz the power, 2016-08-17 14:41:07]

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

[Navid Zolghadr, 2016-08-17 14:47:33]

The CQ bit was unchecked by nzolghadr@chromium.org

[Navid Zolghadr, 2016-08-17 14:54:31]

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

[commit-bot: I haz the power, 2016-08-17 14:54:47]

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

[Navid Zolghadr, 2016-08-17 15:31:42]

The CQ bit was unchecked by nzolghadr@chromium.org

[Navid Zolghadr, 2016-08-17 15:31:52]

The CQ bit was checked by nzolghadr@chromium.org

[Navid Zolghadr, 2016-08-17 15:31:52]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
thestig@chromium.org, bokan@chromium.org, pdr@chromium.org, mustaq@chromium.org
Link to the patchset: https://codereview.chromium.org/2227563003/#ps240001
(title: "Fix new instances")

[commit-bot: I haz the power, 2016-08-17 15:32:05]

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-08-17 16:25:59]

Description was changed from

==========
Refactoring button field and its type

- Remove m_button field from PlatformMouseEvent as 
    there was one in WebPointerProperties.
- Remove duplicate PlatformMouseEvent::MouseButton
    and WebScrollbarBehavior::Button enums and use 
    WebPointerProperties::Button enum instead.
- Making WebPointerProperties::Button an enum class.

- Also cleanup related target for pointerevents. The related target for all
pointerevents (similar to mouseevent) should be null except for boundary events.
So relatedTarget can be removed from some of the functions as they don't send
boundary events.

BUG=635670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

TBR=pfeldman@chromium.org
==========

to

==========
Refactoring button field and its type

- Remove m_button field from PlatformMouseEvent as 
    there was one in WebPointerProperties.
- Remove duplicate PlatformMouseEvent::MouseButton
    and WebScrollbarBehavior::Button enums and use 
    WebPointerProperties::Button enum instead.
- Making WebPointerProperties::Button an enum class.

- Also cleanup related target for pointerevents. The related target for all
pointerevents (similar to mouseevent) should be null except for boundary events.
So relatedTarget can be removed from some of the functions as they don't send
boundary events.

BUG=635670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

TBR=pfeldman@chromium.org
==========

[commit-bot: I haz the power, 2016-08-17 16:26:02]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-08-17 16:27:11]

Description was changed from

==========
Refactoring button field and its type

- Remove m_button field from PlatformMouseEvent as 
    there was one in WebPointerProperties.
- Remove duplicate PlatformMouseEvent::MouseButton
    and WebScrollbarBehavior::Button enums and use 
    WebPointerProperties::Button enum instead.
- Making WebPointerProperties::Button an enum class.

- Also cleanup related target for pointerevents. The related target for all
pointerevents (similar to mouseevent) should be null except for boundary events.
So relatedTarget can be removed from some of the functions as they don't send
boundary events.

BUG=635670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

TBR=pfeldman@chromium.org
==========

to

==========
Refactoring button field and its type

- Remove m_button field from PlatformMouseEvent as
    there was one in WebPointerProperties.
- Remove duplicate PlatformMouseEvent::MouseButton
    and WebScrollbarBehavior::Button enums and use
    WebPointerProperties::Button enum instead.
- Making WebPointerProperties::Button an enum class.

- Also cleanup related target for pointerevents. The related target for all
pointerevents (similar to mouseevent) should be null except for boundary events.
So relatedTarget can be removed from some of the functions as they don't send
boundary events.

BUG=635670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

TBR=pfeldman@chromium.org

Committed: https://crrev.com/3acb734ff34a77e76228425d06952317c10e252b
Cr-Commit-Position: refs/heads/master@{#412552}
==========

[commit-bot: I haz the power, 2016-08-17 16:27:14]

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

[megjablon, 2016-08-17 19:20:33]

A revert of this CL (patchset #13 id:240001) has been created in
https://codereview.chromium.org/2245063006/ by megjablon@chromium.org.

The reason for reverting is: Webkit tests failing on WebKit Linux MSAN

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20MSAN/b...

webkit_tests webkit_tests

unexpected_failures:
imported/wpt/pointerevents/pointerevent_pointertype_touch-manual.html
imported/wpt/pointerevents/pointerevent_touch-action-inherit_child-auto-child-none_touch-manual.html
imported/wpt/pointerevents/pointerevent_touch-action-auto-css_touch-manual.html
imported/wpt/pointerevents/pointerevent_touch-action-pan-x-pan-y-pan-y_touch-manual.html
imported/wpt/pointerevents/pointerevent_touch-action-none-css_touch-manual.html
imported/wpt/pointerevents/pointerevent_change-touch-action-onpointerdown_touch-manual.html
imported/wpt/pointerevents/pointerevent_touch-action-button-test_touch-manual.html
imported/wpt/pointerevents/pointerevent_pointerleave_touch-manual.html
imported/wpt/pointerevents/pointerevent_touch-action-pan-y-css_touch-manual.html
imported/wpt/pointerevents/pointerevent_pointerenter_nohover-manual.html
imported/wpt/pointerevents/pointerevent_touch-action-inherit_highest-parent-none_touch-manual.html
imported/wpt/pointerevents/pointerevent_pointerout_after_pointercancel_touch-manual.html
imported/wpt/pointerevents/pointerevent_pointerleave_after_pointerup_nohover-manual.html
imported/wpt/pointerevents/pointerevent_touch-action-svg-test_touch-manual.html
imported/wpt/pointerevents/pointerevent_pointerout_after_pointerup_nohover-manual.html
imported/wpt/pointerevents/pointerevent_pointerleave_after_pointercancel_touch-manual.html
imported/wpt/pointerevents/pointerevent_releasepointercapture_onpointercancel_touch-manual.html
imported/wpt/pointerevents/pointerevent_touch-action-pan-x-pan-y_touch-manual.html
imported/wpt/pointerevents/pointerevent_touch-action-inherit_parent-none_touch-manual.html
imported/wpt/pointerevents/pointerevent_touch-action-pan-x-css_touch-manual.html
imported/wpt/pointerevents/pointerevent_pointercancel_touch-manual.html
imported/wpt/pointerevents/pointerevent_touch-action-inherit_child-pan-x-child-pan-y_touch-manual.html
imported/wpt/pointerevents/pointerevent_touch-action-table-test_touch-manual.html
imported/wpt/pointerevents/pointerevent_touch-action-span-test_touch-manual.html
imported/wpt/pointerevents/pointerevent_touch-action-inherit_child-none_touch-manual.html
imported/wpt/pointerevents/pointerevent_touch-action-inherit_child-pan-x-child-pan-x_touch-manual.html.

[Navid Zolghadr, 2016-08-17 20:01:32]

Description was changed from

==========
Refactoring button field and its type

- Remove m_button field from PlatformMouseEvent as
    there was one in WebPointerProperties.
- Remove duplicate PlatformMouseEvent::MouseButton
    and WebScrollbarBehavior::Button enums and use
    WebPointerProperties::Button enum instead.
- Making WebPointerProperties::Button an enum class.

- Also cleanup related target for pointerevents. The related target for all
pointerevents (similar to mouseevent) should be null except for boundary events.
So relatedTarget can be removed from some of the functions as they don't send
boundary events.

BUG=635670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

TBR=pfeldman@chromium.org

Committed: https://crrev.com/3acb734ff34a77e76228425d06952317c10e252b
Cr-Commit-Position: refs/heads/master@{#412552}
==========

to

==========
Refactoring button field and its type

- Remove m_button field from PlatformMouseEvent as
    there was one in WebPointerProperties.
- Remove duplicate PlatformMouseEvent::MouseButton
    and WebScrollbarBehavior::Button enums and use
    WebPointerProperties::Button enum instead.
- Making WebPointerProperties::Button an enum class.

- Also cleanup related target for pointerevents. The related target for all
pointerevents (similar to mouseevent) should be null except for boundary events.
So relatedTarget can be removed from some of the functions as they don't send
boundary events.

BUG=635670
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

TBR=pfeldman@chromium.org

Committed: https://crrev.com/3acb734ff34a77e76228425d06952317c10e252b
Cr-Commit-Position: refs/heads/master@{#412552}
==========