IME for Mus: Use the catalog service to decide IME driver's name.

IME for Mus: Use the catalog service to decide IME driver's name.

Previously IME driver's name was hard-coded in IMEServerImpl. This change
queries the catalog service to decide which driver to connect to.

BUG=641041
Committed: https://crrev.com/d7394b33dc3346759294cd155b9ff261e5c6e86a
Cr-Commit-Position: refs/heads/master@{#430390}

[Hadi, 2016-11-03 20:43:38]

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

[commit-bot: I haz the power, 2016-11-03 20:44:26]

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

[Hadi, 2016-11-03 20:45:07]

Description was changed from

==========
IME for Mus: Use catalog api to decide ime driver name.

BUG=641041
==========

to

==========
IME for Mus: Use the catalog service to decide IME driver's name.

Previously IME driver's name was hard-coded in IMEServerImpl. This change
queries the catalog service to decide which driver to connect to.

BUG=641041
==========

[Hadi, 2016-11-03 20:46:17]

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

[Hadi, 2016-11-03 20:46:18]

PTAL.

[sky, 2016-11-03 21:19:10]

sky@chromium.org changed reviewers:
+ ben@chromium.org

[sky, 2016-11-03 21:19:10]

This seems a bit unusual to me. +ben

[Ben Goodger (Google), 2016-11-03 21:31:22]

lgtm

seems fine given my understanding of how ime should work as a configurable app

[commit-bot: I haz the power, 2016-11-03 22:24:06]

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

[commit-bot: I haz the power, 2016-11-03 22:24:07]

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

[sky, 2016-11-03 22:35:07]

One concern I have with this is if we have a test ime and a real ime then it's
going to be random as to which ime service is picked. Do we have more than one
ime around?

[sadrul, 2016-11-05 00:57:10]

On 2016/11/03 22:35:07, sky wrote:
> One concern I have with this is if we have a test ime and a real ime then it's
> going to be random as to which ime service is picked. Do we have more than one
> ime around?

I think we will have test-ime and chrome. Although once chrome is hooked up, we
wouldn't want to include test-ime in the package, and so there should just be
one.

However, it would be entirely possible that there can be multiple apps/services
that provide the same capability. It could be IME, or the WM, or the virtual
keyboard. I suspect we would have to come up with a general scheme to handle
this sort of issues as they come up (e.g. one option could be to popup a dialog
and let the user decide, somewhat like the mime-type handlers). Hadi has filed
https://bugs.chromium.org/p/chromium/issues/detail?id=662157 to track this for
IME (there may be a similar bug filed earlier for wm, not sure).

[Hadi, 2016-11-07 15:44:14]

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

[commit-bot: I haz the power, 2016-11-07 15:44:37]

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

[sky, 2016-11-07 16:15:14]

On Fri, Nov 4, 2016 at 5:57 PM,  <sadrul@chromium.org> wrote:
> On 2016/11/03 22:35:07, sky wrote:
>> One concern I have with this is if we have a test ime and a real ime then
>> it's
>> going to be random as to which ime service is picked. Do we have more than
>> one
>> ime around?
>
> I think we will have test-ime and chrome. Although once chrome is hooked up,
> we
> wouldn't want to include test-ime in the package, and so there should just
> be
> one.

Can you clarify what you mean by that? If there is the test-ime and
chrome both with the ability to provide the ime service then how do we
know which one gets picked up? Worse yet, what if your built is stale?
For example, chrome was a built a lot time ago, but test-ime was just
built. How do we ensure the test-ime is picked up?

>
> However, it would be entirely possible that there can be multiple
> apps/services
> that provide the same capability. It could be IME, or the WM, or the virtual
> keyboard. I suspect we would have to come up with a general scheme to handle
> this sort of issues as they come up (e.g. one option could be to popup a
> dialog
> and let the user decide, somewhat like the mime-type handlers). Hadi has
> filed
> https://bugs.chromium.org/p/chromium/issues/detail?id=662157 to track this
> for
> IME (there may be a similar bug filed earlier for wm, not sure).
>
> https://codereview.chromium.org/2473483008/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Hadi, 2016-11-07 16:16:36]

On 2016/11/03 22:35:07, sky wrote:
> One concern I have with this is if we have a test ime and a real ime then it's
> going to be random as to which ime service is picked. Do we have more than one
> ime around?

Catalog service does directory scan to get the list of available services. So if
we have two compiled service providing the "ime:ime_driver" capability, it will
detect both (even if either of them is not include it in packaged_services, in
which case starting that service may fail).

I am trying to come-up with a good way to solve this, since as sadrul said this
can happen in other parts of system too. For example, an option might be add a
method to CatalogControl to override discovery results in tests. What this
method
should do, I'm not sure yet, as it should handle more general cases than IME.
I'll keep you updated.

[commit-bot: I haz the power, 2016-11-07 16:42:27]

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

[commit-bot: I haz the power, 2016-11-07 16:42:27]

Dry run: This issue passed the CQ dry run.

[sky, 2016-11-07 17:13:30]

Ben, Ken and myself discussed this a bit on Friday. I'm in favor of
baking all the manifests into the executable (resource), so that a
test can configure exactly what it wants. When we support installing
modules on the fly it seems like we'll need manifests installed in
preferences or combined on disk some where.

  -Scott

On Mon, Nov 7, 2016 at 8:16 AM,  <moshayedi@chromium.org> wrote:
> On 2016/11/03 22:35:07, sky wrote:
>> One concern I have with this is if we have a test ime and a real ime then
>> it's
>> going to be random as to which ime service is picked. Do we have more than
>> one
>> ime around?
>
> Catalog service does directory scan to get the list of available services.
> So if
> we have two compiled service providing the "ime:ime_driver" capability, it
> will
> detect both (even if either of them is not include it in packaged_services,
> in
> which case starting that service may fail).
>
> I am trying to come-up with a good way to solve this, since as sadrul said
> this
> can happen in other parts of system too. For example, an option might be add
> a
> method to CatalogControl to override discovery results in tests. What this
> method
> should do, I'm not sure yet, as it should handle more general cases than
> IME.
> I'll keep you updated.
>
>
> https://codereview.chromium.org/2473483008/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ben Goodger (Google), 2016-11-07 21:49:15]

On 2016/11/07 17:13:30, sky wrote:
> Ben, Ken and myself discussed this a bit on Friday. I'm in favor of
> baking all the manifests into the executable (resource), so that a
> test can configure exactly what it wants. When we support installing
> modules on the fly it seems like we'll need manifests installed in
> preferences or combined on disk some where.
> 
>   -Scott
> 
> On Mon, Nov 7, 2016 at 8:16 AM,  <mailto:moshayedi@chromium.org> wrote:
> > On 2016/11/03 22:35:07, sky wrote:
> >> One concern I have with this is if we have a test ime and a real ime then
> >> it's
> >> going to be random as to which ime service is picked. Do we have more than
> >> one
> >> ime around?
> >
> > Catalog service does directory scan to get the list of available services.
> > So if
> > we have two compiled service providing the "ime:ime_driver" capability, it
> > will
> > detect both (even if either of them is not include it in packaged_services,
> > in
> > which case starting that service may fail).
> >
> > I am trying to come-up with a good way to solve this, since as sadrul said
> > this
> > can happen in other parts of system too. For example, an option might be add
> > a
> > method to CatalogControl to override discovery results in tests. What this
> > method
> > should do, I'm not sure yet, as it should handle more general cases than
> > IME.
> > I'll keep you updated.
> >
> >
> > https://codereview.chromium.org/2473483008/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

I'll take an AI to resolve the scenario of weirdness on disk. For the time
being, we should not block this CL.

[Hadi, 2016-11-07 21:51:58]

The CQ bit was checked by moshayedi@chromium.org

[commit-bot: I haz the power, 2016-11-07 21:52:34]

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-11-07 21:58:23]

Description was changed from

==========
IME for Mus: Use the catalog service to decide IME driver's name.

Previously IME driver's name was hard-coded in IMEServerImpl. This change
queries the catalog service to decide which driver to connect to.

BUG=641041
==========

to

==========
IME for Mus: Use the catalog service to decide IME driver's name.

Previously IME driver's name was hard-coded in IMEServerImpl. This change
queries the catalog service to decide which driver to connect to.

BUG=641041
==========

[commit-bot: I haz the power, 2016-11-07 21:58:24]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-07 22:22:04]

Description was changed from

==========
IME for Mus: Use the catalog service to decide IME driver's name.

Previously IME driver's name was hard-coded in IMEServerImpl. This change
queries the catalog service to decide which driver to connect to.

BUG=641041
==========

to

==========
IME for Mus: Use the catalog service to decide IME driver's name.

Previously IME driver's name was hard-coded in IMEServerImpl. This change
queries the catalog service to decide which driver to connect to.

BUG=641041

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

[commit-bot: I haz the power, 2016-11-07 22:22:05]

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