ash: Move VirtualKeyboardStateObserver functionality to ShellObserver

ash: Move VirtualKeyboardStateObserver functionality to ShellObserver

It doesn't need to exist as a separate observer with a single function. This
simplifies ShellDelegate.

Addresses a TODO from https://codereview.chromium.org/2118593002/

BUG=619636
TEST=ash_unittests and chrome unit_tests

Committed: https://crrev.com/66f69e4e0491ea56612c86342b7dccefcb8fe20d
Cr-Commit-Position: refs/heads/master@{#404559}

[James Cook, 2016-07-08 19:30:54]

The CQ bit was checked by jamescook@chromium.org to run a CQ dry run

[James Cook, 2016-07-08 19:31:07]

jamescook@chromium.org changed reviewers:
+ msw@chromium.org

[James Cook, 2016-07-08 19:31:08]

msw, please take a look.

[commit-bot: I haz the power, 2016-07-08 19:31:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msw, 2016-07-08 19:51:18]

lgtm with minor comments.

https://codereview.chromium.org/2134833002/diff/1/ash/common/shell_observer.h
File ash/common/shell_observer.h (right):

https://codereview.chromium.org/2134833002/diff/1/ash/common/shell_observer.h...
ash/common/shell_observer.h:71: virtual void OnVirtualKeyboardStateChanged(bool
activated) {}
Maybe it'd be even nicer to move this to keyboard::KeyboardControllerObserver,
but that doesn't seem as straightforward; and this would probably be an okay
incremental step even then; wdyt?

https://codereview.chromium.org/2134833002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):

https://codereview.chromium.org/2134833002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:303: if
(!chrome::IsRunningInMash())
I wonder if we can remove this mash notimpl now? Ditto below.

https://codereview.chromium.org/2134833002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:929: //
implementation:
optional nit: remove/put above... (was "blah impl:")

[commit-bot: I haz the power, 2016-07-08 20:35:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-08 20:35:18]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-07-08 22:31:03]

msw, please take another look.

https://codereview.chromium.org/2134833002/diff/1/ash/common/shell_observer.h
File ash/common/shell_observer.h (right):

https://codereview.chromium.org/2134833002/diff/1/ash/common/shell_observer.h...
ash/common/shell_observer.h:71: virtual void OnVirtualKeyboardStateChanged(bool
activated) {}
On 2016/07/08 19:51:18, msw wrote:
> Maybe it'd be even nicer to move this to keyboard::KeyboardControllerObserver,
> but that doesn't seem as straightforward; and this would probably be an okay
> incremental step even then; wdyt?

Yeah, that doesn't seem straightforward to me either. It looks like places that
use this notification use it to add/remove observers from KeyboardController,
which is.... odd.

Would you be OK with me landing this as partial cleanup?

https://codereview.chromium.org/2134833002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):

https://codereview.chromium.org/2134833002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:303: if
(!chrome::IsRunningInMash())
On 2016/07/08 19:51:18, msw wrote:
> I wonder if we can remove this mash notimpl now? Ditto below.

I think we should keep it. In mash we're going to need to talk to some mojo
interface to register shell observers, and that isn't implemented yet.

https://codereview.chromium.org/2134833002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:929: //
implementation:
On 2016/07/08 19:51:18, msw wrote:
> optional nit: remove/put above... (was "blah impl:")

Done here and in a couple other places.

[msw, 2016-07-08 22:39:28]

lgtm

https://codereview.chromium.org/2134833002/diff/1/ash/common/shell_observer.h
File ash/common/shell_observer.h (right):

https://codereview.chromium.org/2134833002/diff/1/ash/common/shell_observer.h...
ash/common/shell_observer.h:71: virtual void OnVirtualKeyboardStateChanged(bool
activated) {}
On 2016/07/08 22:31:03, James Cook wrote:
> On 2016/07/08 19:51:18, msw wrote:
> > Maybe it'd be even nicer to move this to
keyboard::KeyboardControllerObserver,
> > but that doesn't seem as straightforward; and this would probably be an okay
> > incremental step even then; wdyt?
> 
> Yeah, that doesn't seem straightforward to me either. It looks like places
that
> use this notification use it to add/remove observers from KeyboardController,
> which is.... odd.
> 
> Would you be OK with me landing this as partial cleanup?

Oh yeah, for sure; my comment was more of an aside/pondering.

https://codereview.chromium.org/2134833002/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (right):

https://codereview.chromium.org/2134833002/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:303: if
(!chrome::IsRunningInMash())
On 2016/07/08 22:31:03, James Cook wrote:
> On 2016/07/08 19:51:18, msw wrote:
> > I wonder if we can remove this mash notimpl now? Ditto below.
> 
> I think we should keep it. In mash we're going to need to talk to some mojo
> interface to register shell observers, and that isn't implemented yet.

Acknowledged; as we discussed, WmShell won't even exist for Chrome.

[James Cook, 2016-07-08 22:48:54]

jamescook@chromium.org changed reviewers:
+ stevenjb@chromium.org

[James Cook, 2016-07-08 22:48:54]

stevenjb, can I get OWNERS for chrome/browser/chromeos?

[James Cook, 2016-07-08 22:49:00]

The CQ bit was checked by jamescook@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-08 22:49:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[stevenjb, 2016-07-08 23:52:26]

c/b RS lgtm

[James Cook, 2016-07-08 23:55:22]

The CQ bit was unchecked by jamescook@chromium.org

[James Cook, 2016-07-08 23:55:28]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2016-07-08 23:55:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-09 01:32:34]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-09 01:34:22]

Description was changed from

==========
ash: Move VirtualKeyboardStateObserver functionality to ShellObserver

It doesn't need to exist as a separate observer with a single function. This
simplifies ShellDelegate.

Addresses a TODO from https://codereview.chromium.org/2118593002/

BUG=619636
TEST=ash_unittests and chrome unit_tests
==========

to

==========
ash: Move VirtualKeyboardStateObserver functionality to ShellObserver

It doesn't need to exist as a separate observer with a single function. This
simplifies ShellDelegate.

Addresses a TODO from https://codereview.chromium.org/2118593002/

BUG=619636
TEST=ash_unittests and chrome unit_tests

Committed: https://crrev.com/66f69e4e0491ea56612c86342b7dccefcb8fe20d
Cr-Commit-Position: refs/heads/master@{#404559}
==========

[commit-bot: I haz the power, 2016-07-09 01:34:23]

Patchset 2 (id:??) landed as
https://crrev.com/66f69e4e0491ea56612c86342b7dccefcb8fe20d
Cr-Commit-Position: refs/heads/master@{#404559}