Use helper DeviceChangeHandler to call mousecontrol/tpcontrol whenever a device is attached.

Use helper DeviceChangeHandler to call mousecontrol/tpcontrol whenever a device is attached.

BUG=240819
TEST=manual

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203868

[achuithb, 2013-05-21 01:31:47]

Please take a look, Xiyuan.

[xiyuan, 2013-05-21 02:01:44]

I am not feeling comfortable putting PointerDeviceObserver into Preferences.
There is a Preferences instance per user profile. How would this work when
multiple profile is turned on? Also, it would not help on the login screen when
no user is logged in.

How about putting PointerDeviceObserver into ChromeBrowserMainPartsChromeos? We
could initialized it when the system starts up (ideally, schedule an
initialization to avoid delaying login screen). Would this work?

https://codereview.chromium.org/15381003/diff/4001/chrome/browser/chromeos/pr...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/15381003/diff/4001/chrome/browser/chromeos/pr...
chrome/browser/chromeos/preferences.cc:52: : prefs_(NULL),
Good catch.

[achuithb, 2013-05-22 01:04:36]

On 2013/05/21 02:01:44, xiyuan wrote:
> I am not feeling comfortable putting PointerDeviceObserver into Preferences.
> There is a Preferences instance per user profile. How would this work when
> multiple profile is turned on? Also, it would not help on the login screen
when
> no user is logged in.
> 
> How about putting PointerDeviceObserver into ChromeBrowserMainPartsChromeos?
We
> could initialized it when the system starts up (ideally, schedule an
> initialization to avoid delaying login screen). Would this work?
> 

So the problem here is that chromeos_preferences_ is buried in profile_impl
here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...

When ChromeBrowserMainPartsChromeos discovers that a new device is attached, it
needs to poke chrome::Preferences and there isn't a way to access it.

[achuithb, 2013-05-22 01:08:21]

On 2013/05/22 01:04:36, achuith.bhandarkar wrote:
> On 2013/05/21 02:01:44, xiyuan wrote:
> > I am not feeling comfortable putting PointerDeviceObserver into Preferences.
> > There is a Preferences instance per user profile. How would this work when
> > multiple profile is turned on? Also, it would not help on the login screen
> when
> > no user is logged in.
> > 
> > How about putting PointerDeviceObserver into ChromeBrowserMainPartsChromeos?
> We
> > could initialized it when the system starts up (ideally, schedule an
> > initialization to avoid delaying login screen). Would this work?
> > 
> 
> So the problem here is that chromeos_preferences_ is buried in profile_impl
> here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...
> 
> When ChromeBrowserMainPartsChromeos discovers that a new device is attached,
it
> needs to poke chrome::Preferences and there isn't a way to access it.

I can add a method like SetPointerDeviceSettings() to Profile:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...
I suspect this will be met with some resistance from the owners

[xiyuan, 2013-05-22 04:52:24]

On 2013/05/22 01:08:21, achuith.bhandarkar wrote:
> On 2013/05/22 01:04:36, achuith.bhandarkar wrote:
> > On 2013/05/21 02:01:44, xiyuan wrote:
> > > I am not feeling comfortable putting PointerDeviceObserver into
Preferences.
> > > There is a Preferences instance per user profile. How would this work when
> > > multiple profile is turned on? Also, it would not help on the login screen
> > when
> > > no user is logged in.
> > > 
> > > How about putting PointerDeviceObserver into
ChromeBrowserMainPartsChromeos?
> > We
> > > could initialized it when the system starts up (ideally, schedule an
> > > initialization to avoid delaying login screen). Would this work?
> > > 
> > 
> > So the problem here is that chromeos_preferences_ is buried in profile_impl
> > here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...
> > 
> > When ChromeBrowserMainPartsChromeos discovers that a new device is attached,
> it
> > needs to poke chrome::Preferences and there isn't a way to access it.
> 
> I can add a method like SetPointerDeviceSettings() to Profile:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...
> I suspect this will be met with some resistance from the owners

We probably should not do that either. Binding the system device settings with
Profile now feels wrong (as multiple profile support is being added). We don't
need to fix that in this CL but we probably should not add more stuff there
either.

Things you want from the chromeos::Preferences are a bunch of user prefs that
you want to apply the newly attached device, right? How about just doing that? 

e.g. we could have a PointerDeviceChangeHandler that does what you are doing now
for the chromeos::Preferences to get notifications of device change. Then, in
the notification handler, just fetch prefs::kTapToClickEnabled etc from the
current user profile and applies it.

[achuithb, 2013-05-23 01:03:37]

Xiyuan, what do you think of this change? I could make the Observer a separate
object owned by ChromeBrowserMainPartsChromeos if that's cleaner.

[xiyuan, 2013-05-23 01:57:45]

On 2013/05/23 01:03:37, achuith.bhandarkar wrote:
> Xiyuan, what do you think of this change? I could make the Observer a separate
> object owned by ChromeBrowserMainPartsChromeos if that's cleaner.

You read my mind. Yes, please do. :)

[achuithb, 2013-05-23 21:35:00]

On 2013/05/23 01:57:45, xiyuan wrote:
> On 2013/05/23 01:03:37, achuith.bhandarkar wrote:
> > Xiyuan, what do you think of this change? I could make the Observer a
separate
> > object owned by ChromeBrowserMainPartsChromeos if that's cleaner.
> 
> You read my mind. Yes, please do. :)

PTAL, Xiyuan. Let me know what you think.

[xiyuan, 2013-05-23 21:40:41]

LGTM

Cool. Thank your for making the changes.

https://codereview.chromium.org/15381003/diff/35001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/system/device_change_handler.cc (right):

https://codereview.chromium.org/15381003/diff/35001/chrome/browser/chromeos/s...
chrome/browser/chromeos/system/device_change_handler.cc:22:
DeviceChangeHandler::~DeviceChangeHandler() {
nit: Do we need to RemoveObserver(this)?

[achuithb, 2013-05-23 21:51:41]

Thanks for the review!

https://codereview.chromium.org/15381003/diff/35001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/system/device_change_handler.cc (right):

https://codereview.chromium.org/15381003/diff/35001/chrome/browser/chromeos/s...
chrome/browser/chromeos/system/device_change_handler.cc:22:
DeviceChangeHandler::~DeviceChangeHandler() {
On 2013/05/23 21:40:42, xiyuan wrote:
> nit: Do we need to RemoveObserver(this)?

Better to be explicit, so I added it. Done.

[achuithb, 2013-05-31 01:30:17]

Xiyuan, I did some more testing, and the handling of owner preferences on the
login screen was not correct.

Now, I check if we're not logged in, and if so, apply owner settings. 

Also, we weren't applying settings on startup, only when the device hierarchy
changed.

Please take another look.

[achuithb, 2013-05-31 01:31:04]

The only changes are in device_change_handler.cc and login_display_host_impl.cc.

[xiyuan, 2013-05-31 03:36:22]

LGTM

[commit-bot: I haz the power, 2013-05-31 03:52:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/achuith@chromium.org/15381003/51002

[commit-bot: I haz the power, 2013-05-31 06:12:04]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-05-31 08:17:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/achuith@chromium.org/15381003/51002

[commit-bot: I haz the power, 2013-05-31 10:37:45]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-05-31 10:48:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/achuith@chromium.org/15381003/51002

[commit-bot: I haz the power, 2013-05-31 13:28:18]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-05-31 19:15:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/achuith@chromium.org/15381003/51002

[commit-bot: I haz the power, 2013-05-31 22:00:32]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[achuithb, 2013-06-03 22:29:15]

Xiyuan: There was one more error in my code. I'm accessing g_browser_process in
DeviceChangeHandler, so I need to do this in PostBrowserStart. Please take
another look.

[commit-bot: I haz the power, 2013-06-04 00:04:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/achuith@chromium.org/15381003/96001

[commit-bot: I haz the power, 2013-06-04 06:35:35]

Change committed as 203868