mash: Fix launching touch hud app in mash mode.

mash: Fix launching touch hud app in mash mode.

Also switching back to listen for move events in touch hud app.

BUG=643396
TEST=manual

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

[riajiang, 2016-09-01 21:28:58]

Description was changed from

==========
mash: Fix launching touch hud app in mash mode.

BUG=none
TEST=manual
==========

to

==========
mash: Fix launching touch hud app in mash mode.

Also switching back to listen for move events in touch hud app.

BUG=none
TEST=manual
==========

[riajiang, 2016-09-01 21:38:35]

riajiang@chromium.org changed reviewers:
+ sadrul@chromium.org, sky@chromium.org

[riajiang, 2016-09-01 21:38:35]

PTAL. Thanks!

[riajiang, 2016-09-01 21:44:53]

Description was changed from

==========
mash: Fix launching touch hud app in mash mode.

Also switching back to listen for move events in touch hud app.

BUG=none
TEST=manual
==========

to

==========
mash: Fix launching touch hud app in mash mode.

Also switching back to listen for move events in touch hud app.

BUG=643396
TEST=manual
==========

[sky, 2016-09-01 22:09:36]

https://codereview.chromium.org/2296763009/diff/1/ash/mus/accelerators/accele...
File ash/mus/accelerators/accelerator_controller_delegate_mus.h (right):

https://codereview.chromium.org/2296763009/diff/1/ash/mus/accelerators/accele...
ash/mus/accelerators/accelerator_controller_delegate_mus.h:19:
AcceleratorControllerDelegateMus(shell::Connector* connector);
explicit

https://codereview.chromium.org/2296763009/diff/1/mash/BUILD.gn
File mash/BUILD.gn (left):

https://codereview.chromium.org/2296763009/diff/1/mash/BUILD.gn#oldcode14
mash/BUILD.gn:14: "//ash/touch_hud/mus:touch_hud",
Don't we still want this built? Without this if you build mash and run via
mojo_runner won't touch_hud not be built?

[sadrul, 2016-09-02 00:52:28]

https://codereview.chromium.org/2296763009/diff/1/ash/mus/accelerators/accele...
File ash/mus/accelerators/accelerator_controller_delegate_mus.cc (right):

https://codereview.chromium.org/2296763009/diff/1/ash/mus/accelerators/accele...
ash/mus/accelerators/accelerator_controller_delegate_mus.cc:63:
connector_->ConnectToInterface("mojo:touch_hud", &launchable);
You should add touch_hud as a data_dep for ash/mus

https://codereview.chromium.org/2296763009/diff/1/ash/touch_hud/mus/touch_hud...
File ash/touch_hud/mus/touch_hud_application.h (right):

https://codereview.chromium.org/2296763009/diff/1/ash/touch_hud/mus/touch_hud...
ash/touch_hud/mus/touch_hud_application.h:45:
mojo::BindingSet<mash::mojom::Launchable> bindings_;
Why would we need a BindingSet<>?

[riajiang, 2016-09-02 18:18:38]

PTAL.Thanks!

Filed an issue (https://bugs.chromium.org/p/chromium/issues/detail?id=643747) to
debug why it was trying to create a new service separately.

https://codereview.chromium.org/2296763009/diff/1/ash/mus/accelerators/accele...
File ash/mus/accelerators/accelerator_controller_delegate_mus.cc (right):

https://codereview.chromium.org/2296763009/diff/1/ash/mus/accelerators/accele...
ash/mus/accelerators/accelerator_controller_delegate_mus.cc:63:
connector_->ConnectToInterface("mojo:touch_hud", &launchable);
On 2016/09/02 00:52:28, sadrul wrote:
> You should add touch_hud as a data_dep for ash/mus

Done.

https://codereview.chromium.org/2296763009/diff/1/ash/mus/accelerators/accele...
File ash/mus/accelerators/accelerator_controller_delegate_mus.h (right):

https://codereview.chromium.org/2296763009/diff/1/ash/mus/accelerators/accele...
ash/mus/accelerators/accelerator_controller_delegate_mus.h:19:
AcceleratorControllerDelegateMus(shell::Connector* connector);
On 2016/09/01 22:09:36, sky wrote:
> explicit

Done.

https://codereview.chromium.org/2296763009/diff/1/ash/touch_hud/mus/touch_hud...
File ash/touch_hud/mus/touch_hud_application.h (right):

https://codereview.chromium.org/2296763009/diff/1/ash/touch_hud/mus/touch_hud...
ash/touch_hud/mus/touch_hud_application.h:45:
mojo::BindingSet<mash::mojom::Launchable> bindings_;
On 2016/09/02 00:52:28, sadrul wrote:
> Why would we need a BindingSet<>?

As discussed, changing back to Binding.

https://codereview.chromium.org/2296763009/diff/1/mash/BUILD.gn
File mash/BUILD.gn (left):

https://codereview.chromium.org/2296763009/diff/1/mash/BUILD.gn#oldcode14
mash/BUILD.gn:14: "//ash/touch_hud/mus:touch_hud",
On 2016/09/01 22:09:36, sky wrote:
> Don't we still want this built? Without this if you build mash and run via
> mojo_runner won't touch_hud not be built?

Added touch_hud as a data_dep for ash/mus.

[sadrul, 2016-09-02 18:34:38]

lgtm

https://codereview.chromium.org/2296763009/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/2296763009/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_command_controller.cc:638:
mash::mojom::LaunchMode::DEFAULT);
Does chrome's manifest not need to list touch-hud?

[riajiang, 2016-09-02 18:38:58]

https://codereview.chromium.org/2296763009/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/2296763009/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_command_controller.cc:638:
mash::mojom::LaunchMode::DEFAULT);
On 2016/09/02 18:34:38, sadrul wrote:
> Does chrome's manifest not need to list touch-hud?

I added it here:
https://codereview.chromium.org/2296763009/diff/20001/chrome/app/mojo/chrome_....
Is this the one you are thinking of?

[sadrul, 2016-09-02 18:40:18]

On 2016/09/02 18:38:58, riajiang wrote:
>
https://codereview.chromium.org/2296763009/diff/20001/chrome/browser/ui/brows...
> File chrome/browser/ui/browser_command_controller.cc (right):
> 
>
https://codereview.chromium.org/2296763009/diff/20001/chrome/browser/ui/brows...
> chrome/browser/ui/browser_command_controller.cc:638:
> mash::mojom::LaunchMode::DEFAULT);
> On 2016/09/02 18:34:38, sadrul wrote:
> > Does chrome's manifest not need to list touch-hud?
> 
> I added it here:
>
https://codereview.chromium.org/2296763009/diff/20001/chrome/app/mojo/chrome_....
> Is this the one you are thinking of?

Yes, sorry, missed that.

[sky, 2016-09-02 18:42:21]

https://codereview.chromium.org/2296763009/diff/20001/ash/mus/BUILD.gn
File ash/mus/BUILD.gn (right):

https://codereview.chromium.org/2296763009/diff/20001/ash/mus/BUILD.gn#newcod...
ash/mus/BUILD.gn:141: "//ash/touch_hud/mus:touch_hud",
Shouldn't it be "lib" that his this data_dep?

[riajiang, 2016-09-02 19:05:02]

https://codereview.chromium.org/2296763009/diff/20001/ash/mus/BUILD.gn
File ash/mus/BUILD.gn (right):

https://codereview.chromium.org/2296763009/diff/20001/ash/mus/BUILD.gn#newcod...
ash/mus/BUILD.gn:141: "//ash/touch_hud/mus:touch_hud",
On 2016/09/02 18:42:21, sky wrote:
> Shouldn't it be "lib" that his this data_dep?

ash/mus is only going to connect to the touch_hud service and not calling any of
its functions?

[sky, 2016-09-02 19:16:14]

https://codereview.chromium.org/2296763009/diff/20001/ash/mus/BUILD.gn
File ash/mus/BUILD.gn (right):

https://codereview.chromium.org/2296763009/diff/20001/ash/mus/BUILD.gn#newcod...
ash/mus/BUILD.gn:141: "//ash/touch_hud/mus:touch_hud",
On 2016/09/02 19:05:02, riajiang wrote:
> On 2016/09/02 18:42:21, sky wrote:
> > Shouldn't it be "lib" that his this data_dep?
> 
> ash/mus is only going to connect to the touch_hud service and not calling any
of
> its functions?

It's the code in accelerator_conroller_delegat_mus doing the connect though,
right? Doesn't that mean anyone linking about "lib" is also calling to touch_hud
and so should get the data_dep?

[riajiang, 2016-09-02 19:31:06]

https://codereview.chromium.org/2296763009/diff/20001/ash/mus/BUILD.gn
File ash/mus/BUILD.gn (right):

https://codereview.chromium.org/2296763009/diff/20001/ash/mus/BUILD.gn#newcod...
ash/mus/BUILD.gn:141: "//ash/touch_hud/mus:touch_hud",
On 2016/09/02 19:16:14, sky wrote:
> On 2016/09/02 19:05:02, riajiang wrote:
> > On 2016/09/02 18:42:21, sky wrote:
> > > Shouldn't it be "lib" that his this data_dep?
> > 
> > ash/mus is only going to connect to the touch_hud service and not calling
any
> of
> > its functions?
> 
> It's the code in accelerator_conroller_delegat_mus doing the connect though,
> right? Doesn't that mean anyone linking about "lib" is also calling to
touch_hud
> and so should get the data_dep?

Yes sorry! I misunderstood your question. Moved it to be under "lib".

[sky, 2016-09-02 19:32:29]

LGTM

[riajiang, 2016-09-02 19:36:29]

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

[commit-bot: I haz the power, 2016-09-02 19:37:00]

Dry run: 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-09-02 19:50:46]

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

[commit-bot: I haz the power, 2016-09-02 19:50:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[riajiang, 2016-09-02 22:20:31]

The CQ bit was checked by riajiang@chromium.org

[riajiang, 2016-09-02 22:20:31]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2296763009/#ps60001
(title: "deps and define")

[commit-bot: I haz the power, 2016-09-02 22:21:12]

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-09-02 23:47:27]

Description was changed from

==========
mash: Fix launching touch hud app in mash mode.

Also switching back to listen for move events in touch hud app.

BUG=643396
TEST=manual
==========

to

==========
mash: Fix launching touch hud app in mash mode.

Also switching back to listen for move events in touch hud app.

BUG=643396
TEST=manual
==========

[commit-bot: I haz the power, 2016-09-02 23:47:28]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-02 23:50:59]

Description was changed from

==========
mash: Fix launching touch hud app in mash mode.

Also switching back to listen for move events in touch hud app.

BUG=643396
TEST=manual
==========

to

==========
mash: Fix launching touch hud app in mash mode.

Also switching back to listen for move events in touch hud app.

BUG=643396
TEST=manual

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

[commit-bot: I haz the power, 2016-09-02 23:51:01]

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