Developer Feature: Add Debug Accelerators to Toggle Touchscreen/Touchpad On or Off (CrOS)

Developer Feature: Add Debug Accelerators to Toggle Touchscreen/Touchpad On or Off

Add two debugging accelerators for toggling the touchscreen / touchpad on or off.
These settings must be persistent between reboots. We ignore the case when users
turn off the debugging accelerators flag.

This is only for ChromeOS.
We add an UMA user metric to track how often they're used.

The two shortcuts are:
Search+Shift+T ---> Toggle touch screen.
Search+Shift+P ---> Toggle touchpad.

BUG=546654
Committed: https://crrev.com/e8eac60b676dc33060e16c1d0e69f8c5150e68ca
Cr-Commit-Position: refs/heads/master@{#358758}

[afakhry, 2015-10-24 00:28:04]

Description was changed from

==========
WIP toggle touchpad / touchscreen

BUG=
==========

to

==========
Developer Feature: Add Debug Accelerators to Toggle Touchscreen/Touchpad On or
Off

Add two debugging accelerators for toggling the touchscreen / touchpad on or
off.
These settings must be persistent between reboots. We ignore the case when users
turn off the debugging accelerators flag.

This is only for ChromeOS.
We add an UMA user metric to track how often they're used.

The two shortcuts are:
Search+Shift+T ---> Toggle touch screen.
Search+Shift+P ---> Toggle touchpad.

BUG=546654
==========

[afakhry, 2015-10-26 19:36:22]

afakhry@chromium.org changed reviewers:
+ oshima@chromium.org

[afakhry, 2015-10-26 19:36:22]

Oshima, can you please review?

[oshima, 2015-10-26 22:32:06]

On 2015/10/26 19:36:22, afakhry wrote:
> Oshima, can you please review?

What happened to the discuss not to use debug accelerators? Or is this really
for developers?

[Daniel Erat, 2015-10-26 22:40:20]

derat@chromium.org changed reviewers:
+ derat@chromium.org

[Daniel Erat, 2015-10-26 22:40:20]

please add some more background to the bug. why are these being added? what
happens if a user hits them accidentally and then has a non-functioning
touchpad?

the original bug doesn't say anything about developers. this looks like it's
just adding undocumented shortcuts.

[afakhry, 2015-10-28 17:15:20]

On 2015/10/26 22:40:20, Daniel Erat wrote:
> please add some more background to the bug. why are these being added? what
> happens if a user hits them accidentally and then has a non-functioning
> touchpad?
> 
> the original bug doesn't say anything about developers. this looks like it's
> just adding undocumented shortcuts.

oshima, friendly ping! :)

[oshima, 2015-10-29 23:38:42]

oshima@chromium.org changed reviewers:
+ sadrul@chromium.org

[oshima, 2015-10-29 23:38:43]

+sadrul@

https://codereview.chromium.org/1412623006/diff/60001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/1412623006/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_controller.cc:664: #endif  // defined(OS_CHROMEOS)
move them to debug_commands.cc

https://codereview.chromium.org/1412623006/diff/60001/ash/accelerators/accele...
File ash/accelerators/accelerator_table.h (right):

https://codereview.chromium.org/1412623006/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_table.h:136: TOGGLE_TOUCH_SCREEN,
DEBUG_TOGGLE_xxx and move it to above.

https://codereview.chromium.org/1412623006/diff/60001/ash/wm/maximize_mode/sc...
File ash/wm/maximize_mode/scoped_disable_internal_mouse_and_keyboard_ozone.cc
(right):

https://codereview.chromium.org/1412623006/diff/60001/ash/wm/maximize_mode/sc...
ash/wm/maximize_mode/scoped_disable_internal_mouse_and_keyboard_ozone.cc:26:
aura::client::GetCursorClient(Shell::GetInstance()->GetPrimaryRootWindow())
Shell::GetInstnace()->cursor_manager()->HideCursor();

https://codereview.chromium.org/1412623006/diff/60001/ash/wm/maximize_mode/sc...
File ash/wm/maximize_mode/scoped_disable_internal_mouse_and_keyboard_ozone.h
(right):

https://codereview.chromium.org/1412623006/diff/60001/ash/wm/maximize_mode/sc...
ash/wm/maximize_mode/scoped_disable_internal_mouse_and_keyboard_ozone.h:24: bool
should_ignore_touch_pad_;
you may initialize inline. (= false)

https://codereview.chromium.org/1412623006/diff/60001/ash/wm/maximize_mode/sc...
File ash/wm/maximize_mode/scoped_disable_internal_mouse_and_keyboard_x11.cc
(right):

https://codereview.chromium.org/1412623006/diff/60001/ash/wm/maximize_mode/sc...
ash/wm/maximize_mode/scoped_disable_internal_mouse_and_keyboard_x11.cc:92:
Shell::GetInstance()->GetPrimaryRootWindow())->HideCursor();
ditto

https://codereview.chromium.org/1412623006/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/system/input_device_settings.cc (right):

https://codereview.chromium.org/1412623006/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/system/input_device_settings.cc:233: PrefService*
local_state = g_browser_process->local_state();
can this be null?

https://codereview.chromium.org/1412623006/diff/60001/ui/base/touch/touch_ena...
File ui/base/touch/touch_enabled.cc (right):

https://codereview.chromium.org/1412623006/diff/60001/ui/base/touch/touch_ena...
ui/base/touch/touch_enabled.cc:19: bool g_touch_events_enabled = true;
i'll leave this for owner, but I'd avoid using g_ for non global (this is file
scoped object)

[afakhry, 2015-10-30 05:18:03]

https://codereview.chromium.org/1412623006/diff/60001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/1412623006/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_controller.cc:664: #endif  // defined(OS_CHROMEOS)
On 2015/10/29 23:38:43, oshima wrote:
> move them to debug_commands.cc

Done.

https://codereview.chromium.org/1412623006/diff/60001/ash/accelerators/accele...
File ash/accelerators/accelerator_table.h (right):

https://codereview.chromium.org/1412623006/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_table.h:136: TOGGLE_TOUCH_SCREEN,
On 2015/10/29 23:38:43, oshima wrote:
> DEBUG_TOGGLE_xxx and move it to above.

Done.

https://codereview.chromium.org/1412623006/diff/60001/ash/wm/maximize_mode/sc...
File ash/wm/maximize_mode/scoped_disable_internal_mouse_and_keyboard_ozone.cc
(right):

https://codereview.chromium.org/1412623006/diff/60001/ash/wm/maximize_mode/sc...
ash/wm/maximize_mode/scoped_disable_internal_mouse_and_keyboard_ozone.cc:26:
aura::client::GetCursorClient(Shell::GetInstance()->GetPrimaryRootWindow())
On 2015/10/29 23:38:43, oshima wrote:
> Shell::GetInstnace()->cursor_manager()->HideCursor();

Done.

https://codereview.chromium.org/1412623006/diff/60001/ash/wm/maximize_mode/sc...
File ash/wm/maximize_mode/scoped_disable_internal_mouse_and_keyboard_ozone.h
(right):

https://codereview.chromium.org/1412623006/diff/60001/ash/wm/maximize_mode/sc...
ash/wm/maximize_mode/scoped_disable_internal_mouse_and_keyboard_ozone.h:24: bool
should_ignore_touch_pad_;
On 2015/10/29 23:38:43, oshima wrote:
> you may initialize inline. (= false)

Done.

https://codereview.chromium.org/1412623006/diff/60001/ash/wm/maximize_mode/sc...
File ash/wm/maximize_mode/scoped_disable_internal_mouse_and_keyboard_x11.cc
(right):

https://codereview.chromium.org/1412623006/diff/60001/ash/wm/maximize_mode/sc...
ash/wm/maximize_mode/scoped_disable_internal_mouse_and_keyboard_x11.cc:92:
Shell::GetInstance()->GetPrimaryRootWindow())->HideCursor();
On 2015/10/29 23:38:43, oshima wrote:
> ditto

Done.

https://codereview.chromium.org/1412623006/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/system/input_device_settings.cc (right):

https://codereview.chromium.org/1412623006/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/system/input_device_settings.cc:233: PrefService*
local_state = g_browser_process->local_state();
On 2015/10/29 23:38:43, oshima wrote:
> can this be null?

I'm not sure. We already have some code that check for null like 
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

And

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/saf...

Added DCHECKs instead.

https://codereview.chromium.org/1412623006/diff/60001/ui/base/touch/touch_ena...
File ui/base/touch/touch_enabled.cc (right):

https://codereview.chromium.org/1412623006/diff/60001/ui/base/touch/touch_ena...
ui/base/touch/touch_enabled.cc:19: bool g_touch_events_enabled = true;
On 2015/10/29 23:38:43, oshima wrote:
> i'll leave this for owner, but I'd avoid using g_ for non global (this is file
> scoped object)

Yes it can be arguable. I meant it's global to this file scope.

[oshima, 2015-10-31 00:27:49]

lgtm my bits but please wait for review from derat and sadrul

https://codereview.chromium.org/1412623006/diff/60001/ui/base/touch/touch_ena...
File ui/base/touch/touch_enabled.cc (right):

https://codereview.chromium.org/1412623006/diff/60001/ui/base/touch/touch_ena...
ui/base/touch/touch_enabled.cc:19: bool g_touch_events_enabled = true;
On 2015/10/30 05:18:03, afakhry wrote:
> On 2015/10/29 23:38:43, oshima wrote:
> > i'll leave this for owner, but I'd avoid using g_ for non global (this is
file
> > scoped object)
> 
> Yes it can be arguable. I meant it's global to this file scope.

global and file scoped variable have well defined definition.

g_ is useful to know that it's defined elsewhere and can be accessed by other
files and thread.

anyhow, that's my 2c.

https://codereview.chromium.org/1412623006/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/system/input_device_settings.cc (right):

https://codereview.chromium.org/1412623006/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/system/input_device_settings.cc:237: touch_screen_status
= local_state->GetBoolean(::prefs::kTouchScreenEnabled);
nit:

or
bool touch_screen_status = local_state->HasXxx() && local_state->GetBoolean(xxx)

same for others.

[oshima, 2015-10-31 00:27:49]

lgtm my bits but please wait for review from derat and sadrul

https://codereview.chromium.org/1412623006/diff/60001/ui/base/touch/touch_ena...
File ui/base/touch/touch_enabled.cc (right):

https://codereview.chromium.org/1412623006/diff/60001/ui/base/touch/touch_ena...
ui/base/touch/touch_enabled.cc:19: bool g_touch_events_enabled = true;
On 2015/10/30 05:18:03, afakhry wrote:
> On 2015/10/29 23:38:43, oshima wrote:
> > i'll leave this for owner, but I'd avoid using g_ for non global (this is
file
> > scoped object)
> 
> Yes it can be arguable. I meant it's global to this file scope.

global and file scoped variable have well defined definition.

g_ is useful to know that it's defined elsewhere and can be accessed by other
files and thread.

anyhow, that's my 2c.

https://codereview.chromium.org/1412623006/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/system/input_device_settings.cc (right):

https://codereview.chromium.org/1412623006/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/system/input_device_settings.cc:237: touch_screen_status
= local_state->GetBoolean(::prefs::kTouchScreenEnabled);
nit:

or
bool touch_screen_status = local_state->HasXxx() && local_state->GetBoolean(xxx)

same for others.

[afakhry, 2015-11-03 18:02:56]

derat@ and sadrul@ Could you please take a look?

https://codereview.chromium.org/1412623006/diff/60001/ui/base/touch/touch_ena...
File ui/base/touch/touch_enabled.cc (right):

https://codereview.chromium.org/1412623006/diff/60001/ui/base/touch/touch_ena...
ui/base/touch/touch_enabled.cc:19: bool g_touch_events_enabled = true;
On 2015/10/31 00:27:49, oshima wrote:
> On 2015/10/30 05:18:03, afakhry wrote:
> > On 2015/10/29 23:38:43, oshima wrote:
> > > i'll leave this for owner, but I'd avoid using g_ for non global (this is
> file
> > > scoped object)
> > 
> > Yes it can be arguable. I meant it's global to this file scope.
> 
> global and file scoped variable have well defined definition.
> 
> g_ is useful to know that it's defined elsewhere and can be accessed by other
> files and thread.
> 
> anyhow, that's my 2c.

Removed the g_ and moved these two functions to base_event_utils so they can be
used by the touch_factory_x11.

https://codereview.chromium.org/1412623006/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/system/input_device_settings.cc (right):

https://codereview.chromium.org/1412623006/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/system/input_device_settings.cc:237: touch_screen_status
= local_state->GetBoolean(::prefs::kTouchScreenEnabled);
On 2015/10/31 00:27:49, oshima wrote:
> nit:
> 
> or
> bool touch_screen_status = local_state->HasXxx() &&
local_state->GetBoolean(xxx)
> 
> same for others.

I used the ternary operator (?:) since I want it to default to true if
local_state->HasXxx() is false.

[sadrul, 2015-11-03 19:09:26]

https://codereview.chromium.org/1412623006/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/system/input_device_settings.cc (right):

https://codereview.chromium.org/1412623006/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/system/input_device_settings.cc:229: void
InputDeviceSettings::InitTouchDevicesStatusFromLocalPrefs() {
Should we really store developer-only flags in the prefs? If a user does get
stuck, at least they can restart and get back their machine. But if we store the
change in prefs and reload, that is much more difficult to recover.

https://codereview.chromium.org/1412623006/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/system/input_device_settings_impl_x11.cc (right):

https://codereview.chromium.org/1412623006/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/system/input_device_settings_impl_x11.cc:274: if
(device_data_manager->IsXInput2Available()) {
early exit instead.

https://codereview.chromium.org/1412623006/diff/160001/ui/events/base_event_u...
File ui/events/base_event_utils.cc (right):

https://codereview.chromium.org/1412623006/diff/160001/ui/events/base_event_u...
ui/events/base_event_utils.cc:75: return false;
We need to cache the return value, so we don't have to look up the command-line
for each touch event.

https://codereview.chromium.org/1412623006/diff/160001/ui/ozone/DEPS
File ui/ozone/DEPS (right):

https://codereview.chromium.org/1412623006/diff/160001/ui/ozone/DEPS#newcode10
ui/ozone/DEPS:10: "+ui/base/touch",
Why?

[spang, 2015-11-03 22:11:44]

spang@chromium.org changed reviewers:
+ spang@chromium.org

[spang, 2015-11-03 22:11:45]

https://codereview.chromium.org/1412623006/diff/160001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/host/drm_window_host.cc (right):

https://codereview.chromium.org/1412623006/diff/160001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/host/drm_window_host.cc:142: if
(!ui::AreTouchEventsEnabled())
If you must do this, this isn't the right place.

Please add something consistent with the touchpad disabling function in
InputController, or have ash handle it by deleting the association via
AssociateTouchscreens().

Then remove the dep on ui/base/touch.

[afakhry, 2015-11-04 02:28:21]

https://codereview.chromium.org/1412623006/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/system/input_device_settings.cc (right):

https://codereview.chromium.org/1412623006/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/system/input_device_settings.cc:229: void
InputDeviceSettings::InitTouchDevicesStatusFromLocalPrefs() {
On 2015/11/03 19:09:26, sadrul wrote:
> Should we really store developer-only flags in the prefs? If a user does get
> stuck, at least they can restart and get back their machine. But if we store
the
> change in prefs and reload, that is much more difficult to recover.

It's one of the requirements for this feature is that the behavior persists
locally between restarts.

https://codereview.chromium.org/1412623006/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/system/input_device_settings_impl_x11.cc (right):

https://codereview.chromium.org/1412623006/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/system/input_device_settings_impl_x11.cc:274: if
(device_data_manager->IsXInput2Available()) {
On 2015/11/03 19:09:26, sadrul wrote:
> early exit instead.

Done.

https://codereview.chromium.org/1412623006/diff/160001/ui/events/base_event_u...
File ui/events/base_event_utils.cc (right):

https://codereview.chromium.org/1412623006/diff/160001/ui/events/base_event_u...
ui/events/base_event_utils.cc:75: return false;
On 2015/11/03 19:09:26, sadrul wrote:
> We need to cache the return value, so we don't have to look up the
command-line
> for each touch event.

Done.

https://codereview.chromium.org/1412623006/diff/160001/ui/ozone/DEPS
File ui/ozone/DEPS (right):

https://codereview.chromium.org/1412623006/diff/160001/ui/ozone/DEPS#newcode10
ui/ozone/DEPS:10: "+ui/base/touch",
On 2015/11/03 19:09:26, sadrul wrote:
> Why?

Sorry, I forgot to remove this. This is no longer needed since I moved
AreTouchEventsEnabled() to base_event_utils.cc

https://codereview.chromium.org/1412623006/diff/160001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/host/drm_window_host.cc (right):

https://codereview.chromium.org/1412623006/diff/160001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/host/drm_window_host.cc:142: if
(!ui::AreTouchEventsEnabled())
On 2015/11/03 22:11:45, spang wrote:
> If you must do this, this isn't the right place.
> 
> Please add something consistent with the touchpad disabling function in
> InputController, or have ash handle it by deleting the association via
> AssociateTouchscreens().
> 
> Then remove the dep on ui/base/touch.

ui/base/touch has been removed from DEPS since it's no longer needed.
I added the call to AreTouchEventsEnabled() here to match what
WindowTreeHostX11::DispatchXI2Event() does by calling
ui::TouchFactory::ShouldProcessXI2Event() which in turn calls
AreTouchEventsEnabled().

I can add a disabling mechanism similar to what the touchpad currently has. For
ozone, the implementation will be similar, but what about
InputDeviceSettingsImplX11? Is there a preferred consistent way of disabling the
touch screen, or should I keep the above mentioned dependency on
ui::TouchFactory::ShouldProcessXI2Event()?

[spang, 2015-11-04 17:23:32]

On 2015/11/04 02:28:21, afakhry wrote:
>
https://codereview.chromium.org/1412623006/diff/160001/chrome/browser/chromeo...
> File chrome/browser/chromeos/system/input_device_settings.cc (right):
> 
>
https://codereview.chromium.org/1412623006/diff/160001/chrome/browser/chromeo...
> chrome/browser/chromeos/system/input_device_settings.cc:229: void
> InputDeviceSettings::InitTouchDevicesStatusFromLocalPrefs() {
> On 2015/11/03 19:09:26, sadrul wrote:
> > Should we really store developer-only flags in the prefs? If a user does get
> > stuck, at least they can restart and get back their machine. But if we store
> the
> > change in prefs and reload, that is much more difficult to recover.
> 
> It's one of the requirements for this feature is that the behavior persists
> locally between restarts.
> 
>
https://codereview.chromium.org/1412623006/diff/160001/chrome/browser/chromeo...
> File chrome/browser/chromeos/system/input_device_settings_impl_x11.cc (right):
> 
>
https://codereview.chromium.org/1412623006/diff/160001/chrome/browser/chromeo...
> chrome/browser/chromeos/system/input_device_settings_impl_x11.cc:274: if
> (device_data_manager->IsXInput2Available()) {
> On 2015/11/03 19:09:26, sadrul wrote:
> > early exit instead.
> 
> Done.
> 
>
https://codereview.chromium.org/1412623006/diff/160001/ui/events/base_event_u...
> File ui/events/base_event_utils.cc (right):
> 
>
https://codereview.chromium.org/1412623006/diff/160001/ui/events/base_event_u...
> ui/events/base_event_utils.cc:75: return false;
> On 2015/11/03 19:09:26, sadrul wrote:
> > We need to cache the return value, so we don't have to look up the
> command-line
> > for each touch event.
> 
> Done.
> 
> https://codereview.chromium.org/1412623006/diff/160001/ui/ozone/DEPS
> File ui/ozone/DEPS (right):
> 
> https://codereview.chromium.org/1412623006/diff/160001/ui/ozone/DEPS#newcode10
> ui/ozone/DEPS:10: "+ui/base/touch",
> On 2015/11/03 19:09:26, sadrul wrote:
> > Why?
> 
> Sorry, I forgot to remove this. This is no longer needed since I moved
> AreTouchEventsEnabled() to base_event_utils.cc
> 
>
https://codereview.chromium.org/1412623006/diff/160001/ui/ozone/platform/drm/...
> File ui/ozone/platform/drm/host/drm_window_host.cc (right):
> 
>
https://codereview.chromium.org/1412623006/diff/160001/ui/ozone/platform/drm/...
> ui/ozone/platform/drm/host/drm_window_host.cc:142: if
> (!ui::AreTouchEventsEnabled())
> On 2015/11/03 22:11:45, spang wrote:
> > If you must do this, this isn't the right place.
> > 
> > Please add something consistent with the touchpad disabling function in
> > InputController, or have ash handle it by deleting the association via
> > AssociateTouchscreens().
> > 
> > Then remove the dep on ui/base/touch.
> 
> ui/base/touch has been removed from DEPS since it's no longer needed.
> I added the call to AreTouchEventsEnabled() here to match what
> WindowTreeHostX11::DispatchXI2Event() does by calling
> ui::TouchFactory::ShouldProcessXI2Event() which in turn calls
> AreTouchEventsEnabled().
> 
> I can add a disabling mechanism similar to what the touchpad currently has.
For
> ozone, the implementation will be similar, but what about
> InputDeviceSettingsImplX11? Is there a preferred consistent way of disabling
the
> touch screen, or should I keep the above mentioned dependency on
> ui::TouchFactory::ShouldProcessXI2Event()?

I'm not sure about the X11 code, but any code of the form

  if (global_knob_)
    drop_touch_event;

is very likely simply broken. For example we are not allowed to drop a
ui::ET_TOUCH_RELEASED corresponding to a ui::ET_TOUCH_PRESSED event that was not
also dropped. When we naively drop events, we risk breaking important guarantees
about event delivery.

It's not extraordinarily complicated to filter out touches, but this isn't the
way. And I actually don't want the filtering to be in DrmWindow, anyway, because
it's not related to the DRM platform or its windowing.

Maybe we should file a bug about the X11 thing, but at least don't replicate
this pattern in ozone.

[afakhry, 2015-11-04 19:17:08]

On 2015/11/04 17:23:32, spang wrote:
> On 2015/11/04 02:28:21, afakhry wrote:
> >
>
https://codereview.chromium.org/1412623006/diff/160001/chrome/browser/chromeo...
> > File chrome/browser/chromeos/system/input_device_settings.cc (right):
> > 
> >
>
https://codereview.chromium.org/1412623006/diff/160001/chrome/browser/chromeo...
> > chrome/browser/chromeos/system/input_device_settings.cc:229: void
> > InputDeviceSettings::InitTouchDevicesStatusFromLocalPrefs() {
> > On 2015/11/03 19:09:26, sadrul wrote:
> > > Should we really store developer-only flags in the prefs? If a user does
get
> > > stuck, at least they can restart and get back their machine. But if we
store
> > the
> > > change in prefs and reload, that is much more difficult to recover.
> > 
> > It's one of the requirements for this feature is that the behavior persists
> > locally between restarts.
> > 
> >
>
https://codereview.chromium.org/1412623006/diff/160001/chrome/browser/chromeo...
> > File chrome/browser/chromeos/system/input_device_settings_impl_x11.cc
(right):
> > 
> >
>
https://codereview.chromium.org/1412623006/diff/160001/chrome/browser/chromeo...
> > chrome/browser/chromeos/system/input_device_settings_impl_x11.cc:274: if
> > (device_data_manager->IsXInput2Available()) {
> > On 2015/11/03 19:09:26, sadrul wrote:
> > > early exit instead.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1412623006/diff/160001/ui/events/base_event_u...
> > File ui/events/base_event_utils.cc (right):
> > 
> >
>
https://codereview.chromium.org/1412623006/diff/160001/ui/events/base_event_u...
> > ui/events/base_event_utils.cc:75: return false;
> > On 2015/11/03 19:09:26, sadrul wrote:
> > > We need to cache the return value, so we don't have to look up the
> > command-line
> > > for each touch event.
> > 
> > Done.
> > 
> > https://codereview.chromium.org/1412623006/diff/160001/ui/ozone/DEPS
> > File ui/ozone/DEPS (right):
> > 
> >
https://codereview.chromium.org/1412623006/diff/160001/ui/ozone/DEPS#newcode10
> > ui/ozone/DEPS:10: "+ui/base/touch",
> > On 2015/11/03 19:09:26, sadrul wrote:
> > > Why?
> > 
> > Sorry, I forgot to remove this. This is no longer needed since I moved
> > AreTouchEventsEnabled() to base_event_utils.cc
> > 
> >
>
https://codereview.chromium.org/1412623006/diff/160001/ui/ozone/platform/drm/...
> > File ui/ozone/platform/drm/host/drm_window_host.cc (right):
> > 
> >
>
https://codereview.chromium.org/1412623006/diff/160001/ui/ozone/platform/drm/...
> > ui/ozone/platform/drm/host/drm_window_host.cc:142: if
> > (!ui::AreTouchEventsEnabled())
> > On 2015/11/03 22:11:45, spang wrote:
> > > If you must do this, this isn't the right place.
> > > 
> > > Please add something consistent with the touchpad disabling function in
> > > InputController, or have ash handle it by deleting the association via
> > > AssociateTouchscreens().
> > > 
> > > Then remove the dep on ui/base/touch.
> > 
> > ui/base/touch has been removed from DEPS since it's no longer needed.
> > I added the call to AreTouchEventsEnabled() here to match what
> > WindowTreeHostX11::DispatchXI2Event() does by calling
> > ui::TouchFactory::ShouldProcessXI2Event() which in turn calls
> > AreTouchEventsEnabled().
> > 
> > I can add a disabling mechanism similar to what the touchpad currently has.
> For
> > ozone, the implementation will be similar, but what about
> > InputDeviceSettingsImplX11? Is there a preferred consistent way of disabling
> the
> > touch screen, or should I keep the above mentioned dependency on
> > ui::TouchFactory::ShouldProcessXI2Event()?
> 
> I'm not sure about the X11 code, but any code of the form
> 
>   if (global_knob_)
>     drop_touch_event;
> 
> is very likely simply broken. For example we are not allowed to drop a
> ui::ET_TOUCH_RELEASED corresponding to a ui::ET_TOUCH_PRESSED event that was
not
> also dropped. When we naively drop events, we risk breaking important
guarantees
> about event delivery.
> 
> It's not extraordinarily complicated to filter out touches, but this isn't the
> way. And I actually don't want the filtering to be in DrmWindow, anyway,
because
> it's not related to the DRM platform or its windowing.
> 
> Maybe we should file a bug about the X11 thing, but at least don't replicate
> this pattern in ozone.

Done. Please take a look.

[spang, 2015-11-04 20:16:27]

thanks, various ozone files lgtm % couple nits

https://codereview.chromium.org/1412623006/diff/200001/ui/events/ozone/evdev/...
File ui/events/ozone/evdev/input_controller_evdev.h (right):

https://codereview.chromium.org/1412623006/diff/200001/ui/events/ozone/evdev/...
ui/events/ozone/evdev/input_controller_evdev.h:66: void
SetTouchscreenEnabled(bool enabled) override;
I think it should be plural: SetTouchscreenEnabled()

https://codereview.chromium.org/1412623006/diff/200001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/host/drm_window_host.cc (right):

https://codereview.chromium.org/1412623006/diff/200001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/host/drm_window_host.cc:8: #include
"ui/events/base_event_utils.h"
not needed anymore?

[spang, 2015-11-04 20:16:58]

https://codereview.chromium.org/1412623006/diff/200001/ui/events/ozone/evdev/...
File ui/events/ozone/evdev/input_controller_evdev.h (right):

https://codereview.chromium.org/1412623006/diff/200001/ui/events/ozone/evdev/...
ui/events/ozone/evdev/input_controller_evdev.h:66: void
SetTouchscreenEnabled(bool enabled) override;
On 2015/11/04 20:16:27, spang wrote:
> I think it should be plural: SetTouchscreenEnabled()

Oops: SetTouchscreensEnabled()

[afakhry, 2015-11-05 01:22:11]

Thanks, spang!

Sadrul, can you please take another look?

https://codereview.chromium.org/1412623006/diff/200001/ui/events/ozone/evdev/...
File ui/events/ozone/evdev/input_controller_evdev.h (right):

https://codereview.chromium.org/1412623006/diff/200001/ui/events/ozone/evdev/...
ui/events/ozone/evdev/input_controller_evdev.h:66: void
SetTouchscreenEnabled(bool enabled) override;
On 2015/11/04 20:16:58, spang wrote:
> On 2015/11/04 20:16:27, spang wrote:
> > I think it should be plural: SetTouchscreenEnabled()
> 
> Oops: SetTouchscreensEnabled()

Done.

https://codereview.chromium.org/1412623006/diff/200001/ui/ozone/platform/drm/...
File ui/ozone/platform/drm/host/drm_window_host.cc (right):

https://codereview.chromium.org/1412623006/diff/200001/ui/ozone/platform/drm/...
ui/ozone/platform/drm/host/drm_window_host.cc:8: #include
"ui/events/base_event_utils.h"
On 2015/11/04 20:16:27, spang wrote:
> not needed anymore?

No I don't. Thanks for spotting it.

[afakhry, 2015-11-06 03:05:19]

afakhry@chromium.org changed reviewers:
+ mpearson@chromium.org, thestig@chromium.org

[afakhry, 2015-11-06 03:05:20]

Sadrul, friendly ping.

+thestig@chromium.org for browser_prefs.cc.
+mpearson@chromium.org for actions.xml.

[Lei Zhang, 2015-11-06 03:55:10]

On 2015/11/06 03:05:20, afakhry wrote:
> Sadrul, friendly ping.
> 
> mailto:+thestig@chromium.org for browser_prefs.cc.
> mailto:+mpearson@chromium.org for actions.xml.

lgtm

[Mark P, 2015-11-06 05:02:38]

https://codereview.chromium.org/1412623006/diff/220001/tools/metrics/actions/...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1412623006/diff/220001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:731: Metric recorded when the user toggles the
touchpad status using the
Are you sure you don't want user actions that indicate what the new value is? 
e.g., have Accel_Toggle_Touchpad_On and Accel_Toggle_Touchpad_Off?

[afakhry, 2015-11-06 17:33:09]

https://codereview.chromium.org/1412623006/diff/220001/tools/metrics/actions/...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/1412623006/diff/220001/tools/metrics/actions/...
tools/metrics/actions/actions.xml:731: Metric recorded when the user toggles the
touchpad status using the
On 2015/11/06 05:02:37, Mark P wrote:
> Are you sure you don't want user actions that indicate what the new value is? 
> e.g., have Accel_Toggle_Touchpad_On and Accel_Toggle_Touchpad_Off?
> 

I think we're interested in just knowing how many users use this debugging
accelerator rather than the outcome of it.

[Mark P, 2015-11-06 19:13:42]

actions.xml lgtm

[sadrul, 2015-11-09 21:38:53]

LGTM

https://codereview.chromium.org/1412623006/diff/220001/ui/events/base_event_u...
File ui/events/base_event_utils.cc (right):

https://codereview.chromium.org/1412623006/diff/220001/ui/events/base_event_u...
ui/events/base_event_utils.cc:88: UpdateAndCacheTouchStatus();
You can get rid of is_touch_events_status_cached.

Define |touch_events_enabled| only for chromeos, and use that in
SetTouchEventsEnabled() and AreTouchEventsEnabled().

For non-chromeos, inside AreTouchEventsEnabled(), do:

  static bool touch_events_enabled = ComputeTouchStatus();
  return touch_events_enabled;

where ComputeTouchStatus() has the code in UpdateAndCacheTouchStatus(), except
it would simply return the value (instead of setting the global
|touch_events_enabled|, which isn't defined for non-chromeos)

[afakhry, 2015-11-10 00:37:11]

https://codereview.chromium.org/1412623006/diff/220001/ui/events/base_event_u...
File ui/events/base_event_utils.cc (right):

https://codereview.chromium.org/1412623006/diff/220001/ui/events/base_event_u...
ui/events/base_event_utils.cc:88: UpdateAndCacheTouchStatus();
On 2015/11/09 21:38:53, sadrul wrote:
> You can get rid of is_touch_events_status_cached.
> 
> Define |touch_events_enabled| only for chromeos, and use that in
> SetTouchEventsEnabled() and AreTouchEventsEnabled().
> 
> For non-chromeos, inside AreTouchEventsEnabled(), do:
> 
>   static bool touch_events_enabled = ComputeTouchStatus();
>   return touch_events_enabled;
> 
> where ComputeTouchStatus() has the code in UpdateAndCacheTouchStatus(), except
> it would simply return the value (instead of setting the global
> |touch_events_enabled|, which isn't defined for non-chromeos)

Done! Thanks for the useful suggestion! :)

[afakhry, 2015-11-10 01:09:57]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2015-11-10 01:09:57]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org,
spang@chromium.org, thestig@chromium.org, mpearson@chromium.org,
sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/1412623006/#ps240001
(title: "sadrul's comment")

[commit-bot: I haz the power, 2015-11-10 01:13:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412623006/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412623006/240001

[commit-bot: I haz the power, 2015-11-10 02:31:31]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-11-10 02:32:17]

Patchset 13 (id:??) landed as
https://crrev.com/e8eac60b676dc33060e16c1d0e69f8c5150e68ca
Cr-Commit-Position: refs/heads/master@{#358758}

[dtapuska, 2015-11-18 04:00:53]

dtapuska@chromium.org changed reviewers:
+ dtapuska@chromium.org

[dtapuska, 2015-11-18 04:00:55]

https://codereview.chromium.org/1412623006/diff/240001/ui/events/base_event_u...
File ui/events/base_event_utils.cc (right):

https://codereview.chromium.org/1412623006/diff/240001/ui/events/base_event_u...
ui/events/base_event_utils.cc:33: switches::kTouchEventsAuto;
If no command switch is set; auto mode is enable and then it is returned as
true.

Shouldn't it be if a touch device is present. That is what it was before.. We
are seeing a regression in M48; issue 555746