Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
988473002:
Correctly release the touch events on Lenovo Horizon device (Closed)
DescriptionCorrectly release the touch events on Lenovo Horizon device.
The Lenovo Horizon device displays the wrong number of the touch pointers when all fingers released from the screen. Because HWND Message sometimes sends inconsistent number of touch pointers, we keep the IDs of touch pointers we have seen from last time, and release the ones which are not in the current message.
BUG=316085
Committed: https://crrev.com/a2a44ba54fe134526dc81014991f951c1fb09db1
Cr-Commit-Position: refs/heads/master@{#329770}
Patch Set 1 : #
Total comments: 14
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #
Total comments: 10
Patch Set 5 : #Patch Set 6 : #
Total comments: 10
Patch Set 7 : #
Total comments: 10
Patch Set 8 : #
Total comments: 22
Patch Set 9 : #
Total comments: 13
Patch Set 10 : #
Total comments: 4
Patch Set 11 : #
Total comments: 22
Patch Set 12 : #
Total comments: 13
Patch Set 13 : #
Total comments: 12
Patch Set 14 : #
Total comments: 10
Patch Set 15 : #
Total comments: 2
Patch Set 16 : #
Total comments: 1
Patch Set 17 : Delete adding missing touchpress #Patch Set 18 : #
Messages
Total messages: 105 (53 generated)
lanwei@chromium.org changed reviewers: + sadrul@chromium.org, tdresser@chromium.org
Patchset #1 (id:1) has been deleted
Patchset #1 (id:1) has been deleted
lanwei@chromium.org changed reviewers: - sadrul@chromium.org
lanwei@chromium.org changed reviewers: + sadrul@chromium.org
https://codereview.chromium.org/988473002/diff/20001/ui/views/win/hwnd_messag... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/20001/ui/views/win/hwnd_messag... ui/views/win/hwnd_message_handler.cc:2508: touch_id_list_[event.touch_id()] = true; Add a comment explaining why we do this. https://codereview.chromium.org/988473002/diff/20001/ui/views/win/hwnd_messag... ui/views/win/hwnd_message_handler.cc:2512: for (std::map<int, bool>::iterator it = touch_id_list_.begin(); You should use the C++11 auto! It's super fun! https://codereview.chromium.org/988473002/diff/20001/ui/views/win/hwnd_messag... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/988473002/diff/20001/ui/views/win/hwnd_messag... ui/views/win/hwnd_message_handler.h:635: std::map<int, bool> touch_id_list_; Might as well use std::set.
https://codereview.chromium.org/988473002/diff/20001/ui/views/win/hwnd_messag... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/20001/ui/views/win/hwnd_messag... ui/views/win/hwnd_message_handler.cc:2507: else if (touch_event_type == ui::ET_TOUCH_MOVED) If you use braces for part of a conditional statement, you need to use braces for each clause. https://codereview.chromium.org/988473002/diff/20001/ui/views/win/hwnd_messag... ui/views/win/hwnd_message_handler.cc:2518: ui::TouchEvent event(ui::ET_TOUCH_RELEASED, Don't you also need to synthesize touch presses in some cases? https://codereview.chromium.org/988473002/diff/20001/ui/views/win/hwnd_messag... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/988473002/diff/20001/ui/views/win/hwnd_messag... ui/views/win/hwnd_message_handler.h:635: std::map<int, bool> touch_id_list_; On 2015/03/05 21:01:08, tdresser wrote: > Might as well use std::set. Or a SmallMap. This is not the first time I've wished for a SmallSet. https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/sm....
On 2015/03/05 21:10:41, tdresser wrote: > https://codereview.chromium.org/988473002/diff/20001/ui/views/win/hwnd_messag... > File ui/views/win/hwnd_message_handler.cc (right): > > https://codereview.chromium.org/988473002/diff/20001/ui/views/win/hwnd_messag... > ui/views/win/hwnd_message_handler.cc:2507: else if (touch_event_type == > ui::ET_TOUCH_MOVED) > If you use braces for part of a conditional statement, you need to use braces > for each clause. > > https://codereview.chromium.org/988473002/diff/20001/ui/views/win/hwnd_messag... > ui/views/win/hwnd_message_handler.cc:2518: ui::TouchEvent > event(ui::ET_TOUCH_RELEASED, > Don't you also need to synthesize touch presses in some cases? > > https://codereview.chromium.org/988473002/diff/20001/ui/views/win/hwnd_messag... > File ui/views/win/hwnd_message_handler.h (right): > > https://codereview.chromium.org/988473002/diff/20001/ui/views/win/hwnd_messag... > ui/views/win/hwnd_message_handler.h:635: std::map<int, bool> touch_id_list_; > On 2015/03/05 21:01:08, tdresser wrote: > > Might as well use std::set. > > Or a SmallMap. This is not the first time I've wished for a SmallSet. > https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/sm.... We should try to factor this out in such a way that we can unit test it.
https://codereview.chromium.org/988473002/diff/20001/ui/views/win/hwnd_messag... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/20001/ui/views/win/hwnd_messag... ui/views/win/hwnd_message_handler.cc:2507: else if (touch_event_type == ui::ET_TOUCH_MOVED) On 2015/03/05 21:10:41, tdresser wrote: > If you use braces for part of a conditional statement, you need to use braces > for each clause. Done. https://codereview.chromium.org/988473002/diff/20001/ui/views/win/hwnd_messag... ui/views/win/hwnd_message_handler.cc:2508: touch_id_list_[event.touch_id()] = true; On 2015/03/05 21:01:08, tdresser wrote: > Add a comment explaining why we do this. Done. https://codereview.chromium.org/988473002/diff/20001/ui/views/win/hwnd_messag... ui/views/win/hwnd_message_handler.cc:2512: for (std::map<int, bool>::iterator it = touch_id_list_.begin(); On 2015/03/05 21:01:08, tdresser wrote: > You should use the C++11 auto! It's super fun! Done. https://codereview.chromium.org/988473002/diff/20001/ui/views/win/hwnd_messag... ui/views/win/hwnd_message_handler.cc:2518: ui::TouchEvent event(ui::ET_TOUCH_RELEASED, On 2015/03/05 21:10:41, tdresser wrote: > Don't you also need to synthesize touch presses in some cases? I did generate a touch press event whenever I saw a TOUCHEVENTF_DOWN message, this is happened afterwards to release the touch pointers we did not see in the current message. https://codereview.chromium.org/988473002/diff/20001/ui/views/win/hwnd_messag... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/988473002/diff/20001/ui/views/win/hwnd_messag... ui/views/win/hwnd_message_handler.h:635: std::map<int, bool> touch_id_list_; On 2015/03/05 21:10:41, tdresser wrote: > On 2015/03/05 21:01:08, tdresser wrote: > > Might as well use std::set. > > Or a SmallMap. This is not the first time I've wished for a SmallSet. > https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/sm.... SmallMap is good choice for this case since we have small amount of touch pointers. https://codereview.chromium.org/988473002/diff/20001/ui/views/win/hwnd_messag... ui/views/win/hwnd_message_handler.h:635: std::map<int, bool> touch_id_list_; On 2015/03/05 21:01:08, tdresser wrote: > Might as well use std::set. If I use set, then I will need two sets, one is for the previous touch pointers and one is for the current touch pointers and do a compare, is there any easier way to use just one set?
https://codereview.chromium.org/988473002/diff/20001/ui/views/win/hwnd_messag... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/20001/ui/views/win/hwnd_messag... ui/views/win/hwnd_message_handler.cc:2518: ui::TouchEvent event(ui::ET_TOUCH_RELEASED, On 2015/03/05 23:37:07, lanwei wrote: > On 2015/03/05 21:10:41, tdresser wrote: > > Don't you also need to synthesize touch presses in some cases? > > I did generate a touch press event whenever I saw a TOUCHEVENTF_DOWN message, > this is happened afterwards to release the touch pointers we did not see in the > current message. So right now you're handing the case where the number of pointers decreases without a TOUCHEVENTF_UP. Shouldn't we also handle the case where the number of pointers increases without a TOUCHEVENTF_DOWN? https://codereview.chromium.org/988473002/diff/20001/ui/views/win/hwnd_messag... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/988473002/diff/20001/ui/views/win/hwnd_messag... ui/views/win/hwnd_message_handler.h:635: std::map<int, bool> touch_id_list_; On 2015/03/05 23:37:07, lanwei wrote: > On 2015/03/05 21:01:08, tdresser wrote: > > Might as well use std::set. > > If I use set, then I will need two sets, one is for the previous touch pointers > and one is for the current touch pointers and do a compare, is there any easier > way to use just one set? Oh, yeah, I missed how you were using the value from the set. It's a little weird to use this map to store global state, as well for performing some computation in OnTouchEvent, but I think it's reasonable. If you're going to use this approach, you should be clear in the comment what existence in the set means, and what the value means. https://codereview.chromium.org/988473002/diff/40001/ui/views/win/hwnd_messag... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/40001/ui/views/win/hwnd_messag... ui/views/win/hwnd_message_handler.cc:2469: if (input[i].dwFlags & TOUCHEVENTF_DOWN) { We might want to synthesize all up and down events solely based on the change in the number of pointers. Relying on both signals, but then sometimes ignoring one of them is a bit confusing. https://codereview.chromium.org/988473002/diff/40001/ui/views/win/hwnd_messag... ui/views/win/hwnd_message_handler.cc:2527: touch_id_list_.erase(it++); This is a bit confusing. I'd prefer we explicitly keep the previous touch id outside the loop, and increment the iterator in the "for ()".
Patchset #3 (id:60001) has been deleted
https://codereview.chromium.org/988473002/diff/80001/ui/views/win/hwnd_messag... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/80001/ui/views/win/hwnd_messag... ui/views/win/hwnd_message_handler.cc:2532: base::SmallMap< std::map<int, bool> >::iterator temp; Use a better name than temp. Perhaps "previous_touch_id" or something like that? https://codereview.chromium.org/988473002/diff/80001/ui/views/win/hwnd_messag... ui/views/win/hwnd_message_handler.cc:2534: while (it != touch_id_list_.end()) { This can still be a for loop. Just always update previous_touch_id at the end of the loop.
lanwei@chromium.org changed reviewers: - sadrul@chromium.org
lanwei@chromium.org changed reviewers: + sadrul@chromium.org
Patchset #4 (id:100001) has been deleted
I think it would be worth a reasonable amount of effort to figure out how to unit test this. https://codereview.chromium.org/988473002/diff/120001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/120001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2491: touch_event_type != ui::ET_TOUCH_PRESSED) { Flip the order of the conditions here. The vast majority of the time we shouldn't bother looking through the map for touch_id. https://codereview.chromium.org/988473002/diff/120001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2519: // Mark the active touch pointers in the touch_id_list_ to be true, so Get rid of double space. https://codereview.chromium.org/988473002/diff/120001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2538: ui::TouchEvent event(ui::ET_TOUCH_RELEASED, We shouldn't send the release at (0, 0), we should send it at the last location we saw it at. Perhaps that information should also go in the map? We've had bugs previously with events being given fake locations. https://codereview.chromium.org/988473002/diff/120001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/988473002/diff/120001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:635: // release the ones which are not in the current message. Describe what the bool is used for. https://codereview.chromium.org/988473002/diff/120001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:636: base::SmallMap< std::map<int, bool> > touch_id_list_; No space between "<" and "std".
sadrul@chromium.org changed reviewers: + ananta@chromium.org
+ananta@
Patchset #5 (id:140001) has been deleted
Patchset #5 (id:160001) has been deleted
Patchset #5 (id:180001) has been deleted
Patchset #5 (id:200001) has been deleted
https://codereview.chromium.org/988473002/diff/120001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/120001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2519: // Mark the active touch pointers in the touch_id_list_ to be true, so On 2015/03/11 14:31:16, tdresser wrote: > Get rid of double space. I don't see it anymore... Ignore this.
Patchset #6 (id:240001) has been deleted
Patchset #5 (id:220001) has been deleted
Patchset #6 (id:280001) has been deleted
Patchset #6 (id:300001) has been deleted
https://codereview.chromium.org/988473002/diff/120001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/120001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2491: touch_event_type != ui::ET_TOUCH_PRESSED) { On 2015/03/11 14:31:16, tdresser wrote: > Flip the order of the conditions here. The vast majority of the time we > shouldn't bother looking through the map for touch_id. Done. https://codereview.chromium.org/988473002/diff/120001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2538: ui::TouchEvent event(ui::ET_TOUCH_RELEASED, On 2015/03/11 14:31:16, tdresser wrote: > We shouldn't send the release at (0, 0), we should send it at the last location > we saw it at. Perhaps that information should also go in the map? We've had bugs > previously with events being given fake locations. Done. https://codereview.chromium.org/988473002/diff/120001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/988473002/diff/120001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:635: // release the ones which are not in the current message. On 2015/03/11 14:31:16, tdresser wrote: > Describe what the bool is used for. Done. https://codereview.chromium.org/988473002/diff/120001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:636: base::SmallMap< std::map<int, bool> > touch_id_list_; On 2015/03/11 14:31:16, tdresser wrote: > No space between "<" and "std". Done.
https://codereview.chromium.org/988473002/diff/320001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/988473002/diff/320001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:214: int GetGeneratedID(uint32 number); There are a few different ways to expose methods to tests without making them public. The easiest is probably to mark the test class as a friend. (See an example here: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...) https://codereview.chromium.org/988473002/diff/320001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:219: base::SmallMap<std::map<int, bool>> TouchIdList() { Make methods const when possible. https://codereview.chromium.org/988473002/diff/320001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:651: base::SmallMap<std::map<int, bool>> touch_id_list_; Oops, these should probably just be vectors... The touch ids will be contiguous and start from 0. https://codereview.chromium.org/988473002/diff/320001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler_unittest.cc (right): https://codereview.chromium.org/988473002/diff/320001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler_unittest.cc:11: TEST(HWNDMessageHandler, TestPrepareTouchEventList) { Split this test up into several smaller tests, which test specific cases. For example, you should have a test that ensures that synthetic touch downs work, a separate test for synthetic touch ups, and another test for a simultaneous synthetic up and synthetic down, along with a test or two for the case where we don't generate any synthetic events. https://codereview.chromium.org/988473002/diff/320001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler_unittest.cc:27: int touch_id = messageHandler.GetGeneratedID(input[0].dwID); I don't think you need to expose GetGeneratedID. The ids take easily predictable values, so you could just use the values when testing, or you could use a sequential id generator in the test.
Patchset #7 (id:340001) has been deleted
Patchset #7 (id:360001) has been deleted
Patchset #7 (id:380001) has been deleted
Patchset #7 (id:400001) has been deleted
https://codereview.chromium.org/988473002/diff/320001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/988473002/diff/320001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:214: int GetGeneratedID(uint32 number); On 2015/03/17 16:02:22, tdresser wrote: > There are a few different ways to expose methods to tests without making them > public. > > The easiest is probably to mark the test class as a friend. (See an example > here: > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...) Done. https://codereview.chromium.org/988473002/diff/320001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:219: base::SmallMap<std::map<int, bool>> TouchIdList() { On 2015/03/17 16:02:22, tdresser wrote: > Make methods const when possible. Done. https://codereview.chromium.org/988473002/diff/320001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:651: base::SmallMap<std::map<int, bool>> touch_id_list_; On 2015/03/17 16:02:22, tdresser wrote: > Oops, these should probably just be vectors... The touch ids will be contiguous > and start from 0. Done. https://codereview.chromium.org/988473002/diff/320001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler_unittest.cc (right): https://codereview.chromium.org/988473002/diff/320001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler_unittest.cc:11: TEST(HWNDMessageHandler, TestPrepareTouchEventList) { On 2015/03/17 16:02:22, tdresser wrote: > Split this test up into several smaller tests, which test specific cases. > > For example, you should have a test that ensures that synthetic touch downs > work, a separate test for synthetic touch ups, and another test for a > simultaneous synthetic up and synthetic down, along with a test or two for the > case where we don't generate any synthetic events. > Done. https://codereview.chromium.org/988473002/diff/320001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler_unittest.cc:27: int touch_id = messageHandler.GetGeneratedID(input[0].dwID); On 2015/03/17 16:02:22, tdresser wrote: > I don't think you need to expose GetGeneratedID. The ids take easily predictable > values, so you could just use the values when testing, or you could use a > sequential id generator in the test. Done.
https://codereview.chromium.org/988473002/diff/420001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/420001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2512: if (touch_event_type != ui::ET_UNKNOWN) { While we're here, lets invert this condition, to reduce the amount of nesting. if (touch_event_type == ui::ET_UNKNOWN) continue; https://codereview.chromium.org/988473002/diff/420001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2562: if (touch_id_list_[i]) { Invert this condition. https://codereview.chromium.org/988473002/diff/420001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2566: input[j].dwID)); It would be nice to avoid calling GetGeneratedID so many times. If we keep touch_id_list_ (or its replacement) up to date, shouldn't we just be able to check if touch_id_list_[i] was in the current message? https://codereview.chromium.org/988473002/diff/420001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2572: if (!found) { Invert this condition. https://codereview.chromium.org/988473002/diff/420001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/988473002/diff/420001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:657: base::SmallMap<std::map<int, gfx::Point>> touch_point_list_; Sorry I keep going back and forth on the data structures here. I think we can be a bit more explicit, and use a single data structure. How about we change to something like: struct TouchPoint { gfx::Point location; bool in_current_message; } base::SmallMap<std::map<int, TouchPoint>> touch_point_list_; or enum class InTouchList {NotPresent, InPreviousMessage, InCurrentMessage}; struct TouchPoint { gfx::Point location; InTouchList in_touch_list; } TouchPoint touch_point_list_[MotionEvent::ui::MotionEvent::MAX_TOUCH_POINT_COUNT]; https://codereview.chromium.org/988473002/diff/420001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler_unittest.cc (right): https://codereview.chromium.org/988473002/diff/420001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler_unittest.cc:22: input[0] = input1; Let's add a helper method for TOUCHINPUT generation. https://codereview.chromium.org/988473002/diff/420001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler_unittest.cc:234: // One finger down and a new figure move, so we miss a touchrelease and figure -> finger
Patchset #8 (id:440001) has been deleted
Patchset #8 (id:460001) has been deleted
Patchset #8 (id:480001) has been deleted
Patchset #8 (id:500001) has been deleted
Patchset #8 (id:520001) has been deleted
https://codereview.chromium.org/988473002/diff/420001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/420001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2512: if (touch_event_type != ui::ET_UNKNOWN) { On 2015/04/22 13:50:21, tdresser wrote: > While we're here, lets invert this condition, to reduce the amount of nesting. > > if (touch_event_type == ui::ET_UNKNOWN) > continue; Done. https://codereview.chromium.org/988473002/diff/420001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2566: input[j].dwID)); On 2015/04/22 13:50:22, tdresser wrote: > It would be nice to avoid calling GetGeneratedID so many times. If we keep > touch_id_list_ (or its replacement) up to date, shouldn't we just be able to > check if touch_id_list_[i] was in the current message? Done. https://codereview.chromium.org/988473002/diff/420001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler_unittest.cc (right): https://codereview.chromium.org/988473002/diff/420001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler_unittest.cc:234: // One finger down and a new figure move, so we miss a touchrelease and On 2015/04/22 13:50:22, tdresser wrote: > figure -> finger Done.
Looking great thanks! I just have a few more comments, and then we'll get other folks to take a look. https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2515: This function is a little long. Perhaps you can factor out everything from here to the end of the loop? Something like void GenerateTouchEvent(...) https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2517: static_cast<int>(id_generator_.GetGeneratedID(input[i].dwID)); TouchPoint& touch_point = touch_id_list_[touch_id]; would simplify things a bit. https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2524: ui::TouchEvent event(ui::ET_TOUCH_PRESSED, gfx::Point(point.x, point.y), Add a gfx::Point location above (same place you define touch_id). You refer to this point a lot. https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2533: const base::TimeTicks now = base::TimeTicks::Now(); Move the computation of |now| (and the associated comment) above where we synthesize the touch press, and use |now| for the press as well. It might make sense to make |now| be a TimeDelta instead of TimeTicks. https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2551: touch_id_list_[touch_id].location = gfx::Point(0, 0); I wouldn't bother zeroing the location. https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2558: for (size_t i = 0; i != touch_id_list_.size(); ++i) { This could also be cleanly factored out. Perhaps void RemoveVanishedTouchPoints(...) https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:520: const std::array<TouchPoint, ui::MotionEvent::MAX_TOUCH_POINT_COUNT>& This type is a bit lengthy, you might want a typedef here. https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler_unittest.cc (right): https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler_unittest.cc:40: EXPECT_EQ(touch_events1[0].location().x(), Can you just expect that the locations are equal, instead of separately expecting on x and y? https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler_unittest.cc:166: EXPECT_EQ(1, touch_events2[2].touch_id()); Why do we have two events with the same touch id? https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler_unittest.cc:169: EXPECT_EQ(messageHandler.TouchIdList()[1].in_touch_list, Why don't we expect on the value of the third element in the list? https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler_unittest.cc:214: HWNDMessageHandler::InTouchList::InPreviousMessage); Why don't we expect on the fourth element in the list?
Patchset #9 (id:560001) has been deleted
Patchset #9 (id:580001) has been deleted
https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2515: On 2015/04/23 14:28:05, tdresser wrote: > This function is a little long. Perhaps you can factor out everything from here > to the end of the loop? > > Something like > void GenerateTouchEvent(...) Done. https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2517: static_cast<int>(id_generator_.GetGeneratedID(input[i].dwID)); On 2015/04/23 14:28:05, tdresser wrote: > TouchPoint& touch_point = touch_id_list_[touch_id]; > would simplify things a bit. Done. https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2524: ui::TouchEvent event(ui::ET_TOUCH_PRESSED, gfx::Point(point.x, point.y), On 2015/04/23 14:28:05, tdresser wrote: > Add a gfx::Point location above (same place you define touch_id). > You refer to this point a lot. Done. https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2551: touch_id_list_[touch_id].location = gfx::Point(0, 0); On 2015/04/23 14:28:05, tdresser wrote: > I wouldn't bother zeroing the location. Done. https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:520: const std::array<TouchPoint, ui::MotionEvent::MAX_TOUCH_POINT_COUNT>& On 2015/04/23 14:28:05, tdresser wrote: > This type is a bit lengthy, you might want a typedef here. Done.
lanwei@chromium.org changed reviewers: - ananta@chromium.org
lanwei@chromium.org changed reviewers: + ananta@chromium.org
https://codereview.chromium.org/988473002/diff/600001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/600001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2471: base::TimeDelta::FromMilliseconds(kTouchDownContextResetTimeout)); Unrelated to this particular CL, but how about counting the number of TOUCH_PRESSED events in the list, and post a single task to decrement touch_down_contexts_, instead of posting a separate task for each touch-point? (separate CL) https://codereview.chromium.org/988473002/diff/600001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2497: last_touch_message_time_ = ::GetMessageTime(); Is it necessary to do this for all touch-points in the same message? https://codereview.chromium.org/988473002/diff/600001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2529: TouchPoint& touch_point = touch_id_list_[touch_id]; Should you check first if it exists in the array? https://codereview.chromium.org/988473002/diff/600001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2534: base::TimeDelta now = base::TimeTicks::Now() - base::TimeTicks(); ui::EventTimeForNow()? https://codereview.chromium.org/988473002/diff/600001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2539: && touch_point.in_touch_list == InTouchList::NotPresent) { && in the previous line. https://codereview.chromium.org/988473002/diff/600001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2579: touch_point.in_touch_list = InTouchList::NotPresent; Shouldn't we ReleaseNumber() here too? https://codereview.chromium.org/988473002/diff/600001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/988473002/diff/600001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:676: TouchPointArray touch_id_list_; touch_point_list_ Since we have this now, can we get rid of touch_ids_?
> Because HWND Message sometimes sends inconsistent number of > touch pointers, we keep the IDs of touch pointers we have > seen from last time, and release the ones which are not in > the current message. We need to confirm with someone who knows Windows that this approach is OK.
On 2015/04/23 22:25:22, sadrul wrote: > > Because HWND Message sometimes sends inconsistent number of > > touch pointers, we keep the IDs of touch pointers we have > > seen from last time, and release the ones which are not in > > the current message. > > We need to confirm with someone who knows Windows that this approach is OK. The problem is that this isn't about knowing Windows, it's about knowing what misbehaving touch drivers do on Windows. We only see this behavior on a small number of displays. We've reached out to MS, but haven't heard anything meaningful back. It is possible that this approach will perform badly on a small set of other displays. One possibility would be to record through UMA how many times we need to synthesize press / release events. If we recorded external touch screen vids/pids on Windows, then we could get a list of the displays which have misbehaving drivers, which would be useful in determining if this ever behaves incorrectly. I'm not sure it's worth the effort though. I think it's probably reasonable to make the change, and see if there are any complaints. What do you think?
On 2015/04/24 12:45:51, tdresser wrote: > On 2015/04/23 22:25:22, sadrul wrote: > > > Because HWND Message sometimes sends inconsistent number of > > > touch pointers, we keep the IDs of touch pointers we have > > > seen from last time, and release the ones which are not in > > > the current message. > > > > We need to confirm with someone who knows Windows that this approach is OK. > > The problem is that this isn't about knowing Windows, it's about knowing what > misbehaving touch drivers do on Windows. We only see this behavior on a small > number of displays. We've reached out to MS, but haven't heard anything > meaningful back. > > It is possible that this approach will perform badly on a small set of other > displays. > One possibility would be to record through UMA how many times we need to > synthesize press / release events. If we recorded external touch screen > vids/pids on Windows, then we could get a list of the displays which have > misbehaving drivers, which would be useful in determining if this ever behaves > incorrectly. > > I'm not sure it's worth the effort though. I think it's probably reasonable to > make the change, and see if there are any complaints. What do you think? What happens if on a well-behaving touchscreen, there are multiple active touch points, but only one touch point moves? Is the event-message guaranteed to contain all the active touch points in that message in this case? Because if not, we can potentially get random taps when doing pinch. That's what I am mostly worried about.
On 2015/04/24 18:24:00, sadrul wrote: > On 2015/04/24 12:45:51, tdresser wrote: > > On 2015/04/23 22:25:22, sadrul wrote: > > > > Because HWND Message sometimes sends inconsistent number of > > > > touch pointers, we keep the IDs of touch pointers we have > > > > seen from last time, and release the ones which are not in > > > > the current message. > > > > > > We need to confirm with someone who knows Windows that this approach is OK. > > > > The problem is that this isn't about knowing Windows, it's about knowing what > > misbehaving touch drivers do on Windows. We only see this behavior on a small > > number of displays. We've reached out to MS, but haven't heard anything > > meaningful back. > > > > It is possible that this approach will perform badly on a small set of other > > displays. > > One possibility would be to record through UMA how many times we need to > > synthesize press / release events. If we recorded external touch screen > > vids/pids on Windows, then we could get a list of the displays which have > > misbehaving drivers, which would be useful in determining if this ever behaves > > incorrectly. > > > > I'm not sure it's worth the effort though. I think it's probably reasonable to > > make the change, and see if there are any complaints. What do you think? > > What happens if on a well-behaving touchscreen, there are multiple active touch > points, but only one touch point moves? Is the event-message guaranteed to > contain all the active touch points in that message in this case? Because if > not, we can potentially get random taps when doing pinch. That's what I am > mostly worried about. I tested if we have multiple active touch points, and there is only one finger move or one more finger down or release, it will send all the active touch points including the ones that do not move at all as TOUCHEVENTF_MOVE.
On 2015/04/24 18:24:00, sadrul wrote: > On 2015/04/24 12:45:51, tdresser wrote: > > On 2015/04/23 22:25:22, sadrul wrote: > > > > Because HWND Message sometimes sends inconsistent number of > > > > touch pointers, we keep the IDs of touch pointers we have > > > > seen from last time, and release the ones which are not in > > > > the current message. > > > > > > We need to confirm with someone who knows Windows that this approach is OK. > > > > The problem is that this isn't about knowing Windows, it's about knowing what > > misbehaving touch drivers do on Windows. We only see this behavior on a small > > number of displays. We've reached out to MS, but haven't heard anything > > meaningful back. > > > > It is possible that this approach will perform badly on a small set of other > > displays. > > One possibility would be to record through UMA how many times we need to > > synthesize press / release events. If we recorded external touch screen > > vids/pids on Windows, then we could get a list of the displays which have > > misbehaving drivers, which would be useful in determining if this ever behaves > > incorrectly. > > > > I'm not sure it's worth the effort though. I think it's probably reasonable to > > make the change, and see if there are any complaints. What do you think? > > What happens if on a well-behaving touchscreen, there are multiple active touch > points, but only one touch point moves? Is the event-message guaranteed to > contain all the active touch points in that message in this case? Because if > not, we can potentially get random taps when doing pinch. That's what I am > mostly worried about. The Microsoft docs are pretty fuzzy here: https://msdn.microsoft.com/en-us/library/windows/desktop/dd317341%28v=vs.85%2... wParam The low-order word contains the number of touch points associated with this message. The high-order word is reserved for future use. https://msdn.microsoft.com/en-us/library/windows/desktop/dd371582(v=vs.85).aspx cInputs [in] The number of structures in the pInputs array. This should ideally be at least equal to the number of touch points associated with the message as indicated in the message WPARAM. If cInputs is less than the number of touch points, the function will still succeed and populate the pInputs buffer with information about cInputs touch points. pInputs [out] A pointer to an array of TOUCHINPUT structures to receive information about the touch points associated with the specified touch input handle. The "should ideally be at least equal to the number of touch points associated with the message" is a little scary... The devices we've tested on report all fingers in every event, but it does look like this isn't strictly required.
lanwei@chromium.org changed reviewers: + girard@chromium.org
Patchset #10 (id:620001) has been deleted
https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2533: const base::TimeTicks now = base::TimeTicks::Now(); On 2015/04/23 14:28:05, tdresser wrote: > Move the computation of |now| (and the associated comment) above where we > synthesize the touch press, and use |now| for the press as well. > > It might make sense to make |now| be a TimeDelta instead of TimeTicks. Done. https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler_unittest.cc (right): https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler_unittest.cc:40: EXPECT_EQ(touch_events1[0].location().x(), On 2015/04/23 14:28:06, tdresser wrote: > Can you just expect that the locations are equal, instead of separately > expecting on x and y? Done. https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler_unittest.cc:166: EXPECT_EQ(1, touch_events2[2].touch_id()); On 2015/04/23 14:28:05, tdresser wrote: > Why do we have two events with the same touch id? The first event we see it is a touchmove, we miss a touchpress for the event with touchID 1, so I send a touchpress and a touchmove for this touchID. https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler_unittest.cc:169: EXPECT_EQ(messageHandler.TouchIdList()[1].in_touch_list, On 2015/04/23 14:28:05, tdresser wrote: > Why don't we expect on the value of the third element in the list? There are three elements in the touch_events2 list, which are the touch events list, but here we test the status for each touchID, which we only have two touchID. https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler_unittest.cc:169: EXPECT_EQ(messageHandler.TouchIdList()[1].in_touch_list, On 2015/04/23 14:28:05, tdresser wrote: > Why don't we expect on the value of the third element in the list? There are three elements in the touch_events2 list, which are the touch events list, but here we test the status for each touchID, which we only have two touchID. https://codereview.chromium.org/988473002/diff/540001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler_unittest.cc:214: HWNDMessageHandler::InTouchList::InPreviousMessage); On 2015/04/23 14:28:05, tdresser wrote: > Why don't we expect on the fourth element in the list? Same as the last one. https://codereview.chromium.org/988473002/diff/600001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/600001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2497: last_touch_message_time_ = ::GetMessageTime(); On 2015/04/23 22:24:03, sadrul wrote: > Is it necessary to do this for all touch-points in the same message? Done. https://codereview.chromium.org/988473002/diff/600001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2529: TouchPoint& touch_point = touch_id_list_[touch_id]; On 2015/04/23 22:24:03, sadrul wrote: > Should you check first if it exists in the array? Done. https://codereview.chromium.org/988473002/diff/600001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2534: base::TimeDelta now = base::TimeTicks::Now() - base::TimeTicks(); On 2015/04/23 22:24:03, sadrul wrote: > ui::EventTimeForNow()? Done. https://codereview.chromium.org/988473002/diff/600001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2539: && touch_point.in_touch_list == InTouchList::NotPresent) { On 2015/04/23 22:24:03, sadrul wrote: > && in the previous line. Done. https://codereview.chromium.org/988473002/diff/600001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2579: touch_point.in_touch_list = InTouchList::NotPresent; On 2015/04/23 22:24:03, sadrul wrote: > Shouldn't we ReleaseNumber() here too? Done. https://codereview.chromium.org/988473002/diff/600001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/988473002/diff/600001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:676: TouchPointArray touch_id_list_; On 2015/04/23 22:24:03, sadrul wrote: > touch_point_list_ > > Since we have this now, can we get rid of touch_ids_? Done.
LGTM https://codereview.chromium.org/988473002/diff/640001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/640001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2498: last_touch_message_time_ = ::GetMessageTime(); Can this just be done outside the loop? https://codereview.chromium.org/988473002/diff/640001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/988473002/diff/640001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:524: TouchEvents* touch_events); Fix indentation.
On 2015/04/27 13:49:47, tdresser wrote: > LGTM > > https://codereview.chromium.org/988473002/diff/640001/ui/views/win/hwnd_messa... > File ui/views/win/hwnd_message_handler.cc (right): > > https://codereview.chromium.org/988473002/diff/640001/ui/views/win/hwnd_messa... > ui/views/win/hwnd_message_handler.cc:2498: last_touch_message_time_ = > ::GetMessageTime(); > Can this just be done outside the loop? > > https://codereview.chromium.org/988473002/diff/640001/ui/views/win/hwnd_messa... > File ui/views/win/hwnd_message_handler.h (right): > > https://codereview.chromium.org/988473002/diff/640001/ui/views/win/hwnd_messa... > ui/views/win/hwnd_message_handler.h:524: TouchEvents* touch_events); > Fix indentation. LGTM
Please use 'git cl format' to fix formatting issues. https://codereview.chromium.org/988473002/diff/660001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/660001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:366: touch_id_list_[i].in_touch_list = InTouchList::NotPresent; Can you do this in TouchPoint constructor? https://codereview.chromium.org/988473002/diff/660001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2465: for (size_t i = 0; i < touch_events.size(); ++i) { This loop can be replaced like: int old_touch_down_contexts = touch_down_contexts_; PrepareTouchEventList(....); int new_touch_presses = touch_down_contexts_ - old_touch_down_contexts; if (new_touch_presses > 0) { ...PostDelayedTask(FROM_HERE, base::Bind(ResetTouchDownContext, weak_ptr, new_touch_presses), ...); } So we use a single task to reset the count. https://codereview.chromium.org/988473002/diff/660001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2522: gfx::Point point_location, const & https://codereview.chromium.org/988473002/diff/660001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2527: TouchPoint& touch_point = touch_id_list_[touch_id]; For release builds, the DCHECK wouldn't trigger. Should we do an early return in that case? https://codereview.chromium.org/988473002/diff/660001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2530: // input[i].dwTime doesn't necessarily relate to the system time at all, Update this comment (there's no input[i] here). https://codereview.chromium.org/988473002/diff/660001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2541: touch_point.location = point_location; It's not necessary to update touch_point here at all, right? Lines 2555:2561 below should take care of this? https://codereview.chromium.org/988473002/diff/660001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2579: } else if (touch_point.in_touch_list == InTouchList::InCurrentMessage) {} here too https://codereview.chromium.org/988473002/diff/660001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2589: return true; Use std::any_of/find_if? https://codereview.chromium.org/988473002/diff/660001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/988473002/diff/660001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:235: TouchPointArray; Use 'using .. == ..' https://codereview.chromium.org/988473002/diff/660001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:528: bool IsTouchIdListEmpty(); Make this const https://codereview.chromium.org/988473002/diff/660001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:530: const TouchPointArray& TouchIdList() const { touch_id_list()
Patchset #12 (id:680001) has been deleted
Patchset #12 (id:700001) has been deleted
Patchset #12 (id:720001) has been deleted
Patchset #12 (id:740001) has been deleted
https://codereview.chromium.org/988473002/diff/640001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/640001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2498: last_touch_message_time_ = ::GetMessageTime(); On 2015/04/27 13:49:47, tdresser wrote: > Can this just be done outside the loop? Done. https://codereview.chromium.org/988473002/diff/640001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/988473002/diff/640001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:524: TouchEvents* touch_events); On 2015/04/27 13:49:47, tdresser wrote: > Fix indentation. Done. https://codereview.chromium.org/988473002/diff/660001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/660001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:366: touch_id_list_[i].in_touch_list = InTouchList::NotPresent; On 2015/04/28 17:08:56, sadrul wrote: > Can you do this in TouchPoint constructor? Yes. Done. https://codereview.chromium.org/988473002/diff/660001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2465: for (size_t i = 0; i < touch_events.size(); ++i) { On 2015/04/28 17:08:56, sadrul wrote: > This loop can be replaced like: > > int old_touch_down_contexts = touch_down_contexts_; > PrepareTouchEventList(....); > int new_touch_presses = touch_down_contexts_ - old_touch_down_contexts; > if (new_touch_presses > 0) { > ...PostDelayedTask(FROM_HERE, > base::Bind(ResetTouchDownContext, weak_ptr, new_touch_presses), > ...); > } > > So we use a single task to reset the count. Done. https://codereview.chromium.org/988473002/diff/660001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2522: gfx::Point point_location, On 2015/04/28 17:08:56, sadrul wrote: > const & Done. https://codereview.chromium.org/988473002/diff/660001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2527: TouchPoint& touch_point = touch_id_list_[touch_id]; On 2015/04/28 17:08:56, sadrul wrote: > For release builds, the DCHECK wouldn't trigger. Should we do an early return in > that case? Done. https://codereview.chromium.org/988473002/diff/660001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2530: // input[i].dwTime doesn't necessarily relate to the system time at all, On 2015/04/28 17:08:56, sadrul wrote: > Update this comment (there's no input[i] here). Done. https://codereview.chromium.org/988473002/diff/660001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2541: touch_point.location = point_location; On 2015/04/28 17:08:56, sadrul wrote: > It's not necessary to update touch_point here at all, right? Lines 2555:2561 > below should take care of this? You are right. https://codereview.chromium.org/988473002/diff/660001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2579: } else if (touch_point.in_touch_list == InTouchList::InCurrentMessage) On 2015/04/28 17:08:56, sadrul wrote: > {} here too Done. https://codereview.chromium.org/988473002/diff/660001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2589: return true; On 2015/04/28 17:08:56, sadrul wrote: > Use std::any_of/find_if? Done. https://codereview.chromium.org/988473002/diff/660001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/988473002/diff/660001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:235: TouchPointArray; On 2015/04/28 17:08:56, sadrul wrote: > Use 'using .. == ..' Done. https://codereview.chromium.org/988473002/diff/660001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:528: bool IsTouchIdListEmpty(); On 2015/04/28 17:08:56, sadrul wrote: > Make this const Done. https://codereview.chromium.org/988473002/diff/660001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:530: const TouchPointArray& TouchIdList() const { On 2015/04/28 17:08:56, sadrul wrote: > touch_id_list() Done.
https://codereview.chromium.org/988473002/diff/760001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/760001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2487: } Why do we still need this loop? https://codereview.chromium.org/988473002/diff/760001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2534: // From the one HWNDMessage, we will generate corresponding touch events. The function doc goes in the header file. https://codereview.chromium.org/988473002/diff/760001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2583: void HWNDMessageHandler::RemoveVanishedTouchPoints(TouchEvents* touch_events) { The doc should go in the header file. (and maybe call this 'UpdateTouchPointStates()' since it does more than removing points). https://codereview.chromium.org/988473002/diff/760001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2603: IsPresent) == touch_id_list_.end(); You can inline this I think: std::find_if(..begin(), ..end(), [](const auto& point) { point.in_touch_list != NotPresent; }) https://codereview.chromium.org/988473002/diff/760001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/988473002/diff/760001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:11: #include <set> This is not longer needed? https://codereview.chromium.org/988473002/diff/760001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:144: std::array<TouchPoint, ui::MotionEvent::MAX_TOUCH_POINT_COUNT>; Can this entire block go into the private section? https://codereview.chromium.org/988473002/diff/760001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:646: bool touch_down_contexts_; As discussed offline, let's do the int --> bool conversion in a separate CL so it's easier to revert if this breaks anything.
Patchset #13 (id:780001) has been deleted
Patchset #13 (id:800001) has been deleted
https://codereview.chromium.org/988473002/diff/760001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/760001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2487: } On 2015/04/29 15:21:13, sadrul wrote: > Why do we still need this loop? Sorry, I forgot to delete this. https://codereview.chromium.org/988473002/diff/760001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2534: // From the one HWNDMessage, we will generate corresponding touch events. On 2015/04/29 15:21:13, sadrul wrote: > The function doc goes in the header file. Done. https://codereview.chromium.org/988473002/diff/760001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2583: void HWNDMessageHandler::RemoveVanishedTouchPoints(TouchEvents* touch_events) { On 2015/04/29 15:21:13, sadrul wrote: > The doc should go in the header file. (and maybe call this > 'UpdateTouchPointStates()' since it does more than removing points). Done. https://codereview.chromium.org/988473002/diff/760001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2603: IsPresent) == touch_id_list_.end(); On 2015/04/29 15:21:13, sadrul wrote: > You can inline this I think: > > std::find_if(..begin(), ..end(), [](const auto& point) { > point.in_touch_list != NotPresent; > }) Done. https://codereview.chromium.org/988473002/diff/760001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/988473002/diff/760001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:11: #include <set> On 2015/04/29 15:21:13, sadrul wrote: > This is not longer needed? Done. https://codereview.chromium.org/988473002/diff/760001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:144: std::array<TouchPoint, ui::MotionEvent::MAX_TOUCH_POINT_COUNT>; On 2015/04/29 15:21:13, sadrul wrote: > Can this entire block go into the private section? Done.
Patchset #13 (id:820001) has been deleted
Patchset #13 (id:840001) has been deleted
https://codereview.chromium.org/988473002/diff/860001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/860001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2471: weak_factory_.GetWeakPtr(), new_touch_presses), git cl format https://codereview.chromium.org/988473002/diff/860001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2550: base::TimeTicks::FromInternalValue(event.time_stamp().ToInternalValue()), Is there a reason to not just use TimeTicks::Now() here? https://codereview.chromium.org/988473002/diff/860001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2721: if (!IsTouchIdListEmpty()) Hm, always iterating over all the touch points for every mouse-event sounds like something we should avoid. Would it make sense to keep a count of touch-points which are not NotPresent, and use that here instead? https://codereview.chromium.org/988473002/diff/860001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/988473002/diff/860001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:234: std::array<TouchPoint, ui::MotionEvent::MAX_TOUCH_POINT_COUNT>; indent? (please run 'git cl format') https://codereview.chromium.org/988473002/diff/860001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:491: void ResetTouchDownContext(int new_touch_presses); Let's rename this: void DecrementTouchDownContext(int decrement); https://codereview.chromium.org/988473002/diff/860001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:645: // down events and reset to false later using a delayed task. Revert this change in the doc
Patchset #14 (id:880001) has been deleted
https://codereview.chromium.org/988473002/diff/860001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/860001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2471: weak_factory_.GetWeakPtr(), new_touch_presses), On 2015/04/29 21:37:47, sadrul wrote: > git cl format Done. https://codereview.chromium.org/988473002/diff/860001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2550: base::TimeTicks::FromInternalValue(event.time_stamp().ToInternalValue()), On 2015/04/29 21:37:47, sadrul wrote: > Is there a reason to not just use TimeTicks::Now() here? Done. https://codereview.chromium.org/988473002/diff/860001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2721: if (!IsTouchIdListEmpty()) On 2015/04/29 21:37:47, sadrul wrote: > Hm, always iterating over all the touch points for every mouse-event sounds like > something we should avoid. > > Would it make sense to keep a count of touch-points which are not NotPresent, > and use that here instead? Done. https://codereview.chromium.org/988473002/diff/860001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/988473002/diff/860001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:234: std::array<TouchPoint, ui::MotionEvent::MAX_TOUCH_POINT_COUNT>; On 2015/04/29 21:37:47, sadrul wrote: > indent? (please run 'git cl format') I will! https://codereview.chromium.org/988473002/diff/860001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:491: void ResetTouchDownContext(int new_touch_presses); On 2015/04/29 21:37:47, sadrul wrote: > Let's rename this: > > void DecrementTouchDownContext(int decrement); Done. https://codereview.chromium.org/988473002/diff/860001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:645: // down events and reset to false later using a delayed task. On 2015/04/29 21:37:47, sadrul wrote: > Revert this change in the doc Done.
LGTM with the nit about WeakPtrFactory<> addressed. We are updating |touch_id_list_|, |active_touch_point_count_|, and |touch_events| from both UpdateTouchPointStates() and GenerateTouchEvent(), and it looks a bit brittle. It would be nice to do some refactor here to clean it up a bit in a follow-up CL. https://codereview.chromium.org/988473002/diff/900001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/988473002/diff/900001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:663: base::WeakPtrFactory<HWNDMessageHandler> weak_factory_; This should remain the last member variable (see the discussion thread https://groups.google.com/a/chromium.org/d/msg/chromium-dev/A9_N8p40l7A/eThX2...). I thought we had some tooling to catch this sort of thing ... I guess not?
https://codereview.chromium.org/988473002/diff/900001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/900001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2503: touch_event_type = ui::ET_TOUCH_RELEASED; The msdn documentation for TOUCHEVENTF_UP states that it can be sent in conjunction with a TOUCHEVENTF_MOVE. Please change the if to handle this. https://codereview.chromium.org/988473002/diff/900001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2533: // The dwTime of every input in the HWNDMessage doesn't necessarily relate to Please rephrase to input in the WM_TOUCH message. https://codereview.chromium.org/988473002/diff/900001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/988473002/diff/900001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:516: // From the one HWNDMessage, we will generate corresponding touch events. Rephrase to From one WM_TOUCH message, https://codereview.chromium.org/988473002/diff/900001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:665: // HWND Message sometimes sends inconsistent number of touch pointers, so we Rephrase to The WM_TOUCH message at times sends inconsistent number of touch pointers.
Patchset #15 (id:920001) has been deleted
https://codereview.chromium.org/988473002/diff/900001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/900001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2503: touch_event_type = ui::ET_TOUCH_RELEASED; On 2015/05/04 19:19:56, ananta wrote: > The msdn documentation for TOUCHEVENTF_UP states that it can be sent in > conjunction with a TOUCHEVENTF_MOVE. Please change the if to handle this. Done. https://codereview.chromium.org/988473002/diff/900001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2533: // The dwTime of every input in the HWNDMessage doesn't necessarily relate to On 2015/05/04 19:19:56, ananta wrote: > Please rephrase to input in the WM_TOUCH message. Done. https://codereview.chromium.org/988473002/diff/900001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/988473002/diff/900001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:516: // From the one HWNDMessage, we will generate corresponding touch events. On 2015/05/04 19:19:56, ananta wrote: > Rephrase to From one WM_TOUCH message, Done. https://codereview.chromium.org/988473002/diff/900001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:663: base::WeakPtrFactory<HWNDMessageHandler> weak_factory_; On 2015/05/04 17:25:08, sadrul wrote: > This should remain the last member variable (see the discussion thread > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/A9_N8p40l7A/eThX2...). > I thought we had some tooling to catch this sort of thing ... I guess not? Done. https://codereview.chromium.org/988473002/diff/900001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.h:665: // HWND Message sometimes sends inconsistent number of touch pointers, so we On 2015/05/04 19:19:57, ananta wrote: > Rephrase to The WM_TOUCH message at times sends inconsistent number of touch > pointers. Done.
https://codereview.chromium.org/988473002/diff/940001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/940001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2505: GenerateTouchEvent(input[i].dwID, point_location, touch_event_type, I don't think that TOUCHEVENTF_DOWN can be combined with move and release. Please verify from msdn and update this code accordingly.
Patchset #17 (id:980001) has been deleted
Patchset #17 (id:1000001) has been deleted
Patchset #16 (id:960001) has been deleted
Patchset #17 (id:1040001) has been deleted
Patchset #16 (id:1020001) has been deleted
https://codereview.chromium.org/988473002/diff/940001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/940001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2505: GenerateTouchEvent(input[i].dwID, point_location, touch_event_type, On 2015/05/05 22:06:58, ananta wrote: > I don't think that TOUCHEVENTF_DOWN can be combined with move and release. > Please verify from msdn and update this code accordingly. You are right, TOUCHEVENTF_DOWN cannot be combined with TOUCHEVENTF_MOVE or TOUCHEVENTF_UP, but someone pointed TOUCHEVENTF_MOVE and TOUCHEVENTF_UP can be combined in the one input.
On 2015/05/06 02:53:47, lanwei wrote: > https://codereview.chromium.org/988473002/diff/940001/ui/views/win/hwnd_messa... > File ui/views/win/hwnd_message_handler.cc (right): > > https://codereview.chromium.org/988473002/diff/940001/ui/views/win/hwnd_messa... > ui/views/win/hwnd_message_handler.cc:2505: GenerateTouchEvent(input[i].dwID, > point_location, touch_event_type, > On 2015/05/05 22:06:58, ananta wrote: > > I don't think that TOUCHEVENTF_DOWN can be combined with move and release. > > Please verify from msdn and update this code accordingly. > > You are right, TOUCHEVENTF_DOWN cannot be combined with TOUCHEVENTF_MOVE or > TOUCHEVENTF_UP, but someone pointed TOUCHEVENTF_MOVE and TOUCHEVENTF_UP can be > combined in the one input. I did. :)
https://codereview.chromium.org/988473002/diff/1060001/ui/views/win/hwnd_mess... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/988473002/diff/1060001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:2530: if (touch_event_type != ui::ET_TOUCH_PRESSED && Folks have been complaining about not receiving expected number of touch release events. Do we have similar problems with touch pressed as well?. What happens to the associated mouse down and up events which are synthesized by Windows. Do we get multiple down events for multi touch cases and if yes does artificially generating touch down events work correctly?
On 2015/05/06 18:28:49, ananta wrote: > https://codereview.chromium.org/988473002/diff/1060001/ui/views/win/hwnd_mess... > File ui/views/win/hwnd_message_handler.cc (right): > > https://codereview.chromium.org/988473002/diff/1060001/ui/views/win/hwnd_mess... > ui/views/win/hwnd_message_handler.cc:2530: if (touch_event_type != > ui::ET_TOUCH_PRESSED && > Folks have been complaining about not receiving expected number of touch release > events. Do we have similar problems with touch pressed as well?. We've not seen any reports of "missed touch presses". I suspect if there were such events, we have a number of asserts that would fire. (Not clear how many windows users have asserts enabled...) > What happens to the associated mouse down and up events which are synthesized by > Windows. Do we get multiple down events for multi touch cases and if yes does > artificially generating touch down events work correctly? Good question. Windows generates synthesized mouse events only at the end of a touch sequence. When you remove your last finger, then WM_LBUTTONDOWN/WM_LBUTTONUP get fired. For two-fingered touches (actually, more than two) you get RBUTTON instead. The messages get released in a pair, so I would think that the Lenovo device is either generating both or neither. If it generates both, no problem. If neither are generated, then I guess the user might be missing a click event. I don't think we'd want to change the behaviour for mouse events.
On 2015/05/11 21:02:56, girard wrote: > On 2015/05/06 18:28:49, ananta wrote: > > > https://codereview.chromium.org/988473002/diff/1060001/ui/views/win/hwnd_mess... > > File ui/views/win/hwnd_message_handler.cc (right): > > > > > https://codereview.chromium.org/988473002/diff/1060001/ui/views/win/hwnd_mess... > > ui/views/win/hwnd_message_handler.cc:2530: if (touch_event_type != > > ui::ET_TOUCH_PRESSED && > > Folks have been complaining about not receiving expected number of touch > release > > events. Do we have similar problems with touch pressed as well?. > > We've not seen any reports of "missed touch presses". I suspect if there were > such events, we have a number of asserts that would fire. (Not clear how many > windows users have asserts enabled...) > > > What happens to the associated mouse down and up events which are synthesized > by > > Windows. Do we get multiple down events for multi touch cases and if yes does > > artificially generating touch down events work correctly? > > Good question. Windows generates synthesized mouse events only at the end of a > touch sequence. When you remove your last finger, then > WM_LBUTTONDOWN/WM_LBUTTONUP get fired. For two-fingered touches (actually, more > than two) you get RBUTTON instead. The messages get released in a pair, so I > would think that the Lenovo device is either generating both or neither. If it > generates both, no problem. If neither are generated, then I guess the user > might be missing a click event. > > I don't think we'd want to change the behaviour for mouse events. If that is the case we should take out the code which generates missing touch presses and have a CHECK or something to gather data.
On 2015/05/12 21:15:53, ananta wrote: > On 2015/05/11 21:02:56, girard wrote: > > On 2015/05/06 18:28:49, ananta wrote: > > > > > > https://codereview.chromium.org/988473002/diff/1060001/ui/views/win/hwnd_mess... > > > File ui/views/win/hwnd_message_handler.cc (right): > > > > > > > > > https://codereview.chromium.org/988473002/diff/1060001/ui/views/win/hwnd_mess... > > > ui/views/win/hwnd_message_handler.cc:2530: if (touch_event_type != > > > ui::ET_TOUCH_PRESSED && > > > Folks have been complaining about not receiving expected number of touch > > release > > > events. Do we have similar problems with touch pressed as well?. > > > > We've not seen any reports of "missed touch presses". I suspect if there were > > such events, we have a number of asserts that would fire. (Not clear how many > > windows users have asserts enabled...) > > > > > What happens to the associated mouse down and up events which are > synthesized > > by > > > Windows. Do we get multiple down events for multi touch cases and if yes > does > > > artificially generating touch down events work correctly? > > > > Good question. Windows generates synthesized mouse events only at the end of a > > touch sequence. When you remove your last finger, then > > WM_LBUTTONDOWN/WM_LBUTTONUP get fired. For two-fingered touches (actually, > more > > than two) you get RBUTTON instead. The messages get released in a pair, so I > > would think that the Lenovo device is either generating both or neither. If it > > generates both, no problem. If neither are generated, then I guess the user > > might be missing a click event. > > > > I don't think we'd want to change the behaviour for mouse events. > > If that is the case we should take out the code which generates missing touch > presses > and have a CHECK or something to gather data. We have not seen that there is missing touch press so far, but it is hard to say that it will never happen, so do you think we can just keep this code and add UMA_Count to collect the data if it happens.
Patchset #17 (id:1080001) has been deleted
Patchset #17 (id:1100001) has been deleted
Patchset #17 (id:1120001) has been deleted
lgtm
The CQ bit was checked by lanwei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from girard@chromium.org, tdresser@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/988473002/#ps1140001 (title: "Delete adding missing touchpress")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/988473002/1140001
Message was sent while issue was closed.
Committed patchset #17 (id:1140001)
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/a2a44ba54fe134526dc81014991f951c1fb09db1 Cr-Commit-Position: refs/heads/master@{#329770}
Message was sent while issue was closed.
A revert of this CL (patchset #17 id:1140001) has been created in https://codereview.chromium.org/1143283002/ by lanwei@chromium.org. The reason for reverting is: It causes crash and some regression for touches on Windows 8. We will recommit it once we fully fix and test it.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
