|
|
DescriptionReplacing WM_TOUCH with WM_POINTER for touch events on Wins 8+
Since we already used WM_POINTER for stylus input, and WM_POINTER provides
better API on touch events, we should use WM_POINTER on touch events as well,
replacing the current WM_TOUCH messages for Windows 8(+).
BUG=726766
Review-Url: https://codereview.chromium.org/2904113002
Cr-Commit-Position: refs/heads/master@{#482977}
Committed: https://chromium.googlesource.com/chromium/src/+/4c69348ec2a2721e25ae02b9bc75aff88e38973c
Patch Set 1 : wm touch #
Total comments: 8
Patch Set 2 : wm touch #
Total comments: 2
Patch Set 3 : wm touch #Patch Set 4 : Add a browser test #
Total comments: 32
Patch Set 5 : wm touch #Patch Set 6 : wm touch #
Total comments: 12
Patch Set 7 : wm touch #
Total comments: 16
Patch Set 8 : wm touch #
Total comments: 17
Patch Set 9 : wm touch #
Total comments: 1
Patch Set 10 : wm touch #
Messages
Total messages: 133 (109 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...
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...)
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.
Description was changed from ========== wm touch wm touch wm touch BUG= ========== to ========== Replacing WM_TOUCH with WM_POINTER for touch events on Wins 8+ Since we already used WM_POINTER for stylus input, and WM_POINTER provides better API on touch events, we should use WM_POINTER on touch events as well, replacing the current WM_TOUCH messages for Windows 8(+). BUG=726766 ==========
Patchset #1 (id:1) has been deleted
lanwei@chromium.org changed reviewers: + dtapuska@chromium.org, mustaq@chromium.org
https://codereview.chromium.org/2904113002/diff/20001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2904113002/diff/20001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:1690: if (pointer_type == PT_PEN) { Can't this be a switch? https://codereview.chromium.org/2904113002/diff/20001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2670: int rotation_angle = pointer_touch_info.orientation; Seems really odd that we have a different definition of rotation angle and twist. Isn't orientation just the mapped version of twist? https://codereview.chromium.org/2904113002/diff/20001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.h (left): https://codereview.chromium.org/2904113002/diff/20001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:525: I don't care for these function names. The should definitely indicate that they are Pointer Events. Like: HandlePointerEventTypePen HandlePointerEventTypeTouch
https://codereview.chromium.org/2904113002/diff/20001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2904113002/diff/20001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2670: int rotation_angle = pointer_touch_info.orientation; On 2017/05/26 17:56:45, dtapuska wrote: > Seems really odd that we have a different definition of rotation angle and > twist. Isn't orientation just the mapped version of twist? Navid will likely need to comment. I see that we don't populate pointer events twist from the rotation angle I'm not sure we should or not. Probably best to try on a Windows Phone that has touch... https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/events/Po...
lanwei@chromium.org changed reviewers: + nzolghadr@chromium.org
https://codereview.chromium.org/2904113002/diff/20001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2904113002/diff/20001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2670: int rotation_angle = pointer_touch_info.orientation; On 2017/05/26 18:08:21, dtapuska wrote: > On 2017/05/26 17:56:45, dtapuska wrote: > > Seems really odd that we have a different definition of rotation angle and > > twist. Isn't orientation just the mapped version of twist? > > Navid will likely need to comment. I see that we don't populate pointer events > twist from the rotation angle I'm not sure we should or not. Probably best to > try on a Windows Phone that has touch... > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/events/Po... I can set twist too in ui::PointerDetails, twist and rotation angle have different range.
https://codereview.chromium.org/2904113002/diff/20001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2904113002/diff/20001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2670: int rotation_angle = pointer_touch_info.orientation; On 2017/05/26 19:24:56, lanwei wrote: > On 2017/05/26 18:08:21, dtapuska wrote: > > On 2017/05/26 17:56:45, dtapuska wrote: > > > Seems really odd that we have a different definition of rotation angle and > > > twist. Isn't orientation just the mapped version of twist? > > > > Navid will likely need to comment. I see that we don't populate pointer events > > twist from the rotation angle I'm not sure we should or not. Probably best to > > try on a Windows Phone that has touch... > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/events/Po... > > I can set twist too in ui::PointerDetails, twist and rotation angle have > different range. Good catch Dave. We need to get rid of one of these angles but not through this CL. This is non-trivial to fix it because their different ranges come from different "base vectors" for angle measurement: - ui::TouchEvent::rotation_angle_ follows the MotionEvent spec. - ui::PointerDetails::twist follows the PointerEvent spec. - And none of them follows the TouchEvent spec, at least as far as I can recall from the comments. We should move all internal angle plumbing to the PointerEvent spec since this is the most expressive one. I will file a bug now. Lan, please add a TODO in ui::TouchEvent::rotation_angle_ with the bug number.
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.
lanwei@chromium.org changed reviewers: + ananta@chromium.org, sky@chromium.org
https://codereview.chromium.org/2904113002/diff/20001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2904113002/diff/20001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:1690: if (pointer_type == PT_PEN) { On 2017/05/26 17:56:45, dtapuska wrote: > Can't this be a switch? Done. https://codereview.chromium.org/2904113002/diff/20001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.h (left): https://codereview.chromium.org/2904113002/diff/20001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:525: On 2017/05/26 17:56:45, dtapuska wrote: > I don't care for these function names. The should definitely indicate that they > are Pointer Events. > > Like: > HandlePointerEventTypePen > HandlePointerEventTypeTouch Done.
On 2017/05/29 18:29:43, lanwei wrote: > https://codereview.chromium.org/2904113002/diff/20001/ui/views/win/hwnd_messa... > File ui/views/win/hwnd_message_handler.cc (right): > > https://codereview.chromium.org/2904113002/diff/20001/ui/views/win/hwnd_messa... > ui/views/win/hwnd_message_handler.cc:1690: if (pointer_type == PT_PEN) { > On 2017/05/26 17:56:45, dtapuska wrote: > > Can't this be a switch? > > Done. > > https://codereview.chromium.org/2904113002/diff/20001/ui/views/win/hwnd_messa... > File ui/views/win/hwnd_message_handler.h (left): > > https://codereview.chromium.org/2904113002/diff/20001/ui/views/win/hwnd_messa... > ui/views/win/hwnd_message_handler.h:525: > On 2017/05/26 17:56:45, dtapuska wrote: > > I don't care for these function names. The should definitely indicate that > they > > are Pointer Events. > > > > Like: > > HandlePointerEventTypePen > > HandlePointerEventTypeTouch > > Done. lgtm
lgtm https://codereview.chromium.org/2904113002/diff/40001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2904113002/diff/40001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:1684: // touch and mouse inputs. nit: Do we need to update this comment?
lgtm https://codereview.chromium.org/2904113002/diff/40001/ui/events/event.h File ui/events/event.h (right): https://codereview.chromium.org/2904113002/diff/40001/ui/events/event.h#newco... ui/events/event.h:720: // TODO(726824): Remove rotation_angle_ from ui::TouchEvent, just use twist Nit: TODO(owner): ... crbug.com/###.
How about some test coverage? We have tests that run on win10. Is it possible to generate touch/pointer events at the native level and verify we are mapping them correctly? I'm thinking of something like what we do for mouse events in https://chromium.googlesource.com/chromium/src/+/master/ui/base/test/ui_contr... .
On 2017/05/30 16:40:24, sky wrote: > How about some test coverage? We have tests that run on win10. Is it possible to > generate touch/pointer events at the native level and verify we are mapping them > correctly? I'm thinking of something like what we do for mouse events in > https://chromium.googlesource.com/chromium/src/+/master/ui/base/test/ui_contr... > . lanwei@ you can use... https://msdn.microsoft.com/en-us/library/windows/desktop/hh802881(v=vs.85).aspx (Mouse is using SendInput and this API is analogous to that you just need to follow the similar model)
Patchset #3 (id:60001) 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: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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_...)
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 #4 (id:100001) has been deleted
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...)
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 #5 (id:140001) has been deleted
Patchset #4 (id:120001) has been deleted
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_...)
https://codereview.chromium.org/2904113002/diff/160001/chrome/browser/touchev... File chrome/browser/touchevents_browsertest.cc (right): https://codereview.chromium.org/2904113002/diff/160001/chrome/browser/touchev... chrome/browser/touchevents_browsertest.cc:52: DISALLOW_COPY_AND_ASSIGN(TouchEventsTest); private: https://codereview.chromium.org/2904113002/diff/160001/chrome/browser/touchev... chrome/browser/touchevents_browsertest.cc:55: #if defined(OS_WIN) Make it so this file is only built for windows. https://codereview.chromium.org/2904113002/diff/160001/ui/base/test/ui_contro... File ui/base/test/ui_controls_aura.h (right): https://codereview.chromium.org/2904113002/diff/160001/ui/base/test/ui_contro... ui/base/test/ui_controls_aura.h:50: virtual bool SendTouchEvents(int action, int num, long x, long y) = 0; Document what action and num are. Also, I would be inclined to use an int for long. I realize we use long else where, but I don't think there is a good reason for that. https://codereview.chromium.org/2904113002/diff/160001/ui/base/test/ui_contro... File ui/base/test/ui_controls_internal_win.cc (right): https://codereview.chromium.org/2904113002/diff/160001/ui/base/test/ui_contro... ui/base/test/ui_controls_internal_win.cc:342: POINTER_TOUCH_INFO contact_[3]; trailing '_' is only for members. https://codereview.chromium.org/2904113002/diff/160001/ui/base/test/ui_contro... ui/base/test/ui_controls_internal_win.cc:342: POINTER_TOUCH_INFO contact_[3]; Where does the 3 come from? https://codereview.chromium.org/2904113002/diff/160001/ui/base/test/ui_contro... ui/base/test/ui_controls_internal_win.cc:343: for (int i = 0; i < num; i++) { It's not at all clear to me what this code is doing, why you have 3 and num. Please add comments. https://codereview.chromium.org/2904113002/diff/160001/ui/base/test/ui_contro... ui/base/test/ui_controls_internal_win.cc:368: Sleep(20); Why do you sleep between injecting events? https://codereview.chromium.org/2904113002/diff/160001/ui/views/win/hwnd_mess... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2904113002/diff/160001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:2642: UINT32 pointer_id = GET_POINTERID_WPARAM(w_param); GET_POINTERID_WPARAM is only available on >= win 8. https://codereview.chromium.org/2904113002/diff/160001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:2658: gfx::Point touch_point = gfx::Point(client_point.x, client_point.y); Does this need to be converted for dips? https://codereview.chromium.org/2904113002/diff/160001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:2662: const base::TimeTicks event_time = base::TimeTicks::Now(); Can't you use the time from the event? https://codereview.chromium.org/2904113002/diff/160001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:2664: float pressure = static_cast<float>(pointer_touch_info.pressure) / 1024; Where does the 1024 come from? https://codereview.chromium.org/2904113002/diff/160001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:2721: float pressure = static_cast<float>(pointer_pen_info.pressure) / 1024; Same question about 1024. https://codereview.chromium.org/2904113002/diff/160001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:2727: gfx::Point point = gfx::Point(client_point.x, client_point.y); And coordinate conversion.
https://codereview.chromium.org/2904113002/diff/160001/ui/base/test/ui_contro... File ui/base/test/ui_controls_internal_win.cc (right): https://codereview.chromium.org/2904113002/diff/160001/ui/base/test/ui_contro... ui/base/test/ui_controls_internal_win.cc:367: InjectTouchInput(num, contact_); // Injecting the touch down on screen The reason the tests are failing on Windows 7 is a direct call to this function which exists on Windows 8+. A GetProcAddress based approach would work here. Alternatively delay load user32 may work. Dunno if that is feasible for this case.
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:220001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
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_...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
Patchset #5 (id:180001) has been deleted
Patchset #5 (id:200001) 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: 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_...)
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_...)
Patchset #5 (id:240001) 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 checked by lanwei@chromium.org to run a CQ dry run
Patchset #6 (id:280001) 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: This issue passed the CQ dry run.
Patchset #5 (id:260001) 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: 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_...)
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: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: 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_...)
Patchset #6 (id:320001) has been deleted
Patchset #6 (id:360001) 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: 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...
Patchset #7 (id:400001) has been deleted
sky@ could you please take a look at the tests? https://codereview.chromium.org/2904113002/diff/160001/chrome/browser/touchev... File chrome/browser/touchevents_browsertest.cc (right): https://codereview.chromium.org/2904113002/diff/160001/chrome/browser/touchev... chrome/browser/touchevents_browsertest.cc:52: DISALLOW_COPY_AND_ASSIGN(TouchEventsTest); On 2017/06/07 19:42:45, sky wrote: > private: Done. https://codereview.chromium.org/2904113002/diff/160001/chrome/browser/touchev... chrome/browser/touchevents_browsertest.cc:55: #if defined(OS_WIN) On 2017/06/07 19:42:45, sky wrote: > Make it so this file is only built for windows. Yes, because we only test WM_POINTER on Windows. https://codereview.chromium.org/2904113002/diff/160001/ui/base/test/ui_contro... File ui/base/test/ui_controls_aura.h (right): https://codereview.chromium.org/2904113002/diff/160001/ui/base/test/ui_contro... ui/base/test/ui_controls_aura.h:50: virtual bool SendTouchEvents(int action, int num, long x, long y) = 0; On 2017/06/07 19:42:45, sky wrote: > Document what action and num are. Also, I would be inclined to use an int for > long. I realize we use long else where, but I don't think there is a good reason > for that. Done. https://codereview.chromium.org/2904113002/diff/160001/ui/base/test/ui_contro... File ui/base/test/ui_controls_internal_win.cc (right): https://codereview.chromium.org/2904113002/diff/160001/ui/base/test/ui_contro... ui/base/test/ui_controls_internal_win.cc:342: POINTER_TOUCH_INFO contact_[3]; On 2017/06/07 19:42:45, sky wrote: > Where does the 3 come from? I removed it. https://codereview.chromium.org/2904113002/diff/160001/ui/base/test/ui_contro... ui/base/test/ui_controls_internal_win.cc:342: POINTER_TOUCH_INFO contact_[3]; On 2017/06/07 19:42:45, sky wrote: > trailing '_' is only for members. Done. https://codereview.chromium.org/2904113002/diff/160001/ui/base/test/ui_contro... ui/base/test/ui_controls_internal_win.cc:367: InjectTouchInput(num, contact_); // Injecting the touch down on screen On 2017/06/09 03:13:12, ananta wrote: > The reason the tests are failing on Windows 7 is a direct call to this function > which exists on Windows 8+. A GetProcAddress based approach would work here. > Alternatively delay load user32 may work. Dunno if that is feasible for this > case. Done. https://codereview.chromium.org/2904113002/diff/160001/ui/base/test/ui_contro... ui/base/test/ui_controls_internal_win.cc:368: Sleep(20); On 2017/06/07 19:42:45, sky wrote: > Why do you sleep between injecting events? Because the test is flaky, I want to make sure we have enough time to receive the events from InjectTouchInput. https://codereview.chromium.org/2904113002/diff/160001/ui/views/win/hwnd_mess... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2904113002/diff/160001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:2642: UINT32 pointer_id = GET_POINTERID_WPARAM(w_param); On 2017/06/07 19:42:45, sky wrote: > GET_POINTERID_WPARAM is only available on >= win 8. In HWNDMessageHandler::OnPointerEvent, I checked the windows version. https://codereview.chromium.org/2904113002/diff/160001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:2658: gfx::Point touch_point = gfx::Point(client_point.x, client_point.y); On 2017/06/07 19:42:45, sky wrote: > Does this need to be converted for dips? No. https://codereview.chromium.org/2904113002/diff/160001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:2662: const base::TimeTicks event_time = base::TimeTicks::Now(); On 2017/06/07 19:42:45, sky wrote: > Can't you use the time from the event? What event? We do not have time information from windows message. https://codereview.chromium.org/2904113002/diff/160001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:2664: float pressure = static_cast<float>(pointer_touch_info.pressure) / 1024; On 2017/06/07 19:42:45, sky wrote: > Where does the 1024 come from? A pressure is normalized to a range between 0 and 1024. https://codereview.chromium.org/2904113002/diff/160001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:2721: float pressure = static_cast<float>(pointer_pen_info.pressure) / 1024; On 2017/06/07 19:42:45, sky wrote: > Same question about 1024. Done. https://codereview.chromium.org/2904113002/diff/160001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:2727: gfx::Point point = gfx::Point(client_point.x, client_point.y); On 2017/06/07 19:42:45, sky wrote: > And coordinate conversion. We do not need coordinate conversion.
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/2904113002/diff/160001/chrome/browser/touchev... File chrome/browser/touchevents_browsertest.cc (right): https://codereview.chromium.org/2904113002/diff/160001/chrome/browser/touchev... chrome/browser/touchevents_browsertest.cc:55: #if defined(OS_WIN) On 2017/06/22 18:38:45, lanwei wrote: > On 2017/06/07 19:42:45, sky wrote: > > Make it so this file is only built for windows. > > Yes, because we only test WM_POINTER on Windows. Sorry if it I wasn't clear. The one test you have in this file is only run on windows (because of the ifdef). Instead of having the ifdef you should make it so the file is only compiled and run on windows. That way no ifdef is needed, and other platforms aren't compiling a file they won't run . https://codereview.chromium.org/2904113002/diff/160001/ui/base/test/ui_contro... File ui/base/test/ui_controls_internal_win.cc (right): https://codereview.chromium.org/2904113002/diff/160001/ui/base/test/ui_contro... ui/base/test/ui_controls_internal_win.cc:368: Sleep(20); On 2017/06/22 18:38:45, lanwei wrote: > On 2017/06/07 19:42:45, sky wrote: > > Why do you sleep between injecting events? > > Because the test is flaky, I want to make sure we have enough time to receive > the events from InjectTouchInput. The problem with random sleeps is that they generally mean something else is wrong. To add random sleeps generally means your test is going to be flakey. Remember that the bots have varying levels of load. https://codereview.chromium.org/2904113002/diff/160001/ui/views/win/hwnd_mess... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2904113002/diff/160001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:2662: const base::TimeTicks event_time = base::TimeTicks::Now(); On 2017/06/22 18:38:45, lanwei wrote: > On 2017/06/07 19:42:45, sky wrote: > > Can't you use the time from the event? > > What event? We do not have time information from windows message. Sorry, I believe you want EventTimeForNow(). https://codereview.chromium.org/2904113002/diff/420001/chrome/browser/touchev... File chrome/browser/touchevents_browsertest.cc (right): https://codereview.chromium.org/2904113002/diff/420001/chrome/browser/touchev... chrome/browser/touchevents_browsertest.cc:57: IN_PROC_BROWSER_TEST_F(TouchEventsTest, TestNativeTouchEvents) { Why is this test necessary? The test you are writing verifying the events are generated is all you need. https://codereview.chromium.org/2904113002/diff/420001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc (right): https://codereview.chromium.org/2904113002/diff/420001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc:2433: class BookmarkBarViewTest28 : public BookmarkBarViewEventTestBase { This test has *nothing* to do with bookmark bars, it should be in a file closer to the changes. Ideally it would be in hwnd_message_handler_interactive_uitest, but because you are using ViewEventTestBase that isn't possible. How about putting this in chrome/browser/ui/views/touch_events_interactive_uitest_win.cc. https://codereview.chromium.org/2904113002/diff/420001/ui/base/test/ui_contro... File ui/base/test/ui_controls.h (right): https://codereview.chromium.org/2904113002/diff/420001/ui/base/test/ui_contro... ui/base/test/ui_controls.h:83: enum TouchType { PRESS = 1, RELEASE = 1 << 2, MOVE = 1 << 3 }; Use enum class. https://codereview.chromium.org/2904113002/diff/420001/ui/base/test/ui_contro... ui/base/test/ui_controls.h:96: bool SendTouchEvents(int action, int num, int x, int y); Document what action and num is. https://codereview.chromium.org/2904113002/diff/420001/ui/base/test/ui_contro... ui/base/test/ui_controls.h:102: const base::Closure& task); There is no point in this variant given you can not verify the touch events have been injected. You should clearly describe that above in SendTouchEvents() and remove this one.
Patchset #6 (id:380001) has been deleted
Patchset #8 (id:460001) has been deleted
Patchset #7 (id:440001) 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...
https://codereview.chromium.org/2904113002/diff/160001/ui/base/test/ui_contro... File ui/base/test/ui_controls_internal_win.cc (right): https://codereview.chromium.org/2904113002/diff/160001/ui/base/test/ui_contro... ui/base/test/ui_controls_internal_win.cc:368: Sleep(20); On 2017/06/22 20:54:21, sky wrote: > On 2017/06/22 18:38:45, lanwei wrote: > > On 2017/06/07 19:42:45, sky wrote: > > > Why do you sleep between injecting events? > > > > Because the test is flaky, I want to make sure we have enough time to receive > > the events from InjectTouchInput. > > The problem with random sleeps is that they generally mean something else is > wrong. To add random sleeps generally means your test is going to be flakey. > Remember that the bots have varying levels of load. I removed them. https://codereview.chromium.org/2904113002/diff/160001/ui/views/win/hwnd_mess... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2904113002/diff/160001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:2662: const base::TimeTicks event_time = base::TimeTicks::Now(); On 2017/06/22 20:54:21, sky wrote: > On 2017/06/22 18:38:45, lanwei wrote: > > On 2017/06/07 19:42:45, sky wrote: > > > Can't you use the time from the event? > > > > What event? We do not have time information from windows message. > > Sorry, I believe you want EventTimeForNow(). Done. https://codereview.chromium.org/2904113002/diff/420001/chrome/browser/touchev... File chrome/browser/touchevents_browsertest.cc (right): https://codereview.chromium.org/2904113002/diff/420001/chrome/browser/touchev... chrome/browser/touchevents_browsertest.cc:57: IN_PROC_BROWSER_TEST_F(TouchEventsTest, TestNativeTouchEvents) { On 2017/06/22 20:54:21, sky wrote: > Why is this test necessary? The test you are writing verifying the events are > generated is all you need. I removed it, just keep the touch_events_interactive_ui_test. https://codereview.chromium.org/2904113002/diff/420001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc (right): https://codereview.chromium.org/2904113002/diff/420001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc:2433: class BookmarkBarViewTest28 : public BookmarkBarViewEventTestBase { On 2017/06/22 20:54:21, sky wrote: > This test has *nothing* to do with bookmark bars, it should be in a file closer > to the changes. Ideally it would be in hwnd_message_handler_interactive_uitest, > but because you are using ViewEventTestBase that isn't possible. How about > putting this in chrome/browser/ui/views/touch_events_interactive_uitest_win.cc. Done. https://codereview.chromium.org/2904113002/diff/420001/ui/base/test/ui_contro... File ui/base/test/ui_controls.h (right): https://codereview.chromium.org/2904113002/diff/420001/ui/base/test/ui_contro... ui/base/test/ui_controls.h:83: enum TouchType { PRESS = 1, RELEASE = 1 << 2, MOVE = 1 << 3 }; On 2017/06/22 20:54:22, sky wrote: > Use enum class. I need to use bit-wise operator, &, |, so I cannot use enum class. https://codereview.chromium.org/2904113002/diff/420001/ui/base/test/ui_contro... ui/base/test/ui_controls.h:96: bool SendTouchEvents(int action, int num, int x, int y); On 2017/06/22 20:54:22, sky wrote: > Document what action and num is. Done. https://codereview.chromium.org/2904113002/diff/420001/ui/base/test/ui_contro... ui/base/test/ui_controls.h:102: const base::Closure& task); On 2017/06/22 20:54:22, sky wrote: > There is no point in this variant given you can not verify the touch events have > been injected. You should clearly describe that above in SendTouchEvents() and > remove this one. I removed the function SendTouchEvents, I need a task to keep the callback function.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for your patience. I think you are close. https://codereview.chromium.org/2904113002/diff/420001/ui/base/test/ui_contro... File ui/base/test/ui_controls.h (right): https://codereview.chromium.org/2904113002/diff/420001/ui/base/test/ui_contro... ui/base/test/ui_controls.h:102: const base::Closure& task); On 2017/06/23 20:06:05, lanwei wrote: > On 2017/06/22 20:54:22, sky wrote: > > There is no point in this variant given you can not verify the touch events > have > > been injected. You should clearly describe that above in SendTouchEvents() and > > remove this one. > > I removed the function SendTouchEvents, I need a task to keep the callback > function. Why do you need a task? The reason the other variants take a callback is because the callback is run asynchronously. The function you are adding executes *synchronously*, because of this there is no reason for this new function to take a callback. The one place you call this from you have: ui_controls::SendTouchEventsNotifyWhenDone( ui_controls::PRESS, 3, in_content.x(), in_content.y(), CreateEventTask(this, &TouchEventsViewTest::Step2)); This is the same thing as: ui_controls::SendTouchEventsNotifyWhenDone( ui_controls::PRESS, 3, in_content.x(), in_content.y()); Step2(); That said, the reason the other tests have multiple functions is because they are using async API. As your function is synchronous, you don't need Step2(). You can put everything in DoTestOnMessageLoop(). When you remove the callback argument, remove 'NotifyWhenDone' from the name as it doesn't apply. And again, clearly document that there is no way to detect when events enter chrome, it's up to users of this API to detect when events arrive. https://codereview.chromium.org/2904113002/diff/480001/chrome/browser/ui/view... File chrome/browser/ui/views/touch_events_interactive_uitest_win.cc (right): https://codereview.chromium.org/2904113002/diff/480001/chrome/browser/ui/view... chrome/browser/ui/views/touch_events_interactive_uitest_win.cc:43: num_touches_--; num_touches_ is really the number of pointers currently down, right? How about num_pointers_down_? https://codereview.chromium.org/2904113002/diff/480001/chrome/browser/ui/view... chrome/browser/ui/views/touch_events_interactive_uitest_win.cc:59: class TouchView : public views::View { You don't need a View subclass here, create View() directly. https://codereview.chromium.org/2904113002/diff/480001/chrome/browser/ui/view... chrome/browser/ui/views/touch_events_interactive_uitest_win.cc:85: return touch_view_->GetPreferredSize(); This returns 0. I think you want to return gfx::Size(600, 600) here and not set the bounds of the touch_view above. https://codereview.chromium.org/2904113002/diff/480001/chrome/browser/ui/view... chrome/browser/ui/views/touch_events_interactive_uitest_win.cc:94: ui_controls::SendTouchEventsNotifyWhenDone( EXPECT_TRUE. https://codereview.chromium.org/2904113002/diff/480001/chrome/browser/ui/view... chrome/browser/ui/views/touch_events_interactive_uitest_win.cc:101: if (base::win::GetVersion() <= base::win::VERSION_WIN7) { This test should be done first in DoTestOnMessageLoop(). https://codereview.chromium.org/2904113002/diff/480001/chrome/browser/ui/view... chrome/browser/ui/views/touch_events_interactive_uitest_win.cc:107: ASSERT_EQ(3, touch_event_handler_.num_touch_presses()); This is the same as the 3 on 95, right? Use a constant for it. https://codereview.chromium.org/2904113002/diff/480001/chrome/browser/ui/view... chrome/browser/ui/views/touch_events_interactive_uitest_win.cc:113: TouchEventHandler touch_event_handler_; When you end up with a single function, make this a local variable of the function. https://codereview.chromium.org/2904113002/diff/480001/ui/base/test/ui_contro... File ui/base/test/ui_controls.h (right): https://codereview.chromium.org/2904113002/diff/480001/ui/base/test/ui_contro... ui/base/test/ui_controls.h:96: // Send WM_POINTER messages to generate touch events. |action| means the type |action| is a bitmask of the TouchType constants that indicate what events are 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...
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:500001) 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/2904113002/diff/420001/ui/base/test/ui_contro... File ui/base/test/ui_controls.h (right): https://codereview.chromium.org/2904113002/diff/420001/ui/base/test/ui_contro... ui/base/test/ui_controls.h:102: const base::Closure& task); On 2017/06/24 00:01:14, sky wrote: > On 2017/06/23 20:06:05, lanwei wrote: > > On 2017/06/22 20:54:22, sky wrote: > > > There is no point in this variant given you can not verify the touch events > > have > > > been injected. You should clearly describe that above in SendTouchEvents() > and > > > remove this one. > > > > I removed the function SendTouchEvents, I need a task to keep the callback > > function. > > Why do you need a task? The reason the other variants take a callback is because > the callback is run asynchronously. The function you are adding executes > *synchronously*, because of this there is no reason for this new function to > take a callback. The one place you call this from you have: > > ui_controls::SendTouchEventsNotifyWhenDone( > ui_controls::PRESS, 3, in_content.x(), in_content.y(), > CreateEventTask(this, &TouchEventsViewTest::Step2)); > > This is the same thing as: > > ui_controls::SendTouchEventsNotifyWhenDone( > ui_controls::PRESS, 3, in_content.x(), in_content.y()); > Step2(); > > That said, the reason the other tests have multiple functions is because they > are using async API. As your function is synchronous, you don't need Step2(). > You can put everything in DoTestOnMessageLoop(). > > When you remove the callback argument, remove 'NotifyWhenDone' from the name as > it doesn't apply. > > And again, clearly document that there is no way to detect when events enter > chrome, it's up to users of this API to detect when events arrive. Acknowledged. https://codereview.chromium.org/2904113002/diff/480001/chrome/browser/ui/view... File chrome/browser/ui/views/touch_events_interactive_uitest_win.cc (right): https://codereview.chromium.org/2904113002/diff/480001/chrome/browser/ui/view... chrome/browser/ui/views/touch_events_interactive_uitest_win.cc:43: num_touches_--; On 2017/06/24 00:01:14, sky wrote: > num_touches_ is really the number of pointers currently down, right? How about > num_pointers_down_? Done. https://codereview.chromium.org/2904113002/diff/480001/chrome/browser/ui/view... chrome/browser/ui/views/touch_events_interactive_uitest_win.cc:59: class TouchView : public views::View { On 2017/06/24 00:01:14, sky wrote: > You don't need a View subclass here, create View() directly. Done. https://codereview.chromium.org/2904113002/diff/480001/chrome/browser/ui/view... chrome/browser/ui/views/touch_events_interactive_uitest_win.cc:85: return touch_view_->GetPreferredSize(); On 2017/06/24 00:01:14, sky wrote: > This returns 0. I think you want to return gfx::Size(600, 600) here and not set > the bounds of the touch_view above. Acknowledged. https://codereview.chromium.org/2904113002/diff/480001/chrome/browser/ui/view... chrome/browser/ui/views/touch_events_interactive_uitest_win.cc:94: ui_controls::SendTouchEventsNotifyWhenDone( On 2017/06/24 00:01:14, sky wrote: > EXPECT_TRUE. Done. https://codereview.chromium.org/2904113002/diff/480001/chrome/browser/ui/view... chrome/browser/ui/views/touch_events_interactive_uitest_win.cc:101: if (base::win::GetVersion() <= base::win::VERSION_WIN7) { On 2017/06/24 00:01:14, sky wrote: > This test should be done first in DoTestOnMessageLoop(). Done. https://codereview.chromium.org/2904113002/diff/480001/chrome/browser/ui/view... chrome/browser/ui/views/touch_events_interactive_uitest_win.cc:107: ASSERT_EQ(3, touch_event_handler_.num_touch_presses()); On 2017/06/24 00:01:14, sky wrote: > This is the same as the 3 on 95, right? Use a constant for it. Done. https://codereview.chromium.org/2904113002/diff/480001/chrome/browser/ui/view... chrome/browser/ui/views/touch_events_interactive_uitest_win.cc:113: TouchEventHandler touch_event_handler_; On 2017/06/24 00:01:14, sky wrote: > When you end up with a single function, make this a local variable of the > function. Done. https://codereview.chromium.org/2904113002/diff/480001/ui/base/test/ui_contro... File ui/base/test/ui_controls.h (right): https://codereview.chromium.org/2904113002/diff/480001/ui/base/test/ui_contro... ui/base/test/ui_controls.h:96: // Send WM_POINTER messages to generate touch events. |action| means the type On 2017/06/24 00:01:14, sky wrote: > |action| is a bitmask of the TouchType constants that indicate what events are > generated. Done.
https://codereview.chromium.org/2904113002/diff/520001/chrome/browser/ui/view... File chrome/browser/ui/views/touch_events_interactive_uitest_win.cc (right): https://codereview.chromium.org/2904113002/diff/520001/chrome/browser/ui/view... chrome/browser/ui/views/touch_events_interactive_uitest_win.cc:62: TouchEventsViewTest() : ViewEventTestBase(), touch_view_(NULL) {} NULL -> nullptr https://codereview.chromium.org/2904113002/diff/520001/chrome/browser/ui/view... chrome/browser/ui/views/touch_events_interactive_uitest_win.cc:71: touch_view_ = NULL; NULL -> nullptr https://codereview.chromium.org/2904113002/diff/520001/chrome/browser/ui/view... chrome/browser/ui/views/touch_events_interactive_uitest_win.cc:82: if (base::win::GetVersion() <= base::win::VERSION_WIN7) { Please add comment as to why this is only available on > win7. https://codereview.chromium.org/2904113002/diff/520001/chrome/browser/ui/view... chrome/browser/ui/views/touch_events_interactive_uitest_win.cc:93: EXPECT_TRUE(ui_controls::SendTouchEvents(ui_controls::PRESS, There is no point in continuing the test if this fails (it's just going to hang). So, ASSERT_TRUE here. https://codereview.chromium.org/2904113002/diff/520001/chrome/browser/ui/view... chrome/browser/ui/views/touch_events_interactive_uitest_win.cc:107: const int touch_pointer_count_ = 3; Make this a local in DodTestOnMessageLoop. https://codereview.chromium.org/2904113002/diff/520001/ui/base/test/ui_contro... File ui/base/test/ui_controls.h (right): https://codereview.chromium.org/2904113002/diff/520001/ui/base/test/ui_contro... ui/base/test/ui_controls.h:100: // pointers, |x| and |y| are the coordinates of the touch pointers. in screen coordinates, right? https://codereview.chromium.org/2904113002/diff/520001/ui/base/test/ui_contro... ui/base/test/ui_controls.h:101: bool SendTouchEvents(int action, int num, int x, int y); I believe you've only implemented this for windows, which is fine. Please put this in an ifdef windows to make that obvious. https://codereview.chromium.org/2904113002/diff/520001/ui/views/win/hwnd_mess... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2904113002/diff/520001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:2662: gfx::Point touch_point = gfx::Point(client_point.x, client_point.y); These coordinates are pixels, but views expects DIPs. How are these converted to dips?
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...
https://codereview.chromium.org/2904113002/diff/520001/chrome/browser/ui/view... File chrome/browser/ui/views/touch_events_interactive_uitest_win.cc (right): https://codereview.chromium.org/2904113002/diff/520001/chrome/browser/ui/view... chrome/browser/ui/views/touch_events_interactive_uitest_win.cc:62: TouchEventsViewTest() : ViewEventTestBase(), touch_view_(NULL) {} On 2017/06/27 02:19:47, sky wrote: > NULL -> nullptr Done. https://codereview.chromium.org/2904113002/diff/520001/chrome/browser/ui/view... chrome/browser/ui/views/touch_events_interactive_uitest_win.cc:71: touch_view_ = NULL; On 2017/06/27 02:19:47, sky wrote: > NULL -> nullptr Done. https://codereview.chromium.org/2904113002/diff/520001/chrome/browser/ui/view... chrome/browser/ui/views/touch_events_interactive_uitest_win.cc:82: if (base::win::GetVersion() <= base::win::VERSION_WIN7) { On 2017/06/27 02:19:47, sky wrote: > Please add comment as to why this is only available on > win7. Done. https://codereview.chromium.org/2904113002/diff/520001/chrome/browser/ui/view... chrome/browser/ui/views/touch_events_interactive_uitest_win.cc:93: EXPECT_TRUE(ui_controls::SendTouchEvents(ui_controls::PRESS, On 2017/06/27 02:19:47, sky wrote: > There is no point in continuing the test if this fails (it's just going to > hang). So, ASSERT_TRUE here. Done. https://codereview.chromium.org/2904113002/diff/520001/chrome/browser/ui/view... chrome/browser/ui/views/touch_events_interactive_uitest_win.cc:107: const int touch_pointer_count_ = 3; On 2017/06/27 02:19:47, sky wrote: > Make this a local in DodTestOnMessageLoop. Done. https://codereview.chromium.org/2904113002/diff/520001/ui/base/test/ui_contro... File ui/base/test/ui_controls.h (right): https://codereview.chromium.org/2904113002/diff/520001/ui/base/test/ui_contro... ui/base/test/ui_controls.h:100: // pointers, |x| and |y| are the coordinates of the touch pointers. On 2017/06/27 02:19:47, sky wrote: > in screen coordinates, right? Yes, they are in screen coordinates. https://codereview.chromium.org/2904113002/diff/520001/ui/base/test/ui_contro... ui/base/test/ui_controls.h:101: bool SendTouchEvents(int action, int num, int x, int y); On 2017/06/27 02:19:47, sky wrote: > I believe you've only implemented this for windows, which is fine. Please put > this in an ifdef windows to make that obvious. Done. https://codereview.chromium.org/2904113002/diff/520001/ui/views/win/hwnd_mess... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2904113002/diff/520001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:2662: gfx::Point touch_point = gfx::Point(client_point.x, client_point.y); On 2017/06/27 02:19:48, sky wrote: > These coordinates are pixels, but views expects DIPs. How are these converted to > dips? I did not see any conversion to DIPs in HWNDMessageHandler::OnTouchEvent, I searched that the ConvertPointToDIP is in RenderWidgetHostViewAura: https://codesearch.chromium.org/chromium/src/content/browser/renderer_host/re...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2904113002/diff/520001/ui/views/win/hwnd_mess... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2904113002/diff/520001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:2662: gfx::Point touch_point = gfx::Point(client_point.x, client_point.y); On 2017/06/27 18:55:42, lanwei wrote: > On 2017/06/27 02:19:48, sky wrote: > > These coordinates are pixels, but views expects DIPs. How are these converted > to > > dips? > > I did not see any conversion to DIPs in HWNDMessageHandler::OnTouchEvent, I > searched that the ConvertPointToDIP is in RenderWidgetHostViewAura: > https://codesearch.chromium.org/chromium/src/content/browser/renderer_host/re... I think it's actually done in WindowEventDispatcher. https://codereview.chromium.org/2904113002/diff/540001/chrome/browser/ui/view... File chrome/browser/ui/views/touch_events_interactive_uitest_win.cc (right): https://codereview.chromium.org/2904113002/diff/540001/chrome/browser/ui/view... chrome/browser/ui/views/touch_events_interactive_uitest_win.cc:100: ASSERT_EQ(touch_pointer_count, touch_event_handler.num_touch_presses()); These two can be EXPECTS (use ASSERT when there is no point to continue execution if there is a failure, or if continued execution would crash).
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
The patchset sent to the CQ was uploaded after l-g-t-m from nzolghadr@chromium.org, mustaq@chromium.org, dtapuska@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2904113002/#ps560001 (title: "wm touch")
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": 560001, "attempt_start_ts": 1498659271726750, "parent_rev": "a4f88bdad6d5b47e1f9f022ecda09b0909af6852", "commit_rev": "4c69348ec2a2721e25ae02b9bc75aff88e38973c"}
Message was sent while issue was closed.
Description was changed from ========== Replacing WM_TOUCH with WM_POINTER for touch events on Wins 8+ Since we already used WM_POINTER for stylus input, and WM_POINTER provides better API on touch events, we should use WM_POINTER on touch events as well, replacing the current WM_TOUCH messages for Windows 8(+). BUG=726766 ========== to ========== Replacing WM_TOUCH with WM_POINTER for touch events on Wins 8+ Since we already used WM_POINTER for stylus input, and WM_POINTER provides better API on touch events, we should use WM_POINTER on touch events as well, replacing the current WM_TOUCH messages for Windows 8(+). BUG=726766 Review-Url: https://codereview.chromium.org/2904113002 Cr-Commit-Position: refs/heads/master@{#482977} Committed: https://chromium.googlesource.com/chromium/src/+/4c69348ec2a2721e25ae02b9bc75... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:560001) as https://chromium.googlesource.com/chromium/src/+/4c69348ec2a2721e25ae02b9bc75... |