Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
330763007:
Swipe Gestures for Accessibility (Closed)
DescriptionSwipe Gestures for Accessibility
Register that gestures have been received by the touch_exploration_controller in Accessibility Mode.
Swipe gestures are registered when the user begins a swipe and completes it before the time out period. If a swipe is successfully completed, the keyboard shortcut <shift>+<search>+direction will called.
If the grace period has elapsed, then the mode changes to touch exploration. If an additional finger has been added before the grace period has elapsed, then the mode changes to passthrough.
BUG=387304
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281614
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281780
Patch Set 1 #Patch Set 2 : Cleaned up Vlogging for gestures #Patch Set 3 : Rebased off Master #
Total comments: 4
Patch Set 4 : Gesture correctly implemented #Patch Set 5 : Time Out added for Gestures #
Total comments: 9
Patch Set 6 : Added keyboard events and fixed nits #
Total comments: 3
Patch Set 7 : Refactored unittest for gestures and added swipe tests #Patch Set 8 : Rebase off master #
Total comments: 39
Patch Set 9 : Addressed Comments and formatted file #Patch Set 10 : Updated overview comments #Patch Set 11 : Updated Gesture In Progress #Patch Set 12 : Removed line that Evy was working on #
Total comments: 3
Patch Set 13 : Nit Changes #Patch Set 14 : Added from gestures to passthrough #Patch Set 15 : Nit Changes #
Total comments: 20
Patch Set 16 : Addressed Alice's comments and changed how gesture events are processed. #
Total comments: 12
Patch Set 17 : Nit changes #Patch Set 18 : Rebase off master #Patch Set 19 : Nit Changes from Alice #Patch Set 20 : Nit changes #Patch Set 21 : Modified browser test to bypass gesture mode #Patch Set 22 : Fixed Debug checks #Patch Set 23 : Deleted cout #Patch Set 24 : Rebase off Master #Patch Set 25 : Rebase off master #Patch Set 26 : Fixed Memory Leak #
Messages
Total messages: 53 (0 generated)
Hacked gesture_provider_aura to get OnGestures in touch_exploration_controller to register gestures. Some questions I have: How does are gestures supposed to be registered through the client? How is handling_event_ changing its state? From the code in gesture_provider_aura, I don't see where it might be reassigned.
On 2014/06/19 23:02:51, lisayin wrote: > Hacked gesture_provider_aura to get OnGestures in touch_exploration_controller > to register gestures. > > Some questions I have: > How does are gestures supposed to be registered through the client? > How is handling_event_ changing its state? From the code in > gesture_provider_aura, I don't see where it might be reassigned. Handling event is modified using base::AutoReset, see the description in /base/auto_reset.h. https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/gestures... Can you clarify what you mean by "How does are gestures supposed to be registered through the client?" To receive gestures, you override GestureProviderAuraClient, and pass in your client to the GestureProviderAura constructor.
Lisa, it looks like you may have uploaded a diff relative to one of your own local branches - could you set the upstream to master and upload again so we can see exactly what you modified? On 2014/06/19 23:14:45, tdresser wrote: > Can you clarify what you mean by "How does are gestures supposed to be > registered through the client?" > To receive gestures, you override GestureProviderAuraClient, and pass in your > client to the GestureProviderAura constructor. She's doing that already: TouchExplorationController is overriding GestureProviderAuraClient and passing "this" in the GestureProviderAura client. Is the code around touch_exploration_controller.cc:158 correct usage? What seems to be happening is that the recognized gestures are all being stored as pending gestures rather than calling OnGestureEvent on the client.
On 2014/06/19 23:22:15, dmazzoni wrote: > Lisa, it looks like you may have uploaded a diff relative to one of your own > local branches - could you set the upstream to master and upload again so we can > see exactly what you modified? > > On 2014/06/19 23:14:45, tdresser wrote: > > Can you clarify what you mean by "How does are gestures supposed to be > > registered through the client?" > > To receive gestures, you override GestureProviderAuraClient, and pass in your > > client to the GestureProviderAura constructor. > > She's doing that already: TouchExplorationController is overriding > GestureProviderAuraClient and passing "this" in the GestureProviderAura client. > > Is the code around touch_exploration_controller.cc:158 correct usage? What seems > to be happening is that the recognized gestures are all being stored as pending > gestures rather than calling OnGestureEvent on the client. Hmmm, it'll be easier to tell when I can view the full diff. It's likely that you aren't passing a valid touch stream to the gesture provider. If you forget to forward the touch press correctly, the gesture provider will ignore all future touches.
On 2014/06/19 23:47:46, tdresser wrote: > On 2014/06/19 23:22:15, dmazzoni wrote: > > Lisa, it looks like you may have uploaded a diff relative to one of your own > > local branches - could you set the upstream to master and upload again so we > can > > see exactly what you modified? > > > > On 2014/06/19 23:14:45, tdresser wrote: > > > Can you clarify what you mean by "How does are gestures supposed to be > > > registered through the client?" > > > To receive gestures, you override GestureProviderAuraClient, and pass in > your > > > client to the GestureProviderAura constructor. > > > > She's doing that already: TouchExplorationController is overriding > > GestureProviderAuraClient and passing "this" in the GestureProviderAura > client. > > > > Is the code around touch_exploration_controller.cc:158 correct usage? What > seems > > to be happening is that the recognized gestures are all being stored as > pending > > gestures rather than calling OnGestureEvent on the client. > > Hmmm, it'll be easier to tell when I can view the full diff. > > It's likely that you aren't passing a valid touch stream to the gesture > provider. If you forget to forward the touch press correctly, the gesture > provider will ignore all future touches. I've uploaded the the new version after setting upstream to master.
https://codereview.chromium.org/330763007/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/330763007/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:197: gesture_provider_.OnTouchEvent(event); I think you're forwarding the same event to the gesture detector twice - once here, and again when you call InGestureInProgress, below. https://codereview.chromium.org/330763007/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:319: gesture_provider_.OnTouchEventAck(true); Why true here and false otherwise?
https://codereview.chromium.org/330763007/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/330763007/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:197: gesture_provider_.OnTouchEvent(event); On 2014/06/20 16:26:54, dmazzoni wrote: > I think you're forwarding the same event to the gesture detector twice - once > here, and again when you call InGestureInProgress, below. Fixed. https://codereview.chromium.org/330763007/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:319: gesture_provider_.OnTouchEventAck(true); On 2014/06/20 16:26:54, dmazzoni wrote: > Why true here and false otherwise? To be honest, it was something that I was just trying out to see if it worked or not. In the description for OnTouchEventAck of filtered_gesture_provider, which gesture_provider_aura calls, it said: // To be called upon ack of an event that was forwarded after a successful // call to |OnTouchEvent()|. So since a touch released or touch cancelled signified the end of a gesture, I thought it would make sense that after it collected all the events, then tell the provider that the events have finished being collected. Also, when I tested it out on the Chromebooks, the gesture provider doesn't seem to acknowledge that a swipe has occurred unless I set it to true.
On 2014/06/20 21:34:39, lisayin wrote: > https://codereview.chromium.org/330763007/diff/40001/ui/chromeos/touch_explor... > File ui/chromeos/touch_exploration_controller.cc (right): > > https://codereview.chromium.org/330763007/diff/40001/ui/chromeos/touch_explor... > ui/chromeos/touch_exploration_controller.cc:197: > gesture_provider_.OnTouchEvent(event); > On 2014/06/20 16:26:54, dmazzoni wrote: > > I think you're forwarding the same event to the gesture detector twice - once > > here, and again when you call InGestureInProgress, below. > > Fixed. > > https://codereview.chromium.org/330763007/diff/40001/ui/chromeos/touch_explor... > ui/chromeos/touch_exploration_controller.cc:319: > gesture_provider_.OnTouchEventAck(true); > On 2014/06/20 16:26:54, dmazzoni wrote: > > Why true here and false otherwise? > > To be honest, it was something that I was just trying out to see if it worked or > not. > > In the description for OnTouchEventAck of filtered_gesture_provider, which > gesture_provider_aura calls, it said: > // To be called upon ack of an event that was forwarded after a successful > // call to |OnTouchEvent()|. > > So since a touch released or touch cancelled signified the end of a gesture, I > thought it would make sense that after it collected all the events, then tell > the provider that the events have finished being collected. > > Also, when I tested it out on the Chromebooks, the gesture provider doesn't seem > to acknowledge that a swipe has occurred unless I set it to true. I believe you should always be passing |true| to |OnTouchEventAck|, though I'm not completely clear on what you're trying to do here. Passing |false| will prevent the gesture detector from creating gestures from the touch (with a few exceptions). For instance, if you pass |false| for a touch release, you'll never get a tap gesture, you'll only get a tap cancel gesture.
On 2014/06/20 21:39:07, tdresser wrote: > On 2014/06/20 21:34:39, lisayin wrote: > > > https://codereview.chromium.org/330763007/diff/40001/ui/chromeos/touch_explor... > > File ui/chromeos/touch_exploration_controller.cc (right): > > > > > https://codereview.chromium.org/330763007/diff/40001/ui/chromeos/touch_explor... > > ui/chromeos/touch_exploration_controller.cc:197: > > gesture_provider_.OnTouchEvent(event); > > On 2014/06/20 16:26:54, dmazzoni wrote: > > > I think you're forwarding the same event to the gesture detector twice - > once > > > here, and again when you call InGestureInProgress, below. > > > > Fixed. > > > > > https://codereview.chromium.org/330763007/diff/40001/ui/chromeos/touch_explor... > > ui/chromeos/touch_exploration_controller.cc:319: > > gesture_provider_.OnTouchEventAck(true); > > On 2014/06/20 16:26:54, dmazzoni wrote: > > > Why true here and false otherwise? > > > > To be honest, it was something that I was just trying out to see if it worked > or > > not. > > > > In the description for OnTouchEventAck of filtered_gesture_provider, which > > gesture_provider_aura calls, it said: > > // To be called upon ack of an event that was forwarded after a successful > > // call to |OnTouchEvent()|. > > > > So since a touch released or touch cancelled signified the end of a gesture, I > > thought it would make sense that after it collected all the events, then tell > > the provider that the events have finished being collected. > > > > Also, when I tested it out on the Chromebooks, the gesture provider doesn't > seem > > to acknowledge that a swipe has occurred unless I set it to true. > > I believe you should always be passing |true| to |OnTouchEventAck|, though I'm > not completely clear on what you're trying to do here. > > Passing |false| will prevent the gesture detector from creating gestures from > the touch (with a few exceptions). > > For instance, if you pass |false| for a touch release, you'll never get a tap > gesture, you'll only get a tap cancel gesture. Oops, I flipped the meaning of the parameter to |OnTouchEventAck| in my comment above. You should always be passing |false|, as passing |true| will prevent gestures from being dispatched.
Fixed the original problems with gestures and only register gestures that finish before the timer runs out.
https://codereview.chromium.org/330763007/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/330763007/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:195: // If the user moves fast enough and far enough Nit: indent the comment to match inside the block. https://codereview.chromium.org/330763007/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:202: // Otherwise, if the user moves far enough from the initial touch location Nit: don't put comments in-between the final } of the last block and the else, to avoid any possible confusion. I suggest you put both comments inside the blocks rather than outside. https://codereview.chromium.org/330763007/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:313: gesture_provider_.OnTouchEvent(event); If you're throwing away "pressed" events for any fingers other than the first, i don't think you should pass those events to the gesture provider from other fingers. https://codereview.chromium.org/330763007/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:417: if (state_ != SINGLE_TAP_RELEASED && state_ != SINGLE_TAP_PRESSED && Nit: for readability, when there are multiple lines of a conditional, put one condition on each line, like this - and use braces around the "return" (whenever either the conditional or the block is multi-line). if (state_ != SINGLE_TAP_RELEASED && state_ != SINGLE_TAP_PRESSED && state_ != GESTURE_IN_PROGRESS) { return; } https://codereview.chromium.org/330763007/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/330763007/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:26: class GestureProviderAura; Nit: sort these (alphabetize) https://codereview.chromium.org/330763007/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:129: // The gesture provider keeps track of all the touch events after Put an extra line at the top of this comment block that says "Overridden from GestureProviderAuraClient." This is convention we have that in any class that inherits from one or more other classes, we always clearly identify which functions are inheriting from which parent classes, and which ones are new. In this case, everything is new except this one. Here's an example of a class that inherits from two others and calls out two overridden functions: https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/... https://codereview.chromium.org/330763007/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:170: // We're currently making a gesture. Add a bit more detail here: explain how we enter this mode because we otherwise would have been in touch exploration mode, but the velocity is greater than a certain threshold, and explain when we exit this state. https://codereview.chromium.org/330763007/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:235: // Gesture Handler to interpret the touch events Nit: end with period.
https://codereview.chromium.org/330763007/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/330763007/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:313: gesture_provider_.OnTouchEvent(event); On 2014/06/23 15:50:44, dmazzoni wrote: > If you're throwing away "pressed" events for any fingers other than the first, i > don't think you should pass those events to the gesture provider from other > fingers. Just to clarify, I should check the id of the event and only process the events that match the id of the initial press.
Added a keyboard event to dispatch when a swipe is successfully completed.
The keyboard events are looking good https://codereview.chromium.org/330763007/diff/100001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/330763007/diff/100001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:457: VLOG(0) << " \n Gesture Triggered: " << gesture->name(); Want to log this no matter what the gesture? https://codereview.chromium.org/330763007/diff/100001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:496: // VKEY_LWIN is the search key on ChromeOS. How about a constant at the top of the file: const ui::KeyboardCode kChromeOSSearchKey = ui::VKEY_LWIN https://codereview.chromium.org/330763007/diff/100001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/330763007/diff/100001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:29: // TouchExplorationController is used in tandem with "Spoken Feedback" to Be sure to update this top comment to talk about gestures and mapping them to keys
Uploaded refactoring for unittest and unittests for swipe.
Almost there! Would love to see an lgtm from tdresser too. https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:205: // from the initial touch location, start gesture detection Nit: end sentence with period. https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:334: gesture_provider_.OnTouchEventAck(true); Are you sure you want to pass true here and false above? https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:30: // make the touch UI accessible. Gestures are mapped to accessible key This comment has a one-sentence intro, then a one-paragraph version, then a detailed long version. You should probably leave this one-sentence intro alone, but add one sentence about gestures to the short version, and probably several sentences about gestures to the long version. The fact that it sends key events is a temporarily implementation detail, probably good for the long version only. In the short version, focus on the user experience. https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:197: // period, the gesture will be collected and sent to the GestureClient. "sent to the GestureClient" is an implementation detail. You should say that the gesture will be interpreted and used to control the UI via discrete actions - currently by synthesizing key events corresponding to each gesture. https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:238: EXPECT_EQ(key_event1->key_code(), key_event2->key_code()); Assert flags also - to check that modifier keys are the same https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:256: void SwipeSuccessfullyCompleted(const ScopedVector<ui::Event>& events, Maybe call this "AssertDirectionalNavigationEvents" or something like that - you're asserting that the key events from a successful swipe were generated, not that the swipe was successful. https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1103: gfx::Point second_location(delta_distance, 1); Nit: indentation https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1104: generator_->MoveTouch(second_location); Wouldn't this have the same timestamp and therefore infinite velocity? I feel like you need to increment time by 1 ms or something - if not, why not?
https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:203: if (delta_distance > gesture_detector_config_.touch_slop) { Invert this condition to reduce nesting. if (delta_distance <= gesture_detector_config_.touch_slop) return EVENT_REWRITE_DISCARD; https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:212: } else { This "else" is unnecessary. https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:331: } Indentation. https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:334: gesture_provider_.OnTouchEventAck(true); On 2014/06/25 05:06:02, dmazzoni wrote: > Are you sure you want to pass true here and false above? I'm fairly certain this should be "false". https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:475: switch (gesture->type()) { Unless you're likely to support other gestures, it would be simpler to just: if (gesture->type() == ET_GESTURE_SWIPE) { OnSwipeEvent(gesture); return; } if (current_touch_ids_.size()) { ... https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:480: if (current_touch_ids_.size() != 0) { From the style guide: "if one part of an if-else statement uses curly braces, the other part must too:". https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:556: OnGestureEvent(*i); Just making sure - there's no way that OnGestureEvent could cause an aura::Window to be destroyed, is there? https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:21: Remove extra newline. https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:110: all_events[i]->IsGestureEvent()) { Indentation. git cl format is worth taking a look at: https://code.google.com/p/chromium/wiki/ClangFormat https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:427: AdvanceSimulatedTimePastTapDelay(); ///////////???? (///////////????)? https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:581: gfx::Point touch2_location(10, 11); A bunch of indentation issues around here.
https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:556: OnGestureEvent(*i); On 2014/06/25 14:48:40, tdresser wrote: > Just making sure - there's no way that OnGestureEvent could cause an > aura::Window to be destroyed, is there? As of this change, no - but in the future you should be able to control the whole computer with accessibility gestures and it's quite possible a gesture could result in closing a Window. What's your concern exactly?
https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:556: OnGestureEvent(*i); On 2014/06/25 15:39:44, dmazzoni wrote: > On 2014/06/25 14:48:40, tdresser wrote: > > Just making sure - there's no way that OnGestureEvent could cause an > > aura::Window to be destroyed, is there? > > As of this change, no - but in the future you should be able to control the > whole computer with accessibility gestures and it's quite possible a gesture > could result in closing a Window. What's your concern exactly? That would make things tricky. If GetAndResetPendingGestures returns several gestures, and one of them destroys a window, the later gestures will try to target to the destroyed Window. This causes some nasty crashes. We process this list here: https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window_eve... normally, and in that case, we check if the event target has been destroyed after each gesture is sent, and stop processing gestures if that is the case.
https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:556: OnGestureEvent(*i); On 2014/06/25 15:57:31, tdresser wrote: > On 2014/06/25 15:39:44, dmazzoni wrote: > > On 2014/06/25 14:48:40, tdresser wrote: > > > Just making sure - there's no way that OnGestureEvent could cause an > > > aura::Window to be destroyed, is there? > > > > As of this change, no - but in the future you should be able to control the > > whole computer with accessibility gestures and it's quite possible a gesture > > could result in closing a Window. What's your concern exactly? > > That would make things tricky. > > If GetAndResetPendingGestures returns several gestures, and one of them destroys > a window, the later gestures will try to target to the destroyed Window. This > causes some nasty crashes. > > We process this list here: > https://code.google.com/p/chromium/codesearch#chromium/src/ui/aura/window_eve... > normally, and in that case, we check if the event target has been destroyed > after each gesture is sent, and stop processing gestures if that is the case. > Though depending on how you target the command to destroy the window, it might work fine.
https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:203: if (delta_distance > gesture_detector_config_.touch_slop) { On 2014/06/25 14:48:40, tdresser wrote: > Invert this condition to reduce nesting. > > if (delta_distance <= gesture_detector_config_.touch_slop) > return EVENT_REWRITE_DISCARD; Done. https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:205: // from the initial touch location, start gesture detection On 2014/06/25 05:06:02, dmazzoni wrote: > Nit: end sentence with period. Done. https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:212: } else { On 2014/06/25 14:48:40, tdresser wrote: > This "else" is unnecessary. Done. https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:331: } On 2014/06/25 14:48:40, tdresser wrote: > Indentation. Done. https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:334: gesture_provider_.OnTouchEventAck(true); On 2014/06/25 05:06:02, dmazzoni wrote: > Are you sure you want to pass true here and false above? Done. https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:475: switch (gesture->type()) { On 2014/06/25 14:48:40, tdresser wrote: > Unless you're likely to support other gestures, it would be simpler to just: > > if (gesture->type() == ET_GESTURE_SWIPE) { > OnSwipeEvent(gesture); > return; > } > if (current_touch_ids_.size()) { > ... Done. https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:480: if (current_touch_ids_.size() != 0) { On 2014/06/25 14:48:40, tdresser wrote: > From the style guide: "if one part of an if-else statement uses curly braces, > the other part must too:". Done. https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:30: // make the touch UI accessible. Gestures are mapped to accessible key On 2014/06/25 05:06:02, dmazzoni wrote: > This comment has a one-sentence intro, then a one-paragraph version, then a > detailed long version. > > You should probably leave this one-sentence intro alone, but add one sentence > about gestures to the short version, and probably several sentences about > gestures to the long version. The fact that it sends key events is a temporarily > implementation detail, probably good for the long version only. In the short > version, focus on the user experience. Done. https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:197: // period, the gesture will be collected and sent to the GestureClient. On 2014/06/25 05:06:02, dmazzoni wrote: > "sent to the GestureClient" is an implementation detail. You should say that the > gesture will be interpreted and used to control the UI via discrete actions - > currently by synthesizing key events corresponding to each gesture. Done. https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:21: On 2014/06/25 14:48:41, tdresser wrote: > Remove extra newline. Done. https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:110: all_events[i]->IsGestureEvent()) { On 2014/06/25 14:48:40, tdresser wrote: > Indentation. > > git cl format is worth taking a look at: > https://code.google.com/p/chromium/wiki/ClangFormat Done. https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:238: EXPECT_EQ(key_event1->key_code(), key_event2->key_code()); On 2014/06/25 05:06:02, dmazzoni wrote: > Assert flags also - to check that modifier keys are the same Done. https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:256: void SwipeSuccessfullyCompleted(const ScopedVector<ui::Event>& events, On 2014/06/25 05:06:02, dmazzoni wrote: > Maybe call this "AssertDirectionalNavigationEvents" or something like that - > you're asserting that the key events from a successful swipe were generated, not > that the swipe was successful. Done. https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:427: AdvanceSimulatedTimePastTapDelay(); ///////////???? On 2014/06/25 14:48:41, tdresser wrote: > (///////////????)? Done. https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:581: gfx::Point touch2_location(10, 11); On 2014/06/25 14:48:41, tdresser wrote: > A bunch of indentation issues around here. Done. https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1103: gfx::Point second_location(delta_distance, 1); On 2014/06/25 05:06:02, dmazzoni wrote: > Nit: indentation Done. https://codereview.chromium.org/330763007/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1104: generator_->MoveTouch(second_location); On 2014/06/25 05:06:02, dmazzoni wrote: > Wouldn't this have the same timestamp and therefore infinite velocity? I feel > like you need to increment time by 1 ms or something - if not, why not? Done.
https://codereview.chromium.org/330763007/diff/220001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/330763007/diff/220001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:104: if (all_events[i]->IsMouseEvent() || all_events[i]->IsTouchEvent() || A previous comment from Dominic: When there are multiple conditions separated by && and ||, prefer one per line. So, more like this: if (foo == 1 && bar == 2 && baz == 3) { DoSomething(); }
LGTM https://codereview.chromium.org/330763007/diff/220001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/330763007/diff/220001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:30: // make the touch UI accessible. Gestures are mapped to accessible key Should "accessible" be "accessibility"? https://codereview.chromium.org/330763007/diff/220001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/330763007/diff/220001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:494: EXPECT_EQ(0u, GetCapturedLocatedEvents().size()); It looks like you're caching |GetCapturedLocatedEvents()|, and then not using the cached value.
lgtm
GestureInProgress now changes to Evy's implemented passthrough mode once additional fingers have been added before the grace period ends.
FYI only (i.e. this shouldn't block committing). Haven't looked at the test yet. https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:57: bool TouchExplorationController::IsInGestureInProgressStateForTesting() const { suggestion: should we just have a GetStateForTesting() method instead? https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:198: float delta_distance = nit: I think this should just be 'distance' https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:206: if (delta_distance <= gesture_detector_config_.touch_slop) Presumably we don't need to compute delta_time and velocity before we early-out here, since we're not using them in this check. Also, a comment explaining this early out would probably be helpful. https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:209: // If the user moves fast enough and far enough Re-flow this comment (I think tab should do it) Also, does "far enough from the initial touch location" simply mean outside the slop region? Since you're early outing above, that should be self explanatory if you add a comment to the early out, so you probably don't need to mention it here. https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:336: scoped_ptr<ScopedVector<ui::GestureEvent> > gestures( Since you're not using |gestures|, you can use ignore_result() here: https://code.google.com/p/chromium/codesearch#chromium/src/base/macros.h&q=ig... https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:348: if (last_touch_exploration_ == NULL) { Should this be uncommented or removed? https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:519: scoped_ptr<ScopedVector<ui::GestureEvent> > gestures( Similarly, you can use ignore_result() here. https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:626: scoped_ptr<ScopedVector<ui::GestureEvent> > gestures; You can initialise gestures with GetAndResetPendingGestures here instead of using reset() on the next line. https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:628: if (gestures != NULL) { if (gestures) should work I think...
https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:57: bool TouchExplorationController::IsInGestureInProgressStateForTesting() const { On 2014/07/02 16:45:57, aboxhall wrote: > suggestion: should we just have a GetStateForTesting() method instead? The state enum is private so I don't think that we would be able to access it to be able to compare states in the testing file. https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:336: scoped_ptr<ScopedVector<ui::GestureEvent> > gestures( On 2014/07/02 16:45:58, aboxhall wrote: > Since you're not using |gestures|, you can use ignore_result() here: > https://code.google.com/p/chromium/codesearch#chromium/src/base/macros.h&q=ig... Will I still need to cast the return from GetAndResetPendingGestures to a scoped pointer?
https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:198: float delta_distance = On 2014/07/02 16:45:57, aboxhall wrote: > nit: I think this should just be 'distance' Done. https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:206: if (delta_distance <= gesture_detector_config_.touch_slop) On 2014/07/02 16:45:57, aboxhall wrote: > Presumably we don't need to compute delta_time and velocity before we early-out > here, since we're not using them in this check. > > Also, a comment explaining this early out would probably be helpful. Done. https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:209: // If the user moves fast enough and far enough On 2014/07/02 16:45:57, aboxhall wrote: > Re-flow this comment (I think tab should do it) > > Also, does "far enough from the initial touch location" simply mean outside the > slop region? Since you're early outing above, that should be self explanatory if > you add a comment to the early out, so you probably don't need to mention it > here. Done. https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:348: if (last_touch_exploration_ == NULL) { On 2014/07/02 16:45:57, aboxhall wrote: > Should this be uncommented or removed? Done. https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:519: scoped_ptr<ScopedVector<ui::GestureEvent> > gestures( On 2014/07/02 16:45:57, aboxhall wrote: > Similarly, you can use ignore_result() here. Done. https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:626: scoped_ptr<ScopedVector<ui::GestureEvent> > gestures; On 2014/07/02 16:45:57, aboxhall wrote: > You can initialise gestures with GetAndResetPendingGestures here instead of > using reset() on the next line. Done. https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:628: if (gestures != NULL) { On 2014/07/02 16:45:57, aboxhall wrote: > if (gestures) > should work I think... Done.
lgtm Nice tests! Just a few nits. https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:57: bool TouchExplorationController::IsInGestureInProgressStateForTesting() const { On 2014/07/02 17:09:50, lisayin wrote: > On 2014/07/02 16:45:57, aboxhall wrote: > > suggestion: should we just have a GetStateForTesting() method instead? > > The state enum is private so I don't think that we would be able to access it to > be able to compare states in the testing file. Ahh, that makes sense, thanks. https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:336: scoped_ptr<ScopedVector<ui::GestureEvent> > gestures( On 2014/07/02 17:09:50, lisayin wrote: > On 2014/07/02 16:45:58, aboxhall wrote: > > Since you're not using |gestures|, you can use ignore_result() here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/macros.h&q=ig... > > Will I still need to cast the return from GetAndResetPendingGestures to a scoped > pointer? I see you figured this out :) https://codereview.chromium.org/330763007/diff/300001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/330763007/diff/300001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:556: if (gestures) { nit: weird indent https://codereview.chromium.org/330763007/diff/300001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/330763007/diff/300001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:268: ui::KeyEvent shift_pressed( You could pull all these KeyEvents out as constants since you're not comparing the times in ConfirmEvents...() https://codereview.chromium.org/330763007/diff/300001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:281: static_cast<ui::KeyEvent*>(events[0])); You shouldn't need to static_cast these since ConfirmEvents...() takes plain Event*s. https://codereview.chromium.org/330763007/diff/300001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1058: ui::TouchEvent first_press(ui::ET_TOUCH_PRESSED, gfx::Point(0, 1), 0, Now()); While it's usually a good idea to initialize things right before you use them, this might be more readable if you initialize all the events one after the other, so you can explain the sequence of events and expectations as you do it. https://codereview.chromium.org/330763007/diff/300001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1103: float delta_distance = gesture_detector_config_.touch_slop + 1; distance? (and everywhere else) https://codereview.chromium.org/330763007/diff/300001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1165: EXPECT_FALSE(IsInTouchToMouseMode()); nit: weird outdent
On 2014/07/02 17:19:44, lisayin wrote: > https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... > File ui/chromeos/touch_exploration_controller.cc (right): > > https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... > ui/chromeos/touch_exploration_controller.cc:198: float delta_distance = > On 2014/07/02 16:45:57, aboxhall wrote: > > nit: I think this should just be 'distance' > > Done. > > https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... > ui/chromeos/touch_exploration_controller.cc:206: if (delta_distance <= > gesture_detector_config_.touch_slop) > On 2014/07/02 16:45:57, aboxhall wrote: > > Presumably we don't need to compute delta_time and velocity before we > early-out > > here, since we're not using them in this check. > > > > Also, a comment explaining this early out would probably be helpful. > > Done. > > https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... > ui/chromeos/touch_exploration_controller.cc:209: // If the user moves fast > enough and far enough > On 2014/07/02 16:45:57, aboxhall wrote: > > Re-flow this comment (I think tab should do it) > > > > Also, does "far enough from the initial touch location" simply mean outside > the > > slop region? Since you're early outing above, that should be self explanatory > if > > you add a comment to the early out, so you probably don't need to mention it > > here. > > Done. > > https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... > ui/chromeos/touch_exploration_controller.cc:348: if (last_touch_exploration_ == > NULL) { > On 2014/07/02 16:45:57, aboxhall wrote: > > Should this be uncommented or removed? > > Done. > > https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... > ui/chromeos/touch_exploration_controller.cc:519: > scoped_ptr<ScopedVector<ui::GestureEvent> > gestures( > On 2014/07/02 16:45:57, aboxhall wrote: > > Similarly, you can use ignore_result() here. > > Done. > > https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... > ui/chromeos/touch_exploration_controller.cc:626: > scoped_ptr<ScopedVector<ui::GestureEvent> > gestures; > On 2014/07/02 16:45:57, aboxhall wrote: > > You can initialise gestures with GetAndResetPendingGestures here instead of > > using reset() on the next line. > > Done. > > https://codereview.chromium.org/330763007/diff/280001/ui/chromeos/touch_explo... > ui/chromeos/touch_exploration_controller.cc:628: if (gestures != NULL) { > On 2014/07/02 16:45:57, aboxhall wrote: > > if (gestures) > > should work I think... > > Done. Still LGTM.
https://codereview.chromium.org/330763007/diff/300001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/330763007/diff/300001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:556: if (gestures) { On 2014/07/02 18:00:13, aboxhall wrote: > nit: weird indent Done. https://codereview.chromium.org/330763007/diff/300001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/330763007/diff/300001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:268: ui::KeyEvent shift_pressed( On 2014/07/02 18:00:14, aboxhall wrote: > You could pull all these KeyEvents out as constants since you're not comparing > the times in ConfirmEvents...() ConfirmEvents...() doesn't take constant Events. https://codereview.chromium.org/330763007/diff/300001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:281: static_cast<ui::KeyEvent*>(events[0])); On 2014/07/02 18:00:13, aboxhall wrote: > You shouldn't need to static_cast these since ConfirmEvents...() takes plain > Event*s. Done. https://codereview.chromium.org/330763007/diff/300001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1058: ui::TouchEvent first_press(ui::ET_TOUCH_PRESSED, gfx::Point(0, 1), 0, Now()); On 2014/07/02 18:00:14, aboxhall wrote: > While it's usually a good idea to initialize things right before you use them, > this might be more readable if you initialize all the events one after the > other, so you can explain the sequence of events and expectations as you do it. Done. https://codereview.chromium.org/330763007/diff/300001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1103: float delta_distance = gesture_detector_config_.touch_slop + 1; On 2014/07/02 18:00:14, aboxhall wrote: > distance? (and everywhere else) Done. https://codereview.chromium.org/330763007/diff/300001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1165: EXPECT_FALSE(IsInTouchToMouseMode()); On 2014/07/02 18:00:13, aboxhall wrote: > nit: weird outdent Done.
lgtm
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lisayin@chromium.org/330763007/380001
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 lisayin@chromium.org
Modified touch_exploration_controller_browsertest.cc to bypass GestureInProgress and enter TouchExploration to pass tests.
The CQ bit was checked by lisayin@chromium.org
The CQ bit was unchecked by lisayin@chromium.org
I've fixed the tests so that the new implementation of gestures will pass the debug checks.
The CQ bit was checked by lisayin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lisayin@chromium.org/330763007/460001
The CQ bit was unchecked by lisayin@chromium.org
The CQ bit was checked by lisayin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lisayin@chromium.org/330763007/480001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
Message was sent while issue was closed.
Change committed as 281614
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/372173002/ by hashimoto@chromium.org. The reason for reverting is: TouchExplorationTest.GestureSwipe is failing on "Linux Chromium OS ASan LSan Tests (3)" http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20....
The CQ bit was checked by lisayin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lisayin@chromium.org/330763007/500001
The CQ bit was checked by lisayin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lisayin@chromium.org/330763007/500001
Message was sent while issue was closed.
Change committed as 281780 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
