Modified state flow for touch explore released in touch_exploration_controller.

Modified state flow for touch explore released in touch_exploration_controller.

Now when performing single tap, the tap timer to begin one finger passthrough is not started. This problem originally arose because touch explore released state and single tap released state are treated similarly in the code, but one finger passthrough should be a state reachable only from single tap released.

Another state change is that if the user presses in single tap/touch explore released and there is no previous touch exploration location, the user is redirected to the wait state.

TEST=TouchExplorationTest.SingleTapLongPress
BUG=402544
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290001

[aboxhall, 2014-08-13 20:34:33]

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

https://codereview.chromium.org/465543003/diff/1/ui/chromeos/touch_exploratio...
ui/chromeos/touch_exploration_controller_unittest.cc:846: // Touch moves should
not send any events through, since the user should be
Assert that no events are sent through?

https://codereview.chromium.org/465543003/diff/1/ui/chromeos/touch_exploratio...
ui/chromeos/touch_exploration_controller_unittest.cc:855:
EXPECT_EQ(ui::ET_MOUSE_MOVED, captured_events[0]->type());
Would it be possible to assert these events are captured at the point that they
should have been captured, rather than all at the end?

[evy, 2014-08-13 22:16:58]

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

https://codereview.chromium.org/465543003/diff/1/ui/chromeos/touch_exploratio...
ui/chromeos/touch_exploration_controller_unittest.cc:846: // Touch moves should
not send any events through, since the user should be
On 2014/08/13 20:34:32, aboxhall wrote:
> Assert that no events are sent through?

Done.

https://codereview.chromium.org/465543003/diff/1/ui/chromeos/touch_exploratio...
ui/chromeos/touch_exploration_controller_unittest.cc:855:
EXPECT_EQ(ui::ET_MOUSE_MOVED, captured_events[0]->type());
On 2014/08/13 20:34:33, aboxhall wrote:
> Would it be possible to assert these events are captured at the point that
they
> should have been captured, rather than all at the end?

Done.

[evy, 2014-08-13 22:41:24]

the other small state change has been added to this CL - and it's actually
closely related.

[dmazzoni, 2014-08-15 15:34:02]

Looks good, just one request

https://codereview.chromium.org/465543003/diff/80001/ui/chromeos/touch_explor...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/465543003/diff/80001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller.cc:323: if (state_ ==
SINGLE_TAP_RELEASED) {
This "if/else" block is now more than half of the function, so I think you
should split InSingleTapOrTouchExploreReleased into two functions,
InSingleTapReleased and InTouchExploreReleased. I'd far prefer to have a few
short lines of duplicate code between the two functions than to have this
function that does two increasingly different things.

[evy, 2014-08-15 16:14:57]

https://codereview.chromium.org/465543003/diff/80001/ui/chromeos/touch_explor...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/465543003/diff/80001/ui/chromeos/touch_explor...
ui/chromeos/touch_exploration_controller.cc:323: if (state_ ==
SINGLE_TAP_RELEASED) {
On 2014/08/15 15:34:01, dmazzoni wrote:
> This "if/else" block is now more than half of the function, so I think you
> should split InSingleTapOrTouchExploreReleased into two functions,
> InSingleTapReleased and InTouchExploreReleased. I'd far prefer to have a few
> short lines of duplicate code between the two functions than to have this
> function that does two increasingly different things.

Done.

[dmazzoni, 2014-08-15 16:54:45]

lgtm

https://codereview.chromium.org/465543003/diff/100001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/465543003/diff/100001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:800: case DOUBLE_TAP_PENDING: {
Nit: I don't think you need { } in this case. You only need it if you also
declare local variables, so that case above needs it.

[evy, 2014-08-15 17:03:03]

https://codereview.chromium.org/465543003/diff/100001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/465543003/diff/100001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:800: case DOUBLE_TAP_PENDING: {
On 2014/08/15 16:54:45, dmazzoni wrote:
> Nit: I don't think you need { } in this case. You only need it if you also
> declare local variables, so that case above needs it.

I declare passthrough_press

[dmazzoni, 2014-08-15 17:29:05]

lgtm

https://codereview.chromium.org/465543003/diff/100001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/465543003/diff/100001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:800: case DOUBLE_TAP_PENDING: {
On 2014/08/15 17:03:03, evy wrote:
> On 2014/08/15 16:54:45, dmazzoni wrote:
> > Nit: I don't think you need { } in this case. You only need it if you also
> > declare local variables, so that case above needs it.
> 
> I declare passthrough_press 

Oh, of course. Nevermind, this is fine.

[aboxhall, 2014-08-15 18:30:33]

lgtm

https://codereview.chromium.org/465543003/diff/100001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/465543003/diff/100001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:763: NOTREACHED() << "tap timer
fired in unrecognized state";
Suggestion: log the state?

https://codereview.chromium.org/465543003/diff/100001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:814: NOTREACHED() << "passthrough
timer fired in unrecognized state";
Similarly, maybe log state?

https://codereview.chromium.org/465543003/diff/100001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller_unittest.cc (right):

https://codereview.chromium.org/465543003/diff/100001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller_unittest.cc:962: ASSERT_EQ(0U,
captured_events.size());
This is way easier to follow now, thanks!

[evy, 2014-08-15 18:39:12]

https://codereview.chromium.org/465543003/diff/100001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller.cc (right):

https://codereview.chromium.org/465543003/diff/100001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:763: NOTREACHED() << "tap timer
fired in unrecognized state";
On 2014/08/15 18:30:32, aboxhall wrote:
> Suggestion: log the state?

Done.

https://codereview.chromium.org/465543003/diff/100001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller.cc:814: NOTREACHED() << "passthrough
timer fired in unrecognized state";
On 2014/08/15 18:30:32, aboxhall wrote:
> Similarly, maybe log state?

Done.

https://codereview.chromium.org/465543003/diff/100001/ui/chromeos/touch_explo...
File ui/chromeos/touch_exploration_controller_unittest.cc (right):

https://codereview.chromium.org/465543003/diff/100001/ui/chromeos/touch_explo...
ui/chromeos/touch_exploration_controller_unittest.cc:962: ASSERT_EQ(0U,
captured_events.size());
On 2014/08/15 18:30:32, aboxhall wrote:
> This is way easier to follow now, thanks!

Acknowledged.

[evy, 2014-08-15 20:08:47]

The CQ bit was checked by evy@chromium.org

[commit-bot: I haz the power, 2014-08-15 20:12:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/evy@chromium.org/465543003/120001

[commit-bot: I haz the power, 2014-08-15 20:31:01]

Committed patchset #7 (120001) as 290001

[sadrul, 2014-08-16 05:43:01]

On 2014/08/15 20:31:01, I haz the power (commit-bot) wrote:
> Committed patchset #7 (120001) as 290001

Reverted in https://codereview.chromium.org/483443002/ because it broke tests
(the revert commit includes the failure log). A build with the failures:
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...