Remove 'Enable tap-to-click' checkbox for non-touchpad devices.

Remove 'Enable tap-to-click' checkbox for non-touchpad devices.

BUG=chromium-os:21361
TEST=Go to System options on a non-touch device. Should not see Enable tap-to-click.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107164

[achuithb, 2011-10-12 23:03:02]

Dave, please review..

[DaveMoore, 2011-10-13 00:04:43]

http://codereview.chromium.org/8249011/diff/8/chrome/browser/chromeos/system/...
File chrome/browser/chromeos/system/touchpad_settings.cc (right):

http://codereview.chromium.org/8249011/diff/8/chrome/browser/chromeos/system/...
chrome/browser/chromeos/system/touchpad_settings.cc:39:
file_util::PathExists(FilePath(kTpControl));
This file exists even on devices with no touchpad.

[achuithb, 2011-10-13 00:06:44]

http://codereview.chromium.org/8249011/diff/8/chrome/browser/chromeos/system/...
File chrome/browser/chromeos/system/touchpad_settings.cc (right):

http://codereview.chromium.org/8249011/diff/8/chrome/browser/chromeos/system/...
chrome/browser/chromeos/system/touchpad_settings.cc:39:
file_util::PathExists(FilePath(kTpControl));
How do you tell if there's no touchpad then?

[DaveMoore, 2011-10-13 00:10:41]

I don't know of a way offhand. Maybe something could be added to tpcontrol that
gives a reasonable error that could be passed back to the caller. Or maybe we
need to probe the x devices, looking for a touchpad (although there could be
different devices that are all touchpads).

Basically there's no current model for this. Maybe there's something I'm not
thinking of, but I don't think this is a small easy fix.

On 2011/10/13 00:06:44, achuith.bhandarkar wrote:
>
http://codereview.chromium.org/8249011/diff/8/chrome/browser/chromeos/system/...
> File chrome/browser/chromeos/system/touchpad_settings.cc (right):
> 
>
http://codereview.chromium.org/8249011/diff/8/chrome/browser/chromeos/system/...
> chrome/browser/chromeos/system/touchpad_settings.cc:39:
> file_util::PathExists(FilePath(kTpControl));
> How do you tell if there's no touchpad then?

[achuithb, 2011-10-13 00:28:04]

Sounds like a lot of work to disable one checkbox!

Hopefully a way to figure out the existence of a touchpad on devices will
emerge. Do you know who would be a good contact for this? Should I pose the
question to chromeos-team?

On 2011/10/13 00:10:41, DaveMoore wrote:
> I don't know of a way offhand. Maybe something could be added to tpcontrol
that
> gives a reasonable error that could be passed back to the caller. Or maybe we
> need to probe the x devices, looking for a touchpad (although there could be
> different devices that are all touchpads).
> 
> Basically there's no current model for this. Maybe there's something I'm not
> thinking of, but I don't think this is a small easy fix.
> 
> On 2011/10/13 00:06:44, achuith.bhandarkar wrote:
> >
>
http://codereview.chromium.org/8249011/diff/8/chrome/browser/chromeos/system/...
> > File chrome/browser/chromeos/system/touchpad_settings.cc (right):
> > 
> >
>
http://codereview.chromium.org/8249011/diff/8/chrome/browser/chromeos/system/...
> > chrome/browser/chromeos/system/touchpad_settings.cc:39:
> > file_util::PathExists(FilePath(kTpControl));
> > How do you tell if there's no touchpad then?

[Daniel Kurtz, 2011-10-18 01:44:25]

On 2011/10/13 00:28:04, achuith.bhandarkar wrote:
> Sounds like a lot of work to disable one checkbox!
> 
> Hopefully a way to figure out the existence of a touchpad on devices will
> emerge. Do you know who would be a good contact for this? Should I pose the
> question to chromeos-team?
> 
> On 2011/10/13 00:10:41, DaveMoore wrote:
> > I don't know of a way offhand. Maybe something could be added to tpcontrol
> that
> > gives a reasonable error that could be passed back to the caller. Or maybe
we
> > need to probe the x devices, looking for a touchpad (although there could be
> > different devices that are all touchpads).
> > 
> > Basically there's no current model for this. Maybe there's something I'm not
> > thinking of, but I don't think this is a small easy fix.
> > 
> > On 2011/10/13 00:06:44, achuith.bhandarkar wrote:
> > >
> >
>
http://codereview.chromium.org/8249011/diff/8/chrome/browser/chromeos/system/...
> > > File chrome/browser/chromeos/system/touchpad_settings.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/8249011/diff/8/chrome/browser/chromeos/system/...
> > > chrome/browser/chromeos/system/touchpad_settings.cc:39:
> > > file_util::PathExists(FilePath(kTpControl));
> > > How do you tell if there's no touchpad then?

One idea is you could listen to XI_HierarchyChanged events and maintain a list
of pointer devices currently attached to the device.  See
"xinput_hierarchy_changed_event_listener.cc" and related code.

[achuithb, 2011-10-21 01:15:45]

I went with the approach of using tpcontrol (which does exist on non-touchpad
devices). 'tpcontrol status' returns nothing on such devices.

I have to run 'tpcontrol status' on the FILE thread, and I did not want to
change the lifetime of SystemOptionsHandler, so I have a separate TouchpadHelper
class to handle moving data between the different threads. It handles the case
where it outlives SystemOptionsHandler.

PTAL, Dave.

[DaveMoore, 2011-10-21 15:58:41]

This will slow down startup as the FILE thread is somewhat saturated and this
will cause us to fetch the session data later. It needs to be launched
asynchonrously.

[achuithb, 2011-10-21 17:49:12]

Dave: I kick off TouchPadExists on the file thread only when the
chrome://system/settings page needs to be displayed. And I cache the results so
the next time would be a little faster (still have to bounce off the slow file
thread unfortunately). 

The UI doesn't wait for the results of TouchPadExists, but instead displays the
Enable-Tap-To-Click div until we get the response that hides it. In practice,
the webui/settings pages are slow enough that the response makes it back in
time. The alternative would be to have the Enable-Tap-To-Click div hidden by
default and show it once we've established that we have a touchpad. I should
probably switch to that model instead...

[DaveMoore, 2011-10-24 21:12:09]

lgtm

[achuithb, 2011-10-24 21:24:43]

Thanks for the review, Dave.

Daniel - are you planning to review this?