Refactors keyboard related code so mash can use a keyboard

Refactors keyboard related code so mash can use a keyboard

This consists of the following:
. Keyboard functionality as needed by mash is now in KeyboardUI.
. KeyboardUI has a mus implementation that uses keyboard.mojom.
. mojom::Keyboard is provided by chrome, but NOTIMPLEMENTED yet.
. Added ChromeInterfaceFactory that is registered in
  ChromeBrowserMainChromeos (I'll move to an ash class later on).

BUG=585253
TEST=none
R=ben@chromium.org, sadrul@chromium.org

Committed: https://crrev.com/fee520c1c2c91a2baca7d777bddd322f3e1854fc
Cr-Commit-Position: refs/heads/master@{#374950}

[sadrul, 2016-02-10 23:57:14]

Looks good. A couple of questions about slightly alternative approaches:

https://codereview.chromium.org/1684823004/diff/20001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/1684823004/diff/20001/ash/shell.cc#newcode1056
ash/shell.cc:1056: 
I wonder if ShellInitParams could include a KeyboardUIFactory. If there's no
factory, Shell creates the default KeyboardUIImpl. //ash/mus/ can then provide a
factory to create KeyboardUIMus instead. With this approach, ash itself can
continue to be unaware of mus, and there's a clear separation of mus-ash vs
non-mus-ash.

https://codereview.chromium.org/1684823004/diff/20001/ash/shell.cc#newcode1103
ash/shell.cc:1103: return;
Similarly, would it make sense to include a flag to ShellInitParams stating
whether ash is running inside mus or not? We could then use that flag to turn
off other systems inside of ash incrementally. Or we could also introduce
something to ViewsDelegate to know whether this is running inside of mus or not
(e.g. ViewsDelegate::IsRunningAsMusClient())

https://codereview.chromium.org/1684823004/diff/20001/mash/BUILD.gn
File mash/BUILD.gn (right):

https://codereview.chromium.org/1684823004/diff/20001/mash/BUILD.gn#newcode21
mash/BUILD.gn:21: }
//mash/shell adds //ash/mus as a data-dependency when use_ash is turned on.

[sky, 2016-02-11 00:39:47]

https://codereview.chromium.org/1684823004/diff/20001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/1684823004/diff/20001/ash/shell.cc#newcode1056
ash/shell.cc:1056: 
On 2016/02/10 23:57:14, sadrul wrote:
> I wonder if ShellInitParams could include a KeyboardUIFactory. If there's no
> factory, Shell creates the default KeyboardUIImpl. //ash/mus/ can then provide
a
> factory to create KeyboardUIMus instead. With this approach, ash itself can
> continue to be unaware of mus, and there's a clear separation of mus-ash vs
> non-mus-ash.

Both of your suggestions are certainly doable. If the long term goal were to
keep the two separate than I would agree we should do as you suggest. The long
term goal is to get rid of non-mus-ash though, so it seems like we may as well
bite the bullet. I tried to keep things nice and self contained here, but I
think other patches may not be as nice and it seems inevitable that we'll want
more knowledge of the environment we're in to make decisions. For example, we
shouldn't be compiling large parts of ash.

But, I suppose I can do as you suggest and we can cross that bridge when we get
to it. WDYT?

[sadrul, 2016-02-11 14:36:07]

https://codereview.chromium.org/1684823004/diff/20001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/1684823004/diff/20001/ash/shell.cc#newcode1056
ash/shell.cc:1056: 
On 2016/02/11 00:39:47, sky wrote:
> On 2016/02/10 23:57:14, sadrul wrote:
> > I wonder if ShellInitParams could include a KeyboardUIFactory. If there's no
> > factory, Shell creates the default KeyboardUIImpl. //ash/mus/ can then
provide
> a
> > factory to create KeyboardUIMus instead. With this approach, ash itself can
> > continue to be unaware of mus, and there's a clear separation of mus-ash vs
> > non-mus-ash.
> 
> Both of your suggestions are certainly doable. If the long term goal were to
> keep the two separate than I would agree we should do as you suggest. The long
> term goal is to get rid of non-mus-ash though,

Even in the complete mustash world, it would be pretty nice to have a separate
non-mus ash component. (But yeah, it would be 'nice', not necessarily
'essential')

> so it seems like we may as well
> bite the bullet. I tried to keep things nice and self contained here, but I
> think other patches may not be as nice and it seems inevitable that we'll want
> more knowledge of the environment we're in to make decisions. For example, we
> shouldn't be compiling large parts of ash.
> 
> But, I suppose I can do as you suggest and we can cross that bridge when we
get
> to it. WDYT?

Yep, sounds good. Thanks!

[sky, 2016-02-11 16:37:15]

On 2016/02/11 14:36:07, sadrul wrote:
> https://codereview.chromium.org/1684823004/diff/20001/ash/shell.cc
> File ash/shell.cc (right):
> 
> https://codereview.chromium.org/1684823004/diff/20001/ash/shell.cc#newcode1056
> ash/shell.cc:1056: 
> On 2016/02/11 00:39:47, sky wrote:
> > On 2016/02/10 23:57:14, sadrul wrote:
> > > I wonder if ShellInitParams could include a KeyboardUIFactory. If there's
no
> > > factory, Shell creates the default KeyboardUIImpl. //ash/mus/ can then
> provide
> > a
> > > factory to create KeyboardUIMus instead. With this approach, ash itself
can
> > > continue to be unaware of mus, and there's a clear separation of mus-ash
vs
> > > non-mus-ash.
> > 
> > Both of your suggestions are certainly doable. If the long term goal were to
> > keep the two separate than I would agree we should do as you suggest. The
long
> > term goal is to get rid of non-mus-ash though,
> 
> Even in the complete mustash world, it would be pretty nice to have a separate
> non-mus ash component. (But yeah, it would be 'nice', not necessarily
> 'essential')
> 
> > so it seems like we may as well
> > bite the bullet. I tried to keep things nice and self contained here, but I
> > think other patches may not be as nice and it seems inevitable that we'll
want
> > more knowledge of the environment we're in to make decisions. For example,
we
> > shouldn't be compiling large parts of ash.
> > 
> > But, I suppose I can do as you suggest and we can cross that bridge when we
> get
> > to it. WDYT?
> 
> Yep, sounds good. Thanks!

Updated. Take another look?

[Ben Goodger (Google), 2016-02-11 17:38:47]

lgtm

[sadrul, 2016-02-11 17:42:11]

Cool! lgtm

https://codereview.chromium.org/1684823004/diff/60001/ash/mus/sysui_applicati...
File ash/mus/sysui_application.cc (right):

https://codereview.chromium.org/1684823004/diff/60001/ash/mus/sysui_applicati...
ash/mus/sysui_application.cc:164: init_params.keyboard_factory =
base::Bind(&KeyboardUIMus::Create);
Send |shell| with Bind instead of GetMojoShell()?

[sky, 2016-02-11 18:00:24]

https://codereview.chromium.org/1684823004/diff/20001/mash/BUILD.gn
File mash/BUILD.gn (right):

https://codereview.chromium.org/1684823004/diff/20001/mash/BUILD.gn#newcode21
mash/BUILD.gn:21: }
On 2016/02/10 23:57:14, sadrul wrote:
> //mash/shell adds //ash/mus as a data-dependency when use_ash is turned on.

Done.

https://codereview.chromium.org/1684823004/diff/60001/ash/mus/sysui_applicati...
File ash/mus/sysui_application.cc (right):

https://codereview.chromium.org/1684823004/diff/60001/ash/mus/sysui_applicati...
ash/mus/sysui_application.cc:164: init_params.keyboard_factory =
base::Bind(&KeyboardUIMus::Create);
On 2016/02/11 17:42:11, sadrul wrote:
> Send |shell| with Bind instead of GetMojoShell()?

Done.

[sky, 2016-02-11 18:00:31]

The CQ bit was checked by sky@chromium.org

[sky, 2016-02-11 18:00:31]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
ben@chromium.org
Link to the patchset: https://codereview.chromium.org/1684823004/#ps80001
(title: "bind shell")

[commit-bot: I haz the power, 2016-02-11 18:04:08]

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

[commit-bot: I haz the power, 2016-02-11 18:08:09]

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

[commit-bot: I haz the power, 2016-02-11 18:08:09]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sky, 2016-02-11 18:23:05]

The CQ bit was checked by sky@chromium.org

[sky, 2016-02-11 18:23:05]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
ben@chromium.org
Link to the patchset: https://codereview.chromium.org/1684823004/#ps100001
(title: "merge to trunk")

[commit-bot: I haz the power, 2016-02-11 18:23:28]

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

[commit-bot: I haz the power, 2016-02-11 19:57:56]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-16 22:36:58]

Description was changed from

==========
Refactors keyboard related code so mash can use a keyboard

This consists of the following:
. Keyboard functionality as needed by mash is now in KeyboardUI.
. KeyboardUI has a mus implementation that uses keyboard.mojom.
. mojom::Keyboard is provided by chrome, but NOTIMPLEMENTED yet.
. Added ChromeInterfaceFactory that is registered in
  ChromeBrowserMainChromeos (I'll move to an ash class later on).

BUG=585253
TEST=none
R=ben@chromium.org, sadrul@chromium.org
==========

to

==========
Refactors keyboard related code so mash can use a keyboard

This consists of the following:
. Keyboard functionality as needed by mash is now in KeyboardUI.
. KeyboardUI has a mus implementation that uses keyboard.mojom.
. mojom::Keyboard is provided by chrome, but NOTIMPLEMENTED yet.
. Added ChromeInterfaceFactory that is registered in
  ChromeBrowserMainChromeos (I'll move to an ash class later on).

BUG=585253
TEST=none
R=ben@chromium.org, sadrul@chromium.org

Committed: https://crrev.com/fee520c1c2c91a2baca7d777bddd322f3e1854fc
Cr-Commit-Position: refs/heads/master@{#374950}
==========

[commit-bot: I haz the power, 2016-02-16 22:36:59]

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