|
|
DescriptionIntroduce WM_POINTER to Handle pointer events with pen type
BUG=367113
Review-Url: https://codereview.chromium.org/2648683003
Cr-Commit-Position: refs/heads/master@{#460522}
Committed: https://chromium.googlesource.com/chromium/src/+/499bc4f53d3e04ac9e56cd86a2cf7dc723625ef7
Patch Set 1 #
Total comments: 3
Patch Set 2 : wmpointer #Patch Set 3 : wmpointer #Patch Set 4 : wm pointer #Patch Set 5 : wm pointer #
Total comments: 6
Patch Set 6 : wm pointer #
Total comments: 1
Patch Set 7 : remove if #
Total comments: 2
Patch Set 8 : remove pointerinfo #
Total comments: 4
Patch Set 9 : change to ptPixelLocationRaw #Patch Set 10 : wm pointer #
Total comments: 21
Patch Set 11 : wm pointer #
Total comments: 1
Patch Set 12 : NOTREACHED() when default #
Messages
Total messages: 120 (92 generated)
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== wmpointer BUG= ========== to ========== Introduce WM_POINTER to Handle pointer events with pen type BUG=367113 ==========
lanwei@chromium.org changed reviewers: + dtapuska@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
dtapuska@chromium.org changed reviewers: + anantha@chromium.org
anantha@ this isn't ready for review yet but if you have some rough comments about the approach here it would be appreciated. https://codereview.chromium.org/2648683003/diff/1/ui/views/win/hwnd_message_h... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2648683003/diff/1/ui/views/win/hwnd_message_h... ui/views/win/hwnd_message_handler.cc:1700: ui::PointerDetails pointer_details( We need to make another change to aura to add rotation to aura's pointer details. Do we want to listen to WM_POINTERDEVICEOUTOFRANGE and WM_POINTERDEVICEINRANGE as well? Ideally these would go out as pointer events but I don't believe aura fully supports pointer events (only mus does) https://codereview.chromium.org/2648683003/diff/1/ui/views/win/hwnd_message_h... ui/views/win/hwnd_message_handler.cc:1712: SetMsgHandled(handled); I wonder if you need to always return SetMsgHandled(TRUE) here to make sure the WM_MOUSE events aren't generated. anantha@ may know better.
lanwei@chromium.org changed reviewers: + ananta@chromium.org - anantha@chromium.org
ananta@ this isn't ready for review yet but if you have some rough comments about the approach here it would be appreciated.
mustaq@chromium.org changed reviewers: + mustaq@chromium.org
https://codereview.chromium.org/2648683003/diff/1/ui/views/win/hwnd_message_h... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/2648683003/diff/1/ui/views/win/hwnd_message_h... ui/views/win/hwnd_message_handler.h:226: LRESULT HandleMouseMessage(unsigned int message, Do we need to do something to prevent double-handling? IIRC from a meeting with MS last year, the OS suppresses dispatching legacy events to an app if the app "handles" WM_POINTER. Not sure if this means the LRESULT from HandlePointerMessage() is enough, or we need to notify the OS separately. Can someone please confirm this? Perhaps through the spec?
On 2017/01/26 19:19:35, mustaq wrote: > https://codereview.chromium.org/2648683003/diff/1/ui/views/win/hwnd_message_h... > File ui/views/win/hwnd_message_handler.h (right): > > https://codereview.chromium.org/2648683003/diff/1/ui/views/win/hwnd_message_h... > ui/views/win/hwnd_message_handler.h:226: LRESULT HandleMouseMessage(unsigned int > message, > Do we need to do something to prevent double-handling? IIRC from a meeting with > MS last year, the OS suppresses dispatching legacy events to an app if the app > "handles" WM_POINTER. Not sure if this means the LRESULT from > HandlePointerMessage() is enough, or we need to notify the OS separately. > > Can someone please confirm this? Perhaps through the spec? Please verify with test programs that handling WM_POINTER suppresses the mousemoves and other messages reliably. These tests should be done with tablet mode as well. Thanks Ananta
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Patchset #3 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
https://codereview.chromium.org/2648683003/diff/100001/ui/views/win/hwnd_mess... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2648683003/diff/100001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:946: if (is_pointer_in_range_) Does not appear to be set anywhere. https://codereview.chromium.org/2648683003/diff/100001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1668: if (get_pointer_type && get_pointer_type(pointer_id, &pointer_type)) { If we are getting a WM_POINTER*** message then we can assume Windows 8+ and avoid these null checks here? https://codereview.chromium.org/2648683003/diff/100001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1698: ui::EventType event_type = ui::ET_MOUSE_MOVED; Please add some comments here. The code below is creating a fake mouse message from the pointer event and then setting up an associated pointer details structure in the MouseEvent structure. Once we have that, does everything else automatically work?
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:120001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lanwei@chromium.org changed reviewers: + sadrul@chromium.org
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Patchset #7 (id:160001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:140001) has been deleted
https://codereview.chromium.org/2648683003/diff/100001/ui/views/win/hwnd_mess... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2648683003/diff/100001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:946: if (is_pointer_in_range_) On 2017/03/09 00:37:33, ananta wrote: > Does not appear to be set anywhere. It is not useful now, I deleted it. https://codereview.chromium.org/2648683003/diff/100001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1668: if (get_pointer_type && get_pointer_type(pointer_id, &pointer_type)) { On 2017/03/09 00:37:33, ananta wrote: > If we are getting a WM_POINTER*** message then we can assume Windows 8+ and > avoid these null checks here? You are right, but the other places calling the GetProcAddress also have the null check, I think it maybe possible that the function call will fail even on Windows 8+, and on Windows 7, someone may send WM_POINTER messages manually. https://msdn.microsoft.com/en-us/library/windows/desktop/ms683212(v=vs.85).aspx https://codereview.chromium.org/2648683003/diff/100001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1698: ui::EventType event_type = ui::ET_MOUSE_MOVED; On 2017/03/09 00:37:33, ananta wrote: > Please add some comments here. The code below is creating a fake mouse message > from the pointer event and then setting up an associated pointer details > structure in the MouseEvent structure. > > Once we have that, does everything else automatically work? Yes. Touch and mouse work as before, and we are receiving stylus information through WM_POINTER messages.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2648683003/diff/180001/ui/views/win/hwnd_mess... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2648683003/diff/180001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1663: static GetPointerTypeFn get_pointer_type = reinterpret_cast<GetPointerTypeFn>( Instead of these if's for function pointers, perhaps we can check for OS version less than 8 in the beginning of the function and bail if it is win7. We can then assume these pointers to be non null and can validate them with a CHECK? The code looks good.
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #7 (id:200001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2648683003/diff/220001/ui/views/win/hwnd_mess... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2648683003/diff/220001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1698: using GetPointerInfoFn = BOOL(WINAPI*)(UINT32, POINTER_INFO*); This looks a little odd why we have to fetch the pointer info again. Can't we use the embedded pointer info in the POINTER_PEN_INFO object?
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #8 (id:240001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2648683003/diff/220001/ui/views/win/hwnd_mess... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2648683003/diff/220001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1698: using GetPointerInfoFn = BOOL(WINAPI*)(UINT32, POINTER_INFO*); On 2017/03/17 13:49:39, dtapuska wrote: > This looks a little odd why we have to fetch the pointer info again. Can't we > use the embedded pointer info in the POINTER_PEN_INFO object? Done.
https://codereview.chromium.org/2648683003/diff/260001/ui/views/win/hwnd_mess... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2648683003/diff/260001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1697: POINT client_point = pointer_pen_info.pointerInfo.ptPixelLocation; I think ptPixelLocationRaw is more appropriate here. Even though it is the same as ptPixelLocation, the later seems to be for predicted points: https://msdn.microsoft.com/en-us/library/windows/desktop/hh454907(v=vs.85).aspx
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM % nit https://codereview.chromium.org/2648683003/diff/260001/ui/views/win/hwnd_mess... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2648683003/diff/260001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1693: float pressure = (float)pointer_pen_info.pressure / 1024; static_cast<float> here/
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sadrul, could you please take a look, thank you? https://codereview.chromium.org/2648683003/diff/260001/ui/views/win/hwnd_mess... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2648683003/diff/260001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1693: float pressure = (float)pointer_pen_info.pressure / 1024; On 2017/03/22 19:03:26, ananta wrote: > static_cast<float> here/ Done. https://codereview.chromium.org/2648683003/diff/260001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1697: POINT client_point = pointer_pen_info.pointerInfo.ptPixelLocation; On 2017/03/20 14:31:16, mustaq wrote: > I think ptPixelLocationRaw is more appropriate here. Even though it is the same > as ptPixelLocation, the later seems to be for predicted points: > https://msdn.microsoft.com/en-us/library/windows/desktop/hh454907(v=vs.85).aspx Acknowledged.
lgtm
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #10 (id:300001) has been deleted
On 2017/03/23 19:00:22, mustaq wrote: > lgtm seems I haven't lgtm'd this yet
sadrul@chromium.org changed reviewers: + sky@chromium.org - sadrul@chromium.org
--> sky@ for ui/views/win changes I have some nits: https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:982: base::WeakPtr<HWNDMessageHandler> ref(weak_factory_.GetWeakPtr()); I am not sure what the WeakPtr<> is useful for (I notice that we use in HandleTouchMessage etc. functions too). https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1696: int tiltY = pointer_pen_info.tiltY; tilt_x, tilt_y https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1737: if (ref.get()) if (ref) should work too
https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:982: base::WeakPtr<HWNDMessageHandler> ref(weak_factory_.GetWeakPtr()); On 2017/03/23 19:59:38, sadrul wrote: > I am not sure what the WeakPtr<> is useful for (I notice that we use in > HandleTouchMessage etc. functions too). IsMsgHandled is a macro written by sky here: https://chromium.googlesource.com/chromium/src/+/c169c53%5E%21/#F0
https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:982: base::WeakPtr<HWNDMessageHandler> ref(weak_factory_.GetWeakPtr()); On 2017/03/23 19:59:38, sadrul wrote: > I am not sure what the WeakPtr<> is useful for (I notice that we use in > HandleTouchMessage etc. functions too). The instance can be destroyed while processing a message. Hence the weakptr.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
On 2017/03/23 20:04:38, ananta wrote: > https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... > File ui/views/win/hwnd_message_handler.cc (right): > > https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... > ui/views/win/hwnd_message_handler.cc:982: base::WeakPtr<HWNDMessageHandler> > ref(weak_factory_.GetWeakPtr()); > On 2017/03/23 19:59:38, sadrul wrote: > > I am not sure what the WeakPtr<> is useful for (I notice that we use in > > HandleTouchMessage etc. functions too). > > The instance can be destroyed while processing a message. Hence the weakptr. sky@ ping
sky@chromium.org changed reviewers: + sadrul@chromium.org
I tend to think this code should generate a PointerEvent. Sadrul, WDTY? https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1660: if (base::win::GetVersion() <= base::win::VERSION_WIN7) { Should this be a DCHECK? How can we end up here if WM_POINTER isn't supported on win7 or lower? https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1670: CHECK(get_pointer_type); Generally we don't assume we can get the function. Is there a reason this code is different? Seem similar code in this class that uses conditional with return type rather than CHECK. https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1684: CHECK(get_pointer_pen_info); Similar comment about CHECK. https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1695: int tiltX = pointer_pen_info.tiltX; titl_x (and tilt_y below) https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1699: gfx::Point point = gfx::Point(client_point.x, client_point.y); Do you need to convert to dips? https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1720: default: Should WM_POINTERENTER generate ET_MOUSE_ENTERED? https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1723: ui::MouseEvent event(event_type, point, point, base::TimeTicks::Now(), flag, Doesn't the event have a time? Also, how come you are using the same location for both points? One should be screen.
+1 for PointerEvent generation https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1660: if (base::win::GetVersion() <= base::win::VERSION_WIN7) { On 2017/03/27 15:33:58, sky wrote: > Should this be a DCHECK? How can we end up here if WM_POINTER isn't supported on > win7 or lower? Shouldn't be a DCHECK i think. We may have drivers who post these messages?. The WM_POINTER messages AFAIK are generated by the OS only on Windows 8+
On 2017/03/27 15:33:58, sky wrote: > I tend to think this code should generate a PointerEvent. Sadrul, WDTY? That would be ideal. But: (1) views code does not actually know how to deal with a ui::PointerEvent [yet] (2) I don't think we support dispatching a ui::PointerEvent to the renderer (and the compositor thread does not know how to deal with them either). Blink generates its own set of pointer-events, but afaik, that happens internally in blink core, and is different from ui::PointerEvent. Until we resolve these issues, generating a PointerEvent here may not make sense. However, I would love for us to have a plan forward that involves doing the necessary work so that we can generate a ui::PointerEvent here.
On 2017/03/27 21:31:47, sadrul wrote: > On 2017/03/27 15:33:58, sky wrote: > > I tend to think this code should generate a PointerEvent. Sadrul, WDTY? > > That would be ideal. But: (1) views code does not actually know how to deal with > a ui::PointerEvent [yet] (2) I don't think we support dispatching a > ui::PointerEvent to the renderer (and the compositor thread does not know how to > deal with them either). Blink generates its own set of pointer-events, but > afaik, that happens internally in blink core, and is different from > ui::PointerEvent. Until we resolve these issues, generating a PointerEvent here > may not make sense. However, I would love for us to have a plan forward that > involves doing the necessary work so that we can generate a ui::PointerEvent > here. *SIGH* I was afraid you would say that.
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: CQ has no permission to schedule in bucket master.tryserver.chromium.linux
Patchset #11 (id:340001) has been deleted
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: CQ has no permission to schedule in bucket master.tryserver.chromium.linux
https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1670: CHECK(get_pointer_type); On 2017/03/27 15:33:58, sky wrote: > Generally we don't assume we can get the function. Is there a reason this code > is different? Seem similar code in this class that uses conditional with return > type rather than CHECK. Done. https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1684: CHECK(get_pointer_pen_info); On 2017/03/27 15:33:58, sky wrote: > Similar comment about CHECK. Done. https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1695: int tiltX = pointer_pen_info.tiltX; On 2017/03/27 15:33:58, sky wrote: > titl_x (and tilt_y below) Done. https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1696: int tiltY = pointer_pen_info.tiltY; On 2017/03/23 19:59:39, sadrul wrote: > tilt_x, tilt_y Done. https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1699: gfx::Point point = gfx::Point(client_point.x, client_point.y); On 2017/03/27 15:33:58, sky wrote: > Do you need to convert to dips? https://codesearch.chromium.org/chromium/src/ui/events/win/events_win.cc?type... The position is directly used from the value of lParam if it is a client mouse event. I think the conversion happens later. https://codesearch.chromium.org/chromium/src/content/browser/renderer_host/re... https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1720: default: On 2017/03/27 15:33:58, sky wrote: > Should WM_POINTERENTER generate ET_MOUSE_ENTERED? Done. https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1723: ui::MouseEvent event(event_type, point, point, base::TimeTicks::Now(), flag, On 2017/03/27 15:33:58, sky wrote: > Doesn't the event have a time? Also, how come you are using the same location > for both points? One should be screen. https://codesearch.chromium.org/chromium/src/ui/events/event.cc?type=cs&l=482 I saw here location_ and root_location_ are set to the same value. https://codesearch.chromium.org/chromium/src/ui/events/win/events_win.cc?type... We are now using |TimeTicks::Now()| for event timestamp instead of the native timestamp. https://codereview.chromium.org/2648683003/diff/320001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1737: if (ref.get()) On 2017/03/23 19:59:38, sadrul wrote: > if (ref) should work too Done.
LGTM https://codereview.chromium.org/2648683003/diff/360001/ui/views/win/hwnd_mess... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2648683003/diff/360001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1724: break; NOTREACHED?
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by lanwei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ananta@chromium.org, mustaq@chromium.org, dtapuska@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2648683003/#ps380001 (title: "NOTREACHED() when default")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 380001, "attempt_start_ts": 1490815690622480, "parent_rev": "eded34c4a1df9950ea622424f2ee9f76ce852213", "commit_rev": "499bc4f53d3e04ac9e56cd86a2cf7dc723625ef7"}
Message was sent while issue was closed.
Description was changed from ========== Introduce WM_POINTER to Handle pointer events with pen type BUG=367113 ========== to ========== Introduce WM_POINTER to Handle pointer events with pen type BUG=367113 Review-Url: https://codereview.chromium.org/2648683003 Cr-Commit-Position: refs/heads/master@{#460522} Committed: https://chromium.googlesource.com/chromium/src/+/499bc4f53d3e04ac9e56cd86a2cf... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:380001) as https://chromium.googlesource.com/chromium/src/+/499bc4f53d3e04ac9e56cd86a2cf... |