Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
333623003:
Added split tap to TouchExplorationController (Closed)
DescriptionAdded split tap to TouchExplorationController.
Split tap allows a user to click while in touch exploration mode by tapping a second finger anywhere on the screen.
*Added TOUCH_EXPL_SECOND_PRESS state and a InTouchExplSecondPress function in the controller
*Added tests for tap and tap hold events, and also tested the case where the user lifts the touch exploration finger first.
BUG=385151
TEST = TouchExplorationTest.*
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278093
Patch Set 1 #
Total comments: 16
Patch Set 2 : Dominic's changes #
Total comments: 6
Patch Set 3 : Alice's changes #
Total comments: 27
Patch Set 4 : Addressed more comments #
Total comments: 3
Patch Set 5 : all comments addressed #
Total comments: 10
Patch Set 6 : Dominic's changes, including new test for double tap without previous touch exploration location #
Total comments: 4
Patch Set 7 : Little nit changes #
Total comments: 21
Patch Set 8 : rebased #Patch Set 9 : Changes from James #
Messages
Total messages: 25 (0 generated)
We added split tap and some tests for it. We've encountered a few bugs since, and they may or may not be related to the code we added - but the basis of the code is here and should be ready for code review.
https://codereview.chromium.org/333623003/diff/1/ui/chromeos/touch_exploratio... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/333623003/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:253: ui::TouchEvent* rewritten_release_event = this should be rewritten_touch_event https://codereview.chromium.org/333623003/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:329: if (tap_timer_.IsRunning()) This shouldn't be necessary - it's already done before entering this state, right? https://codereview.chromium.org/333623003/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:341: if (event.touch_id() == 0){ I don't think you can assume the touch id is 0 for the touch exploration finger - you should use the touch id from when that finger was first pressed. https://codereview.chromium.org/333623003/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:347: // Continue to rlease the touch only if the touch explore finger is the rlease -> release https://codereview.chromium.org/333623003/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:356: event.touch_id(), This touch id might not be the same as the one that was pressed. Suppose you touch explore with 1, then add finger 2, which generates a press with touch id 2, then add finger 3, which is discarded, then release 2, and finally release 3. The end result is a press of 2 followed by a release of 3. https://codereview.chromium.org/333623003/diff/1/ui/chromeos/touch_exploratio... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/333623003/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:108: ui::EventRewriteStatus InTESecondPress( TE is too abbreviated for me. How about InTouchExplSecondPress? https://codereview.chromium.org/333623003/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:164: // The user was in touch exploration, but has placed down another finger Nit: end this sentence with a period. https://codereview.chromium.org/333623003/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:165: // If the user releases the second finger, a click will go through To be entirely accurate, we don't send a click, we send the touch press and release from the second finger - that may be interpreted as a click, but sometimes it will be subtly different.
last_touch_exploration_location_ is now a pointer last_touch_exploration_ so that the id of the last touch exploration can be used. https://codereview.chromium.org/333623003/diff/1/ui/chromeos/touch_exploratio... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/333623003/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:253: ui::TouchEvent* rewritten_release_event = On 2014/06/13 05:52:38, dmazzoni wrote: > this should be rewritten_touch_event Done. https://codereview.chromium.org/333623003/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:329: if (tap_timer_.IsRunning()) On 2014/06/13 05:52:38, dmazzoni wrote: > This shouldn't be necessary - it's already done before entering this state, > right? Yeah - right before the state change - thanks https://codereview.chromium.org/333623003/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:341: if (event.touch_id() == 0){ On 2014/06/13 05:52:38, dmazzoni wrote: > I don't think you can assume the touch id is 0 for the touch exploration finger > - you should use the touch id from when that finger was first pressed. Done. I changed last_touch_exploration_location to a pointer for the whole last touch exploration event, and can now access its id. https://codereview.chromium.org/333623003/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:347: // Continue to rlease the touch only if the touch explore finger is the On 2014/06/13 05:52:38, dmazzoni wrote: > rlease -> release Done. https://codereview.chromium.org/333623003/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:356: event.touch_id(), On 2014/06/13 05:52:38, dmazzoni wrote: > This touch id might not be the same as the one that was pressed. Suppose you > touch explore with 1, then add finger 2, which generates a press with touch id > 2, then add finger 3, which is discarded, then release 2, and finally release 3. > The end result is a press of 2 followed by a release of 3. Done. I used initial_press_ instead - which should be the first touch (since every touch after is discarded) https://codereview.chromium.org/333623003/diff/1/ui/chromeos/touch_exploratio... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/333623003/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:108: ui::EventRewriteStatus InTESecondPress( On 2014/06/13 05:52:38, dmazzoni wrote: > TE is too abbreviated for me. How about InTouchExplSecondPress? Done. Should the state name be changed from TE_SECOND_PRESS to TOUCH_EXPL_SECOND_PRESS? https://codereview.chromium.org/333623003/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:164: // The user was in touch exploration, but has placed down another finger On 2014/06/13 05:52:38, dmazzoni wrote: > Nit: end this sentence with a period. Done. https://codereview.chromium.org/333623003/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:165: // If the user releases the second finger, a click will go through On 2014/06/13 05:52:38, dmazzoni wrote: > To be entirely accurate, we don't send a click, we send the touch press and > release from the second finger - that may be interpreted as a click, but > sometimes it will be subtly different. Done.
https://codereview.chromium.org/333623003/diff/20001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/333623003/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:151: // If this is the first ever touch, initialize last_touch_exploration_ This seems odd to me. If this isn't a touch exploration, why is it going to be effectively treated as one iff it's the first touch? In what circumstances is accessing last_touch_exploration_ causing a NPR - i.e. how can it get to a state where last_touch_exploration_ is being checked before it's initialized? Does this mean that it's sometimes using an out of date last_touch_exploration_ value? I'd consider doing a CHECK(last_touch_exploration_) wherever it's referenced and fixing the NPR where it occurs, rather than doing this. Also, note that in Chromium code style, the braces in this one-line if block are optional - feel free to leave them in, though. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit... https://codereview.chromium.org/333623003/diff/20001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/333623003/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:169: // is changed to wait for all fingers to be released before continuing. "...wait for all fingers to be released" etc: does this mean that if you're aiming for a split tap but brush a third finger against the screen, the split tap will fail to register and you'll have to take all fingers off before retrying? https://codereview.chromium.org/333623003/diff/20001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/333623003/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:699: EXPECT_EQ(gesture_detector_config_.longpress_timeout, Nice :)
Alice's changes. https://codereview.chromium.org/333623003/diff/20001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/333623003/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:151: // If this is the first ever touch, initialize last_touch_exploration_ On 2014/06/13 17:06:48, aboxhall wrote: > This seems odd to me. If this isn't a touch exploration, why is it going to be > effectively treated as one iff it's the first touch? > > In what circumstances is accessing last_touch_exploration_ causing a NPR - i.e. > how can it get to a state where last_touch_exploration_ is being checked before > it's initialized? Does this mean that it's sometimes using an out of date > last_touch_exploration_ value? I'd consider doing a > CHECK(last_touch_exploration_) wherever it's referenced and fixing the NPR where > it occurs, rather than doing this. > > Also, note that in Chromium code style, the braces in this one-line if block are > optional - feel free to leave them in, though. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit... This will happen if the user hasn't entered touch exploration, but tries to do a double tap (or double tap press). I've moved the code into InSingleTapReleased, where it is decided that there will be a double tap, and if there is no last touch exploration declared at this point, I just make it where the second tap is. Would it still be good to add the CHECK(last_touch_exploration_) where I reference it elsewhere? https://codereview.chromium.org/333623003/diff/20001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/333623003/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:169: // is changed to wait for all fingers to be released before continuing. On 2014/06/13 17:06:48, aboxhall wrote: > "...wait for all fingers to be released" etc: does this mean that if you're > aiming for a split tap but brush a third finger against the screen, the split > tap will fail to register and you'll have to take all fingers off before > retrying? Oh sorry, this was an old comment from how I used to think it was going to work. Currently, extra fingers should be ignored. (there's some kind of bug there though)
https://codereview.chromium.org/333623003/diff/20001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/333623003/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:151: // If this is the first ever touch, initialize last_touch_exploration_ On 2014/06/13 17:48:59, evy wrote: > On 2014/06/13 17:06:48, aboxhall wrote: > > This seems odd to me. If this isn't a touch exploration, why is it going to be > > effectively treated as one iff it's the first touch? > > > > In what circumstances is accessing last_touch_exploration_ causing a NPR - > i.e. > > how can it get to a state where last_touch_exploration_ is being checked > before > > it's initialized? Does this mean that it's sometimes using an out of date > > last_touch_exploration_ value? I'd consider doing a > > CHECK(last_touch_exploration_) wherever it's referenced and fixing the NPR > where > > it occurs, rather than doing this. > > > > Also, note that in Chromium code style, the braces in this one-line if block > are > > optional - feel free to leave them in, though. > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit... > > This will happen if the user hasn't entered touch exploration, but tries to do a > double tap (or double tap press). I see, so this would be the case where a user double taps somewhere on the screen before ever doing a touch explore in this session - definitely an unusual but not impossible case :) > I've moved the code into InSingleTapReleased, > where it is decided that there will be a double tap, and if there is no last > touch exploration declared at this point, I just make it where the second tap > is. > Would it still be good to add the CHECK(last_touch_exploration_) where I > reference it elsewhere? You probably don't need to do that if that's definitely the only situation where this can happen. https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:204: // to the this location. grammar weirdness: "the this" https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:206: last_touch_exploration_.reset(new TouchEvent(event)); This makes more sense now, but I wonder whether we want to discard double-taps which occur when no touch exploration has occurred yet. What does TalkBack do? https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:708: gfx::Point tap_location(11, 12); Suggestion: initial_touch_location https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:710: // Confirm events from other fingers go through as is. This comment is a bit confusing as you're not actually asserting anything yet (and it's at a weird indent). https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:712: ui::TouchEvent touch1_press( Consider constructing these just before they're used rather than here. https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:713: ui::ET_TOUCH_PRESSED, gfx::Point(33, 34), 1, Now()); Suggest using a variable for this location too, e.g. "second_touch_location" https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:765: generator_->Dispatch(&touch0_press); It might be worthwhile pulling the assertions for the first tap into a method on the test fixture, since it's reused in a few places and isn't the main point of the test. https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:807: simulated_clock_->Advance(gesture_detector_config_.longpress_timeout); This confuses me. Could you also just do Now() + gesture_detector_config_.longpress_timeout in the constructor call below?
https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:807: simulated_clock_->Advance(gesture_detector_config_.longpress_timeout); On 2014/06/13 18:20:01, aboxhall wrote: > This confuses me. Could you also just do Now() + > gesture_detector_config_.longpress_timeout in the constructor call below? I think that'd be a lot more bookkeeping - you'd have to keep adding that to every event from this point on. What's confusing about advancing the time?
https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:807: simulated_clock_->Advance(gesture_detector_config_.longpress_timeout); On 2014/06/13 18:36:13, dmazzoni wrote: > On 2014/06/13 18:20:01, aboxhall wrote: > > This confuses me. Could you also just do Now() + > > gesture_detector_config_.longpress_timeout in the constructor call below? > > I think that'd be a lot more bookkeeping - you'd have to keep adding that to > every event from this point on. > > What's confusing about advancing the time? No more events are created after this one, so the bookkeeping issue is moot. The confusing thing is that later on we call AdvanceSimulatedTimePastTapDelay(), so the original Now() is really not Now(), it's really Now() - 1000ms - longpress_timeout. It seems like the simulated time is doing double duty here, as an initialization value and as a value during the test.
https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:807: simulated_clock_->Advance(gesture_detector_config_.longpress_timeout); On 2014/06/13 18:40:29, aboxhall wrote: > On 2014/06/13 18:36:13, dmazzoni wrote: > > On 2014/06/13 18:20:01, aboxhall wrote: > > > This confuses me. Could you also just do Now() + > > > gesture_detector_config_.longpress_timeout in the constructor call below? > > > > I think that'd be a lot more bookkeeping - you'd have to keep adding that to > > every event from this point on. > > > > What's confusing about advancing the time? > > No more events are created after this one, so the bookkeeping issue is moot. > > The confusing thing is that later on we call AdvanceSimulatedTimePastTapDelay(), > so the original Now() is really not Now(), it's really Now() - 1000ms - > longpress_timeout. It seems like the simulated time is doing double duty here, > as an initialization value and as a value during the test. Oh! I see what you mean. I think it'd be more clear if events were dispatched as they're created. Create the touch0_press, dispatch it. Then advance the simulated time, to enter touch exploration mode. Then create the touch1_press, and dispatch it. Then advance the simulated time again - so we're past the long-press timeout. ...and so on.
https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:807: simulated_clock_->Advance(gesture_detector_config_.longpress_timeout); On 2014/06/13 18:46:06, dmazzoni wrote: > On 2014/06/13 18:40:29, aboxhall wrote: > > On 2014/06/13 18:36:13, dmazzoni wrote: > > > On 2014/06/13 18:20:01, aboxhall wrote: > > > > This confuses me. Could you also just do Now() + > > > > gesture_detector_config_.longpress_timeout in the constructor call below? > > > > > > I think that'd be a lot more bookkeeping - you'd have to keep adding that to > > > every event from this point on. > > > > > > What's confusing about advancing the time? > > > > No more events are created after this one, so the bookkeeping issue is moot. > > > > The confusing thing is that later on we call > AdvanceSimulatedTimePastTapDelay(), > > so the original Now() is really not Now(), it's really Now() - 1000ms - > > longpress_timeout. It seems like the simulated time is doing double duty here, > > as an initialization value and as a value during the test. > > Oh! I see what you mean. > > I think it'd be more clear if events were dispatched as they're created. > > Create the touch0_press, dispatch it. > Then advance the simulated time, to enter touch exploration mode. > Then create the touch1_press, and dispatch it. > Then advance the simulated time again - so we're past the long-press timeout. > > ...and so on. Yeah, that would work well.
Fixed up some of the stuff we were talking about - some things I'm still not clear on, questions in the comments. https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:204: // to the this location. On 2014/06/13 18:20:00, aboxhall wrote: > grammar weirdness: "the this" Done. https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:206: last_touch_exploration_.reset(new TouchEvent(event)); On 2014/06/13 18:20:00, aboxhall wrote: > This makes more sense now, but I wonder whether we want to discard double-taps > which occur when no touch exploration has occurred yet. What does TalkBack do? Hm, so we just wouldn't process the second click - ignore the finger and return a rewrite discard? What is TalkBack? https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:708: gfx::Point tap_location(11, 12); On 2014/06/13 18:20:01, aboxhall wrote: > Suggestion: initial_touch_location Done. https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:710: // Confirm events from other fingers go through as is. On 2014/06/13 18:20:01, aboxhall wrote: > This comment is a bit confusing as you're not actually asserting anything yet > (and it's at a weird indent). Whoops that was a comment from the passthrough code we copied. Gone now. https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:712: ui::TouchEvent touch1_press( On 2014/06/13 18:20:01, aboxhall wrote: > Consider constructing these just before they're used rather than here. Done. This is what Dominic later suggested below, right? https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:713: ui::ET_TOUCH_PRESSED, gfx::Point(33, 34), 1, Now()); On 2014/06/13 18:20:00, aboxhall wrote: > Suggest using a variable for this location too, e.g. "second_touch_location" For the tapping finger, or the touch explore finger? We could also say something like "touch_explore_press" and "split_tap_press". How do you feel about that? https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:765: generator_->Dispatch(&touch0_press); On 2014/06/13 18:20:00, aboxhall wrote: > It might be worthwhile pulling the assertions for the first tap into a method on > the test fixture, since it's reused in a few places and isn't the main point of > the test. Done. Great idea! https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:807: simulated_clock_->Advance(gesture_detector_config_.longpress_timeout); On 2014/06/13 18:47:22, aboxhall wrote: > On 2014/06/13 18:46:06, dmazzoni wrote: > > On 2014/06/13 18:40:29, aboxhall wrote: > > > On 2014/06/13 18:36:13, dmazzoni wrote: > > > > On 2014/06/13 18:20:01, aboxhall wrote: > > > > > This confuses me. Could you also just do Now() + > > > > > gesture_detector_config_.longpress_timeout in the constructor call > below? > > > > > > > > I think that'd be a lot more bookkeeping - you'd have to keep adding that > to > > > > every event from this point on. > > > > > > > > What's confusing about advancing the time? > > > > > > No more events are created after this one, so the bookkeeping issue is moot. > > > > > > The confusing thing is that later on we call > > AdvanceSimulatedTimePastTapDelay(), > > > so the original Now() is really not Now(), it's really Now() - 1000ms - > > > longpress_timeout. It seems like the simulated time is doing double duty > here, > > > as an initialization value and as a value during the test. > > > > Oh! I see what you mean. > > > > I think it'd be more clear if events were dispatched as they're created. > > > > Create the touch0_press, dispatch it. > > Then advance the simulated time, to enter touch exploration mode. > > Then create the touch1_press, and dispatch it. > > Then advance the simulated time again - so we're past the long-press timeout. > > > > ...and so on. > > Yeah, that would work well. Done. Events dispatched as they're created
https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:206: last_touch_exploration_.reset(new TouchEvent(event)); On 2014/06/13 20:27:39, evy wrote: > On 2014/06/13 18:20:00, aboxhall wrote: > > This makes more sense now, but I wonder whether we want to discard double-taps > > which occur when no touch exploration has occurred yet. What does TalkBack do? > > Hm, so we just wouldn't process the second click - ignore the finger and return > a rewrite discard? Yeah, that's what I'm suggesting, but I'm not asserting that it's definitely the right thing to do :) > What is TalkBack? TalkBack is the screen reader on Android. If you don't have another Android device handy to test with, there's a Nexus 7 under my desk on top of my Mac (I think...). Feel free to factory reset it and create your own account. https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:712: ui::TouchEvent touch1_press( On 2014/06/13 20:27:39, evy wrote: > On 2014/06/13 18:20:01, aboxhall wrote: > > Consider constructing these just before they're used rather than here. > > Done. This is what Dominic later suggested below, right? Basically, yeah. https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:713: ui::ET_TOUCH_PRESSED, gfx::Point(33, 34), 1, Now()); On 2014/06/13 20:27:39, evy wrote: > On 2014/06/13 18:20:00, aboxhall wrote: > > Suggest using a variable for this location too, e.g. "second_touch_location" > > For the tapping finger, or the touch explore finger? I meant for the actual location (i.e. the gfx::Point). But giving the TouchEvents meaningful names would probably also be useful. > We could also say something like "touch_explore_press" and "split_tap_press". > How do you feel about that? For the events, yeah, names like that would be good: touch_explore_press, split_tap_press and split_tap_release make sense. https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:807: simulated_clock_->Advance(gesture_detector_config_.longpress_timeout); On 2014/06/13 20:27:39, evy wrote: > On 2014/06/13 18:47:22, aboxhall wrote: > > On 2014/06/13 18:46:06, dmazzoni wrote: > > > On 2014/06/13 18:40:29, aboxhall wrote: > > > > On 2014/06/13 18:36:13, dmazzoni wrote: > > > > > On 2014/06/13 18:20:01, aboxhall wrote: > > > > > > This confuses me. Could you also just do Now() + > > > > > > gesture_detector_config_.longpress_timeout in the constructor call > > below? > > > > > > > > > > I think that'd be a lot more bookkeeping - you'd have to keep adding > that > > to > > > > > every event from this point on. > > > > > > > > > > What's confusing about advancing the time? > > > > > > > > No more events are created after this one, so the bookkeeping issue is > moot. > > > > > > > > The confusing thing is that later on we call > > > AdvanceSimulatedTimePastTapDelay(), > > > > so the original Now() is really not Now(), it's really Now() - 1000ms - > > > > longpress_timeout. It seems like the simulated time is doing double duty > > here, > > > > as an initialization value and as a value during the test. > > > > > > Oh! I see what you mean. > > > > > > I think it'd be more clear if events were dispatched as they're created. > > > > > > Create the touch0_press, dispatch it. > > > Then advance the simulated time, to enter touch exploration mode. > > > Then create the touch1_press, and dispatch it. > > > Then advance the simulated time again - so we're past the long-press > timeout. > > > > > > ...and so on. > > > > Yeah, that would work well. > > Done. Events dispatched as they're created Thanks, this is much easier to follow now. https://codereview.chromium.org/333623003/diff/60001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/333623003/diff/60001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:127: void TouchExplorationModeAtLocation(gfx::Point tap_location, int id){ Nit: space between ) and { Also, this might be better named as a verb, e.g. "EnterTouchExplorationModeAtLocation". Also also, it seems like |id| is always 0, and is never checked later, so perhaps it doesn't need to be an argument.
All comments should be addressed now! https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:206: last_touch_exploration_.reset(new TouchEvent(event)); On 2014/06/13 20:52:49, aboxhall wrote: > On 2014/06/13 20:27:39, evy wrote: > > On 2014/06/13 18:20:00, aboxhall wrote: > > > This makes more sense now, but I wonder whether we want to discard > double-taps > > > which occur when no touch exploration has occurred yet. What does TalkBack > do? > > > > Hm, so we just wouldn't process the second click - ignore the finger and > return > > a rewrite discard? > > Yeah, that's what I'm suggesting, but I'm not asserting that it's definitely the > right thing to do :) > > > What is TalkBack? > > TalkBack is the screen reader on Android. If you don't have another Android > device handy to test with, there's a Nexus 7 under my desk on top of my Mac (I > think...). Feel free to factory reset it and create your own account. TalkBack just discards it, so I'll have our code do that too. https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:713: ui::ET_TOUCH_PRESSED, gfx::Point(33, 34), 1, Now()); On 2014/06/13 20:52:49, aboxhall wrote: > On 2014/06/13 20:27:39, evy wrote: > > On 2014/06/13 18:20:00, aboxhall wrote: > > > Suggest using a variable for this location too, e.g. "second_touch_location" > > > > For the tapping finger, or the touch explore finger? > > I meant for the actual location (i.e. the gfx::Point). But giving the > TouchEvents meaningful names would probably also be useful. > > > We could also say something like "touch_explore_press" and "split_tap_press". > > How do you feel about that? > > For the events, yeah, names like that would be good: touch_explore_press, > split_tap_press and split_tap_release make sense. Done. https://codereview.chromium.org/333623003/diff/60001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/333623003/diff/60001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:127: void TouchExplorationModeAtLocation(gfx::Point tap_location, int id){ On 2014/06/13 20:52:50, aboxhall wrote: > Nit: space between ) and { > > Also, this might be better named as a verb, e.g. > "EnterTouchExplorationModeAtLocation". > > Also also, it seems like |id| is always 0, and is never checked later, so > perhaps it doesn't need to be an argument. I made it like that just in case we want to try testing something later where touch explore isn't entered with id 0. Would it be best to just change the function to use id if we ever encounter the need, and keep it without for now?
lgtm https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/333623003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:206: last_touch_exploration_.reset(new TouchEvent(event)); On 2014/06/13 22:03:47, evy wrote: > On 2014/06/13 20:52:49, aboxhall wrote: > > On 2014/06/13 20:27:39, evy wrote: > > > On 2014/06/13 18:20:00, aboxhall wrote: > > > > This makes more sense now, but I wonder whether we want to discard > > double-taps > > > > which occur when no touch exploration has occurred yet. What does TalkBack > > do? > > > > > > Hm, so we just wouldn't process the second click - ignore the finger and > > return > > > a rewrite discard? > > > > Yeah, that's what I'm suggesting, but I'm not asserting that it's definitely > the > > right thing to do :) > > > > > What is TalkBack? > > > > TalkBack is the screen reader on Android. If you don't have another Android > > device handy to test with, there's a Nexus 7 under my desk on top of my Mac (I > > think...). Feel free to factory reset it and create your own account. > > TalkBack just discards it, so I'll have our code do that too. Looks good! https://codereview.chromium.org/333623003/diff/60001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/333623003/diff/60001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:127: void TouchExplorationModeAtLocation(gfx::Point tap_location, int id){ On 2014/06/13 22:03:47, evy wrote: > On 2014/06/13 20:52:50, aboxhall wrote: > > Nit: space between ) and { > > > > Also, this might be better named as a verb, e.g. > > "EnterTouchExplorationModeAtLocation". > > > > Also also, it seems like |id| is always 0, and is never checked later, so > > perhaps it doesn't need to be an argument. > > I made it like that just in case we want to try testing something later where > touch explore isn't entered with id 0. > Would it be best to just change the function to use id if we ever encounter the > need, and keep it without for now? Yep, that sounds like a good idea. You could even make it a optional argument at that point.
Added bug number
https://codereview.chromium.org/333623003/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/333623003/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:204: if (last_touch_exploration_ == NULL) { A scoped_ptr has a "!" operator, so instead of testing whether it's NULL, you can write: if (!last_touch_exploration_) Also, will the release be discarded? I'd like to see a unit test that if the user does a double-tap without ever touch-exploring first, no events are generated at all. https://codereview.chromium.org/333623003/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:253: // Split-tap Nit: Make this a sentence, like "Handle split-tap". https://codereview.chromium.org/333623003/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/333623003/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:39: // ** Long version ** You should update the description in the header file to include an explanation of how split-tap works. https://codereview.chromium.org/333623003/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:169: TE_SECOND_PRESS, I'd prefer TOUCH_EXPL_SECOND_PRESS or something like that here - TE is a bit too short. I'd be fine with agreeing on something even more concise too, like TOUCHEX, and especially if we used it consistently throughout the file. https://codereview.chromium.org/333623003/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/333623003/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:752: // All fingers should be released after the click goes through. In the generated/simulated events output by touch exploration controller, there's only one finger down, right? If so, "All fingers" doesn't sound right
more changes from Dominic's comments https://codereview.chromium.org/333623003/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/333623003/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:204: if (last_touch_exploration_ == NULL) { On 2014/06/16 18:21:19, dmazzoni wrote: > A scoped_ptr has a "!" operator, so instead of testing whether it's NULL, you > can write: > > if (!last_touch_exploration_) > > Also, will the release be discarded? I'd like to see a unit test that if the > user does a double-tap without ever touch-exploring first, no events are > generated at all. The release was supposed to be discarded! Added that, and also made a test for it. also: This is actually a tangent from split tap - darn. We got here because split tap redirects to single_tap_pressed when the touch exploration finger is lifted first. So this is somewhat related? Let me know if you'd like to me to try to separate the changes - might be a bit tricky though because I've been committing them together. https://codereview.chromium.org/333623003/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:253: // Split-tap On 2014/06/16 18:21:19, dmazzoni wrote: > Nit: Make this a sentence, like "Handle split-tap". Done. https://codereview.chromium.org/333623003/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/333623003/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:39: // ** Long version ** On 2014/06/16 18:21:19, dmazzoni wrote: > You should update the description in the header file to include an explanation > of how split-tap works. Done. https://codereview.chromium.org/333623003/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:169: TE_SECOND_PRESS, On 2014/06/16 18:21:19, dmazzoni wrote: > I'd prefer TOUCH_EXPL_SECOND_PRESS or something like that here - TE is a bit too > short. I'd be fine with agreeing on something even more concise too, like > TOUCHEX, and especially if we used it consistently throughout the file. Done. I think I'll stick with TOUCH_EXPL_SECOND_PRESS like the function right now, and we find that there are a lot more states/functions that need to be added that refer to touch exploration, I might change it to something shorter at that point. https://codereview.chromium.org/333623003/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/333623003/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:752: // All fingers should be released after the click goes through. On 2014/06/16 18:21:20, dmazzoni wrote: > In the generated/simulated events output by touch exploration controller, > there's only one finger down, right? If so, "All fingers" doesn't sound right In this test, that's true - so I'll change it. I was thinking about the case when extra fingers are added - but we didn't actually test this, and there are still bugs in that case so I'll be adding tests specific to checking all fingers released in a bit.
lgtm +jamescook for owners review https://codereview.chromium.org/333623003/diff/90001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/333623003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:258: // Handle split-tap Nit: period at end of sentence. https://codereview.chromium.org/333623003/diff/90001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/333623003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:62: //// If the user enters touch exploration mode, they can click without lifting nit: I think there's a newline missing?
https://codereview.chromium.org/333623003/diff/90001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/333623003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:258: // Handle split-tap On 2014/06/17 15:50:40, dmazzoni wrote: > Nit: period at end of sentence. Done. https://codereview.chromium.org/333623003/diff/90001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/333623003/diff/90001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:62: //// If the user enters touch exploration mode, they can click without lifting On 2014/06/17 15:50:40, dmazzoni wrote: > nit: I think there's a newline missing? Done.
Also, please change the CL description. A great CL description looks like this: Add new_feature to existing_subsystem One or two sentence description of what the feature does and/or why this change was made. * Specific thing changed 1 * Specific thing changed 2 BUG=12345 TEST=ui_unittests TestSuite.* (or TestSuite.MyNewTest) https://codereview.chromium.org/333623003/diff/110001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/333623003/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:218: if (type == ui::ET_TOUCH_RELEASED && !last_touch_exploration_){ nit: space between ) and { (I'm surprised git cl format didn't catch that. Maybe we don't have git cl format as part of presubmit here.) https://codereview.chromium.org/333623003/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:341: // Currently this is a discard, but could be something like rotor rotor? https://codereview.chromium.org/333623003/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:360: ui::TouchEvent* rewritten_release_event = new ui::TouchEvent( Can you do it like this? rewritten_event->reset(new ui::TouchEvent(...)); rewritten_event->set_flags(event.flags); That makes it very clear who owns the new ui::TouchEvent from the beginning. https://codereview.chromium.org/333623003/diff/110001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/333623003/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:112: ui::EventRewriteStatus InTouchExplSecondPress( Expl -> Explore or Exploration please. "Expl" makes me think of "expletive". :-) https://codereview.chromium.org/333623003/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:173: TOUCH_EXPL_SECOND_PRESS, ditto https://codereview.chromium.org/333623003/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:205: // The last location where we synthesized a mouse move event. "last location" -> what this really is https://codereview.chromium.org/333623003/diff/110001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/333623003/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:695: simulated_clock_->Advance(gesture_detector_config_.longpress_timeout); Nice that you're not using hardcoded times! https://codereview.chromium.org/333623003/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:789: ClearCapturedEvents(); optional: It seems like you do this set of assertions a lot, but they don't provide any additional coverage. The test might be easier to read if you left out the 8 lines above and focused on the new behavior. https://codereview.chromium.org/333623003/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:798: ui::TouchEvent touch_explore_release( I like how these variable names are very descriptive. https://codereview.chromium.org/333623003/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:833: ClearCapturedEvents(); optional: as above https://codereview.chromium.org/333623003/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:865: // All fingers should be released after the click goes through. merge with line above, or blank line above
Made the suggested changes except for the rotor comment - see my question in the reply. https://codereview.chromium.org/333623003/diff/110001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/333623003/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:218: if (type == ui::ET_TOUCH_RELEASED && !last_touch_exploration_){ On 2014/06/17 17:06:17, James Cook wrote: > nit: space between ) and { > > (I'm surprised git cl format didn't catch that. Maybe we don't have git cl > format as part of presubmit here.) Done. https://codereview.chromium.org/333623003/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:341: // Currently this is a discard, but could be something like rotor On 2014/06/17 17:06:17, James Cook wrote: > rotor? On an iPad, rotating two fingers (like they are opposite ends of a circle) lets the user select from a menu. A gesture like this is an option for something we could support later - but I agree it's a bit vague here. Would it be best to take out this comment until we decide to put something here? https://codereview.chromium.org/333623003/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:360: ui::TouchEvent* rewritten_release_event = new ui::TouchEvent( On 2014/06/17 17:06:17, James Cook wrote: > Can you do it like this? > > rewritten_event->reset(new ui::TouchEvent(...)); > rewritten_event->set_flags(event.flags); > > That makes it very clear who owns the new ui::TouchEvent from the beginning. Done. I also changed it the other few times this pattern shows up in this file. https://codereview.chromium.org/333623003/diff/110001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/333623003/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:112: ui::EventRewriteStatus InTouchExplSecondPress( On 2014/06/17 17:06:18, James Cook wrote: > Expl -> Explore or Exploration please. "Expl" makes me think of "expletive". > :-) Done! :) https://codereview.chromium.org/333623003/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:173: TOUCH_EXPL_SECOND_PRESS, On 2014/06/17 17:06:18, James Cook wrote: > ditto Done. https://codereview.chromium.org/333623003/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:205: // The last location where we synthesized a mouse move event. On 2014/06/17 17:06:18, James Cook wrote: > "last location" -> what this really is Ah yes, changed the variable type but not the comment. Thanks. Done. https://codereview.chromium.org/333623003/diff/110001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/333623003/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:789: ClearCapturedEvents(); On 2014/06/17 17:06:19, James Cook wrote: > optional: It seems like you do this set of assertions a lot, but they don't > provide any additional coverage. The test might be easier to read if you left > out the 8 lines above and focused on the new behavior. Good point! Up to this point, the code is the same as many other tests. No point in checking the same things. https://codereview.chromium.org/333623003/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:798: ui::TouchEvent touch_explore_release( On 2014/06/17 17:06:18, James Cook wrote: > I like how these variable names are very descriptive. Thanks to Alice's suggestions! :) https://codereview.chromium.org/333623003/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:833: ClearCapturedEvents(); On 2014/06/17 17:06:18, James Cook wrote: > optional: as above Done. https://codereview.chromium.org/333623003/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:865: // All fingers should be released after the click goes through. On 2014/06/17 17:06:18, James Cook wrote: > merge with line above, or blank line above Done.
LGTM. Nice patch.
The CQ bit was checked by evy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/evy@chromium.org/333623003/150001
Message was sent while issue was closed.
Change committed as 278093 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
