Add pointer id to the WebMouseEvent's constructors

Add pointer id to the WebMouseEvent's constructors

In order to make the WebMouseEvent's pointer id consistent with ui::MouseEvent, we
add pointer id to the WebMouseEvent's constructors. Then we can enable the DCHECK
of pointer id in PointerEventFactory.

BUG=694742
Review-Url: https://codereview.chromium.org/2849083002
Cr-Commit-Position: refs/heads/master@{#472106}
Committed: https://chromium.googlesource.com/chromium/src/+/2db36b51a5de1d110a0d9dbe6969ab9603df9ad8

[lanwei, 2017-04-30 16:53:17]

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

[commit-bot: I haz the power, 2017-04-30 16:53:23]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-04-30 17:08:01]

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

[commit-bot: I haz the power, 2017-04-30 17:08:02]

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

[lanwei, 2017-04-30 17:47:03]

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

[commit-bot: I haz the power, 2017-04-30 17:47:06]

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-04-30 19:58:22]

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

[commit-bot: I haz the power, 2017-04-30 19:58:22]

Dry run: This issue passed the CQ dry run.

[lanwei, 2017-04-30 23:36:49]

Patchset #1 (id:1) has been deleted

[lanwei, 2017-04-30 23:37:00]

Patchset #1 (id:20001) has been deleted

[lanwei, 2017-05-01 00:00:18]

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

[commit-bot: I haz the power, 2017-05-01 00:00:35]

Dry run: CQ is trying da patch.

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

[lanwei, 2017-05-01 00:01:41]

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

[commit-bot: I haz the power, 2017-05-01 00:01:50]

Dry run: CQ is trying da patch.

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

[lanwei, 2017-05-01 00:01:58]

Patchset #2 (id:60001) has been deleted

[lanwei, 2017-05-01 00:09:40]

Description was changed from

==========
webmouseid


webmouseid


webmouseid

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

to

==========
Add pointer id to the WebMouseEvent's constructors

In order to make the WebMouseEvent's pointer id consistent with ui::MouseEvent,
we
add pointer id to the WebMouseEvent's constructors. Then we can enable the
DCHECK
of pointer id in PointerEventFactory.

BUG=694742
==========

[lanwei, 2017-05-01 00:09:41]

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

[lanwei, 2017-05-01 00:10:33]

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

[lanwei, 2017-05-01 00:11:02]

Patchset #1 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-05-01 02:07:52]

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

[commit-bot: I haz the power, 2017-05-01 02:07:53]

Dry run: This issue passed the CQ dry run.

[sadrul, 2017-05-01 15:42:11]

ui lgtm

https://codereview.chromium.org/2849083002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebMouseWheelEvent.h (right):

https://codereview.chromium.org/2849083002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebMouseWheelEvent.h:61: 0),
Shouldn't this be the default mouse-pointer-id?

https://codereview.chromium.org/2849083002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebPointerProperties.h (right):

https://codereview.chromium.org/2849083002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebPointerProperties.h:49:
WebPointerProperties(int id_param)
explicit

[dtapuska, 2017-05-01 15:49:06]

https://codereview.chromium.org/2849083002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebInputEvent.h (right):

https://codereview.chromium.org/2849083002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebInputEvent.h:281: static const int32_t
kMousePointerId = std::numeric_limits<int32_t>::max();
Use constexpr.

Should this be in WebMouseEvent.h?

https://codereview.chromium.org/2849083002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebMouseEvent.h (right):

https://codereview.chromium.org/2849083002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebMouseEvent.h:33: int id_param =
kMousePointerId)
Why do you use an int here and an int32_t for the definition?

[lanwei, 2017-05-01 19:15:23]

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

[commit-bot: I haz the power, 2017-05-01 19:15: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, 2017-05-01 19:28:43]

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

[commit-bot: I haz the power, 2017-05-01 19:28:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_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_...)

[lanwei, 2017-05-01 20:51:00]

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

[commit-bot: I haz the power, 2017-05-01 20:51:37]

Dry run: CQ is trying da patch.

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

[lanwei, 2017-05-01 21:12:48]

Patchset #2 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-05-01 22:59:58]

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

[commit-bot: I haz the power, 2017-05-01 22:59:59]

Dry run: This issue passed the CQ dry run.

[lanwei, 2017-05-01 23:51:20]

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

[lanwei, 2017-05-01 23:51:21]

haraken@chromium.org: Please review changes in 

third_party/Webkit/*,

thank you.

https://codereview.chromium.org/2849083002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebInputEvent.h (right):

https://codereview.chromium.org/2849083002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebInputEvent.h:281: static const int32_t
kMousePointerId = std::numeric_limits<int32_t>::max();
On 2017/05/01 15:49:05, dtapuska wrote:
> Use constexpr.
> 
> Should this be in WebMouseEvent.h?

Done.

https://codereview.chromium.org/2849083002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebMouseEvent.h (right):

https://codereview.chromium.org/2849083002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebMouseEvent.h:33: int id_param =
kMousePointerId)
On 2017/05/01 15:49:05, dtapuska wrote:
> Why do you use an int here and an int32_t for the definition?

Done.

https://codereview.chromium.org/2849083002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebMouseWheelEvent.h (right):

https://codereview.chromium.org/2849083002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebMouseWheelEvent.h:61: 0),
On 2017/05/01 15:42:11, sadrul wrote:
> Shouldn't this be the default mouse-pointer-id?

Done.

https://codereview.chromium.org/2849083002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebPointerProperties.h (right):

https://codereview.chromium.org/2849083002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebPointerProperties.h:49:
WebPointerProperties(int id_param)
On 2017/05/01 15:42:11, sadrul wrote:
> explicit

Done.

[haraken, 2017-05-02 00:27:23]

WebKit LGTM

https://codereview.chromium.org/2849083002/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebMouseEvent.h (right):

https://codereview.chromium.org/2849083002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMouseEvent.h:24: static constexpr int
kMousePointerId = std::numeric_limits<int>::max();

kMousePointerId => kUndefinedMousePointerID or something?

[lanwei, 2017-05-02 21:04:38]

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

[lanwei, 2017-05-03 00:10:09]

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

[commit-bot: I haz the power, 2017-05-03 00:10: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, 2017-05-03 02:14:49]

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

[commit-bot: I haz the power, 2017-05-03 02:14:51]

Dry run: This issue passed the CQ dry run.

[lanwei, 2017-05-03 15:21:11]

lanwei@chromium.org changed reviewers:
+ mbarbella@chromium.org, sky@chromium.org

[lanwei, 2017-05-03 15:22:11]

lanwei@chromium.org changed reviewers:
+ dcheng@chromium.org
- mbarbella@chromium.org

[lanwei, 2017-05-03 15:24:36]

sky@ could you please take a look at content/* and services/*

dcheng@ could you please take a look at ui/events/mojo/event_struct_traits.cc

Thank you.

[dcheng, 2017-05-03 17:44:05]

rs lgtm for struct traits changes

[sky, 2017-05-04 15:26:03]

https://codereview.chromium.org/2849083002/diff/140001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/2849083002/diff/140001/ui/events/event.h#newc...
ui/events/event.h:484: static const int kDefaultMousePointerId;
Could you clarify why you are changing this? Isn't there only one mouse pointer,
in which case kMousePointerId is a better name?

[lanwei, 2017-05-04 15:30:32]

On 2017/05/04 15:26:03, sky wrote:
> https://codereview.chromium.org/2849083002/diff/140001/ui/events/event.h
> File ui/events/event.h (right):
> 
>
https://codereview.chromium.org/2849083002/diff/140001/ui/events/event.h#newc...
> ui/events/event.h:484: static const int kDefaultMousePointerId;
> Could you clarify why you are changing this? Isn't there only one mouse
pointer,
> in which case kMousePointerId is a better name?

Yes, there is only one mouse pointer, but this pointer id is not a real id, that
is why I change the name to kDefaultMousePointerId, to distinguish if later we
use the real id from the OS event.

[sky, 2017-05-04 17:34:14]

Could you elaborate on why we would want to do that? By that I mean why
would we want to use the system one as well as having a define that doesn't
map to what events are actually using?

  -Scott

On Thu, May 4, 2017 at 8:30 AM, <lanwei@chromium.org> wrote:

> On 2017/05/04 15:26:03, sky wrote:
> > https://codereview.chromium.org/2849083002/diff/140001/ui/events/event.h
> > File ui/events/event.h (right):
> >
> >
> https://codereview.chromium.org/2849083002/diff/140001/ui/
> events/event.h#newcode484
> > ui/events/event.h:484: static const int kDefaultMousePointerId;
> > Could you clarify why you are changing this? Isn't there only one mouse
> pointer,
> > in which case kMousePointerId is a better name?
>
> Yes, there is only one mouse pointer, but this pointer id is not a real
> id, that
> is why I change the name to kDefaultMousePointerId, to distinguish if
> later we
> use the real id from the OS event.
>
> https://codereview.chromium.org/2849083002/
>

-- 
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, 2017-05-04 17:34:16]

Could you elaborate on why we would want to do that? By that I mean why
would we want to use the system one as well as having a define that doesn't
map to what events are actually using?

  -Scott

On Thu, May 4, 2017 at 8:30 AM, <lanwei@chromium.org> wrote:

> On 2017/05/04 15:26:03, sky wrote:
> > https://codereview.chromium.org/2849083002/diff/140001/ui/events/event.h
> > File ui/events/event.h (right):
> >
> >
> https://codereview.chromium.org/2849083002/diff/140001/ui/
> events/event.h#newcode484
> > ui/events/event.h:484: static const int kDefaultMousePointerId;
> > Could you clarify why you are changing this? Isn't there only one mouse
> pointer,
> > in which case kMousePointerId is a better name?
>
> Yes, there is only one mouse pointer, but this pointer id is not a real
> id, that
> is why I change the name to kDefaultMousePointerId, to distinguish if
> later we
> use the real id from the OS event.
>
> https://codereview.chromium.org/2849083002/
>

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

[lanwei, 2017-05-04 18:08:21]

On 2017/05/04 17:34:16, sky wrote:
> Could you elaborate on why we would want to do that? By that I mean why
> would we want to use the system one as well as having a define that doesn't
> map to what events are actually using?
> 
>   -Scott
> 

I know that right now we only support one moue device, so it does not really
matter what value we use for mouse pointer id.
In the future, we will support multiple mouse devices, it does make sense to use
the real id from OS event. 

Right now, The dom pointer event for mouse type does not use the pointer id from
WebMouseEvent, it is always 1 like what Edge sets for its mouse pointers.
However, we would like to change this later to use the real id from OS events,
like what we did for pointer events of pen type.
Even though we only support one pen at this time, we are using the id from OS
event. We are slowly replacing the default values with real values. 




> On Thu, May 4, 2017 at 8:30 AM, <mailto:lanwei@chromium.org> wrote:
> 
> > On 2017/05/04 15:26:03, sky wrote:
> > > https://codereview.chromium.org/2849083002/diff/140001/ui/events/event.h
> > > File ui/events/event.h (right):
> > >
> > >
> > https://codereview.chromium.org/2849083002/diff/140001/ui/
> > events/event.h#newcode484
> > > ui/events/event.h:484: static const int kDefaultMousePointerId;
> > > Could you clarify why you are changing this? Isn't there only one mouse
> > pointer,
> > > in which case kMousePointerId is a better name?
> >
> > Yes, there is only one mouse pointer, but this pointer id is not a real
> > id, that
> > is why I change the name to kDefaultMousePointerId, to distinguish if
> > later we
> > use the real id from the OS event.
> >
> > https://codereview.chromium.org/2849083002/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

[sky, 2017-05-04 19:34:37]

I'm concerned that having a constant that says 'the mouse id' when we may
use a different id is a recipe for bugs. If you are saying we want to use
the system one, then we should get rid of the constant.

On Thu, May 4, 2017 at 11:08 AM, <lanwei@chromium.org> wrote:

> On 2017/05/04 17:34:16, sky wrote:
> > Could you elaborate on why we would want to do that? By that I mean why
> > would we want to use the system one as well as having a define that
> doesn't
> > map to what events are actually using?
> >
> > -Scott
> >
>
> I know that right now we only support one moue device, so it does not
> really
> matter what value we use for mouse pointer id.
> In the future, we will support multiple mouse devices, it does make sense
> to use
> the real id from OS event.
>
> Right now, The dom pointer event for mouse type does not use the pointer
> id from
> WebMouseEvent, it is always 1 like what Edge sets for its mouse pointers.
> However, we would like to change this later to use the real id from OS
> events,
> like what we did for pointer events of pen type.
> Even though we only support one pen at this time, we are using the id from
> OS
> event. We are slowly replacing the default values with real values.
>
>
>
>
> > On Thu, May 4, 2017 at 8:30 AM, <mailto:lanwei@chromium.org> wrote:
> >
> > > On 2017/05/04 15:26:03, sky wrote:
> > > > https://codereview.chromium.org/2849083002/diff/140001/ui/
> events/event.h
> > > > File ui/events/event.h (right):
> > > >
> > > >
> > > https://codereview.chromium.org/2849083002/diff/140001/ui/
> > > events/event.h#newcode484
> > > > ui/events/event.h:484: static const int kDefaultMousePointerId;
> > > > Could you clarify why you are changing this? Isn't there only one
> mouse
> > > pointer,
> > > > in which case kMousePointerId is a better name?
> > >
> > > Yes, there is only one mouse pointer, but this pointer id is not a real
> > > id, that
> > > is why I change the name to kDefaultMousePointerId, to distinguish if
> > > later we
> > > use the real id from the OS event.
> > >
> > > https://codereview.chromium.org/2849083002/
> > >
> >
> > --
> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2849083002/
>

-- 
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, 2017-05-04 19:34:37]

I'm concerned that having a constant that says 'the mouse id' when we may
use a different id is a recipe for bugs. If you are saying we want to use
the system one, then we should get rid of the constant.

On Thu, May 4, 2017 at 11:08 AM, <lanwei@chromium.org> wrote:

> On 2017/05/04 17:34:16, sky wrote:
> > Could you elaborate on why we would want to do that? By that I mean why
> > would we want to use the system one as well as having a define that
> doesn't
> > map to what events are actually using?
> >
> > -Scott
> >
>
> I know that right now we only support one moue device, so it does not
> really
> matter what value we use for mouse pointer id.
> In the future, we will support multiple mouse devices, it does make sense
> to use
> the real id from OS event.
>
> Right now, The dom pointer event for mouse type does not use the pointer
> id from
> WebMouseEvent, it is always 1 like what Edge sets for its mouse pointers.
> However, we would like to change this later to use the real id from OS
> events,
> like what we did for pointer events of pen type.
> Even though we only support one pen at this time, we are using the id from
> OS
> event. We are slowly replacing the default values with real values.
>
>
>
>
> > On Thu, May 4, 2017 at 8:30 AM, <mailto:lanwei@chromium.org> wrote:
> >
> > > On 2017/05/04 15:26:03, sky wrote:
> > > > https://codereview.chromium.org/2849083002/diff/140001/ui/
> events/event.h
> > > > File ui/events/event.h (right):
> > > >
> > > >
> > > https://codereview.chromium.org/2849083002/diff/140001/ui/
> > > events/event.h#newcode484
> > > > ui/events/event.h:484: static const int kDefaultMousePointerId;
> > > > Could you clarify why you are changing this? Isn't there only one
> mouse
> > > pointer,
> > > > in which case kMousePointerId is a better name?
> > >
> > > Yes, there is only one mouse pointer, but this pointer id is not a real
> > > id, that
> > > is why I change the name to kDefaultMousePointerId, to distinguish if
> > > later we
> > > use the real id from the OS event.
> > >
> > > https://codereview.chromium.org/2849083002/
> > >
> >
> > --
> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2849083002/
>

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

[lanwei, 2017-05-04 20:07:50]

On 2017/05/04 19:34:37, sky wrote:
> I'm concerned that having a constant that says 'the mouse id' when we may
> use a different id is a recipe for bugs. If you are saying we want to use
> the system one, then we should get rid of the constant.


You are right. Using the real ID right now may bring some inconsistent issue,
and we are not sure every platforms has the id information.
We decide to use the kMousePointerId as the pointer id for all mouse events, so
if we change the name back to kMousePointerId, do you think it should be good?




> On Thu, May 4, 2017 at 11:08 AM, <mailto:lanwei@chromium.org> wrote:
> 
> > On 2017/05/04 17:34:16, sky wrote:
> > > Could you elaborate on why we would want to do that? By that I mean why
> > > would we want to use the system one as well as having a define that
> > doesn't
> > > map to what events are actually using?
> > >
> > > -Scott
> > >
> >
> > I know that right now we only support one moue device, so it does not
> > really
> > matter what value we use for mouse pointer id.
> > In the future, we will support multiple mouse devices, it does make sense
> > to use
> > the real id from OS event.
> >
> > Right now, The dom pointer event for mouse type does not use the pointer
> > id from
> > WebMouseEvent, it is always 1 like what Edge sets for its mouse pointers.
> > However, we would like to change this later to use the real id from OS
> > events,
> > like what we did for pointer events of pen type.
> > Even though we only support one pen at this time, we are using the id from
> > OS
> > event. We are slowly replacing the default values with real values.
> >
> >
> >
> >
> > > On Thu, May 4, 2017 at 8:30 AM, <mailto:lanwei@chromium.org> wrote:
> > >
> > > > On 2017/05/04 15:26:03, sky wrote:
> > > > > https://codereview.chromium.org/2849083002/diff/140001/ui/
> > events/event.h
> > > > > File ui/events/event.h (right):
> > > > >
> > > > >
> > > > https://codereview.chromium.org/2849083002/diff/140001/ui/
> > > > events/event.h#newcode484
> > > > > ui/events/event.h:484: static const int kDefaultMousePointerId;
> > > > > Could you clarify why you are changing this? Isn't there only one
> > mouse
> > > > pointer,
> > > > > in which case kMousePointerId is a better name?
> > > >
> > > > Yes, there is only one mouse pointer, but this pointer id is not a real
> > > > id, that
> > > > is why I change the name to kDefaultMousePointerId, to distinguish if
> > > > later we
> > > > use the real id from the OS event.
> > > >
> > > > https://codereview.chromium.org/2849083002/
> > > >
> > >
> > > --
> > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> >
> >
> > https://codereview.chromium.org/2849083002/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

[sky, 2017-05-04 22:28:45]

SGTM. Thanks!

On Thu, May 4, 2017 at 1:07 PM, <lanwei@chromium.org> wrote:

> On 2017/05/04 19:34:37, sky wrote:
> > I'm concerned that having a constant that says 'the mouse id' when we may
> > use a different id is a recipe for bugs. If you are saying we want to use
> > the system one, then we should get rid of the constant.
>
>
> You are right. Using the real ID right now may bring some inconsistent
> issue,
> and we are not sure every platforms has the id information.
> We decide to use the kMousePointerId as the pointer id for all mouse
> events, so
> if we change the name back to kMousePointerId, do you think it should be
> good?
>
>
>
>
>
> > On Thu, May 4, 2017 at 11:08 AM, <mailto:lanwei@chromium.org> wrote:
> >
> > > On 2017/05/04 17:34:16, sky wrote:
> > > > Could you elaborate on why we would want to do that? By that I mean
> why
> > > > would we want to use the system one as well as having a define that
> > > doesn't
> > > > map to what events are actually using?
> > > >
> > > > -Scott
> > > >
> > >
> > > I know that right now we only support one moue device, so it does not
> > > really
> > > matter what value we use for mouse pointer id.
> > > In the future, we will support multiple mouse devices, it does make
> sense
> > > to use
> > > the real id from OS event.
> > >
> > > Right now, The dom pointer event for mouse type does not use the
> pointer
> > > id from
> > > WebMouseEvent, it is always 1 like what Edge sets for its mouse
> pointers.
> > > However, we would like to change this later to use the real id from OS
> > > events,
> > > like what we did for pointer events of pen type.
> > > Even though we only support one pen at this time, we are using the id
> from
> > > OS
> > > event. We are slowly replacing the default values with real values.
> > >
> > >
> > >
> > >
> > > > On Thu, May 4, 2017 at 8:30 AM, <mailto:lanwei@chromium.org> wrote:
> > > >
> > > > > On 2017/05/04 15:26:03, sky wrote:
> > > > > > https://codereview.chromium.org/2849083002/diff/140001/ui/
> > > events/event.h
> > > > > > File ui/events/event.h (right):
> > > > > >
> > > > > >
> > > > > https://codereview.chromium.org/2849083002/diff/140001/ui/
> > > > > events/event.h#newcode484
> > > > > > ui/events/event.h:484: static const int kDefaultMousePointerId;
> > > > > > Could you clarify why you are changing this? Isn't there only one
> > > mouse
> > > > > pointer,
> > > > > > in which case kMousePointerId is a better name?
> > > > >
> > > > > Yes, there is only one mouse pointer, but this pointer id is not a
> real
> > > > > id, that
> > > > > is why I change the name to kDefaultMousePointerId, to distinguish
> if
> > > > > later we
> > > > > use the real id from the OS event.
> > > > >
> > > > > https://codereview.chromium.org/2849083002/
> > > > >
> > > >
> > > > --
> > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > >
> > >
> > >
> > > https://codereview.chromium.org/2849083002/
> > >
> >
> > --
> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2849083002/
>

-- 
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, 2017-05-04 22:28:46]

SGTM. Thanks!

On Thu, May 4, 2017 at 1:07 PM, <lanwei@chromium.org> wrote:

> On 2017/05/04 19:34:37, sky wrote:
> > I'm concerned that having a constant that says 'the mouse id' when we may
> > use a different id is a recipe for bugs. If you are saying we want to use
> > the system one, then we should get rid of the constant.
>
>
> You are right. Using the real ID right now may bring some inconsistent
> issue,
> and we are not sure every platforms has the id information.
> We decide to use the kMousePointerId as the pointer id for all mouse
> events, so
> if we change the name back to kMousePointerId, do you think it should be
> good?
>
>
>
>
>
> > On Thu, May 4, 2017 at 11:08 AM, <mailto:lanwei@chromium.org> wrote:
> >
> > > On 2017/05/04 17:34:16, sky wrote:
> > > > Could you elaborate on why we would want to do that? By that I mean
> why
> > > > would we want to use the system one as well as having a define that
> > > doesn't
> > > > map to what events are actually using?
> > > >
> > > > -Scott
> > > >
> > >
> > > I know that right now we only support one moue device, so it does not
> > > really
> > > matter what value we use for mouse pointer id.
> > > In the future, we will support multiple mouse devices, it does make
> sense
> > > to use
> > > the real id from OS event.
> > >
> > > Right now, The dom pointer event for mouse type does not use the
> pointer
> > > id from
> > > WebMouseEvent, it is always 1 like what Edge sets for its mouse
> pointers.
> > > However, we would like to change this later to use the real id from OS
> > > events,
> > > like what we did for pointer events of pen type.
> > > Even though we only support one pen at this time, we are using the id
> from
> > > OS
> > > event. We are slowly replacing the default values with real values.
> > >
> > >
> > >
> > >
> > > > On Thu, May 4, 2017 at 8:30 AM, <mailto:lanwei@chromium.org> wrote:
> > > >
> > > > > On 2017/05/04 15:26:03, sky wrote:
> > > > > > https://codereview.chromium.org/2849083002/diff/140001/ui/
> > > events/event.h
> > > > > > File ui/events/event.h (right):
> > > > > >
> > > > > >
> > > > > https://codereview.chromium.org/2849083002/diff/140001/ui/
> > > > > events/event.h#newcode484
> > > > > > ui/events/event.h:484: static const int kDefaultMousePointerId;
> > > > > > Could you clarify why you are changing this? Isn't there only one
> > > mouse
> > > > > pointer,
> > > > > > in which case kMousePointerId is a better name?
> > > > >
> > > > > Yes, there is only one mouse pointer, but this pointer id is not a
> real
> > > > > id, that
> > > > > is why I change the name to kDefaultMousePointerId, to distinguish
> if
> > > > > later we
> > > > > use the real id from the OS event.
> > > > >
> > > > > https://codereview.chromium.org/2849083002/
> > > > >
> > > >
> > > > --
> > > > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
> > >
> > >
> > >
> > > https://codereview.chromium.org/2849083002/
> > >
> >
> > --
> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2849083002/
>

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

[lanwei, 2017-05-04 23:18:03]

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

[commit-bot: I haz the power, 2017-05-04 23:18:53]

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-05 00:33:02]

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

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

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

[lanwei, 2017-05-05 18:37:15]

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

[lanwei, 2017-05-05 18:37:27]

Patchset #4 (id:160001) has been deleted

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

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-05 21:17:00]

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

[commit-bot: I haz the power, 2017-05-05 21:17:01]

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

[sky, 2017-05-05 22:51:46]

https://codereview.chromium.org/2849083002/diff/180001/ui/events/event.h
File ui/events/event.h (right):

https://codereview.chromium.org/2849083002/diff/180001/ui/events/event.h#newc...
ui/events/event.h:484: static const int kMousePointerId;
This value is serialized in mojom as an int32. It would be safer to convert
PointerDetails to use int32_t rather than changing this. See
https://chromium.googlesource.com/chromium/src/+/master/ui/events/mojo/event....

[lanwei, 2017-05-08 14:40:10]

On 2017/05/05 22:51:46, sky wrote:
> https://codereview.chromium.org/2849083002/diff/180001/ui/events/event.h
> File ui/events/event.h (right):
> 
>
https://codereview.chromium.org/2849083002/diff/180001/ui/events/event.h#newc...
> ui/events/event.h:484: static const int kMousePointerId;
> This value is serialized in mojom as an int32. It would be safer to convert
> PointerDetails to use int32_t rather than changing this. See
>
https://chromium.googlesource.com/chromium/src/+/master/ui/events/mojo/event....

Because the type of id in WebPointerProperties is int, so is the id in
blink::PointerEvent, we want to make it consistent it in PointerDetails too.

I looked at only in ui/events/mojo/event_struct_traits.cc, it constructs
ui::PointerEvents from mojom::PointerData, I can add a cast from int32_t to int,
what do you think?

[sky, 2017-05-08 16:07:37]

On 2017/05/08 14:40:10, lanwei wrote:
> On 2017/05/05 22:51:46, sky wrote:
> > https://codereview.chromium.org/2849083002/diff/180001/ui/events/event.h
> > File ui/events/event.h (right):
> > 
> >
>
https://codereview.chromium.org/2849083002/diff/180001/ui/events/event.h#newc...
> > ui/events/event.h:484: static const int kMousePointerId;
> > This value is serialized in mojom as an int32. It would be safer to convert
> > PointerDetails to use int32_t rather than changing this. See
> >
>
https://chromium.googlesource.com/chromium/src/+/master/ui/events/mojo/event....
> 
> Because the type of id in WebPointerProperties is int, so is the id in
> blink::PointerEvent, we want to make it consistent it in PointerDetails too.
> 
> I looked at only in ui/events/mojo/event_struct_traits.cc, it constructs
> ui::PointerEvents from mojom::PointerData, I can add a cast from int32_t to
int,
> what do you think?

Does WebPointerProperties need to be int? Can it be int32? If we don't use int32
consistently we're bound to run into serialization issues down the line.

[lanwei, 2017-05-08 17:52:49]

On 2017/05/08 16:07:37, sky wrote:
> On 2017/05/08 14:40:10, lanwei wrote:
> > On 2017/05/05 22:51:46, sky wrote:
> > > https://codereview.chromium.org/2849083002/diff/180001/ui/events/event.h
> > > File ui/events/event.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2849083002/diff/180001/ui/events/event.h#newc...
> > > ui/events/event.h:484: static const int kMousePointerId;
> > > This value is serialized in mojom as an int32. It would be safer to
convert
> > > PointerDetails to use int32_t rather than changing this. See
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/master/ui/events/mojo/event....
> > 
> > Because the type of id in WebPointerProperties is int, so is the id in
> > blink::PointerEvent, we want to make it consistent it in PointerDetails too.
> > 
> > I looked at only in ui/events/mojo/event_struct_traits.cc, it constructs
> > ui::PointerEvents from mojom::PointerData, I can add a cast from int32_t to
> int,
> > what do you think?
> 
> Does WebPointerProperties need to be int? Can it be int32? If we don't use
int32
> consistently we're bound to run into serialization issues down the line.

WebPointerProperties does not need to be int, we can change id in PointerDetails
and WebPointerProperties to be int32_t.
sadrul@ suggests we introduce a specific type for this, e.g. using PointerId =
<...>;
what do you think?

[sky, 2017-05-09 19:27:01]

On 2017/05/08 17:52:49, lanwei wrote:
> On 2017/05/08 16:07:37, sky wrote:
> > On 2017/05/08 14:40:10, lanwei wrote:
> > > On 2017/05/05 22:51:46, sky wrote:
> > > > https://codereview.chromium.org/2849083002/diff/180001/ui/events/event.h
> > > > File ui/events/event.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2849083002/diff/180001/ui/events/event.h#newc...
> > > > ui/events/event.h:484: static const int kMousePointerId;
> > > > This value is serialized in mojom as an int32. It would be safer to
> convert
> > > > PointerDetails to use int32_t rather than changing this. See
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/master/ui/events/mojo/event....
> > > 
> > > Because the type of id in WebPointerProperties is int, so is the id in
> > > blink::PointerEvent, we want to make it consistent it in PointerDetails
too.
> > > 
> > > I looked at only in ui/events/mojo/event_struct_traits.cc, it constructs
> > > ui::PointerEvents from mojom::PointerData, I can add a cast from int32_t
to
> > int,
> > > what do you think?
> > 
> > Does WebPointerProperties need to be int? Can it be int32? If we don't use
> int32
> > consistently we're bound to run into serialization issues down the line.
> 
> WebPointerProperties does not need to be int, we can change id in
PointerDetails
> and WebPointerProperties to be int32_t.
> sadrul@ suggests we introduce a specific type for this, e.g. using PointerId =
> <...>;
> what do you think?

SGTM

[lanwei, 2017-05-10 16:20:34]

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

[commit-bot: I haz the power, 2017-05-10 16:21:30]

Dry run: CQ is trying da patch.

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

[lanwei, 2017-05-10 17:39:07]

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

[lanwei, 2017-05-10 17:40:16]

Patchset #5 (id:200001) has been deleted

[commit-bot: I haz the power, 2017-05-10 17:40:26]

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-10 20:05:52]

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

[commit-bot: I haz the power, 2017-05-10 20:05:53]

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

[sky, 2017-05-11 19:53:35]

LGTM! Thanks!

[dtapuska, 2017-05-12 19:02:15]

https://codereview.chromium.org/2849083002/diff/220001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebMouseEvent.h (right):

https://codereview.chromium.org/2849083002/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMouseEvent.h:106: WebMouseEvent(unsigned
size_param, int id)
should this not be PointerId

https://codereview.chromium.org/2849083002/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebMouseEvent.h:113: int id)
ditto

[lanwei, 2017-05-12 19:33:12]

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

[commit-bot: I haz the power, 2017-05-12 19:34:46]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-13 00:44:23]

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

[commit-bot: I haz the power, 2017-05-13 00:44:25]

Dry run: This issue passed the CQ dry run.

[lanwei, 2017-05-15 18:33:59]

The CQ bit was checked by lanwei@chromium.org

[lanwei, 2017-05-15 18:34:00]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
haraken@chromium.org, dcheng@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2849083002/#ps240001
(title: "webmouseid")

[commit-bot: I haz the power, 2017-05-15 18:35:09]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-15 19:57:11]

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

[commit-bot: I haz the power, 2017-05-15 19:57:13]

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

[lanwei, 2017-05-16 00:08:16]

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

[commit-bot: I haz the power, 2017-05-16 00:09:20]

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-16 01:47:54]

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

[commit-bot: I haz the power, 2017-05-16 01:47:57]

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

[lanwei, 2017-05-16 12:47:52]

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

[commit-bot: I haz the power, 2017-05-16 12:48:24]

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-16 13:53:29]

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

[commit-bot: I haz the power, 2017-05-16 13:53:31]

Dry run: This issue passed the CQ dry run.

[lanwei, 2017-05-16 14:50:57]

The CQ bit was checked by lanwei@chromium.org

[commit-bot: I haz the power, 2017-05-16 14:51:27]

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-16 14:58:23]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1494946257166920,
"parent_rev": "3c5da4ae54598b8ebed887a52ee9b2763abb0047", "commit_rev":
"2db36b51a5de1d110a0d9dbe6969ab9603df9ad8"}

[commit-bot: I haz the power, 2017-05-16 14:58:33]

Description was changed from

==========
Add pointer id to the WebMouseEvent's constructors

In order to make the WebMouseEvent's pointer id consistent with ui::MouseEvent,
we
add pointer id to the WebMouseEvent's constructors. Then we can enable the
DCHECK
of pointer id in PointerEventFactory.

BUG=694742
==========

to

==========
Add pointer id to the WebMouseEvent's constructors

In order to make the WebMouseEvent's pointer id consistent with ui::MouseEvent,
we
add pointer id to the WebMouseEvent's constructors. Then we can enable the
DCHECK
of pointer id in PointerEventFactory.

BUG=694742

Review-Url: https://codereview.chromium.org/2849083002
Cr-Commit-Position: refs/heads/master@{#472106}
Committed:
https://chromium.googlesource.com/chromium/src/+/2db36b51a5de1d110a0d9dbe6969...
==========

[commit-bot: I haz the power, 2017-05-16 14:58:35]

Committed patchset #6 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/2db36b51a5de1d110a0d9dbe6969...