Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(16)

Issue 2880043002: Implement touch exploration touch typing (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 2 weeks ago by David Tseng
Modified:
3 months, 3 weeks ago
CC:
chromium-reviews, kalyank, sadrul, oshima+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Implement touch exploration touch typing The functional change occurs for the following two event sequences. Each results in a synthesized tap at the anchor point. Single finger tap: Function name: InNoFingersDown State: SINGLE_TAP_PRESSED Function name: InSingleTapPressed State: SINGLE_TAP_RELEASED Function name: OnTapTimerFired State: NO_FINGERS_DOWN Touch exploration lift: Function name: InNoFingersDown State: SINGLE_TAP_PRESSED Function name: InSingleTapPressed State: GESTURE_IN_PROGRESS Function name: OnTapTimerFired State: TOUCH_EXPLORATION Function name: InTouchExploration State: TOUCH_EXPLORE_RELEASED Function name: OnTapTimerFired State: NO_FINGERS_DOWN TEST=ui_chromeos_unittests --gtest_filter=TouchExplore*.* BUG=670894 Review-Url: https://codereview.chromium.org/2880043002 Cr-Commit-Position: refs/heads/master@{#475650} Committed: https://chromium.googlesource.com/chromium/src/+/64b2d8e9fedef8add46fef50558ebb0cca831232

Patch Set 1 #

Total comments: 6

Patch Set 2 : Address feedback. #

Total comments: 2

Patch Set 3 : Nits. #

Total comments: 3

Patch Set 4 : Remove observer. #

Total comments: 4

Patch Set 5 : Use ShellObserver. #

Total comments: 12

Patch Set 6 : Add comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+183 lines, -12 lines) Patch
M ash/ash_touch_exploration_manager_chromeos.h View 1 2 3 4 4 chunks +21 lines, -1 line 0 comments Download
M ash/ash_touch_exploration_manager_chromeos.cc View 1 2 3 4 6 chunks +32 lines, -1 line 0 comments Download
M ui/chromeos/touch_exploration_controller.h View 1 2 3 4 5 3 chunks +19 lines, -1 line 0 comments Download
M ui/chromeos/touch_exploration_controller.cc View 1 8 chunks +24 lines, -9 lines 0 comments Download
M ui/chromeos/touch_exploration_controller_unittest.cc View 1 2 3 4 5 3 chunks +87 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 54 (30 generated)
David Tseng
PTAL. Note that I tried a few different approaches and this turned out to be ...
4 months, 2 weeks ago (2017-05-12 22:20:08 UTC) #2
dmazzoni
No objections to this approach in general. A unittest in touch_exploration_controller_unittest.cc would be good. https://codereview.chromium.org/2880043002/diff/1/ui/chromeos/touch_exploration_controller.cc ...
4 months, 1 week ago (2017-05-15 21:24:35 UTC) #7
David Tseng
Responding to click comment. If agreed not to use click, I'll write some tests for ...
4 months, 1 week ago (2017-05-16 16:07:34 UTC) #8
dmazzoni
On Tue, May 16, 2017 at 9:07 AM <dtseng@chromium.org> wrote: > > I think you'll ...
4 months, 1 week ago (2017-05-16 16:29:08 UTC) #9
dmazzoni
As discussed in person, just make the distinction between SendSimulatedClick and DispatchSimulatedTap more clear, and ...
4 months, 1 week ago (2017-05-16 16:55:23 UTC) #10
David Tseng
PTAL. Added tests for both cases; note that the quick single finger tap case is ...
4 months, 1 week ago (2017-05-16 21:17:19 UTC) #12
David Tseng
Friendly ping; would like to get this in for m60 before branch.
4 months, 1 week ago (2017-05-18 19:11:25 UTC) #18
dmazzoni
lgtm Tests look great! https://codereview.chromium.org/2880043002/diff/20001/ui/chromeos/touch_exploration_controller_unittest.cc File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/2880043002/diff/20001/ui/chromeos/touch_exploration_controller_unittest.cc#newcode1987 ui/chromeos/touch_exploration_controller_unittest.cc:1987: TEST_F(TouchExplorationTest, SingleTapInActivationArea) { Maybe ...InLiftActivationArea ...
4 months, 1 week ago (2017-05-19 19:44:49 UTC) #19
David Tseng
https://codereview.chromium.org/2880043002/diff/20001/ui/chromeos/touch_exploration_controller_unittest.cc File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/2880043002/diff/20001/ui/chromeos/touch_exploration_controller_unittest.cc#newcode1987 ui/chromeos/touch_exploration_controller_unittest.cc:1987: TEST_F(TouchExplorationTest, SingleTapInActivationArea) { On 2017/05/19 19:44:49, dmazzoni wrote: > ...
4 months ago (2017-05-24 15:40:20 UTC) #20
David Tseng
+ oshima for ash/
4 months ago (2017-05-24 15:41:54 UTC) #22
David Tseng
+ derat for ash/.
4 months ago (2017-05-25 22:50:35 UTC) #24
oshima
https://codereview.chromium.org/2880043002/diff/40001/ash/ash_touch_exploration_manager_chromeos.cc File ash/ash_touch_exploration_manager_chromeos.cc (right): https://codereview.chromium.org/2880043002/diff/40001/ash/ash_touch_exploration_manager_chromeos.cc#newcode195 ash/ash_touch_exploration_manager_chromeos.cc:195: keyboard_controller->AddObserver(this); don't you have to remove it later?
4 months ago (2017-05-25 23:31:58 UTC) #26
Daniel Erat
https://codereview.chromium.org/2880043002/diff/40001/ash/ash_touch_exploration_manager_chromeos.cc File ash/ash_touch_exploration_manager_chromeos.cc (right): https://codereview.chromium.org/2880043002/diff/40001/ash/ash_touch_exploration_manager_chromeos.cc#newcode195 ash/ash_touch_exploration_manager_chromeos.cc:195: keyboard_controller->AddObserver(this); On 2017/05/25 23:31:58, oshima wrote: > don't you ...
4 months ago (2017-05-25 23:54:11 UTC) #27
David Tseng
https://codereview.chromium.org/2880043002/diff/40001/ash/ash_touch_exploration_manager_chromeos.cc File ash/ash_touch_exploration_manager_chromeos.cc (right): https://codereview.chromium.org/2880043002/diff/40001/ash/ash_touch_exploration_manager_chromeos.cc#newcode195 ash/ash_touch_exploration_manager_chromeos.cc:195: keyboard_controller->AddObserver(this); On 2017/05/25 23:31:58, oshima wrote: > don't you ...
4 months ago (2017-05-26 16:05:15 UTC) #28
oshima
On 2017/05/26 16:05:15, David Tseng wrote: > https://codereview.chromium.org/2880043002/diff/40001/ash/ash_touch_exploration_manager_chromeos.cc > File ash/ash_touch_exploration_manager_chromeos.cc (right): > > https://codereview.chromium.org/2880043002/diff/40001/ash/ash_touch_exploration_manager_chromeos.cc#newcode195 ...
4 months ago (2017-05-26 16:30:36 UTC) #31
Daniel Erat
https://codereview.chromium.org/2880043002/diff/60001/ash/ash_touch_exploration_manager_chromeos.cc File ash/ash_touch_exploration_manager_chromeos.cc (right): https://codereview.chromium.org/2880043002/diff/60001/ash/ash_touch_exploration_manager_chromeos.cc#newcode43 ash/ash_touch_exploration_manager_chromeos.cc:43: keyboard_controller->RemoveObserver(this); yes, please use ScopedObserver. this is still wrong, ...
4 months ago (2017-05-26 16:33:38 UTC) #32
David Tseng
https://codereview.chromium.org/2880043002/diff/60001/ash/ash_touch_exploration_manager_chromeos.cc File ash/ash_touch_exploration_manager_chromeos.cc (right): https://codereview.chromium.org/2880043002/diff/60001/ash/ash_touch_exploration_manager_chromeos.cc#newcode43 ash/ash_touch_exploration_manager_chromeos.cc:43: keyboard_controller->RemoveObserver(this); On 2017/05/26 16:33:38, Daniel Erat wrote: > yes, ...
4 months ago (2017-05-26 21:13:47 UTC) #35
Daniel Erat
https://codereview.chromium.org/2880043002/diff/60001/ash/ash_touch_exploration_manager_chromeos.cc File ash/ash_touch_exploration_manager_chromeos.cc (right): https://codereview.chromium.org/2880043002/diff/60001/ash/ash_touch_exploration_manager_chromeos.cc#newcode43 ash/ash_touch_exploration_manager_chromeos.cc:43: keyboard_controller->RemoveObserver(this); On 2017/05/26 21:13:47, David Tseng wrote: > On ...
4 months ago (2017-05-26 21:37:23 UTC) #36
David Tseng
PTAL. https://codereview.chromium.org/2880043002/diff/60001/ash/ash_touch_exploration_manager_chromeos.cc File ash/ash_touch_exploration_manager_chromeos.cc (right): https://codereview.chromium.org/2880043002/diff/60001/ash/ash_touch_exploration_manager_chromeos.cc#newcode43 ash/ash_touch_exploration_manager_chromeos.cc:43: keyboard_controller->RemoveObserver(this); On 2017/05/26 21:37:23, Daniel Erat wrote: > ...
3 months, 3 weeks ago (2017-05-30 16:45:30 UTC) #37
Daniel Erat
https://codereview.chromium.org/2880043002/diff/80001/ui/chromeos/touch_exploration_controller.h File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/2880043002/diff/80001/ui/chromeos/touch_exploration_controller.h#newcode197 ui/chromeos/touch_exploration_controller.h:197: // For touch explore release state that fall within ...
3 months, 3 weeks ago (2017-05-30 17:15:51 UTC) #40
David Tseng
https://codereview.chromium.org/2880043002/diff/80001/ui/chromeos/touch_exploration_controller.h File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/2880043002/diff/80001/ui/chromeos/touch_exploration_controller.h#newcode197 ui/chromeos/touch_exploration_controller.h:197: // For touch explore release state that fall within ...
3 months, 3 weeks ago (2017-05-30 17:48:50 UTC) #43
Daniel Erat
lgtm
3 months, 3 weeks ago (2017-05-30 19:59:10 UTC) #48
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2880043002/100001
3 months, 3 weeks ago (2017-05-30 20:09:54 UTC) #51
commit-bot: I haz the power
3 months, 3 weeks ago (2017-05-30 20:17:10 UTC) #54
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/64b2d8e9fedef8add46fef50558e...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b