Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(68)

Issue 225143007: First crack at implementing the touch explaration mode. (Closed)

Created:
6 years, 8 months ago by mfomitchev
Modified:
6 years, 6 months ago
Reviewers:
sadrul
CC:
chromium-reviews, kalyank, ben+aura_chromium.org, sadrul, ben+ash_chromium.org, kpschoedel
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

First crack at implementing the touch explaration mode. See https://docs.google.com/a/google.com/document/d/1uOphhlascPTfNjsI2QTdeVTp-ANEz-g2rqGli5yVv34/edit#heading=h.a67n1i6wy36k This includes logging, TODOs, lacks some comments, and generally needs more cleanup. Also it sounds like this needs to be rewritten using EventRewriters. The purpose of the initial review is to get feedback on the general approach and major details of the implementation.

Patch Set 1 #

Patch Set 2 : Minor changes #

Total comments: 10
Unified diffs Side-by-side diffs Delta from patch set Stats (+272 lines, -1 line) Patch
M ash/ash.gyp View 1 chunk +2 lines, -0 lines 0 comments Download
M ash/root_window_controller.h View 4 chunks +13 lines, -1 line 0 comments Download
M ash/root_window_controller.cc View 3 chunks +24 lines, -0 lines 0 comments Download
A ash/touch/touch_exploration_controller.h View 1 1 chunk +57 lines, -0 lines 2 comments Download
A ash/touch/touch_exploration_controller.cc View 1 1 chunk +174 lines, -0 lines 8 comments Download
M ui/aura/window_event_dispatcher.cc View 1 chunk +1 line, -0 lines 0 comments Download
M ui/events/gestures/gesture_sequence.cc View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 2 (0 generated)
mfomitchev
https://codereview.chromium.org/225143007/diff/20001/ash/touch/touch_exploration_controller.cc File ash/touch/touch_exploration_controller.cc (right): https://codereview.chromium.org/225143007/diff/20001/ash/touch/touch_exploration_controller.cc#newcode40 ash/touch/touch_exploration_controller.cc:40: // TODO: Here and in other cases we take ...
6 years, 8 months ago (2014-04-11 21:00:58 UTC) #1
sadrul
6 years, 8 months ago (2014-04-14 20:22:11 UTC) #2
I think (hope) switching to using EventRewriter will make the code simpler.

/cc+ kpschoedel@ FYI

What is the desired behaviour for cursor visibility with this change? e.g.
should the cursor be visible at all times, no matter how many touch-points are
active? Or be visible when just one point is active, but invisible for more than
one fingers, etc.? tdanderson@ should be able to help with getting the correct
behaviour for that.

https://codereview.chromium.org/225143007/diff/20001/ash/touch/touch_explorat...
File ash/touch/touch_exploration_controller.cc (right):

https://codereview.chromium.org/225143007/diff/20001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:40: // TODO: Here and in other cases
we take the flags from the touch event
On 2014/04/11 21:00:58, mfomitchev wrote:
> Question here

Yes. The EventRewriter implementation should do the same. It may be a good idea
to set the IS_SYNTHETIZED and/or FROM_TOUCH flags on the mouse-event created.

https://codereview.chromium.org/225143007/diff/20001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:45: event->StopPropagation();
On 2014/04/11 21:00:58, mfomitchev wrote:
> If I don't call StopPropagation() (here and in other places), gestures don't
> seem to work.

Yeah. We need to stop propagating the touch-event to make sure it doesn't
contribute to any gestures.

https://codereview.chromium.org/225143007/diff/20001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:54: GenerateMouseEvent(
WindowEventDispatcher takes care of dispatching mouse-enter/exit events from
mouse-move events. If that is working correctly, you shouldn't need to generate
the ENTER/EXIT events from here.

https://codereview.chromium.org/225143007/diff/20001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:169: // TODO: Should I use 
WindowEventDispatcher instead?
On 2014/04/11 21:00:58, mfomitchev wrote:
> Question here

This is fine.

https://codereview.chromium.org/225143007/diff/20001/ash/touch/touch_explorat...
File ash/touch/touch_exploration_controller.h (right):

https://codereview.chromium.org/225143007/diff/20001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.h:18: // TODO: How much screen/display
logic (DisplayController::Observer,
On 2014/04/11 21:00:58, mfomitchev wrote:
> Bunch of questions in TODOs here.

Unsure. oshima@/dnicoara@ would have good answers.

Powered by Google App Engine
This is Rietveld 408576698