Fix a regression in checking the status of the touch events.

Fix a regression in checking the status of the touch events.

When the #touch-events flag is set to Automatic, we ought to check whether
there's actually a touch device present.

BUG=555390, 555746, 556390
Committed: https://crrev.com/b2794c1972bddd3fc1f3d4afcb079f8e4b504c49
Cr-Commit-Position: refs/heads/master@{#360472}

[afakhry, 2015-11-17 19:41:44]

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

[afakhry, 2015-11-17 19:41:45]

Hi Sadrul, could you please take a look at this CL? I would like to discuss with
you if there's a way to avoid changing the DEPS file so that we can use
ui::IsTouchDevicePresent().

[sadrul, 2015-11-17 22:32:05]

not lgtm

https://codereview.chromium.org/1453813003/diff/1/ui/events/base_event_utils.cc
File ui/events/base_event_utils.cc (right):

https://codereview.chromium.org/1453813003/diff/1/ui/events/base_event_utils....
ui/events/base_event_utils.cc:10: #include "ui/base/touch/touch_device.h"
This is in the ui_base target, which depends on the events_base target, which
base_event_utils.cc belongs to. This introduces a dependency cycle, and we
cannot have it.

[afakhry, 2015-11-18 19:04:46]

Hey Sadrul. Please check this new solution. Thanks!

[afakhry, 2015-11-18 19:30:41]

Description was changed from

==========
Fix a regression in checking the status of the touch events.

When the #touch-events flag is set to Automatic, we ought to check whether
there's actually a touch device present.

BUG=555390
==========

to

==========
Fix a regression in checking the status of the touch events.

When the #touch-events flag is set to Automatic, we ought to check whether
there's actually a touch device present.

BUG=555390,555746,556390
==========

[sadrul, 2015-11-18 19:59:24]

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

[sadrul, 2015-11-18 19:59:24]

+dtapuska@ mind double checking the code in touch_enabled.cc?

The approach in the CL looks good to me. I will stamp with dtapuska@'s blessing

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

https://codereview.chromium.org/1453813003/diff/80001/ui/base/touch/touch_ena...
ui/base/touch/touch_enabled.cc:19: auto command_line =
base::CommandLine::ForCurrentProcess();
auto*

https://codereview.chromium.org/1453813003/diff/80001/ui/base/touch/touch_ena...
ui/base/touch/touch_enabled.cc:34: LOG(ERROR) << "Invalid --touch-events option:
" << touch_enabled_switch;
Use [D]LOG_IF instead

[afakhry, 2015-11-18 20:08:53]

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

https://codereview.chromium.org/1453813003/diff/80001/ui/base/touch/touch_ena...
ui/base/touch/touch_enabled.cc:19: auto command_line =
base::CommandLine::ForCurrentProcess();
On 2015/11/18 19:59:24, sadrul wrote:
> auto*

Done.

https://codereview.chromium.org/1453813003/diff/80001/ui/base/touch/touch_ena...
ui/base/touch/touch_enabled.cc:34: LOG(ERROR) << "Invalid --touch-events option:
" << touch_enabled_switch;
On 2015/11/18 19:59:24, sadrul wrote:
> Use [D]LOG_IF instead

Done.

[dtapuska, 2015-11-18 20:13:06]

https://codereview.chromium.org/1453813003/diff/80001/ui/events/devices/x11/t...
File ui/events/devices/x11/touch_factory_x11.cc (right):

https://codereview.chromium.org/1453813003/diff/80001/ui/events/devices/x11/t...
ui/events/devices/x11/touch_factory_x11.cc:38: return
!GetTouchEventsCrOsMasterSwitch() && touch_flag_status;
So the problem I have with this is that this is a singleton class and what
happens when GetTouchEventsCrOsMasterSwitch() changes underneath?

Specifically startup sequence of this class, or if the user toggles the value;
or when are settings restored. Is it ensured that they are restored before
anyone calls this singleton GetInstance call?

https://codereview.chromium.org/1453813003/diff/100001/ui/base/touch/touch_en...
File ui/base/touch/touch_enabled.cc (right):

https://codereview.chromium.org/1453813003/diff/100001/ui/base/touch/touch_en...
ui/base/touch/touch_enabled.cc:44: return GetTouchEventsCrOsMasterSwitch() &&
touch_status;
I personally would have written this as touch_status &&
GetTouchEventsCrOsMasterSwitch as the first conditional is a static boolean in
this scope (cheap call); and not the cross library call.

[afakhry, 2015-11-18 20:36:16]

https://codereview.chromium.org/1453813003/diff/80001/ui/events/devices/x11/t...
File ui/events/devices/x11/touch_factory_x11.cc (right):

https://codereview.chromium.org/1453813003/diff/80001/ui/events/devices/x11/t...
ui/events/devices/x11/touch_factory_x11.cc:38: return
!GetTouchEventsCrOsMasterSwitch() && touch_flag_status;
On 2015/11/18 20:13:05, dtapuska wrote:
> So the problem I have with this is that this is a singleton class and what
> happens when GetTouchEventsCrOsMasterSwitch() changes underneath?
> 
> Specifically startup sequence of this class, or if the user toggles the value;
> or when are settings restored. Is it ensured that they are restored before
> anyone calls this singleton GetInstance call?

Thank you for catching that! The touch events master switch for ChromeOS X11 can
change after this singleton is initialized using a debug accelerator.

I modified the code to account for that.

https://codereview.chromium.org/1453813003/diff/100001/ui/base/touch/touch_en...
File ui/base/touch/touch_enabled.cc (right):

https://codereview.chromium.org/1453813003/diff/100001/ui/base/touch/touch_en...
ui/base/touch/touch_enabled.cc:44: return GetTouchEventsCrOsMasterSwitch() &&
touch_status;
On 2015/11/18 20:13:06, dtapuska wrote:
> I personally would have written this as touch_status &&
> GetTouchEventsCrOsMasterSwitch as the first conditional is a static boolean in
> this scope (cheap call); and not the cross library call.

Done.

[afakhry, 2015-11-18 20:46:08]

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

[afakhry, 2015-11-18 20:46:08]

Oshima, could you please review
chrome/browser/chromeos/system/input_device_settings_impl_x11.cc?

[dtapuska, 2015-11-18 20:46:40]

On 2015/11/18 20:36:16, afakhry wrote:
>
https://codereview.chromium.org/1453813003/diff/80001/ui/events/devices/x11/t...
> File ui/events/devices/x11/touch_factory_x11.cc (right):
> 
>
https://codereview.chromium.org/1453813003/diff/80001/ui/events/devices/x11/t...
> ui/events/devices/x11/touch_factory_x11.cc:38: return
> !GetTouchEventsCrOsMasterSwitch() && touch_flag_status;
> On 2015/11/18 20:13:05, dtapuska wrote:
> > So the problem I have with this is that this is a singleton class and what
> > happens when GetTouchEventsCrOsMasterSwitch() changes underneath?
> > 
> > Specifically startup sequence of this class, or if the user toggles the
value;
> > or when are settings restored. Is it ensured that they are restored before
> > anyone calls this singleton GetInstance call?
> 
> Thank you for catching that! The touch events master switch for ChromeOS X11
can
> change after this singleton is initialized using a debug accelerator.
> 
> I modified the code to account for that.
> 
>
https://codereview.chromium.org/1453813003/diff/100001/ui/base/touch/touch_en...
> File ui/base/touch/touch_enabled.cc (right):
> 
>
https://codereview.chromium.org/1453813003/diff/100001/ui/base/touch/touch_en...
> ui/base/touch/touch_enabled.cc:44: return GetTouchEventsCrOsMasterSwitch() &&
> touch_status;
> On 2015/11/18 20:13:06, dtapuska wrote:
> > I personally would have written this as touch_status &&
> > GetTouchEventsCrOsMasterSwitch as the first conditional is a static boolean
in
> > this scope (cheap call); and not the cross library call.
> 
> Done.

lgtm (non-owner)

[sadrul, 2015-11-18 20:58:46]

https://codereview.chromium.org/1453813003/diff/120001/ui/base/touch/touch_en...
File ui/base/touch/touch_enabled.cc (right):

https://codereview.chromium.org/1453813003/diff/120001/ui/base/touch/touch_en...
ui/base/touch/touch_enabled.cc:44: return touch_status &&
GetTouchEventsCrOsX11MasterSwitch();
On a chromeos device build (i.e. when USE_X11 is not defined), we are not
looking at the master switch ... which means the debug accelerators will not do
the right thing anymore?

[afakhry, 2015-11-18 21:03:14]

https://codereview.chromium.org/1453813003/diff/120001/ui/base/touch/touch_en...
File ui/base/touch/touch_enabled.cc (right):

https://codereview.chromium.org/1453813003/diff/120001/ui/base/touch/touch_en...
ui/base/touch/touch_enabled.cc:44: return touch_status &&
GetTouchEventsCrOsX11MasterSwitch();
On 2015/11/18 20:58:46, sadrul wrote:
> On a chromeos device build (i.e. when USE_X11 is not defined), we are not
> looking at the master switch ... which means the debug accelerators will not
do
> the right thing anymore?

On non-X11 ChromeOS builds (i.e. Ozone builds) we are using a different approach
to toggle the touch device. Please take a look here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

[sadrul, 2015-11-18 21:14:30]

https://codereview.chromium.org/1453813003/diff/120001/ui/base/touch/touch_en...
File ui/base/touch/touch_enabled.cc (right):

https://codereview.chromium.org/1453813003/diff/120001/ui/base/touch/touch_en...
ui/base/touch/touch_enabled.cc:44: return touch_status &&
GetTouchEventsCrOsX11MasterSwitch();
On 2015/11/18 21:03:13, afakhry wrote:
> On 2015/11/18 20:58:46, sadrul wrote:
> > On a chromeos device build (i.e. when USE_X11 is not defined), we are not
> > looking at the master switch ... which means the debug accelerators will not
> do
> > the right thing anymore?
> 
> On non-X11 ChromeOS builds (i.e. Ozone builds) we are using a different
approach
> to toggle the touch device. Please take a look here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

The various places that call AreTouchEventsEnabled() are going to get the wrong
value though, right? (e.g. new pages created after turning off the touchscreen
using the debug accelerator will still think that touch events are available)

[afakhry, 2015-11-18 21:33:04]

https://codereview.chromium.org/1453813003/diff/120001/ui/base/touch/touch_en...
File ui/base/touch/touch_enabled.cc (right):

https://codereview.chromium.org/1453813003/diff/120001/ui/base/touch/touch_en...
ui/base/touch/touch_enabled.cc:44: return touch_status &&
GetTouchEventsCrOsX11MasterSwitch();
On 2015/11/18 21:14:30, sadrul wrote:
> On 2015/11/18 21:03:13, afakhry wrote:
> > On 2015/11/18 20:58:46, sadrul wrote:
> > > On a chromeos device build (i.e. when USE_X11 is not defined), we are not
> > > looking at the master switch ... which means the debug accelerators will
not
> > do
> > > the right thing anymore?
> > 
> > On non-X11 ChromeOS builds (i.e. Ozone builds) we are using a different
> approach
> > to toggle the touch device. Please take a look here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
> 
> The various places that call AreTouchEventsEnabled() are going to get the
wrong
> value though, right? (e.g. new pages created after turning off the touchscreen
> using the debug accelerator will still think that touch events are available)

Valid point! Thanks. I now set the switch on ozone as well.

[sadrul, 2015-11-18 22:42:30]

lgtm

[oshima, 2015-11-18 22:49:42]

lgtm given that sadrul is happy and we need to fix this for beta.
Following are for future improvements.

https://codereview.chromium.org/1453813003/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/system/input_device_settings_impl_ozone.cc (right):

https://codereview.chromium.org/1453813003/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/system/input_device_settings_impl_ozone.cc:140:
ui::SetTouchEventsCrOsMasterSwitch(enabled);
I think it's better for the input controller to set this so that there is a
single place that manages the state in ozone.

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

https://codereview.chromium.org/1453813003/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/system/input_device_settings_impl_x11.cc:297:
ui::SetTouchEventsCrOsMasterSwitch(enabled);
It's also be more consistent if we can do something like

ui::TocuhFactory::GetInstance()->SetTouchscreensEnabled(enabled)

and let the InputFactory sets this flag and update its own flag like ozone case.

https://codereview.chromium.org/1453813003/diff/140001/ui/base/touch/touch_en...
File ui/base/touch/touch_enabled.h (right):

https://codereview.chromium.org/1453813003/diff/140001/ui/base/touch/touch_en...
ui/base/touch/touch_enabled.h:14: UI_BASE_EXPORT bool AreTouchEventsEnabled();
This is really TouchDeviceAvaiableAndTouchEventsEnabled(), which is too long.
how about:

ShouldHandleTouchEvents()

?

[afakhry, 2015-11-19 00:10:49]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2015-11-19 00:10:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtapuska@chromium.org
Link to the patchset: https://codereview.chromium.org/1453813003/#ps140001
(title: "Use the master switch on ozone as well.")

[commit-bot: I haz the power, 2015-11-19 00:12:33]

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

[commit-bot: I haz the power, 2015-11-19 01:50:31]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-11-19 01:50:59]

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