Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
359453003:
Added accurate TouchToMouseMode testing to SplitTap (Closed)
DescriptionTo allow the user to return to TouchExplorationMode with mouse moves passed through, there is a one line change in the .cc file (386).
Manual cursor_client changes have been added to the unit tests.
A browser test has been added to more accurately test the cursor state.
TEST= TouchExplorationTest.SplitTap (ui_unittest) TouchExplorationTest.SplitTapExplore (browser_test)
BUG=389584
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282339
Patch Set 1 #Patch Set 2 : moved the new line of code to where it should be #Patch Set 3 : An old and new test that fail without the new added line. #
Total comments: 24
Patch Set 4 : #
Total comments: 8
Patch Set 5 : working on timed cursor test, link error with calling tap timer #Patch Set 6 : addressed comments on patch 4, still have linker error #Patch Set 7 : browser test now checks for hidden cursor aftera long press #
Total comments: 3
Patch Set 8 : removed extra browser test #
Total comments: 5
Patch Set 9 : #
Total comments: 2
Patch Set 10 : rebased #
Total comments: 10
Patch Set 11 : two split tap tests now fail without/pass with the added line #
Total comments: 14
Patch Set 12 : #Patch Set 13 : clearer comments and variable names for time simulation #Patch Set 14 : rebased #
Messages
Total messages: 32 (0 generated)
https://codereview.chromium.org/359453003/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc (right): https://codereview.chromium.org/359453003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:90: IN_PROC_BROWSER_TEST_F(TouchExplorationTest, SplitTapExplore) { Can you please add validation for the cursor client's state here? Eg: 1. Do a mouse move in the very beginning of the test, before the press. Check that mouse events are enabled and the cursor is visible 2. Check that mouse events are enabled and the cursor is hidden after the first press 3. Same after the first move 4. Check that mouse events are disabled and cursor is hidden after the second press. 5. Check that mouse events are enabled and the cursor is hidden after the release. https://codereview.chromium.org/359453003/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/359453003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:784: ClearCapturedEvents(); Add EXPECT_TRUE(IsInTouchToMouseMode()) https://codereview.chromium.org/359453003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:793: cursor_client()->DisableMouseEvents(); A comment explaining that we are simulating the behavior of the real device here would be helpful. https://codereview.chromium.org/359453003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:794: cursor_client()->HideCursor(); You can probably get rid of HideCursor() since like you said it stays hidden all along. https://codereview.chromium.org/359453003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:799: EXPECT_TRUE(IsInTouchToMouseMode()); Comment: Releasing the second finger should re-enable mouse events putting us back into the touch exploration mode. https://codereview.chromium.org/359453003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:807: } At the end of the test would be good to do the release of the first finger and do EXPECT_TRUE(IsInNoFingersDownState());
Hey Evy, I've added a few comments below: https://codereview.chromium.org/359453003/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc (right): https://codereview.chromium.org/359453003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:101: generator.MoveTouchId(gfx::Point(109, 209), 1); Add EXPECT_EQ(1, event_handler->num_touch_events()); https://codereview.chromium.org/359453003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:105: // touch press and release events which should send a click press and release. For documentation's sake, it would be good to mention that the tap/click goes to the location of the first finger. (Is that already tested somewhere?) https://codereview.chromium.org/359453003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:115: EXPECT_GT(event_handler->num_mouse_events(), 0); It's better to use EXPECT_EQ if you know how many mouse events you're expecting. https://codereview.chromium.org/359453003/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/359453003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:793: cursor_client()->DisableMouseEvents(); On 2014/06/27 16:36:05, mfomitchev wrote: > A comment explaining that we are simulating the behavior of the real device here > would be helpful. I'd also mention that you've added the browser test TouchExplorationTest.SplitTapExplore to test this without relying on manually setting the state.
https://codereview.chromium.org/359453003/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc (right): https://codereview.chromium.org/359453003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:90: IN_PROC_BROWSER_TEST_F(TouchExplorationTest, SplitTapExplore) { On 2014/06/27 16:36:05, mfomitchev wrote: > Can you please add validation for the cursor client's state here? Eg: > 1. Do a mouse move in the very beginning of the test, before the press. Check > that mouse events are enabled and the cursor is visible > 2. Check that mouse events are enabled and the cursor is hidden after the first > press > 3. Same after the first move > 4. Check that mouse events are disabled and cursor is hidden after the second > press. > 5. Check that mouse events are enabled and the cursor is hidden after the > release. Done. Though it turns out that mouse events aren't enabled after the first press. Does that sound right to you? https://codereview.chromium.org/359453003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:101: generator.MoveTouchId(gfx::Point(109, 209), 1); On 2014/06/27 17:01:07, tdanderson wrote: > Add EXPECT_EQ(1, event_handler->num_touch_events()); There actually aren't any touch events passed through here, it's the same thing as a mouse move. https://codereview.chromium.org/359453003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:105: // touch press and release events which should send a click press and release. On 2014/06/27 17:01:07, tdanderson wrote: > For documentation's sake, it would be good to mention that the tap/click goes to > the location of the first finger. (Is that already tested somewhere?) Yup! This is tested in the SplitTap unit test. Should I check it here too? https://codereview.chromium.org/359453003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:115: EXPECT_GT(event_handler->num_mouse_events(), 0); On 2014/06/27 17:01:07, tdanderson wrote: > It's better to use EXPECT_EQ if you know how many mouse events you're expecting. I noticed a comment above in another test: // Number of mouse events may be greater than 1 because of ET_MOUSE_ENTERED. Does that apply here? note: this line has now been removed (from Hangouts discussion). https://codereview.chromium.org/359453003/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/359453003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:784: ClearCapturedEvents(); On 2014/06/27 16:36:05, mfomitchev wrote: > Add EXPECT_TRUE(IsInTouchToMouseMode()) Done. https://codereview.chromium.org/359453003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:793: cursor_client()->DisableMouseEvents(); On 2014/06/27 16:36:05, mfomitchev wrote: > A comment explaining that we are simulating the behavior of the real device here > would be helpful. Done. https://codereview.chromium.org/359453003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:793: cursor_client()->DisableMouseEvents(); On 2014/06/27 17:01:07, tdanderson wrote: > On 2014/06/27 16:36:05, mfomitchev wrote: > > A comment explaining that we are simulating the behavior of the real device > here > > would be helpful. > > I'd also mention that you've added the browser test > TouchExplorationTest.SplitTapExplore to test this without relying on manually > setting the state. Done. https://codereview.chromium.org/359453003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:794: cursor_client()->HideCursor(); On 2014/06/27 16:36:05, mfomitchev wrote: > You can probably get rid of HideCursor() since like you said it stays hidden all > along. Done. https://codereview.chromium.org/359453003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:799: EXPECT_TRUE(IsInTouchToMouseMode()); On 2014/06/27 16:36:05, mfomitchev wrote: > Comment: Releasing the second finger should re-enable mouse events putting us > back into the touch exploration mode. Done. https://codereview.chromium.org/359453003/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:807: } On 2014/06/27 16:36:05, mfomitchev wrote: > At the end of the test would be good to do the release of the first finger and > do EXPECT_TRUE(IsInNoFingersDownState()); Done.
https://codereview.chromium.org/359453003/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc (right): https://codereview.chromium.org/359453003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:87: // This test makes sure that after the user clicks with split tap, One more thing: I've just realized that a change I've landed today will interfere with this test: https://codereview.chromium.org/330113002/ You'll need to rebase and WaitForResizeComplete() just like the other test does now (see the diff on touch_exploration_controller_browsertest.cc in the review). https://codereview.chromium.org/359453003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:90: IN_PROC_BROWSER_TEST_F(TouchExplorationTest, SplitTapExplore) { Right, that actually makes sense because the controller waits 300?ms before moving to the touch exploration mode. So they should be enabled after press, but get disabled after the move. If you are feeling ambitious, you can add a test (at the end of this one) where you press, advance the clock by 500ms, and then confirm the cursor is hidden. https://codereview.chromium.org/359453003/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc (right): https://codereview.chromium.org/359453003/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:91: IN_PROC_BROWSER_TEST_F(TouchExplorationTest, SplitTapExplore) { One more thing: I've just realized that a change I've landed today will interfere with this test: https://codereview.chromium.org/330113002/ You'll need to rebase and WaitForResizeComplete() just like the other test does now (see the diff on touch_exploration_controller_browsertest.cc in the review). https://codereview.chromium.org/359453003/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:102: //generator.MoveMouseTo(gfx::Point(30, 31)); Why is this commented out? https://codereview.chromium.org/359453003/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:106: // After a press the cursor should be hidden. Shown after immediately after press, hidden after move https://codereview.chromium.org/359453003/diff/60001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/359453003/diff/60001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:817: AdvanceSimulatedTimePastTapDelay(); Hmm.. why do we advance the time? Doesn't seems like this should be needed.
here's the error message: obj/chrome/browser/chromeos/accessibility/browser_tests.touch_exploration_controller_browsertest.o:touch_exploration_controller_browsertest.cc:function ui::TouchExplorationTest_PressMouseMove_Test::RunTestOnMainThread(): error: undefined reference to 'ui::TouchExplorationController::CallTapTimerNowForTesting()' collect2: ld returned 1 exit status ninja: build stopped: subcommand failed. https://codereview.chromium.org/359453003/diff/40001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc (right): https://codereview.chromium.org/359453003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:87: // This test makes sure that after the user clicks with split tap, On 2014/06/27 19:15:46, mfomitchev wrote: > One more thing: I've just realized that a change I've landed today will > interfere with this test: https://codereview.chromium.org/330113002/ > > You'll need to rebase and WaitForResizeComplete() just like the other test does > now (see the diff on touch_exploration_controller_browsertest.cc in the review). > Done. https://codereview.chromium.org/359453003/diff/40001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:90: IN_PROC_BROWSER_TEST_F(TouchExplorationTest, SplitTapExplore) { On 2014/06/27 19:15:46, mfomitchev wrote: > Right, that actually makes sense because the controller waits 300?ms before > moving to the touch exploration mode. So they should be enabled after press, but > get disabled after the move. If you are feeling ambitious, you can add a test > (at the end of this one) where you press, advance the clock by 500ms, and then > confirm the cursor is hidden. Working on it. Turns out this is more complicated than I thought. https://codereview.chromium.org/359453003/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc (right): https://codereview.chromium.org/359453003/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:91: IN_PROC_BROWSER_TEST_F(TouchExplorationTest, SplitTapExplore) { On 2014/06/27 19:15:46, mfomitchev wrote: > One more thing: I've just realized that a change I've landed today will > interfere with this test: https://codereview.chromium.org/330113002/ > > You'll need to rebase and WaitForResizeComplete() just like the other test does > now (see the diff on touch_exploration_controller_browsertest.cc in the review). > Done. https://codereview.chromium.org/359453003/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:102: //generator.MoveMouseTo(gfx::Point(30, 31)); On 2014/06/27 19:15:46, mfomitchev wrote: > Why is this commented out? whoops, fixed https://codereview.chromium.org/359453003/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:106: // After a press the cursor should be hidden. On 2014/06/27 19:15:46, mfomitchev wrote: > Shown after immediately after press, hidden after move Done. https://codereview.chromium.org/359453003/diff/60001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/359453003/diff/60001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:817: AdvanceSimulatedTimePastTapDelay(); On 2014/06/27 19:15:46, mfomitchev wrote: > Hmm.. why do we advance the time? Doesn't seems like this should be needed. We added single tap, which means that when you lift your finger from touch exploration you have the option to tap within 300ms anywhere and a click goes through at the last touch exploration location.
I will patch this in and help figure out the link error shortly On Mon, Jun 30, 2014 at 4:28 PM, <evy@chromium.org> wrote: > here's the error message: > > obj/chrome/browser/chromeos/accessibility/browser_tests. > touch_exploration_controller_browsertest.o:touch_exploration_controller_ > browsertest.cc:function > ui::TouchExplorationTest_PressMouseMove_Test::RunTestOnMainThread(): > error: > undefined reference to > 'ui::TouchExplorationController::CallTapTimerNowForTesting()' > collect2: ld returned 1 exit status > ninja: build stopped: subcommand failed. > > > > > https://codereview.chromium.org/359453003/diff/40001/ > chrome/browser/chromeos/accessibility/touch_exploration_controller_ > browsertest.cc > File > chrome/browser/chromeos/accessibility/touch_exploration_controller_ > browsertest.cc > (right): > > https://codereview.chromium.org/359453003/diff/40001/ > chrome/browser/chromeos/accessibility/touch_exploration_controller_ > browsertest.cc#newcode87 > chrome/browser/chromeos/accessibility/touch_exploration_controller_ > browsertest.cc:87: > // This test makes sure that after the user clicks with split tap, > On 2014/06/27 19:15:46, mfomitchev wrote: > >> One more thing: I've just realized that a change I've landed today >> > will > >> interfere with this test: https://codereview.chromium.org/330113002/ >> > > You'll need to rebase and WaitForResizeComplete() just like the other >> > test does > >> now (see the diff on touch_exploration_controller_browsertest.cc in >> > the review). > > > Done. > > > https://codereview.chromium.org/359453003/diff/40001/ > chrome/browser/chromeos/accessibility/touch_exploration_controller_ > browsertest.cc#newcode90 > chrome/browser/chromeos/accessibility/touch_exploration_controller_ > browsertest.cc:90: > IN_PROC_BROWSER_TEST_F(TouchExplorationTest, SplitTapExplore) { > On 2014/06/27 19:15:46, mfomitchev wrote: > >> Right, that actually makes sense because the controller waits 300?ms >> > before > >> moving to the touch exploration mode. So they should be enabled after >> > press, but > >> get disabled after the move. If you are feeling ambitious, you can add >> > a test > >> (at the end of this one) where you press, advance the clock by 500ms, >> > and then > >> confirm the cursor is hidden. >> > > Working on it. Turns out this is more complicated than I thought. > > > https://codereview.chromium.org/359453003/diff/60001/ > chrome/browser/chromeos/accessibility/touch_exploration_controller_ > browsertest.cc > File > chrome/browser/chromeos/accessibility/touch_exploration_controller_ > browsertest.cc > (right): > > https://codereview.chromium.org/359453003/diff/60001/ > chrome/browser/chromeos/accessibility/touch_exploration_controller_ > browsertest.cc#newcode91 > chrome/browser/chromeos/accessibility/touch_exploration_controller_ > browsertest.cc:91: > IN_PROC_BROWSER_TEST_F(TouchExplorationTest, SplitTapExplore) { > On 2014/06/27 19:15:46, mfomitchev wrote: > >> One more thing: I've just realized that a change I've landed today >> > will > >> interfere with this test: https://codereview.chromium.org/330113002/ >> > > You'll need to rebase and WaitForResizeComplete() just like the other >> > test does > >> now (see the diff on touch_exploration_controller_browsertest.cc in >> > the review). > > > Done. > > > https://codereview.chromium.org/359453003/diff/60001/ > chrome/browser/chromeos/accessibility/touch_exploration_controller_ > browsertest.cc#newcode102 > chrome/browser/chromeos/accessibility/touch_exploration_controller_ > browsertest.cc:102: > //generator.MoveMouseTo(gfx::Point(30, 31)); > On 2014/06/27 19:15:46, mfomitchev wrote: > >> Why is this commented out? >> > > whoops, fixed > > > https://codereview.chromium.org/359453003/diff/60001/ > chrome/browser/chromeos/accessibility/touch_exploration_controller_ > browsertest.cc#newcode106 > chrome/browser/chromeos/accessibility/touch_exploration_controller_ > browsertest.cc:106: > // After a press the cursor should be hidden. > On 2014/06/27 19:15:46, mfomitchev wrote: > >> Shown after immediately after press, hidden after move >> > > Done. > > > https://codereview.chromium.org/359453003/diff/60001/ui/ > chromeos/touch_exploration_controller_unittest.cc > File ui/chromeos/touch_exploration_controller_unittest.cc (right): > > https://codereview.chromium.org/359453003/diff/60001/ui/ > chromeos/touch_exploration_controller_unittest.cc#newcode817 > ui/chromeos/touch_exploration_controller_unittest.cc:817: > AdvanceSimulatedTimePastTapDelay(); > On 2014/06/27 19:15:46, mfomitchev wrote: > >> Hmm.. why do we advance the time? Doesn't seems like this should be >> > needed. > > We added single tap, which means that when you lift your finger from > touch exploration you have the option to tap within 300ms anywhere and a > click goes through at the last touch exploration location. > > https://codereview.chromium.org/359453003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
OK, you just need to make one gyp change.
In chrome/chrome_tests.gypi, the browser_tests target needs to directly
depend on ui_chromeos (when chromeos=1) - the linker is unable to allow
browser_tests to *directly* call a function in ui_chromeos without that
dependency.
Here's the exact patch:
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index a5e1a05..b12968c 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1791,6 +1791,7 @@
'dependencies': [
'../dbus/dbus.gyp:dbus_test_support',
'../build/linux/system.gyp:dbus',
+ '../ui/chromeos/ui_chromeos.gyp:ui_chromeos',
],
}],
['configuration_policy==0', {
On Tue, Jul 1, 2014 at 9:38 AM, Dominic Mazzoni <dmazzoni@chromium.org>
wrote:
> I will patch this in and help figure out the link error shortly
>
>
> On Mon, Jun 30, 2014 at 4:28 PM, <evy@chromium.org> wrote:
>
>> here's the error message:
>>
>> obj/chrome/browser/chromeos/accessibility/browser_tests.
>> touch_exploration_controller_browsertest.o:touch_exploration_controller_
>> browsertest.cc:function
>> ui::TouchExplorationTest_PressMouseMove_Test::RunTestOnMainThread():
>> error:
>> undefined reference to
>> 'ui::TouchExplorationController::CallTapTimerNowForTesting()'
>> collect2: ld returned 1 exit status
>> ninja: build stopped: subcommand failed.
>>
>>
>>
>>
>> https://codereview.chromium.org/359453003/diff/40001/
>> chrome/browser/chromeos/accessibility/touch_exploration_controller_
>> browsertest.cc
>> File
>> chrome/browser/chromeos/accessibility/touch_exploration_controller_
>> browsertest.cc
>> (right):
>>
>> https://codereview.chromium.org/359453003/diff/40001/
>> chrome/browser/chromeos/accessibility/touch_exploration_controller_
>> browsertest.cc#newcode87
>> chrome/browser/chromeos/accessibility/touch_exploration_controller_
>> browsertest.cc:87:
>> // This test makes sure that after the user clicks with split tap,
>> On 2014/06/27 19:15:46, mfomitchev wrote:
>>
>>> One more thing: I've just realized that a change I've landed today
>>>
>> will
>>
>>> interfere with this test: https://codereview.chromium.org/330113002/
>>>
>>
>> You'll need to rebase and WaitForResizeComplete() just like the other
>>>
>> test does
>>
>>> now (see the diff on touch_exploration_controller_browsertest.cc in
>>>
>> the review).
>>
>>
>> Done.
>>
>>
>> https://codereview.chromium.org/359453003/diff/40001/
>> chrome/browser/chromeos/accessibility/touch_exploration_controller_
>> browsertest.cc#newcode90
>> chrome/browser/chromeos/accessibility/touch_exploration_controller_
>> browsertest.cc:90:
>> IN_PROC_BROWSER_TEST_F(TouchExplorationTest, SplitTapExplore) {
>> On 2014/06/27 19:15:46, mfomitchev wrote:
>>
>>> Right, that actually makes sense because the controller waits 300?ms
>>>
>> before
>>
>>> moving to the touch exploration mode. So they should be enabled after
>>>
>> press, but
>>
>>> get disabled after the move. If you are feeling ambitious, you can add
>>>
>> a test
>>
>>> (at the end of this one) where you press, advance the clock by 500ms,
>>>
>> and then
>>
>>> confirm the cursor is hidden.
>>>
>>
>> Working on it. Turns out this is more complicated than I thought.
>>
>>
>> https://codereview.chromium.org/359453003/diff/60001/
>> chrome/browser/chromeos/accessibility/touch_exploration_controller_
>> browsertest.cc
>> File
>> chrome/browser/chromeos/accessibility/touch_exploration_controller_
>> browsertest.cc
>> (right):
>>
>> https://codereview.chromium.org/359453003/diff/60001/
>> chrome/browser/chromeos/accessibility/touch_exploration_controller_
>> browsertest.cc#newcode91
>> chrome/browser/chromeos/accessibility/touch_exploration_controller_
>> browsertest.cc:91:
>> IN_PROC_BROWSER_TEST_F(TouchExplorationTest, SplitTapExplore) {
>> On 2014/06/27 19:15:46, mfomitchev wrote:
>>
>>> One more thing: I've just realized that a change I've landed today
>>>
>> will
>>
>>> interfere with this test: https://codereview.chromium.org/330113002/
>>>
>>
>> You'll need to rebase and WaitForResizeComplete() just like the other
>>>
>> test does
>>
>>> now (see the diff on touch_exploration_controller_browsertest.cc in
>>>
>> the review).
>>
>>
>> Done.
>>
>>
>> https://codereview.chromium.org/359453003/diff/60001/
>> chrome/browser/chromeos/accessibility/touch_exploration_controller_
>> browsertest.cc#newcode102
>> chrome/browser/chromeos/accessibility/touch_exploration_controller_
>> browsertest.cc:102:
>> //generator.MoveMouseTo(gfx::Point(30, 31));
>> On 2014/06/27 19:15:46, mfomitchev wrote:
>>
>>> Why is this commented out?
>>>
>>
>> whoops, fixed
>>
>>
>> https://codereview.chromium.org/359453003/diff/60001/
>> chrome/browser/chromeos/accessibility/touch_exploration_controller_
>> browsertest.cc#newcode106
>> chrome/browser/chromeos/accessibility/touch_exploration_controller_
>> browsertest.cc:106:
>> // After a press the cursor should be hidden.
>> On 2014/06/27 19:15:46, mfomitchev wrote:
>>
>>> Shown after immediately after press, hidden after move
>>>
>>
>> Done.
>>
>>
>> https://codereview.chromium.org/359453003/diff/60001/ui/
>> chromeos/touch_exploration_controller_unittest.cc
>> File ui/chromeos/touch_exploration_controller_unittest.cc (right):
>>
>> https://codereview.chromium.org/359453003/diff/60001/ui/
>> chromeos/touch_exploration_controller_unittest.cc#newcode817
>> ui/chromeos/touch_exploration_controller_unittest.cc:817:
>> AdvanceSimulatedTimePastTapDelay();
>> On 2014/06/27 19:15:46, mfomitchev wrote:
>>
>>> Hmm.. why do we advance the time? Doesn't seems like this should be
>>>
>> needed.
>>
>> We added single tap, which means that when you lift your finger from
>> touch exploration you have the option to tap within 300ms anywhere and a
>> click goes through at the last touch exploration location.
>>
>> https://codereview.chromium.org/359453003/
>>
>
>
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/359453003/diff/120001/ash/root_window_control... File ash/root_window_controller.h (right): https://codereview.chromium.org/359453003/diff/120001/ash/root_window_control... ash/root_window_controller.h:118: ui::TouchExplorationController* GetTouchExplorationController(); Creating GetTouchExplorationController() in RWC just for the test seems bad. Also, this would need to be surrounded by ifdef(OS_CHROMEOS). To make this a bit cleaner, I'd suggest creating a test class wrapping RWC and putting the method there. Take a look at WindowEventDispatcherTestApi and how it's used as an example. You can then put the tests class into a chromeos-specific folder or add "_chromeos" suffix to the filename to make sure it's not compiled on other oses. Also a nit (not specific to this change): I think it would be good to get rid of CallTapTimerNowForTesting() and instead inject base::MockTimer into TouchExplorationCOntroller and then operate the MockTimer. https://codereview.chromium.org/359453003/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc (right): https://codereview.chromium.org/359453003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:160: content::WaitForResizeComplete(web_contents); Can you please put this setup logic into SetupOnMainThread() (or similar) rather than duplicating it in the two tests? Similarly, you may want to put the teardown logic into CleanUpOnMainThread() (although you'd have to make event_handler into a class member for that). https://codereview.chromium.org/359453003/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:173: generator.SetTickClock(scoped_ptr<base::TickClock>(simulated_clock_)); I'd just make simulated_clock_ into a local variable in this test and make it a scoped_ptr. I don't see much reason to make it a class member, and the way it's done here, it will actually leak for tests that don't use it.
I removed the extra browser test because it's ending up being fairly complicated to add, and I'd like to change that original line so that split tap works better. I'll go back after and try the extra test in its own CL.
https://codereview.chromium.org/359453003/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc (right): https://codereview.chromium.org/359453003/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:94: aura::Window* root_window = ash::Shell::GetInstance()->GetPrimaryRootWindow(); I think you need to call WaitForResizeComplete(), otherwise the test will be flaky. https://codereview.chromium.org/359453003/diff/140001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/359453003/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:682: // client is set up so this test will fail. Need to fix the comment. Also make it so the test fails without the fix like in the other test.
https://codereview.chromium.org/359453003/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc (right): https://codereview.chromium.org/359453003/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:94: aura::Window* root_window = ash::Shell::GetInstance()->GetPrimaryRootWindow(); On 2014/07/03 17:31:55, mfomitchev wrote: > I think you need to call WaitForResizeComplete(), otherwise the test will be > flaky. Done. https://codereview.chromium.org/359453003/diff/140001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/359453003/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:682: // client is set up so this test will fail. On 2014/07/03 17:31:55, mfomitchev wrote: > Need to fix the comment. Also make it so the test fails without the fix like in > the other test. Done.
I'd love to finish this up soon to get the bug fixed, so if you have any more comments please let me know.
LGTM with one nit https://codereview.chromium.org/359453003/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc (right): https://codereview.chromium.org/359453003/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:118: generator.MoveTouchId(gfx::Point(109, 209), 1); These magic numbers make me nervous; could this be expressed in terms of the slop distance?
https://codereview.chromium.org/359453003/diff/140001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/359453003/diff/140001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:682: // client is set up so this test will fail. Does it fail without the fix now? I thought you needed to manually set the cursor state after the tap to make it fail. https://codereview.chromium.org/359453003/diff/180001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc (right): https://codereview.chromium.org/359453003/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:134: content::WaitForResizeComplete(web_contents); Can you please put this setup logic into SetupOnMainThread() (or similar) rather than duplicating it in the two tests? Add the comment from the first test there as well. Similarly, you can consider putting the teardown logic into CleanUpOnMainThread() (although you'd have to make event_handler into a class member for that). https://codereview.chromium.org/359453003/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:149: // After a press the cursor should be shown after immediately after press, This comment needs fixing https://codereview.chromium.org/359453003/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:160: (initial_press.location() - gfx::Point(109, 209)).Length() / If you want to move slower that min swipe velocity, don't you need delta time to be greater than this value? As written, you will have the finger moving faster or slower than the minimum_swipe_velocity - depending on where the float precision error falls. https://codereview.chromium.org/359453003/diff/180001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/359453003/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:750: // Now tap and release at a different location. This should result in a Would be nice to clarify we are doing a long press, not just a tap and release, https://codereview.chromium.org/359453003/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:754: // touches from this finger should trigger mouse events. Nit: It doesn't seem we are testing that "touches from this finger" are triggering mouse moves... If we are not doing it, why update the comment?
https://codereview.chromium.org/359453003/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc (right): https://codereview.chromium.org/359453003/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:118: generator.MoveTouchId(gfx::Point(109, 209), 1); On 2014/07/08 21:07:49, aboxhall wrote: > These magic numbers make me nervous; could this be expressed in terms of the > slop distance? I'm doing this a different way now, with no distance change. https://codereview.chromium.org/359453003/diff/180001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc (right): https://codereview.chromium.org/359453003/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:134: content::WaitForResizeComplete(web_contents); On 2014/07/09 13:26:01, mfomitchev wrote: > Can you please put this setup logic into SetupOnMainThread() (or similar) rather > than duplicating it in the two tests? Add the comment from the first test there > as well. > Similarly, you can consider putting the teardown logic into > CleanUpOnMainThread() (although you'd have to make event_handler into a class > member for that). Done. https://codereview.chromium.org/359453003/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:149: // After a press the cursor should be shown after immediately after press, On 2014/07/09 13:26:02, mfomitchev wrote: > This comment needs fixing Done. https://codereview.chromium.org/359453003/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:160: (initial_press.location() - gfx::Point(109, 209)).Length() / On 2014/07/09 13:26:01, mfomitchev wrote: > If you want to move slower that min swipe velocity, don't you need delta time to > be greater than this value? As written, you will have the finger moving faster > or slower than the minimum_swipe_velocity - depending on where the float > precision error falls. I changed this to wait for the timeout instead. I "move" to the same place after the timeout time and then the user will go into touch exploration. What do you think of this strategy? Should I do this for the other browser test that I had moved to a different branch? https://codereview.chromium.org/359453003/diff/180001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/359453003/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:750: // Now tap and release at a different location. This should result in a On 2014/07/09 13:26:02, mfomitchev wrote: > Would be nice to clarify we are doing a long press, not just a tap and release, Done. https://codereview.chromium.org/359453003/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:754: // touches from this finger should trigger mouse events. On 2014/07/09 13:26:02, mfomitchev wrote: > Nit: It doesn't seem we are testing that "touches from this finger" are > triggering mouse moves... If we are not doing it, why update the comment? Done.
https://codereview.chromium.org/359453003/diff/220001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc (right): https://codereview.chromium.org/359453003/diff/220001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:159: gesture_detector_config_.double_tap_timeout + Using double_tap_timeout this way doesn't seem right. If you wait double_tap_timeout - you are supposed enter touch exploration mode regardless of whether you move or not. If you move far enough, you are supposed to enter TEM regardless of the time (provided you haven't generated a swipe). Also aboxhall@ made a comment about not using "magic numbers" for location.. https://codereview.chromium.org/359453003/diff/220001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/359453003/diff/220001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:49: void TouchExplorationController::CallTapTimerNowIfRunningForTesting() { Do we really need both of these methods? Seems like CallTapTimerNowIfRunningForTesting() alone should do. https://codereview.chromium.org/359453003/diff/220001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:69: void TouchExplorationController::SuppressVLOGsForTesting(bool suppress) { Why do we need to suppress logging for testing? PS: Realized that this line and the previous one I commented on weren't part of the your change - guess they appeared in the diff because of the rebase. So you don't need to fix this in this CL.
https://codereview.chromium.org/359453003/diff/220001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc (right): https://codereview.chromium.org/359453003/diff/220001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:159: gesture_detector_config_.double_tap_timeout + On 2014/07/09 19:20:37, mfomitchev wrote: > Using double_tap_timeout this way doesn't seem right. If you wait > double_tap_timeout - you are supposed enter touch exploration mode regardless of > whether you move or not. If you move far enough, you are supposed to enter TEM > regardless of the time (provided you haven't generated a swipe). > > Also aboxhall@ made a comment about not using "magic numbers" for location.. I need to change the comment to "Initiate touch explore by waiting for the tap timer timeout". The touch_move doesn't actually move location - oh wait here it does. See ToggleOnOff for how this should actually work, and I'll update this in the next patch. My update removes the use of magic numbers as well, since I don't use location anymore to go into touch exploration. https://codereview.chromium.org/359453003/diff/220001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/359453003/diff/220001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:49: void TouchExplorationController::CallTapTimerNowIfRunningForTesting() { On 2014/07/09 19:20:37, mfomitchev wrote: > Do we really need both of these methods? Seems like > CallTapTimerNowIfRunningForTesting() alone should do. The IfRunning version was added in the last CL I committed, where I run a bunch of permutations of different touch events. In these cases I didn't want to keep track of if the timer was running, but wanted to call it when the sequence of events were done. For the most part, though, I think it's better to only call the tap timer in unit tests if we know it's definitely running, which has been the case so far. If the timer actually isn't running, the DCHECK fails and that could be useful. https://codereview.chromium.org/359453003/diff/220001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:69: void TouchExplorationController::SuppressVLOGsForTesting(bool suppress) { On 2014/07/09 19:20:37, mfomitchev wrote: > Why do we need to suppress logging for testing? > > PS: Realized that this line and the previous one I commented on weren't part of > the your change - guess they appeared in the diff because of the rebase. So you > don't need to fix this in this CL. Yeah - also part of the permutations test CL. There are over 30000 touch events dispatched and I was manually turning off logging, but they still run by default and in the trybot tests so Dominic suggested I just write a function to make it so they're never called.
https://codereview.chromium.org/359453003/diff/220001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc (right): https://codereview.chromium.org/359453003/diff/220001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:159: gesture_detector_config_.double_tap_timeout + I think this is a bit artificial, but not terrible if that's the way you want to do it. I think the previous way was a bit cleaner. That's because timeout should technically occur without any moves - you are relying on the fallback mechanism for the case when the timer doesn't fire to trigger the transition here. But if that's how you want to do it, that's fine, just explain what you are doing in comments (i.e. that it's not really important as a move, it's just a way for you to fake the time advancing). https://codereview.chromium.org/359453003/diff/220001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/359453003/diff/220001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:69: void TouchExplorationController::SuppressVLOGsForTesting(bool suppress) { Ok. We are starting to get a lot of these ForTesting methods, polluting the interface and making it more difficult to figure out what this class actually does. It would be good to create a wrapper class for testing and put all these methods in there - see ui/aura/test/window_test_api for example.
https://codereview.chromium.org/359453003/diff/220001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc (right): https://codereview.chromium.org/359453003/diff/220001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:159: gesture_detector_config_.double_tap_timeout + On 2014/07/09 20:47:21, mfomitchev wrote: > I think this is a bit artificial, but not terrible if that's the way you want to > do it. I think the previous way was a bit cleaner. That's because timeout should > technically occur without any moves - you are relying on the fallback mechanism > for the case when the timer doesn't fire to trigger the transition here. But if > that's how you want to do it, that's fine, just explain what you are doing in > comments (i.e. that it's not really important as a move, it's just a way for you > to fake the time advancing). I think the only cleaner thing (this might be what you were referring to) was actually getting the touch exploration controller and advancing time - which is what I was doing in the other branch. I'll keep it this way for now, and might play around with the time advancement later. I'll update the comments/variable names to make it more clear what I'm doing. https://codereview.chromium.org/359453003/diff/220001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/359453003/diff/220001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:69: void TouchExplorationController::SuppressVLOGsForTesting(bool suppress) { On 2014/07/09 20:47:21, mfomitchev wrote: > Ok. We are starting to get a lot of these ForTesting methods, polluting the > interface and making it more difficult to figure out what this class actually > does. It would be good to create a wrapper class for testing and put all these > methods in there - see ui/aura/test/window_test_api for example. That's a good idea - I think I'll start it in a new branch because it isn't really related to this bug. Would you like me to add you as a reviewer on that, or is it fine to just get my hosts to look it over?
LGTM with nits, since the remaining changes are fairly minor.
https://codereview.chromium.org/359453003/diff/220001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc (right): https://codereview.chromium.org/359453003/diff/220001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:159: gesture_detector_config_.double_tap_timeout + I was actually referring to performing a move like you did before (and ensuring that swipe isn't generated by adjusting the time). But either way, it's fine. https://codereview.chromium.org/359453003/diff/220001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/359453003/diff/220001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:69: void TouchExplorationController::SuppressVLOGsForTesting(bool suppress) { Yup, doing it in a separate CL makes sense. Feel free to add me as a reviewer if you want, but you don't need to.
The CQ bit was checked by evy@google.com
The CQ bit was unchecked by evy@google.com
https://codereview.chromium.org/359453003/diff/220001/chrome/browser/chromeos... File chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc (right): https://codereview.chromium.org/359453003/diff/220001/chrome/browser/chromeos... chrome/browser/chromeos/accessibility/touch_exploration_controller_browsertest.cc:159: gesture_detector_config_.double_tap_timeout + On 2014/07/09 21:53:02, mfomitchev wrote: > I was actually referring to performing a move like you did before (and ensuring > that swipe isn't generated by adjusting the time). But either way, it's fine. I'm more comfortable with what I have now, mostly because gesture vs. touch exploration categorization is somewhat complicated right now and we're considering simplifying it in the future. https://codereview.chromium.org/359453003/diff/220001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/359453003/diff/220001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:69: void TouchExplorationController::SuppressVLOGsForTesting(bool suppress) { On 2014/07/09 21:53:03, mfomitchev wrote: > Yup, doing it in a separate CL makes sense. Feel free to add me as a reviewer if > you want, but you don't need to. Sounds good - thanks for the suggestion!
The CQ bit was checked by evy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/evy@chromium.org/359453003/280001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
lgtm
Message was sent while issue was closed.
Change committed as 282339 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
