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

Issue 2880043002: Implement touch exploration touch typing

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks ago by David Tseng
Modified:
6 hours, 46 minutes 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

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: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+154 lines, -11 lines) Patch
M ash/ash_touch_exploration_manager_chromeos.h View 3 chunks +7 lines, -1 line 0 comments Download
M ash/ash_touch_exploration_manager_chromeos.cc View 1 2 3 4 chunks +25 lines, -0 lines 3 comments Download
M ui/chromeos/touch_exploration_controller.h View 1 3 chunks +9 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 chunks +89 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 36 (18 generated)
David Tseng
PTAL. Note that I tried a few different approaches and this turned out to be ...
2 weeks ago (2017-05-12 22:20:08 UTC) #2
dmazzoni_ooo_until_may_30
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 ...
1 week, 4 days 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 ...
1 week, 3 days ago (2017-05-16 16:07:34 UTC) #8
dmazzoni_ooo_until_may_30
On Tue, May 16, 2017 at 9:07 AM <dtseng@chromium.org> wrote: > > I think you'll ...
1 week, 3 days ago (2017-05-16 16:29:08 UTC) #9
dmazzoni_ooo_until_may_30
As discussed in person, just make the distinction between SendSimulatedClick and DispatchSimulatedTap more clear, and ...
1 week, 3 days 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 ...
1 week, 3 days ago (2017-05-16 21:17:19 UTC) #12
David Tseng
Friendly ping; would like to get this in for m60 before branch.
1 week, 1 day ago (2017-05-18 19:11:25 UTC) #18
dmazzoni_ooo_until_may_30
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 ...
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: > ...
2 days, 12 hours ago (2017-05-24 15:40:20 UTC) #20
David Tseng
+ oshima for ash/
2 days, 12 hours ago (2017-05-24 15:41:54 UTC) #22
David Tseng
+ derat for ash/.
1 day, 5 hours 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?
1 day, 4 hours 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 ...
1 day, 4 hours 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 ...
12 hours, 19 minutes 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 ...
11 hours, 53 minutes 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, ...
11 hours, 50 minutes 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, ...
7 hours, 10 minutes ago (2017-05-26 21:13:47 UTC) #35
Daniel Erat
6 hours, 46 minutes ago (2017-05-26 21:37:23 UTC) #36
https://codereview.chromium.org/2880043002/diff/60001/ash/ash_touch_explorati...
File ash/ash_touch_exploration_manager_chromeos.cc (right):

https://codereview.chromium.org/2880043002/diff/60001/ash/ash_touch_explorati...
ash/ash_touch_exploration_manager_chromeos.cc:43:
keyboard_controller->RemoveObserver(this);
On 2017/05/26 21:13:47, David Tseng wrote:
> On 2017/05/26 16:33:38, Daniel Erat wrote:
> > yes, please use ScopedObserver. this is still wrong, as the observer is
> removed
> > in the d'tor but not added in the c'tor, i.e. you'll remove without having
> added
> > earlier if UpdateTouchExplorationState isn't called before destruction.
> 
> Sorry, I must be mistaken, but removing if not previously added is a no-op,
> right, so why does this matter?
> 
> The larger issue here is that the keyboard controller gets re-initialized on
> session state changed, so though the keyboard controller looks like a
singleton,
> it's not and AshTouchExplorationManager has a slightly longer lifetime than
> KeyboardControllers.
> 
> It's not entirely clear to me what the relationship between
> AshTouchExplorationManager (which is owned by RootWindowController) and
> KeyboardController should be.

it's standard practice in chrome to keep track of whether you've added an
observer and remove it iff it's been added. you can't double-add, for instance:
https://chromium.googlesource.com/chromium/src/+/master/base/observer_list.h#276

i didn't realize that the KeyboardController is reinitialized. do you have some
way to register to be notified when it's destroyed? (does it have an
OnDestroying() method in its observer class?)
Sign in to reply to this message.

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