Fix Touch Exploration tests so they don't break eager gesture detection.

Fix Touch Exploration tests so they don't break eager gesture detection.

Eager gesture detection is a little bit stricter about requiring
a valid event stream. The touch exploration tests give the
gesture detector an invalid event stream in a few places.

BUG=398106
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285983

[tdresser, 2014-07-28 18:31:43]

I've been running into issues with Touch Exploration tests giving the gesture
recognizer invalid touch event streams, during my work on eager gesture
detection. (https://codereview.chromium.org/393953012/)

This gets the tests into a state where they seem to be passing the gesture
detector reasonably sane touch sequences.

PTAL!

https://codereview.chromium.org/428673002/diff/1/ui/chromeos/touch_exploratio...
File ui/chromeos/touch_exploration_controller_unittest.cc (right):

https://codereview.chromium.org/428673002/diff/1/ui/chromeos/touch_exploratio...
ui/chromeos/touch_exploration_controller_unittest.cc:430:
cursor_client()->EnableMouseEvents();
I'm not super clear what's going on here. Is it OK to forcibly enable mouse
events?

I would have thought that they should be enabled by the
TouchExplorationController, but that doesn't appear to be happening.

[dmazzoni, 2014-07-28 18:52:00]

+lisayin

Lisa, could you look at this too?

[lisayin, 2014-07-28 19:14:55]

https://codereview.chromium.org/428673002/diff/1/chrome/browser/chromeos/acce...
File
chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc
(right):

https://codereview.chromium.org/428673002/diff/1/chrome/browser/chromeos/acce...
chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:144:
// gestures. Here, the touch move event is not important as a move, but
Nit: Move this comment to the previous test for clarity.

https://codereview.chromium.org/428673002/diff/1/ui/chromeos/touch_exploratio...
File ui/chromeos/touch_exploration_controller_unittest.cc (right):

https://codereview.chromium.org/428673002/diff/1/ui/chromeos/touch_exploratio...
ui/chromeos/touch_exploration_controller_unittest.cc:430:
cursor_client()->EnableMouseEvents();
On 2014/07/28 18:31:43, tdresser wrote:
> I'm not super clear what's going on here. Is it OK to forcibly enable mouse
> events?
> 
> I would have thought that they should be enabled by the
> TouchExplorationController, but that doesn't appear to be happening.

By default, passing true to SwitchTouchExplorationMode will disable mouse
events.

Currently, mouse events are only turned on if the state changes to
TOUCH_EXPLORATION. I think that the name of the function might be a little
misleading since it doesn't actually switch the state to TOUCH_EXPLORATION, it
makes a new controller to handle the accessible touch events. 

Although, Dominic, it doesn't look like mouse events are ever disabled after
they're turned on once. Should this be the case?

[dmazzoni, 2014-07-28 19:28:37]

+mfomitchev to help explain cursor_client()->EnableMouseEvents

https://codereview.chromium.org/428673002/diff/1/ui/chromeos/touch_exploratio...
File ui/chromeos/touch_exploration_controller_unittest.cc (right):

https://codereview.chromium.org/428673002/diff/1/ui/chromeos/touch_exploratio...
ui/chromeos/touch_exploration_controller_unittest.cc:430:
cursor_client()->EnableMouseEvents();
On 2014/07/28 19:14:54, lisayin wrote:
> Currently, mouse events are only turned on if the state changes to
> TOUCH_EXPLORATION. I think that the name of the function might be a little
> misleading since it doesn't actually switch the state to TOUCH_EXPLORATION, it
> makes a new controller to handle the accessible touch events. 

Yes, could we rename that?

> Although, Dominic, it doesn't look like mouse events are ever disabled after
> they're turned on once. Should this be the case?

I'm pretty sure they get re-enabled again automatically. Mikhail can explain
better.

[tdresser, 2014-07-28 20:11:30]

https://codereview.chromium.org/428673002/diff/1/chrome/browser/chromeos/acce...
File
chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc
(right):

https://codereview.chromium.org/428673002/diff/1/chrome/browser/chromeos/acce...
chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:144:
// gestures. Here, the touch move event is not important as a move, but
On 2014/07/28 19:14:54, lisayin wrote:
> Nit: Move this comment to the previous test for clarity.

I've duplicated the comment.

https://codereview.chromium.org/428673002/diff/1/ui/chromeos/touch_exploratio...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/428673002/diff/1/ui/chromeos/touch_exploratio...
ui/chromeos/touch_exploration_controller.cc:606:
DispatchEvent(mouse_move.get());
Do we need a |EnterTouchToMouseMode()| before this line?

[tdresser, 2014-07-28 20:18:53]

Looks like there was a bug where if you just pressed and release (with no touch
move events), no mouse move would be generated.

I've fixed it, which means there are no more scary looking calls to forcibly
enable mouse events.

[dmazzoni, 2014-07-28 20:44:29]

lgtm

[tdresser, 2014-07-28 20:46:48]

The CQ bit was checked by tdresser@chromium.org

[commit-bot: I haz the power, 2014-07-28 20:48:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tdresser@chromium.org/428673002/40001

[commit-bot: I haz the power, 2014-07-28 21:27:31]

Change committed as 285983

[hashimoto, 2014-07-29 05:11:17]

A revert of this CL has been created in
https://codereview.chromium.org/428643008/ by hashimoto@chromium.org.

The reason for reverting is: The "TouchExplorationTest.RewritesEventsWhenOn"
test failing on "Linux Chromium OS ASan LSan Tests (2)":
http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20....