wip: Second crack at implementing the touch exploration mode.

Second crack at implementing the touch exploration mode.
First implementation can be found here: https://codereview.chromium.org/225143007/

The implementation has been rewwritten using EventRewriters. Also mouse cursor
handling has been fixed.

See https://docs.google.com/a/google.com/document/d/1uOphhlascPTfNjsI2QTdeVTp-ANEz-g2rqGli5yVv34/edit#heading=h.a67n1i6wy36k

[mfomitchev, 2014-04-16 19:30:25]

New implementation of touch exploration mode using EventRewriter.
Donn't be scared by the number of files in the CL - most of it is due to the
adding GetEventSource() to WindowTreeHost. Plus there's some logging.

[mfomitchev, 2014-04-16 19:31:09]

New implementation of touch exploration mode using EventRewriter.
Donn't be scared by the number of files in the CL - most of it is due to the
adding GetEventSource() to WindowTreeHost. Plus there's some logging.

[mfomitchev, 2014-04-16 19:32:16]

https://codereview.chromium.org/240333007/diff/1/ash/touch/touch_exploration_...
File ash/touch/touch_exploration_controller.cc (right):

https://codereview.chromium.org/240333007/diff/1/ash/touch/touch_exploration_...
ash/touch/touch_exploration_controller.cc:57:
cursor_client->EnableMouseEvents();
This makes mouse events go through

https://codereview.chromium.org/240333007/diff/1/ash/touch/touch_exploration_...
ash/touch/touch_exploration_controller.cc:58: cursor_client->HideCursor();
Don't think we want to see the cursor while touching...

[mfomitchev, 2014-04-17 18:23:54]

+dmazzoni FYI

Minor fix

[Daniel Erat, 2014-04-18 00:44:07]

https://codereview.chromium.org/240333007/diff/30001/ash/root_window_controll...
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/240333007/diff/30001/ash/root_window_controll...
ash/root_window_controller.cc:735:
Shell::GetInstance()->system_tray_notifier()->AddAccessibilityObserver(this);
remove the observer in the d'tor (probably need to check that it was added first
in case Init() wasn't called, though)

https://codereview.chromium.org/240333007/diff/30001/ash/root_window_controll...
ash/root_window_controller.cc:1028:
Shell::GetInstance()->accessibility_delegate();
fix indenting: should be four spaces beyond previous line

https://codereview.chromium.org/240333007/diff/30001/ash/root_window_controll...
ash/root_window_controller.cc:1031: LOG(ERROR) <<
"RWC::UpdateTouchExplorationState: enabled=" << enabled;
remove debugging logging

https://codereview.chromium.org/240333007/diff/30001/ash/root_window_controll...
File ash/root_window_controller.h (right):

https://codereview.chromium.org/240333007/diff/30001/ash/root_window_controll...
ash/root_window_controller.h:10: #include "ash/accessibility_delegate.h"
do you need this include?

https://codereview.chromium.org/240333007/diff/30001/ash/root_window_controll...
ash/root_window_controller.h:16: #include
"ash/touch/touch_exploration_controller.h"
forward-declare TouchExplorationController and move this include to the .cc file
if possible

https://codereview.chromium.org/240333007/diff/30001/ash/root_window_controll...
ash/root_window_controller.h:278: void UpdateTouchExplorationState();
nit: add a comment and move this above the ShellObserver overrides

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

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:19: aura::Window* root_window) :
see the style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:24:
event_source->AddEventRewriter(this);
nit: combine this line with the previous one as in the d'tor

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:29: if (root_window_)
why does the c'tor assume that root_window is non-NULL but this code needs to
check it?

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:36: if (!event.IsTouchEvent()) {
nit: leave out these curly brackets since the statement fits on a single line

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:41: static_cast<const ui::TouchEvent&>
(event);
get rid of space before '('

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:45: int flags = touch_event.flags();
nit: make all of the above types be const

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:48: LOG(ERROR) << "ET_TOUCH_PRESSED";
remove debug messages here and below (i.e. switch to VLOG(1))

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:51: if (touch_ids_.size() == 0) {
touch_ids_.empty()

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:61:
touch_locations_.insert(std::pair<int, gfx::Point>(touch_id, location));
please make this method return early whenever it can so there aren't so many
nested if-else statements

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:63: type == ui::ET_TOUCH_CANCELLED)) {
fix indenting and get rid of extra parentheses

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:166: gfx::Point location, int flags) {
nit: one parameter per line in function sigs if the whole thing doesn't fit on a
single line

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

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.h:16: // TODO: How much screen/display
logic (DisplayController::Observer,
nit: please include your username in these TODOs (e.g. TODO(username): etc.)

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.h:25: 
delete extra blank line

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.h:30: //Overridden from ui::EventRewriter
add a space after '//'

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.h:36: ui::Event*
CreateMouseMoveEvent(gfx::Point location, int flags);
add a comment describing what this does

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.h:38: void EnterMouseMoveMode();
add a comment

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.h:41: // map of touch ids to their last
know locations
s/map/Map/, s/know/known/, add trailing period

https://codereview.chromium.org/240333007/diff/30001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/240333007/diff/30001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:2412: LOG(ERROR)
<< "RenderWidgetHostViewAura::OnMouseEvent";
delete

https://codereview.chromium.org/240333007/diff/30001/ui/aura/window_event_dis...
File ui/aura/window_event_dispatcher.cc (right):

https://codereview.chromium.org/240333007/diff/30001/ui/aura/window_event_dis...
ui/aura/window_event_dispatcher.cc:165: LOG(ERROR) << "WED::ProcessedTouchEvent,
type=" << event->type();
delete, also below

https://codereview.chromium.org/240333007/diff/30001/ui/aura/window_tree_host.h
File ui/aura/window_tree_host.h (right):

https://codereview.chromium.org/240333007/diff/30001/ui/aura/window_tree_host...
ui/aura/window_tree_host.h:69: virtual ui::EventSource* GetEventSource() = 0;
move this down to where all of the other abstract methods are and add a comment

https://codereview.chromium.org/240333007/diff/30001/ui/events/event_dispatch...
File ui/events/event_dispatcher.cc (right):

https://codereview.chromium.org/240333007/diff/30001/ui/events/event_dispatch...
ui/events/event_dispatcher.cc:49: << ", handled=" << event->handled();
delete

https://codereview.chromium.org/240333007/diff/30001/ui/events/event_processo...
File ui/events/event_processor.cc (right):

https://codereview.chromium.org/240333007/diff/30001/ui/events/event_processo...
ui/events/event_processor.cc:13: LOG(ERROR) <<
"EventProcessor::OnEventFromSource";
delete

https://codereview.chromium.org/240333007/diff/30001/ui/events/event_source.cc
File ui/events/event_source.cc (right):

https://codereview.chromium.org/240333007/diff/30001/ui/events/event_source.c...
ui/events/event_source.cc:33: LOG(ERROR) << "EventSource::SendEventToProcessor";
delete

https://codereview.chromium.org/240333007/diff/30001/ui/events/gestures/gestu...
File ui/events/gestures/gesture_sequence.cc (right):

https://codereview.chromium.org/240333007/diff/30001/ui/events/gestures/gestu...
ui/events/gestures/gesture_sequence.cc:515: LOG(ERROR) <<
"GS::ProcessTouchEventForGesture, type=" << event.type();
delete

https://codereview.chromium.org/240333007/diff/30001/ui/wm/core/cursor_manage...
File ui/wm/core/cursor_manager.cc (right):

https://codereview.chromium.org/240333007/diff/30001/ui/wm/core/cursor_manage...
ui/wm/core/cursor_manager.cc:46: LOG(ERROR) <<
"CursorState::SetMouseEventsEnabled, enabled=" << enabled;
delete

[Daniel Erat, 2014-04-18 00:45:57]

On 2014/04/18 00:44:07, Daniel Erat wrote:
>
https://codereview.chromium.org/240333007/diff/30001/ash/root_window_controll...
> File ash/root_window_controller.cc (right):
> 
>
https://codereview.chromium.org/240333007/diff/30001/ash/root_window_controll...
> ash/root_window_controller.cc:735:
> Shell::GetInstance()->system_tray_notifier()->AddAccessibilityObserver(this);
> remove the observer in the d'tor (probably need to check that it was added
first
> in case Init() wasn't called, though)
> 
>
https://codereview.chromium.org/240333007/diff/30001/ash/root_window_controll...
> ash/root_window_controller.cc:1028:
> Shell::GetInstance()->accessibility_delegate();
> fix indenting: should be four spaces beyond previous line
> 
>
https://codereview.chromium.org/240333007/diff/30001/ash/root_window_controll...
> ash/root_window_controller.cc:1031: LOG(ERROR) <<
> "RWC::UpdateTouchExplorationState: enabled=" << enabled;
> remove debugging logging
> 
>
https://codereview.chromium.org/240333007/diff/30001/ash/root_window_controll...
> File ash/root_window_controller.h (right):
> 
>
https://codereview.chromium.org/240333007/diff/30001/ash/root_window_controll...
> ash/root_window_controller.h:10: #include "ash/accessibility_delegate.h"
> do you need this include?
> 
>
https://codereview.chromium.org/240333007/diff/30001/ash/root_window_controll...
> ash/root_window_controller.h:16: #include
> "ash/touch/touch_exploration_controller.h"
> forward-declare TouchExplorationController and move this include to the .cc
file
> if possible
> 
>
https://codereview.chromium.org/240333007/diff/30001/ash/root_window_controll...
> ash/root_window_controller.h:278: void UpdateTouchExplorationState();
> nit: add a comment and move this above the ShellObserver overrides
> 
>
https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
> File ash/touch/touch_exploration_controller.cc (right):
> 
>
https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
> ash/touch/touch_exploration_controller.cc:19: aura::Window* root_window) :
> see the style guide:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...
> 
>
https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
> ash/touch/touch_exploration_controller.cc:24:
> event_source->AddEventRewriter(this);
> nit: combine this line with the previous one as in the d'tor
> 
>
https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
> ash/touch/touch_exploration_controller.cc:29: if (root_window_)
> why does the c'tor assume that root_window is non-NULL but this code needs to
> check it?
> 
>
https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
> ash/touch/touch_exploration_controller.cc:36: if (!event.IsTouchEvent()) {
> nit: leave out these curly brackets since the statement fits on a single line
> 
>
https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
> ash/touch/touch_exploration_controller.cc:41: static_cast<const
ui::TouchEvent&>
> (event);
> get rid of space before '('
> 
>
https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
> ash/touch/touch_exploration_controller.cc:45: int flags = touch_event.flags();
> nit: make all of the above types be const
> 
>
https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
> ash/touch/touch_exploration_controller.cc:48: LOG(ERROR) <<
"ET_TOUCH_PRESSED";
> remove debug messages here and below (i.e. switch to VLOG(1))
> 
>
https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
> ash/touch/touch_exploration_controller.cc:51: if (touch_ids_.size() == 0) {
> touch_ids_.empty()
> 
>
https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
> ash/touch/touch_exploration_controller.cc:61:
> touch_locations_.insert(std::pair<int, gfx::Point>(touch_id, location));
> please make this method return early whenever it can so there aren't so many
> nested if-else statements
> 
>
https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
> ash/touch/touch_exploration_controller.cc:63: type == ui::ET_TOUCH_CANCELLED))
{
> fix indenting and get rid of extra parentheses
> 
>
https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
> ash/touch/touch_exploration_controller.cc:166: gfx::Point location, int flags)
{
> nit: one parameter per line in function sigs if the whole thing doesn't fit on
a
> single line
> 
>
https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
> File ash/touch/touch_exploration_controller.h (right):
> 
>
https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
> ash/touch/touch_exploration_controller.h:16: // TODO: How much screen/display
> logic (DisplayController::Observer,
> nit: please include your username in these TODOs (e.g. TODO(username): etc.)
> 
>
https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
> ash/touch/touch_exploration_controller.h:25: 
> delete extra blank line
> 
>
https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
> ash/touch/touch_exploration_controller.h:30: //Overridden from
ui::EventRewriter
> add a space after '//'
> 
>
https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
> ash/touch/touch_exploration_controller.h:36: ui::Event*
> CreateMouseMoveEvent(gfx::Point location, int flags);
> add a comment describing what this does
> 
>
https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
> ash/touch/touch_exploration_controller.h:38: void EnterMouseMoveMode();
> add a comment
> 
>
https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
> ash/touch/touch_exploration_controller.h:41: // map of touch ids to their last
> know locations
> s/map/Map/, s/know/known/, add trailing period
> 
>
https://codereview.chromium.org/240333007/diff/30001/content/browser/renderer...
> File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> 
>
https://codereview.chromium.org/240333007/diff/30001/content/browser/renderer...
> content/browser/renderer_host/render_widget_host_view_aura.cc:2412: LOG(ERROR)
> << "RenderWidgetHostViewAura::OnMouseEvent";
> delete
> 
>
https://codereview.chromium.org/240333007/diff/30001/ui/aura/window_event_dis...
> File ui/aura/window_event_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/240333007/diff/30001/ui/aura/window_event_dis...
> ui/aura/window_event_dispatcher.cc:165: LOG(ERROR) <<
"WED::ProcessedTouchEvent,
> type=" << event->type();
> delete, also below
> 
>
https://codereview.chromium.org/240333007/diff/30001/ui/aura/window_tree_host.h
> File ui/aura/window_tree_host.h (right):
> 
>
https://codereview.chromium.org/240333007/diff/30001/ui/aura/window_tree_host...
> ui/aura/window_tree_host.h:69: virtual ui::EventSource* GetEventSource() = 0;
> move this down to where all of the other abstract methods are and add a
comment
> 
>
https://codereview.chromium.org/240333007/diff/30001/ui/events/event_dispatch...
> File ui/events/event_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/240333007/diff/30001/ui/events/event_dispatch...
> ui/events/event_dispatcher.cc:49: << ", handled=" << event->handled();
> delete
> 
>
https://codereview.chromium.org/240333007/diff/30001/ui/events/event_processo...
> File ui/events/event_processor.cc (right):
> 
>
https://codereview.chromium.org/240333007/diff/30001/ui/events/event_processo...
> ui/events/event_processor.cc:13: LOG(ERROR) <<
> "EventProcessor::OnEventFromSource";
> delete
> 
> https://codereview.chromium.org/240333007/diff/30001/ui/events/event_source.cc
> File ui/events/event_source.cc (right):
> 
>
https://codereview.chromium.org/240333007/diff/30001/ui/events/event_source.c...
> ui/events/event_source.cc:33: LOG(ERROR) <<
"EventSource::SendEventToProcessor";
> delete
> 
>
https://codereview.chromium.org/240333007/diff/30001/ui/events/gestures/gestu...
> File ui/events/gestures/gesture_sequence.cc (right):
> 
>
https://codereview.chromium.org/240333007/diff/30001/ui/events/gestures/gestu...
> ui/events/gestures/gesture_sequence.cc:515: LOG(ERROR) <<
> "GS::ProcessTouchEventForGesture, type=" << event.type();
> delete
> 
>
https://codereview.chromium.org/240333007/diff/30001/ui/wm/core/cursor_manage...
> File ui/wm/core/cursor_manager.cc (right):
> 
>
https://codereview.chromium.org/240333007/diff/30001/ui/wm/core/cursor_manage...
> ui/wm/core/cursor_manager.cc:46: LOG(ERROR) <<
> "CursorState::SetMouseEventsEnabled, enabled=" << enabled;
> delete

hmm, i wasn't even trying to review this change. i got added to
https://codereview.chromium.org/240173007/ and clicked the wrong link in the
email notification. :-(

[sadrul, 2014-04-22 16:46:12]

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

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:137: if (it != touch_ids_.begin()) {
You can just do 'touch_ids_.front() != touch_id' instead of std::find() and
compare with begin()/end()

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:154: }
The nested ifs and incorrect indentation in some places make this a bit
difficult to follow. I also have questions about some of the behaviour. I have
asked them on the doc.

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:161:
new_event->reset(next_dispatch_event_.release());
I think you can do *new_event = next_dispatch_event_.Pass()

Also, do a CHECK(next_dispatch_event_) first.

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

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.h:21: // OnDisplayModeChanged()?
Depends on what you are trying to do.

If you want to make sure that this rewriter is added to new WindowTreeHost
instances, then you can use an EnvObserver instead. See
https://codereview.chromium.org/240173007/ (It may make sense to have something
like this in ui/wm/)

[sadrul, 2014-04-22 16:49:22]

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

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:171: ui::EF_IS_SYNTHESIZED | flags,
Should this also set EF_FROM_TOUCH?

[mfomitchev, 2014-04-22 16:58:27]

> hmm, i wasn't even trying to review this change. i got added to
> https://codereview.chromium.org/240173007/ and clicked the wrong link in the
> email notification. :-(

@derat Haha! Well, thanks anyway :) This wasn't quite ready for the review, but
a lot of your comments are valuable even at this stage!

[mfomitchev, 2014-04-22 16:58:30]

> hmm, i wasn't even trying to review this change. i got added to
> https://codereview.chromium.org/240173007/ and clicked the wrong link in the
> email notification. :-(

@derat Haha! Well, thanks anyway :) This wasn't quite ready for the review, but
a lot of your comments are valuable even at this stage!

[mfomitchev, 2014-04-23 18:27:37]

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

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:19: aura::Window* root_window) :
On 2014/04/18 00:44:08, Daniel Erat wrote:
> see the style guide:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...

Done.

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:24:
event_source->AddEventRewriter(this);
On 2014/04/18 00:44:08, Daniel Erat wrote:
> nit: combine this line with the previous one as in the d'tor

Done.

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:29: if (root_window_)
On 2014/04/18 00:44:08, Daniel Erat wrote:
> why does the c'tor assume that root_window is non-NULL but this code needs to
> check it?

Done.

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:36: if (!event.IsTouchEvent()) {
On 2014/04/18 00:44:08, Daniel Erat wrote:
> nit: leave out these curly brackets since the statement fits on a single line

Done.

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:41: static_cast<const ui::TouchEvent&>
(event);
On 2014/04/18 00:44:08, Daniel Erat wrote:
> get rid of space before '('

Done.

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:45: int flags = touch_event.flags();
On 2014/04/18 00:44:08, Daniel Erat wrote:
> nit: make all of the above types be const

Done.

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:48: LOG(ERROR) << "ET_TOUCH_PRESSED";
I am going to leave the logging for now, since this is still WIP and I need to
add unit tests. Will delete before the final submission.

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:61:
touch_locations_.insert(std::pair<int, gfx::Point>(touch_id, location));
Done.. kind of

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:63: type == ui::ET_TOUCH_CANCELLED)) {
On 2014/04/18 00:44:08, Daniel Erat wrote:
> fix indenting and get rid of extra parentheses

Done.

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:137: if (it != touch_ids_.begin()) {
On 2014/04/22 16:46:12, sadrul wrote:
> You can just do 'touch_ids_.front() != touch_id' instead of std::find() and
> compare with begin()/end()

Done.

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:154: }
I've tried to make the code more readable. I also plan to add a 'general'
function-level or class-level comment about how event rewriting works. If you
have any suggestions on how to further improve the code readability here -
please let me know.

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:161:
new_event->reset(next_dispatch_event_.release());
On 2014/04/22 16:46:12, sadrul wrote:
> I think you can do *new_event = next_dispatch_event_.Pass()
> 
> Also, do a CHECK(next_dispatch_event_) first.

Done.

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.cc:171: ui::EF_IS_SYNTHESIZED | flags,
On 2014/04/22 16:49:22, sadrul wrote:
> Should this also set EF_FROM_TOUCH?

Done.

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

https://codereview.chromium.org/240333007/diff/30001/ash/touch/touch_explorat...
ash/touch/touch_exploration_controller.h:21: // OnDisplayModeChanged()?
After thinking about this, I don't think I need to implement any of these
interfaces. I will have RootWindowController owning TouchExplorationController
and creating a new instance whenever a new RootWindow is created. I think this
should work. I will get rid of these TODOs.

[mfomitchev, 2014-04-24 17:47:18]

@dmazzoni, @sadrul:
My understanding is that we only really need this working on Chrome OS, correct?
Should we the touch_exploration_controller.* files from ash/touch/ to
chrome/browser/ui/ash/ or to chrome/browser/chromeos/accessibility/? Or at least
change the names to touch_exploration_controller_chromeos.*?

Currently the solution doesn't work end-to-end on other platforms. This is
because on non-Chrome OS platforms DefaultAccessibilityDelegate is used, which
doesn't do anything when you call ToggleSpokenFeedback(). (On Chrome OS
AccessibilityDelegateImpl is used which sends notifications). In particular this
means that if I put my test in ash_unittests, it would fail, because
ash_unittests runs with DefaultAccessibilityDelegate. I could get around this by
creating a backdoor method in RootWindowController to activate
TouchExplorationController, but that doesn't seem like a great solution.

[dmazzoni, 2014-04-24 18:25:00]

On 2014/04/24 17:47:18, mfomitchev wrote:
> @dmazzoni, @sadrul:
> My understanding is that we only really need this working on Chrome OS,
correct?
> Should we the touch_exploration_controller.* files from ash/touch/ to
> chrome/browser/ui/ash/ or to chrome/browser/chromeos/accessibility/? Or at
least
> change the names to touch_exploration_controller_chromeos.*?

Yes, it doesn't make sense to implement it on other platforms. On Windows 8,
Windows Narrator, JAWS, NVDA, or some other third-party solution may be
providing touch exploration, but they'll be intercepting the events before ash
gets them. We don't have any reason to implement our own touch exploration
there.

chrome/browser/chromeos/accessibility works for me. It'd be next to the other
related accessibility code.

> Currently the solution doesn't work end-to-end on other platforms. This is
> because on non-Chrome OS platforms DefaultAccessibilityDelegate is used, which
> doesn't do anything when you call ToggleSpokenFeedback(). (On Chrome OS
> AccessibilityDelegateImpl is used which sends notifications). In particular
this
> means that if I put my test in ash_unittests, it would fail, because
> ash_unittests runs with DefaultAccessibilityDelegate. I could get around this
by
> creating a backdoor method in RootWindowController to activate
> TouchExplorationController, but that doesn't seem like a great solution.

Agreed, moving the code to chrome/ makes more sense.

[sadrul, 2014-04-24 18:33:30]

On 2014/04/24 18:25:00, dmazzoni wrote:
> On 2014/04/24 17:47:18, mfomitchev wrote:
> > @dmazzoni, @sadrul:
> > My understanding is that we only really need this working on Chrome OS,
> correct?
> > Should we the touch_exploration_controller.* files from ash/touch/ to
> > chrome/browser/ui/ash/ or to chrome/browser/chromeos/accessibility/? Or at
> least
> > change the names to touch_exploration_controller_chromeos.*?
> 
> Yes, it doesn't make sense to implement it on other platforms. On Windows 8,
> Windows Narrator, JAWS, NVDA, or some other third-party solution may be
> providing touch exploration, but they'll be intercepting the events before ash
> gets them. We don't have any reason to implement our own touch exploration
> there.
> 
> chrome/browser/chromeos/accessibility works for me. It'd be next to the other
> related accessibility code.
> 
> > Currently the solution doesn't work end-to-end on other platforms. This is
> > because on non-Chrome OS platforms DefaultAccessibilityDelegate is used,
which
> > doesn't do anything when you call ToggleSpokenFeedback(). (On Chrome OS
> > AccessibilityDelegateImpl is used which sends notifications). In particular
> this
> > means that if I put my test in ash_unittests, it would fail, because
> > ash_unittests runs with DefaultAccessibilityDelegate. I could get around
this
> by
> > creating a backdoor method in RootWindowController to activate
> > TouchExplorationController, but that doesn't seem like a great solution.
> 
> Agreed, moving the code to chrome/ makes more sense.

ui/chromeos/ might be a better place for this.

[mfomitchev, 2014-04-24 19:19:06]

On 2014/04/24 18:33:30, sadrul wrote:
> On 2014/04/24 18:25:00, dmazzoni wrote:
> > On 2014/04/24 17:47:18, mfomitchev wrote:
> > > @dmazzoni, @sadrul:
> > > My understanding is that we only really need this working on Chrome OS,
> > correct?
> > > Should we the touch_exploration_controller.* files from ash/touch/ to
> > > chrome/browser/ui/ash/ or to chrome/browser/chromeos/accessibility/? Or at
> > least
> > > change the names to touch_exploration_controller_chromeos.*?
> > 
> > Yes, it doesn't make sense to implement it on other platforms. On Windows 8,
> > Windows Narrator, JAWS, NVDA, or some other third-party solution may be
> > providing touch exploration, but they'll be intercepting the events before
ash
> > gets them. We don't have any reason to implement our own touch exploration
> > there.
> > 
> > chrome/browser/chromeos/accessibility works for me. It'd be next to the
other
> > related accessibility code.
> > 
> > > Currently the solution doesn't work end-to-end on other platforms. This is
> > > because on non-Chrome OS platforms DefaultAccessibilityDelegate is used,
> which
> > > doesn't do anything when you call ToggleSpokenFeedback(). (On Chrome OS
> > > AccessibilityDelegateImpl is used which sends notifications). In
particular
> > this
> > > means that if I put my test in ash_unittests, it would fail, because
> > > ash_unittests runs with DefaultAccessibilityDelegate. I could get around
> this
> > by
> > > creating a backdoor method in RootWindowController to activate
> > > TouchExplorationController, but that doesn't seem like a great solution.
> > 
> > Agreed, moving the code to chrome/ makes more sense.
> 
> ui/chromeos/ might be a better place for this.

Hmm.. ui/chromeos/ (or perhaps ui/chromeos/accessibility/) does sound like a
good home for this because the feature works for all of the UI, not just for the
browser. Dominic, what do you think?

[dmazzoni, 2014-04-24 19:24:20]

ui/chromeos doesn't seem to have any visibility into ash or chrome os
settings like whether spoken feedback is on, so I'm assuming the logic that
enables/injects the touch exploration controller would stay where it is,
and only the touch exploration controller and unit tests would move to
ui/chromeos. Should be fine.


On Thu, Apr 24, 2014 at 12:19 PM, <mfomitchev@chromium.org> wrote:

> On 2014/04/24 18:33:30, sadrul wrote:
>
>> On 2014/04/24 18:25:00, dmazzoni wrote:
>> > On 2014/04/24 17:47:18, mfomitchev wrote:
>> > > @dmazzoni, @sadrul:
>> > > My understanding is that we only really need this working on Chrome
>> OS,
>> > correct?
>> > > Should we the touch_exploration_controller.* files from ash/touch/ to
>> > > chrome/browser/ui/ash/ or to chrome/browser/chromeos/accessibility/?
>> Or at
>> > least
>> > > change the names to touch_exploration_controller_chromeos.*?
>> >
>> > Yes, it doesn't make sense to implement it on other platforms. On
>> Windows 8,
>> > Windows Narrator, JAWS, NVDA, or some other third-party solution may be
>> > providing touch exploration, but they'll be intercepting the events
>> before
>>
> ash
>
>> > gets them. We don't have any reason to implement our own touch
>> exploration
>> > there.
>> >
>> > chrome/browser/chromeos/accessibility works for me. It'd be next to the
>>
> other
>
>> > related accessibility code.
>> >
>> > > Currently the solution doesn't work end-to-end on other platforms.
>> This is
>> > > because on non-Chrome OS platforms DefaultAccessibilityDelegate is
>> used,
>> which
>> > > doesn't do anything when you call ToggleSpokenFeedback(). (On Chrome
>> OS
>> > > AccessibilityDelegateImpl is used which sends notifications). In
>>
> particular
>
>> > this
>> > > means that if I put my test in ash_unittests, it would fail, because
>> > > ash_unittests runs with DefaultAccessibilityDelegate. I could get
>> around
>> this
>> > by
>> > > creating a backdoor method in RootWindowController to activate
>> > > TouchExplorationController, but that doesn't seem like a great
>> solution.
>> >
>> > Agreed, moving the code to chrome/ makes more sense.
>>
>
>  ui/chromeos/ might be a better place for this.
>>
>
> Hmm.. ui/chromeos/ (or perhaps ui/chromeos/accessibility/) does sound like
> a
> good home for this because the feature works for all of the UI, not just
> for the
> browser. Dominic, what do you think?
>
> https://codereview.chromium.org/240333007/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mfomitchev, 2014-04-25 16:08:11]

On 2014/04/24 19:24:20, dmazzoni wrote:
> ui/chromeos doesn't seem to have any visibility into ash or chrome os
> settings like whether spoken feedback is on, so I'm assuming the logic that
> enables/injects the touch exploration controller would stay where it is,
> and only the touch exploration controller and unit tests would move to
> ui/chromeos. Should be fine.

Yes, that logic would stay in RootWindowController, and
TouchExplorationContrtoller itself can move to ui/chromeos. The unit test on the
other hand needs to enable TouchExplorationController, so it seems the unit test
would need to go into chrome/browser/chromeos/accessibility/...