Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
296403011:
Support double-tap to click in touch accessibility controller. (Closed)
|
Created:
6 years, 6 months ago by dmazzoni Modified:
6 years, 6 months ago CC:
chromium-reviews, sadrul, stevenjb+watch_chromium.org, ben+aura_chromium.org, oshima+watch_chromium.org, kalyank Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSupport double-tap to click in touch accessibility controller.
This refactors the touch accessibility controller to work like a state
machine, and implements support for double-tap to send a click to the
last location where the user did touch exploration previously. This also
means reimplementing existing touch interactions so that nothing is
passed through until the double-tap timeout passes or the user moves
outside the slop region.
BUG=377900
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275784
Patch Set 1 #
Total comments: 68
Patch Set 2 : Fix existing tests and add double-tap test #
Total comments: 49
Patch Set 3 : Address feedback #Patch Set 4 : Rebase #
Total comments: 4
Patch Set 5 : Address more feedback #
Total comments: 23
Patch Set 6 : Assert that it's in the no_fingers_down state at the end of tests #Patch Set 7 : Address feedback #
Total comments: 2
Patch Set 8 : Last nit #Patch Set 9 : Rebase #
Total comments: 1
Patch Set 10 : Switch to TickClock / SimpleTestTickClock #Patch Set 11 : Update browser test for touch exploration too #
Messages
Total messages: 32 (0 generated)
I'm not done updating the unit test yet, but this is ready for an initial look.
https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:67: // The rest of the processing depends on what state we're in. I really like how you've broken it up into multiple methods. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:97: initial_press_.reset(new TouchEvent(event)); Why create a new event instead of using the one we have? https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:98: tap_timer_.Start(FROM_HERE, We should probably Stop() the timer as we transition through states. It's not quite clear to me what happens if you start the timer which has already been started - does it update both the delay and the task or only the task? https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:106: return ui::EVENT_REWRITE_DISCARD; I don't think we can get here since we'd have no touch id in touch_ids_ and we'd bail out earlier - in RewriteEvent(). If somehow we do get here, I think this should return a continue, not a discard. Or at least it should be consistent with what we return in RewriteEvent() for this case. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:109: return ui::EVENT_REWRITE_DISCARD; Same as above https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:121: state_ = TOUCH_EXPLORATION; I think you need to somehow dispatch the mouse move corresponding to initial_press_ here. This is relevant if event is a release for example. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:131: if (touch_ids_.size() == 0) Isn't this if superflous? Should this be a DCHECK() instead? https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:136: // If the user moves far enough from the initial touch location (outside When we write the code to determine if this is a swipe or a slow move, we should try to leverage GestureProviderAura as much as we can so that we don't duplicate the gesture recognition logic here. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:158: // Ignore any additional fingers when we're already in touch exploration That's not how Android works. I guess we are okay with this? https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:164: return ui::EVENT_REWRITE_DISCARD; We used to generate a mouse move for the release in this spot. Maybe this doesn't matter - in practice I never saw a release without a prior move to the exact location of the release. If it can happen though, dispatching a mouse move here would be relevant. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:170: return ui::EVENT_REWRITE_REWRITTEN; This would get a bit nuts if you are moving multiple fingers on the screen. I guess that's okay. However if we are dispatching mouse events for all moves regardless of the finger, perhaps we should be dispatching moves for all presses regardless of the finger as well for consistency's sake? https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:191: // Can happen if touch exploration is enabled while fingers were down. I think this is NOTREACHED() as we have code to catch this in RewriteEvent() https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:195: return ui::EVENT_REWRITE_CONTINUE; Same https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:238: // If removing a finger that was passed through, and another finger Ok, so this means you can release any finger, not necessarily the "first" finger and continue interacting w/o "minus one". https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:241: int releasing_id = event.touch_id(); I think this code would become a lot simpler if we kept track of the "minus one" finger instead of keeping track of all the other fingers. We wouldn't need to do all these searches. touch_id_map_ would have zero entries in "minus one" mode and one entry in "non-minus one" mode after a finger has been lifted. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:247: type = ui::ET_TOUCH_MOVED; We are changing the type here, so I assume we want the move dispatched? I think that's right, but: - It looks like we will in the if check below and return DISCARD - I think we also need to change the location - instead of dispatching the move event at the release point we want to dispatch the event at the location of still_down_id, i.e. the new effective finger location? https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:259: return ui::EVENT_REWRITE_DISCARD; I don't realluy understand why is this needed - see above https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:261: ui::TouchEvent* rewritten_passthrough_event = new ui::TouchEvent( It seems suboptimal to rewrite the event every time. In many cases we can just let the event pass through. If we make the changes to tracking "minus one" finger as I suggested above, it should be easy to deduce when to return rewrite vs. continue. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:272: if (state_ != SINGLE_TAP_PENDING) Don't we need to dispatch a mouse move if we are in GRACE_PERIOD state? https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:275: ui::MouseEvent mouse_move(ui::ET_MOUSE_MOVED, Why not use CreateMouseMoveEvent() here? https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:31: // Whenever two or more fingers are pressed, the events are passed through This doesn't seem entirely accurate - the minus one is applied iff the second finger is pressed within the grace period. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:32: // withone finger removed - so if you swipe down with two fingers, the "withone" - missing space https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:40: // perform a gesture in that time, they enter touch exploration mode, and I'd say smth like "supported accessibility gesture" instead of "gesture" https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:43: // Also, if the user moves their single finger outside a certain slop region This won't be accurate once we support swipe gestures, since a swipe gesture isn't performed until the finger is lifted. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:60: // once in passthrough mode, if one finger is released, the remaining finger "remaining fingers continue" https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:114: GRACE_PERIOD, IMHO the naming is a bit confusing, since GRACE_PERIOD and SINGLE_TAP_PENDING are both in "grace period", and DOUBLE_TAP_PASSED could be in grace period as well. Perhaps rename these to SINGLE_TAP_PRESSED, SINGLE_TAP_RELEASED, DOUBLE_TAP_PRESSED? https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:116: // A single finger is down and after the grace period the user You could have multiple fingers down in this mode, you only need to have one down during the grace period... https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:118: // We're now in touch exploration mode until this finger is lifted. "until all fingers are lifted" https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:119: TOUCH_EXPLORATION, I'd move this down next to PASSTHROUGH_MINUS_ONE - IMHO the ordering/grouping would be more logical that way. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:121: // The user pressed and released a single finger - a tap - but we have This sounds a bit confusing IMHO. I'd say like "..wait until the end of the grace period to allow the user to tap the second time. If the second tap doesn't occur within the grace period, we dispatch a mouse move at the location of the first tap". https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:133: // two ore more fingers are down, we subtract one finger but otherwise Doesn't seem like this is entirely accurate - based on implementation it looks like we only substract one finger until one finger is released. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:146: // A map from the actual touch ids to the rewritten touch ids when in Hmm.. why change touch ids? https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller_unittest.cc:169: EXPECT_TRUE(IsInTouchToMouseMode()); Can we move this check up before the move? https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller_unittest.cc:193: if (num_mouse_moves == 0) { I think there should be a synthesized mouse move after the grace period expires and then another one for the touch move, and possibly another one for the release.
https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:67: // The rest of the processing depends on what state we're in. On 2014/05/28 17:47:45, mfomitchev wrote: > I really like how you've broken it up into multiple methods. Thanks! https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:97: initial_press_.reset(new TouchEvent(event)); On 2014/05/28 17:47:45, mfomitchev wrote: > Why create a new event instead of using the one we have? We don't have ownership of the event passed in, we need to make our own copy - right? https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:98: tap_timer_.Start(FROM_HERE, On 2014/05/28 17:47:45, mfomitchev wrote: > We should probably Stop() the timer as we transition through states. It's not > quite clear to me what happens if you start the timer which has already been > started - does it update both the delay and the task or only the task? Starting the timer again does what it should - it updates both the delay and the task. I could call Stop, but I actually think that would complicate things for no good reason and could potentially lead to bugs. I think it's simpler that when the timer fires, it checks for one specific state - and if it's not in the state that's waiting on a timer to fire, it exits. What do you think? If we did add Stop, we'd have to add it to each transition to: TOUCH_EXPLORATION DOUBLE_TAP_PRESSED PASSTHROUGH_MINUS_ONE That's potentially several transitions. My worry is that future changes would require updating this code. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:106: return ui::EVENT_REWRITE_DISCARD; On 2014/05/28 17:47:45, mfomitchev wrote: > I don't think we can get here since we'd have no touch id in touch_ids_ and we'd > bail out earlier - in RewriteEvent(). If somehow we do get here, I think this > should return a continue, not a discard. Or at least it should be consistent > with what we return in RewriteEvent() for this case. I agree that it shouldn't happen, so I added a NOTREACHED. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:109: return ui::EVENT_REWRITE_DISCARD; On 2014/05/28 17:47:45, mfomitchev wrote: > Same as above Done. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:121: state_ = TOUCH_EXPLORATION; On 2014/05/28 17:47:45, mfomitchev wrote: > I think you need to somehow dispatch the mouse move corresponding to > initial_press_ here. This is relevant if event is a release for example. If this is a mouse move, calling OnTouchExploration will generate the mouse move. I modified OnTouchExploration so that a release does get rewritten as a mouse move event, just in case. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:131: if (touch_ids_.size() == 0) On 2014/05/28 17:47:45, mfomitchev wrote: > Isn't this if superflous? Should this be a DCHECK() instead? Agreed. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:136: // If the user moves far enough from the initial touch location (outside On 2014/05/28 17:47:45, mfomitchev wrote: > When we write the code to determine if this is a swipe or a slow move, we should > try to leverage GestureProviderAura as much as we can so that we don't duplicate > the gesture recognition logic here. Sounds good. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:158: // Ignore any additional fingers when we're already in touch exploration On 2014/05/28 17:47:45, mfomitchev wrote: > That's not how Android works. I guess we are okay with this? I think I prefer this for now, but we'll do some user testing this summer. I think it's enough of a corner case that I'd rather implement the most straightforward solution rather than copy Android. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:164: return ui::EVENT_REWRITE_DISCARD; On 2014/05/28 17:47:45, mfomitchev wrote: > We used to generate a mouse move for the release in this spot. Maybe this > doesn't matter - in practice I never saw a release without a prior move to the > exact location of the release. If it can happen though, dispatching a mouse move > here would be relevant. Agreed, I changed the logic to generate a mouse move here. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:170: return ui::EVENT_REWRITE_REWRITTEN; On 2014/05/28 17:47:45, mfomitchev wrote: > This would get a bit nuts if you are moving multiple fingers on the screen. I > guess that's okay. However if we are dispatching mouse events for all moves > regardless of the finger, perhaps we should be dispatching moves for all presses > regardless of the finger as well for consistency's sake? I'm tempted to not worry about this case for now because it will be moot once we implement split-tap. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:191: // Can happen if touch exploration is enabled while fingers were down. On 2014/05/28 17:47:45, mfomitchev wrote: > I think this is NOTREACHED() as we have code to catch this in RewriteEvent() Done. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:195: return ui::EVENT_REWRITE_CONTINUE; On 2014/05/28 17:47:45, mfomitchev wrote: > Same Done. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:238: // If removing a finger that was passed through, and another finger On 2014/05/28 17:47:45, mfomitchev wrote: > Ok, so this means you can release any finger, not necessarily the "first" finger > and continue interacting w/o "minus one". Yes, that's the idea https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:241: int releasing_id = event.touch_id(); On 2014/05/28 17:47:45, mfomitchev wrote: > I think this code would become a lot simpler if we kept track of the "minus one" > finger instead of keeping track of all the other fingers. We wouldn't need to do > all these searches. touch_id_map_ would have zero entries in "minus one" mode > and one entry in "non-minus one" mode after a finger has been lifted. Good idea. We don't even need a map now, just the id to remap the first finger to. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:247: type = ui::ET_TOUCH_MOVED; On 2014/05/28 17:47:45, mfomitchev wrote: > We are changing the type here, so I assume we want the move dispatched? I think > that's right, but: > > - It looks like we will in the if check below and return DISCARD > > - I think we also need to change the location - instead of dispatching the move > event at the release point we want to dispatch the event at the location of > still_down_id, i.e. the new effective finger location? Good catch, done. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:261: ui::TouchEvent* rewritten_passthrough_event = new ui::TouchEvent( On 2014/05/28 17:47:45, mfomitchev wrote: > It seems suboptimal to rewrite the event every time. In many cases we can just > let the event pass through. If we make the changes to tracking "minus one" > finger as I suggested above, it should be easy to deduce when to return rewrite > vs. continue. Done. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:272: if (state_ != SINGLE_TAP_PENDING) On 2014/05/28 17:47:45, mfomitchev wrote: > Don't we need to dispatch a mouse move if we are in GRACE_PERIOD state? Done. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:275: ui::MouseEvent mouse_move(ui::ET_MOUSE_MOVED, On 2014/05/28 17:47:45, mfomitchev wrote: > Why not use CreateMouseMoveEvent() here? Done. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller_unittest.cc:169: EXPECT_TRUE(IsInTouchToMouseMode()); On 2014/05/28 17:47:45, mfomitchev wrote: > Can we move this check up before the move? Yes, that makes sense. To do that I had to create an abstraction for advancing time and faking calling the tap timer. It's able to assert that the timer was started and then stop it afterwards, so that seems safe and appropriate for a unit test. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller_unittest.cc:193: if (num_mouse_moves == 0) { On 2014/05/28 17:47:45, mfomitchev wrote: > I think there should be a synthesized mouse move after the grace period expires > and then another one for the touch move, and possibly another one for the > release. Agreed, you see all of those now. (The one for the release was missing because the coordinates of the "real" mouse move were the same. If you make the "real" mouse move go to a different location, then the touch release fires one more fake mouse move.
Tests updated now too, including a new test for double-tap.
Haven't looked at test yet. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:117: } else if (type == ui::ET_TOUCH_RELEASED || type == ui::ET_TOUCH_CANCELLED) { What's the point of these last two if blocks? They both do the same thing, and do essentially the same thing as the fall-through as well. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:131: if (event.time_stamp() - initial_press_->time_stamp() > Even though it's reasonably self-explanatory, it might be helpful to have a brief comment here explaining the early return. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:149: // the "slop" region, jump to the touch exploration mode early. Note that Rephrase this note as an explicit TODO? https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:171: // mode. Later, we should support "split-tap". TODO? https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:190: if (type == ui::ET_TOUCH_PRESSED) { Comment stating explicitly that this represents a (potential) double-tap? https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:201: } else if (type == ui::ET_TOUCH_RELEASED || type == ui::ET_TOUCH_CANCELLED) { Similarly, why are these else blocks here? Do they need TODOs? https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:215: if (type == ui::ET_TOUCH_PRESSED) { How could we get a TOUCH_PRESSED directly after a TOUCH_PRESSED without a TOUCH_RELEASED or TOUCH_CANCELLED in between? https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:218: if (touch_ids_.size() != 0) It took me quite a while to work out what was going on here. Perhaps touch_ids_ might be more clearly named active_touch_ids_ or something? https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:249: bool first_finger_still_down = Does the rest of this need to go inside this if block? I find it confusing that half of the logic is in here and half is outside the if block. Given each other branch early-returns, I think we could move it below. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:281: initial_touch_id_passthrough_mapping_, I don't follow this either. It looks like we only ever pass through the original touch ID? Do we only handle the two-finger case? https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:132: SINGLE_TAP_PENDING, SINGLE_TAP? The single tap has occurred; what's pending is to see what that means, but I don't think the name needs to reflect that. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:136: DOUBLE_TAP_PRESSED, SECOND_TAP_PRESSED? (Since we can't quite mark it as a double-tap just yet.)
https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:97: initial_press_.reset(new TouchEvent(event)); On 2014/05/31 06:53:24, dmazzoni wrote: > On 2014/05/28 17:47:45, mfomitchev wrote: > > Why create a new event instead of using the one we have? > > We don't have ownership of the event passed in, we need to make our own copy - > right? Done. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.cc:98: tap_timer_.Start(FROM_HERE, Yes, makes sense. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:135: return OnTouchExploration(event, rewritten_event); I think there still is a problem: Suppose event is a press. We will call OnTouchExploration, which will exit discarding the event. The state will be changed, so when the timer fires there will be no mouse move dispatched. So we will be in touch exploration mode, but we won't dispatch any mouse moves, which is wrong. I think it wold be good to write a unit test for this case - the case where an event arrives after the timeout expires but before the timer fires. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:189: const ui::EventType type = event.type(); Should we have a case here for (event.time_stamp() - initial_press_->time_stamp() >gesture_detector_config_.double_tap_timeout) like we do in OnGracePeriod()? https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:244: return ui::EVENT_REWRITE_CONTINUE; Nit: It might make sense to get rid of this if. The only special case is release/cancel. The moves and the presses can be handled by the code after the if. We can also get rid of the if for unrecognized types - just apply the same rules to them as to moves/presses.. or add a DCHECK(). Might make the code easier to rid if there's just one if instead of three if/else. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:247: state_ = NO_FINGERS_DOWN; We also need to set initial_touch_id_passthrough_mapping_ to 0 here. Might make sense to create some sort of a Reset() method setting the state to NO_FINGERS_DOWN and clearing all the data structures as needed and use that method whenever we set state to NO_FINGERS_DOWN. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:254: touch_ids_.size() == 1 && Hmm.. why do we only do this when there's only one finger remaining now? In the previous version we activated the mapping on any release regardless of the number of fingers. Also, I think we should set initial_touch_id_passthrough_mapping to -1 or similar when the first finger is released. This is because I think the id may get reused if this (or another) finger is pressed again after it is released. Also if we do this, the condition code could be simplified: we can get rid of first_finger_still_down_ and just have: if (initial_touch_id_passthrough_mapping == 0) { if (event.touch_id() == initial_press_->touch_id()) { initial_touch_id_passthrough_mapping = -1; // Can return DISCARD here or let it fall through and return below } else { // The current rewriting code } } https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:275: if (!initial_touch_id_passthrough_mapping_) This would need to be changed to <= 0 if we use -1. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. It would be good to do some controller state validation here. Checking the state each step of the way might be too much, but I think we should at least end a bunch of tests with lifting all the fingers and confirming that the controller is in NO_FINGERS_DOWN state. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:73: simulated_time_ms_ = 1000; Perhaps just get the time from the generator whenever you need it so that we don't store it twice (in the test and in the generator)? https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:76: cursor_client()->ShowCursor(); Do we need to do this here considering we do this in SwitchTouchExplorationMode? https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:201: EXPECT_FALSE(IsInTouchToMouseMode()); Is there any way we can obtain the slop value from the config file and use it here? https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:464: // finger is the only finger left touching, we should enter a mouse move mode, This comment seems wrong - we won't enter the mouse move mode. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:477: // Release of finger 1 should be ignored. Would be good to have another test where we are releasing the second finger - confirming that re-mapping works correctly https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:482: // Release of finger 2 should be passed through. Might be worth doing a move, etc. before the release.
https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:117: } else if (type == ui::ET_TOUCH_RELEASED || type == ui::ET_TOUCH_CANCELLED) { On 2014/06/02 16:33:14, aboxhall wrote: > What's the point of these last two if blocks? They both do the same thing, and > do essentially the same thing as the fall-through as well. You're right. Simplified. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:131: if (event.time_stamp() - initial_press_->time_stamp() > On 2014/06/02 16:33:14, aboxhall wrote: > Even though it's reasonably self-explanatory, it might be helpful to have a > brief comment here explaining the early return. Done. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:135: return OnTouchExploration(event, rewritten_event); On 2014/06/02 23:12:18, mfomitchev wrote: > I think there still is a problem: > > Suppose event is a press. We will call OnTouchExploration, which will exit > discarding the event. The state will be changed, so when the timer fires there > will be no mouse move dispatched. So we will be in touch exploration mode, but > we won't dispatch any mouse moves, which is wrong. > > I think it wold be good to write a unit test for this case - the case where an > event arrives after the timeout expires but before the timer fires. Good catch. I realized that we can handle this case up in the main function rather than here. That way all we need to do is say, if the tap handler is pending and should be called, but it hasn't been called yet, call it now - then we can do the switch and handle the event based on the new state. I added two new tests for this. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:149: // the "slop" region, jump to the touch exploration mode early. Note that On 2014/06/02 16:33:14, aboxhall wrote: > Rephrase this note as an explicit TODO? Done. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:171: // mode. Later, we should support "split-tap". On 2014/06/02 16:33:14, aboxhall wrote: > TODO? Done. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:189: const ui::EventType type = event.type(); On 2014/06/02 23:12:18, mfomitchev wrote: > Should we have a case here for > (event.time_stamp() - initial_press_->time_stamp() > >gesture_detector_config_.double_tap_timeout) like we do in OnGracePeriod()? Yes, but I think it's cleaner to have it up above. Done. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:190: if (type == ui::ET_TOUCH_PRESSED) { On 2014/06/02 16:33:14, aboxhall wrote: > Comment stating explicitly that this represents a (potential) double-tap? Done. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:201: } else if (type == ui::ET_TOUCH_RELEASED || type == ui::ET_TOUCH_CANCELLED) { On 2014/06/02 16:33:14, aboxhall wrote: > Similarly, why are these else blocks here? Do they need TODOs? Done. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:215: if (type == ui::ET_TOUCH_PRESSED) { On 2014/06/02 16:33:14, aboxhall wrote: > How could we get a TOUCH_PRESSED directly after a TOUCH_PRESSED without a > TOUCH_RELEASED or TOUCH_CANCELLED in between? You have lots of fingers. :) https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:218: if (touch_ids_.size() != 0) On 2014/06/02 16:33:14, aboxhall wrote: > It took me quite a while to work out what was going on here. Perhaps touch_ids_ > might be more clearly named active_touch_ids_ or something? Sure, how about current_touch_ids_ https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:244: return ui::EVENT_REWRITE_CONTINUE; On 2014/06/02 23:12:18, mfomitchev wrote: > Nit: It might make sense to get rid of this if. The only special case is > release/cancel. The moves and the presses can be handled by the code after the > if. We can also get rid of the if for unrecognized types - just apply the same > rules to them as to moves/presses.. or add a DCHECK(). Might make the code > easier to rid if there's just one if instead of three if/else. Done. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:247: state_ = NO_FINGERS_DOWN; On 2014/06/02 23:12:18, mfomitchev wrote: > We also need to set initial_touch_id_passthrough_mapping_ to 0 here. Might make > sense to create some sort of a Reset() method setting the state to > NO_FINGERS_DOWN and clearing all the data structures as needed and use that > method whenever we set state to NO_FINGERS_DOWN. Done. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:249: bool first_finger_still_down = On 2014/06/02 16:33:14, aboxhall wrote: > Does the rest of this need to go inside this if block? I find it confusing that > half of the logic is in here and half is outside the if block. Given each other > branch early-returns, I think we could move it below. No, the ET_TOUCH_MOVED case doesn't early-return. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:254: touch_ids_.size() == 1 && On 2014/06/02 23:12:18, mfomitchev wrote: > Hmm.. why do we only do this when there's only one finger remaining now? In the > previous version we activated the mapping on any release regardless of the > number of fingers. > > Also, I think we should set initial_touch_id_passthrough_mapping to -1 or > similar when the first finger is released. This is because I think the id may > get reused if this (or another) finger is pressed again after it is released. > Also if we do this, the condition code could be simplified: we can get rid of > first_finger_still_down_ and just have: > > if (initial_touch_id_passthrough_mapping == 0) { > if (event.touch_id() == initial_press_->touch_id()) { > initial_touch_id_passthrough_mapping = -1; > // Can return DISCARD here or let it fall through and return below > } else { > // The current rewriting code > } > } > Sure, this is a bit simpler. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:275: if (!initial_touch_id_passthrough_mapping_) On 2014/06/02 23:12:18, mfomitchev wrote: > This would need to be changed to <= 0 if we use -1. Done. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:281: initial_touch_id_passthrough_mapping_, On 2014/06/02 16:33:14, aboxhall wrote: > I don't follow this either. It looks like we only ever pass through the original > touch ID? Do we only handle the two-finger case? When you press two fingers, the first finger is ignored and the second one is passed through as-is (same touch id). The only tricky thing happens if you then release the second finger - then the first finger gets rewritten as the second finger's touch id from then on - that's what initial_touch_id_passthrough_mapping_ gives us. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:73: simulated_time_ms_ = 1000; On 2014/06/02 23:12:18, mfomitchev wrote: > Perhaps just get the time from the generator whenever you need it so that we > don't store it twice (in the test and in the generator)? Done. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:76: cursor_client()->ShowCursor(); On 2014/06/02 23:12:18, mfomitchev wrote: > Do we need to do this here considering we do this in SwitchTouchExplorationMode? Done. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:201: EXPECT_FALSE(IsInTouchToMouseMode()); On 2014/06/02 23:12:18, mfomitchev wrote: > Is there any way we can obtain the slop value from the config file and use it > here? Done. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:464: // finger is the only finger left touching, we should enter a mouse move mode, On 2014/06/02 23:12:18, mfomitchev wrote: > This comment seems wrong - we won't enter the mouse move mode. Done. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:477: // Release of finger 1 should be ignored. On 2014/06/02 23:12:18, mfomitchev wrote: > Would be good to have another test where we are releasing the second finger - > confirming that re-mapping works correctly This is already covered a bit by TwoFingerTouch, but I added another test that resembles this one more. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:482: // Release of finger 2 should be passed through. On 2014/06/02 23:12:18, mfomitchev wrote: > Might be worth doing a move, etc. before the release. Done.
Just a couple of fairly minor things: - I posted a bunch of comments for the .h file in the first review. Looks like you missed them? - Also looks like you may have missed this comment on the unit test: >It would be good to do some controller state validation here. Checking the state each step of the way might be too much, but I think we should at least end a bunch of tests with lifting all the fingers and confirming that the controller is in NO_FINGERS_DOWN state.
https://codereview.chromium.org/296403011/diff/60001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/296403011/diff/60001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:80: // If the tap timer should have fired by now but hasn't, run it now and Cool! Nit: It might be worth moving this up to the very beginning of the method. Just in case the part above evolves in the future into something more than it is now. https://codereview.chromium.org/296403011/diff/60001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/296403011/diff/60001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:159: // discarded. Nit: Might want to comment here what -1 means.
Thanks! I think I've addressed all of the feedback now. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:31: // Whenever two or more fingers are pressed, the events are passed through On 2014/05/28 17:47:45, mfomitchev wrote: > This doesn't seem entirely accurate - the minus one is applied iff the second > finger is pressed within the grace period. I tried to clarify, and I also tried to make it more clear that this is the short version, which glosses over details like the grace period. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:32: // withone finger removed - so if you swipe down with two fingers, the On 2014/05/28 17:47:45, mfomitchev wrote: > "withone" - missing space Done. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:40: // perform a gesture in that time, they enter touch exploration mode, and On 2014/05/28 17:47:45, mfomitchev wrote: > I'd say smth like "supported accessibility gesture" instead of "gesture" Done. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:43: // Also, if the user moves their single finger outside a certain slop region On 2014/05/28 17:47:45, mfomitchev wrote: > This won't be accurate once we support swipe gestures, since a swipe gesture > isn't performed until the finger is lifted. True, but let's update the comment then. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:60: // once in passthrough mode, if one finger is released, the remaining finger On 2014/05/28 17:47:45, mfomitchev wrote: > "remaining fingers continue" Done. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:114: GRACE_PERIOD, On 2014/05/28 17:47:45, mfomitchev wrote: > IMHO the naming is a bit confusing, since GRACE_PERIOD and SINGLE_TAP_PENDING > are both in "grace period", and DOUBLE_TAP_PASSED could be in grace period as > well. Perhaps rename these to SINGLE_TAP_PRESSED, SINGLE_TAP_RELEASED, > DOUBLE_TAP_PRESSED? I like that, thanks. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:116: // A single finger is down and after the grace period the user On 2014/05/28 17:47:45, mfomitchev wrote: > You could have multiple fingers down in this mode, you only need to have one > down during the grace period... Done. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:118: // We're now in touch exploration mode until this finger is lifted. On 2014/05/28 17:47:45, mfomitchev wrote: > "until all fingers are lifted" Done. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:119: TOUCH_EXPLORATION, On 2014/05/28 17:47:45, mfomitchev wrote: > I'd move this down next to PASSTHROUGH_MINUS_ONE - IMHO the ordering/grouping > would be more logical that way. Done. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:121: // The user pressed and released a single finger - a tap - but we have On 2014/05/28 17:47:45, mfomitchev wrote: > This sounds a bit confusing IMHO. I'd say like "..wait until the end of the > grace period to allow the user to tap the second time. If the second tap doesn't > occur within the grace period, we dispatch a mouse move at the location of the > first tap". Done. https://codereview.chromium.org/296403011/diff/1/ui/chromeos/touch_exploratio... ui/chromeos/touch_exploration_controller.h:133: // two ore more fingers are down, we subtract one finger but otherwise On 2014/05/28 17:47:45, mfomitchev wrote: > Doesn't seem like this is entirely accurate - based on implementation it looks > like we only substract one finger until one finger is released. Fixed. FWIW, we may need to tweak this -the mouse move caused by rewriting one finger as another is something resulting in strange effects like an unwanted pinch. But let's save that for the next change. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:132: SINGLE_TAP_PENDING, On 2014/06/02 16:33:14, aboxhall wrote: > SINGLE_TAP? > > The single tap has occurred; what's pending is to see what that means, but I > don't think the name needs to reflect that. Done. https://codereview.chromium.org/296403011/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:136: DOUBLE_TAP_PRESSED, On 2014/06/02 16:33:14, aboxhall wrote: > SECOND_TAP_PRESSED? > > (Since we can't quite mark it as a double-tap just yet.) I think "double tap" is better here - if you hold it down it's usually called a double-tap-hold or double-tap-long-press or something like that - I checked both Apple and Android documentation. https://codereview.chromium.org/296403011/diff/60001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/296403011/diff/60001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:80: // If the tap timer should have fired by now but hasn't, run it now and On 2014/06/04 20:39:30, mfomitchev wrote: > Cool! > Nit: It might be worth moving this up to the very beginning of the method. Just > in case the part above evolves in the future into something more than it is now. Done. https://codereview.chromium.org/296403011/diff/60001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/296403011/diff/60001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:159: // discarded. On 2014/06/04 20:39:30, mfomitchev wrote: > Nit: Might want to comment here what -1 means. Done.
+jamescook for ui/chromeos/OWNERS +sadrul for ui/aura/OWNERS
I will look at this today, apologies for delay.
You may have missed the comment about doing state validation in the the unit tests: >It would be good to do some controller state validation. Checking the state each step of the way might be too much, but I think we should at least end a bunch of tests with lifting all the fingers and confirming that the controller is in NO_FINGERS_DOWN state. That's somewhat minor though, so LGTM!
Looking good. A few nits and questions. https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:32: CHECK(tap_timer_.IsRunning()); Could this be a DCHECK? https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:136: ui::EventRewriteStatus TouchExplorationController::OnSingleTapPressed( optional: What do you think of naming these methods InSingleTapPressed() or RewriteForSingleTapPressed() or SingleTapPressedState() instead of OnSingleTapPressed()? The current names make me think the function is called like a callback when the tap event occurs, rather than any event happening while in that state. https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:247: initial_touch_id_passthrough_mapping_ = -1; Use a named constant instead of -1. https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:265: if (initial_touch_id_passthrough_mapping_ <= 0) Could compare directly to your named constant, since I presume this is checking for the -1 case. https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:11: #include "ui/events/event_handler.h" Could this be forward declared? https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:45: // perform a supported accessibility gesture in that time, they enter nit: Does "supported accessibility gesture" mean double-tap and/or second-finger down? Might give one example here. https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:72: // it remains in that mode until all fingers have been released. Nice explanation of how the feature works. https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:134: // second time. If the second tap doesn't occus within the grace period, nit: occus -> occur https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:148: TOUCH_EXPLORATION, Question: Can you get into a state where the device is in touch-exploration mode but no finger lift is generated? For example, if the device is running low on power and suspends will it come back up in touch-exploration with no fingers down? Is that OK? (I'm not familiar with how events are generated in weird cases like this, I just want to be sure the user doesn't get stuck.) https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:159: }; Again, nice description - very thorough. https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:196: generator_->set_current_location(gfx::Point(11, 12)); Thanks for using different values for x and y! https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:259: EXPECT_EQ(location_end, events[3]->location()); Maybe comment this block? I think I understand this but it's not immediately obvious why there's another synthesized move.
On 2014/06/05 16:39:02, mfomitchev wrote: > You may have missed the comment about doing state validation in the the unit > tests: > > >It would be good to do some controller state validation. Checking the state > each step of the way might be too much, but I think we should at least end a > bunch of tests with lifting all the fingers and confirming that the controller > is in NO_FINGERS_DOWN state. > > That's somewhat minor though, so LGTM! Oops, no, I really intended to do this too. Done - I added code to check for the "no fingers down" state in most of the tests, I think that's a good start.
https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:32: CHECK(tap_timer_.IsRunning()); On 2014/06/05 18:04:41, James Cook wrote: > Could this be a DCHECK? OK. Honest question, though - how many people run Chrome OS with DCHECKs enabled? I always use a release build on my device, I only test debug when running locally on my desktop - which isn't helpful for touch. https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:136: ui::EventRewriteStatus TouchExplorationController::OnSingleTapPressed( On 2014/06/05 18:04:41, James Cook wrote: > optional: What do you think of naming these methods InSingleTapPressed() or > RewriteForSingleTapPressed() or SingleTapPressedState() instead of > OnSingleTapPressed()? The current names make me think the function is called > like a callback when the tap event occurs, rather than any event happening while > in that state. Make sense. I'm fine with InSingleTapPressed, etc. I left OnTapTimerFired with "On". https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:247: initial_touch_id_passthrough_mapping_ = -1; On 2014/06/05 18:04:41, James Cook wrote: > Use a named constant instead of -1. Done. https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:265: if (initial_touch_id_passthrough_mapping_ <= 0) On 2014/06/05 18:04:41, James Cook wrote: > Could compare directly to your named constant, since I presume this is checking > for the -1 case. Done. https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:11: #include "ui/events/event_handler.h" On 2014/06/05 18:04:42, James Cook wrote: > Could this be forward declared? Done. https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:45: // perform a supported accessibility gesture in that time, they enter On 2014/06/05 18:04:41, James Cook wrote: > nit: Does "supported accessibility gesture" mean double-tap and/or second-finger > down? Might give one example here. This is supposed to mean a "gesture", like swipe. I clarified that. I don't think double-tap is normally considered a gesture, right? I agree it's a bit of a gray area here. https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:134: // second time. If the second tap doesn't occus within the grace period, On 2014/06/05 18:04:42, James Cook wrote: > nit: occus -> occur Done. https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:148: TOUCH_EXPLORATION, On 2014/06/05 18:04:41, James Cook wrote: > Question: Can you get into a state where the device is in touch-exploration mode > but no finger lift is generated? For example, if the device is running low on > power and suspends will it come back up in touch-exploration with no fingers > down? Is that OK? (I'm not familiar with how events are generated in weird > cases like this, I just want to be sure the user doesn't get stuck.) That's a good question. Turning spoken feedback off and on would fix it in the worst case, but we should try to prevent this from happening at all. I filed http://crbug.com/381428, will look at this later. https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:259: EXPECT_EQ(location_end, events[3]->location()); On 2014/06/05 18:04:42, James Cook wrote: > Maybe comment this block? I think I understand this but it's not immediately > obvious why there's another synthesized move. Done. It's because a touch release needs to be rewritten as a mouse move in touch exploration mode, in case its location isn't the same as the most recent touch_press or touch_move.
LGTM. Nice patch. https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:32: CHECK(tap_timer_.IsRunning()); On 2014/06/05 23:32:24, dmazzoni wrote: > On 2014/06/05 18:04:41, James Cook wrote: > > Could this be a DCHECK? > > OK. Honest question, though - how many people run Chrome OS with DCHECKs > enabled? I always use a release build on my device, I only test debug when > running locally on my desktop - which isn't helpful for touch. No one runs Chrome OS in debug. However, this function is only used for testing, and unit tests are run in debug on the waterfall linux_chromeos builds, so DCHECK seems good for test code. Also, I don't know if the compiler is smart enough to strip out unused *ForTesting() functions in shipping builds, but using DCHECK ensures that our shipping code size doesn't increase as much. https://codereview.chromium.org/296403011/diff/110001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/296403011/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:30: }; nit: "} // namespace" and no ;
https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/296403011/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:32: CHECK(tap_timer_.IsRunning()); On 2014/06/06 00:09:45, James Cook wrote: > On 2014/06/05 23:32:24, dmazzoni wrote: > > On 2014/06/05 18:04:41, James Cook wrote: > > > Could this be a DCHECK? > > > > OK. Honest question, though - how many people run Chrome OS with DCHECKs > > enabled? I always use a release build on my device, I only test debug when > > running locally on my desktop - which isn't helpful for touch. > > No one runs Chrome OS in debug. However, this function is only used for testing, > and unit tests are run in debug on the waterfall linux_chromeos builds, so > DCHECK seems good for test code. Also, I don't know if the compiler is smart > enough to strip out unused *ForTesting() functions in shipping builds, but using > DCHECK ensures that our shipping code size doesn't increase as much. Ah, of course - I wasn't thinking about this being test-only. Thanks. https://codereview.chromium.org/296403011/diff/110001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/296403011/diff/110001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:30: }; On 2014/06/06 00:09:45, James Cook wrote: > nit: "} // namespace" and no ; > Done.
@sky, could I have OWNERS review for ui/aura/test since sadrul is out?
https://codereview.chromium.org/296403011/diff/150001/ui/aura/test/event_gene... File ui/aura/test/event_generator.h (right): https://codereview.chromium.org/296403011/diff/150001/ui/aura/test/event_gene... ui/aura/test/event_generator.h:319: void UseSimulatedTime(); Instead of UseSimulateTime/Advance... how about a SetClock(scoped_ptr<Clock>)? Less API and lets you accomplish what you have here. See SimpleTestClock for the test one you would use.
And I didn't think Sadrul ever took vacation;)
Good idea! I used TickClock / SimpleTestTickClock since events want an always-increasing clock. On Fri, Jun 6, 2014 at 9:35 AM, <sky@chromium.org> wrote: > And I didn't think Sadrul ever took vacation;) > > https://codereview.chromium.org/296403011/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
LGTM
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/296403011/170001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/296403011/190001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
Message was sent while issue was closed.
Change committed as 275784 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
