[IME] migrate the xkb ID to extension based xkb ID.

[IME] migrate the xkb ID to extension based xkb ID.

BUG=345604
TEST=None

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

[Shu Chen, 2014-02-25 15:01:59]

Hi Nona/Yuki, can you please review this cl?

Thanks,
Shu

[Shu Chen, 2014-02-25 15:02:42]

Please notice this cl depends on cl https://codereview.chromium.org/177093002/.

[Yuki, 2014-02-27 02:40:32]

https://codereview.chromium.org/178343005/diff/40002/chrome/browser/chromeos/...
File chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/178343005/diff/40002/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:1223:
EXPECT_TRUE(StartsWithASCII("_comp_ime_", input_method_ids[i], true);
This code doesn't seem being able to compile.  Did you run the test?

https://codereview.chromium.org/178343005/diff/40002/chrome/browser/chromeos/...
File chrome/browser/chromeos/input_method/input_method_util.cc (right):

https://codereview.chromium.org/178343005/diff/40002/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_util.cc:618: if
(IsKeyboardLayout(input_method_ids->at(i))) {
Why at(i)?  You already checked the range.

https://codereview.chromium.org/178343005/diff/40002/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_util.cc:618: if
(IsKeyboardLayout(input_method_ids->at(i))) {
IsKeyboardLayout() returns almost always true doesn't it?  |input_method_ids[i]|
should be "xkb:..." or component extention id, and then IsKeyboardLayout()
returns true.

So, if the input is valid, MigrateXkbInputMethods always returns true regardless
of whether it actually updated the content or not.

https://codereview.chromium.org/178343005/diff/40002/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_util.cc:619:
input_method_ids->at(i).assign(
input_method_ids[i] = ...
has the same effect and would be much more common.

https://codereview.chromium.org/178343005/diff/40002/chromeos/ime/extension_i...
File chromeos/ime/extension_ime_util.cc (right):

https://codereview.chromium.org/178343005/diff/40002/chromeos/ime/extension_i...
chromeos/ime/extension_ime_util.cc:59: return kExtensionXkbIdPrefix +
keyboard_layout_id;
I think it would be safer to DCHECK(keyboard_layout_id starts with "xkb:");

https://codereview.chromium.org/178343005/diff/70001/chrome/browser/chromeos/...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/178343005/diff/70001/chrome/browser/chromeos/...
chrome/browser/chromeos/preferences.cc:590:
preload_engines_.SetValue(JoinString(split_values, ','));
I don't know well about preferences.  Are these values saved to persistent
preferences?  If so, what happens if unfortunately we had to roll back the
changes?  I'm concerned about both of forward and backward compatibility.

[Shu Chen, 2014-02-27 03:12:11]

Thanks for your review, Yuki. Please see my replies & new patch set.

https://codereview.chromium.org/178343005/diff/40002/chrome/browser/chromeos/...
File chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/178343005/diff/40002/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:1223:
EXPECT_TRUE(StartsWithASCII("_comp_ime_", input_method_ids[i], true);
On 2014/02/27 02:40:33, Yuki wrote:
> This code doesn't seem being able to compile.  Did you run the test?

Done. Sorry I didn't run the test.

https://codereview.chromium.org/178343005/diff/40002/chrome/browser/chromeos/...
File chrome/browser/chromeos/input_method/input_method_util.cc (right):

https://codereview.chromium.org/178343005/diff/40002/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_util.cc:618: if
(IsKeyboardLayout(input_method_ids->at(i))) {
On 2014/02/27 02:40:33, Yuki wrote:
> IsKeyboardLayout() returns almost always true doesn't it? 
|input_method_ids[i]|
> should be "xkb:..." or component extention id, and then IsKeyboardLayout()
> returns true.
> 
> So, if the input is valid, MigrateXkbInputMethods always returns true
regardless
> of whether it actually updated the content or not.

Please notice that Preferences::SetLanguageConfigStringListAsCSV() will pass in
any input method IDs (from user preferences) as input_method_ids.

https://codereview.chromium.org/178343005/diff/40002/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_util.cc:618: if
(IsKeyboardLayout(input_method_ids->at(i))) {
On 2014/02/27 02:40:33, Yuki wrote:
> Why at(i)?  You already checked the range.

Notice that input_method_ids is a pointer: std::vector<std::string>*, instead of
std::vector<std::string>.

But anyway, I've changed it to a reference instead of a pointer.

https://codereview.chromium.org/178343005/diff/40002/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_util.cc:619:
input_method_ids->at(i).assign(
On 2014/02/27 02:40:33, Yuki wrote:
> input_method_ids[i] = ...
> has the same effect and would be much more common.

Notice that input_method_ids is a pointer: std::vector<std::string>*, instead of
std::vector<std::string>.

But anyway, I've changed it to a reference instead of a pointer.

https://codereview.chromium.org/178343005/diff/40002/chromeos/ime/extension_i...
File chromeos/ime/extension_ime_util.cc (right):

https://codereview.chromium.org/178343005/diff/40002/chromeos/ime/extension_i...
chromeos/ime/extension_ime_util.cc:59: return kExtensionXkbIdPrefix +
keyboard_layout_id;
On 2014/02/27 02:40:33, Yuki wrote:
> I think it would be safer to DCHECK(keyboard_layout_id starts with "xkb:");

The passed in keyboard_layout_id could be a new extension-based xkb id.
It would be safer to let this function handle that.

https://codereview.chromium.org/178343005/diff/70001/chrome/browser/chromeos/...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/178343005/diff/70001/chrome/browser/chromeos/...
chrome/browser/chromeos/preferences.cc:590:
preload_engines_.SetValue(JoinString(split_values, ','));
On 2014/02/27 02:40:33, Yuki wrote:
> I don't know well about preferences.  Are these values saved to persistent
> preferences?  If so, what happens if unfortunately we had to roll back the
> changes?  I'm concerned about both of forward and backward compatibility.

When we roll back this cl, we need to additional change to migrate
"_comp_ime_..." to "xkb:...".
I don't think this cl can handle that.

[Seigo Nonaka, 2014-02-27 04:30:39]

https://codereview.chromium.org/178343005/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/178343005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:1223:
EXPECT_TRUE(StartsWithASCII("_comp_ime_", input_method_ids[i], true));
please also add checking unique.
If xkb:us::eng and _comp_ime_asdf_us_eng is co-exists, MigrateXkbInputMethods
should make unique the list.

https://codereview.chromium.org/178343005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:1224:
}
Also nice to add keeping order case.
The order of engine should be kept with migration.

https://codereview.chromium.org/178343005/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_util.cc (right):

https://codereview.chromium.org/178343005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_util.cc:619: if
(IsKeyboardLayout(ids[i]) &&
IIRC, IsKeyboardLayout returns true if and only if the id[i] is like
"xkb:*:*:*", so that checking
extension_ime_util::IsKeyboardLayoutExtension(ids[i]) is redundant.

https://codereview.chromium.org/178343005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_util.cc:624: }
please making unique just in case.

https://codereview.chromium.org/178343005/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_display_host_impl.cc (right):

https://codereview.chromium.org/178343005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_display_host_impl.cc:132: oem_layout);
Please note that, this oem_layout contains multiple layouts with comma
separated. So you need split it first, then join them into single string.

[Shu Chen, 2014-02-27 09:22:00]

Please see patch #9. some test failed, will fix them in the next patches.

Thanks,
Shu

https://codereview.chromium.org/178343005/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/178343005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:1223:
EXPECT_TRUE(StartsWithASCII("_comp_ime_", input_method_ids[i], true));
On 2014/02/27 04:30:39, Seigo Nonaka wrote:
> please also add checking unique.
> If xkb:us::eng and _comp_ime_asdf_us_eng is co-exists, MigrateXkbInputMethods
> should make unique the list.

Done.

https://codereview.chromium.org/178343005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:1224:
}
On 2014/02/27 04:30:39, Seigo Nonaka wrote:
> Also nice to add keeping order case.
> The order of engine should be kept with migration.

Done.

https://codereview.chromium.org/178343005/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_util.cc (right):

https://codereview.chromium.org/178343005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_util.cc:619: if
(IsKeyboardLayout(ids[i]) &&
On 2014/02/27 04:30:39, Seigo Nonaka wrote:
> IIRC, IsKeyboardLayout returns true if and only if the id[i] is like
> "xkb:*:*:*", so that checking
> extension_ime_util::IsKeyboardLayoutExtension(ids[i]) is redundant.

Please see the recent code change in IsKeyboardLayout(), now it returns true for
both "xkb:*:*:*" and "_comp_ime_...xkb:*:*:*".

https://codereview.chromium.org/178343005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_util.cc:624: }
On 2014/02/27 04:30:39, Seigo Nonaka wrote:
> please making unique just in case.

Done.

https://codereview.chromium.org/178343005/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_display_host_impl.cc (right):

https://codereview.chromium.org/178343005/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_display_host_impl.cc:132: oem_layout);
On 2014/02/27 04:30:39, Seigo Nonaka wrote:
> Please note that, this oem_layout contains multiple layouts with comma
> separated. So you need split it first, then join them into single string.

Done.

[Shu Chen, 2014-02-27 14:23:29]

Nona/Yuki, please take a look at the latest patch set. I've added a flag to
guard the migration.

Btw, do you know how to turn on the flag by default?

Thanks,
Shu

[Seigo Nonaka, 2014-02-28 06:14:07]

https://codereview.chromium.org/178343005/diff/210001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/178343005/diff/210001/chrome/app/generated_re...
chrome/app/generated_resources.grd:14774: +    <message
name="IDS_FLAGS_MIGRATE_XKB_NAME" desc="Migrates XKB to extension">
How about change the flag to "Use extension wrapped keyboard layouts instead of
native XKB layouts.

https://codereview.chromium.org/178343005/diff/210001/chrome/app/generated_re...
chrome/app/generated_resources.grd:14775: +      Migrates XKB
How about "Use extension wrapped keyboard layouts.

https://codereview.chromium.org/178343005/diff/210001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/178343005/diff/210001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1975:
SINGLE_VALUE_TYPE(chromeos::switches::kMigrateXkb)
To use extension wrapped keyboard layouts by default, how about change this flag
to "use-legacy-xkb" or "disable-extension-keyboards"? Then you can enable by
default except for the flag is passed.

BTW, I don't think any visible changes are expected with this migration. So that
no flags are necessary I think.
Even if you fails the migration, you have still chance to migrate from extension
IDs to the old xkb:* values.

https://codereview.chromium.org/178343005/diff/210001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_util.cc (right):

https://codereview.chromium.org/178343005/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_util.cc:641: }
How about simply
size_t t = 0;
for (size_t i = 0; i < ids.size(); ++i) {
  size_t j = 0;
  for (j = 0; j < t; ++j) {
    if (ids[i] == ids[j])
      break;
  }
  if (j == t)
    ids[t++] = ids[i]; 
}
ids.resize(t);

https://codereview.chromium.org/178343005/diff/210001/chromeos/ime/extension_...
File chromeos/ime/extension_ime_util.cc (right):

https://codereview.chromium.org/178343005/diff/210001/chromeos/ime/extension_...
chromeos/ime/extension_ime_util.cc:93: bool IsMigrateXkbEnabled() {
Sounds "UseWrappedExtensionKeyboardLayouts" clear to me.
WDYT?

[Shu Chen, 2014-02-28 07:32:49]

https://codereview.chromium.org/178343005/diff/210001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/178343005/diff/210001/chrome/app/generated_re...
chrome/app/generated_resources.grd:14774: +    <message
name="IDS_FLAGS_MIGRATE_XKB_NAME" desc="Migrates XKB to extension">
On 2014/02/28 06:14:07, Seigo Nonaka wrote:
> How about change the flag to "Use extension wrapped keyboard layouts instead
of
> native XKB layouts.

I've removed the runtime flag, and use a hard coded flag.

https://codereview.chromium.org/178343005/diff/210001/chrome/app/generated_re...
chrome/app/generated_resources.grd:14775: +      Migrates XKB
On 2014/02/28 06:14:07, Seigo Nonaka wrote:
> How about "Use extension wrapped keyboard layouts.

I've removed the runtime flag, and use a hard coded flag.

https://codereview.chromium.org/178343005/diff/210001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/178343005/diff/210001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:1975:
SINGLE_VALUE_TYPE(chromeos::switches::kMigrateXkb)
On 2014/02/28 06:14:07, Seigo Nonaka wrote:
> To use extension wrapped keyboard layouts by default, how about change this
flag
> to "use-legacy-xkb" or "disable-extension-keyboards"? Then you can enable by
> default except for the flag is passed.
> 
> BTW, I don't think any visible changes are expected with this migration. So
that
> no flags are necessary I think.
> Even if you fails the migration, you have still chance to migrate from
extension
> IDs to the old xkb:* values.

I've removed the runtime flag, and use a hard coded flag.

https://codereview.chromium.org/178343005/diff/210001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_util.cc (right):

https://codereview.chromium.org/178343005/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_util.cc:641: }
On 2014/02/28 06:14:07, Seigo Nonaka wrote:
> How about simply
> size_t t = 0;
> for (size_t i = 0; i < ids.size(); ++i) {
>   size_t j = 0;
>   for (j = 0; j < t; ++j) {
>     if (ids[i] == ids[j])
>       break;
>   }
>   if (j == t)
>     ids[t++] = ids[i]; 
> }
> ids.resize(t);

Done.

https://codereview.chromium.org/178343005/diff/210001/chromeos/ime/extension_...
File chromeos/ime/extension_ime_util.cc (right):

https://codereview.chromium.org/178343005/diff/210001/chromeos/ime/extension_...
chromeos/ime/extension_ime_util.cc:93: bool IsMigrateXkbEnabled() {
On 2014/02/28 06:14:07, Seigo Nonaka wrote:
> Sounds "UseWrappedExtensionKeyboardLayouts" clear to me.
> WDYT? 

Done.

[Shu Chen, 2014-02-28 07:46:34]

+nkostylev@

Hi Nikita, this cl needs your approval for changes in:
  chrome/browser/chromeos/login/login_display_host_impl.cc
  chrome/browser/chromeos/preferences.cc

Can you please take a look?

Thanks,
Shu

[Seigo Nonaka, 2014-02-28 07:59:59]

almost close to l-gtm.
please address my comments.

https://codereview.chromium.org/178343005/diff/230001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/178343005/diff/230001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:1305:
TEST_F(InputMethodManagerImplTest, MigrateXkbInputMethodTest) {
If you keep backward migration code in MigrateXkbInputMethodTest, I think it is
nice to add unit test for that, just in case.

Also this test will be broken if the hard coded flag is flipped for backward
migration.

I recommend you to add
void SetUseWrappedExtensionKeyboardLayoutsFlagForTesting(bool use_extension)
function to extension_ime_util.h and introduce test with setting flag
explicitly.

https://codereview.chromium.org/178343005/diff/230001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_display_host_impl.cc (right):

https://codereview.chromium.org/178343005/diff/230001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_display_host_impl.cc:135: layouts[0]);
Please use all listed layouts.
And I think putting migration code into L153 seems better.

https://codereview.chromium.org/178343005/diff/230001/chromeos/ime/component_...
File chromeos/ime/component_extension_ime_manager.cc (right):

https://codereview.chromium.org/178343005/diff/230001/chromeos/ime/component_...
chromeos/ime/component_extension_ime_manager.cc:15: const char*
kLoginLayoutWhitelist[] = {
nit: Could you add comments what this whitelist is used for.

https://codereview.chromium.org/178343005/diff/230001/chromeos/ime/component_...
chromeos/ime/component_extension_ime_manager.cc:18: "be",
making unique?

[Shu Chen, 2014-02-28 08:24:19]

https://codereview.chromium.org/178343005/diff/230001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/178343005/diff/230001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:1305:
TEST_F(InputMethodManagerImplTest, MigrateXkbInputMethodTest) {
On 2014/02/28 08:00:00, Seigo Nonaka wrote:
> If you keep backward migration code in MigrateXkbInputMethodTest, I think it
is
> nice to add unit test for that, just in case.
> 
> Also this test will be broken if the hard coded flag is flipped for backward
> migration.
> 
> I recommend you to add
> void SetUseWrappedExtensionKeyboardLayoutsFlagForTesting(bool use_extension)
> function to extension_ime_util.h and introduce test with setting flag
> explicitly.

Done.

https://codereview.chromium.org/178343005/diff/230001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_display_host_impl.cc (right):

https://codereview.chromium.org/178343005/diff/230001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_display_host_impl.cc:135: layouts[0]);
On 2014/02/28 08:00:00, Seigo Nonaka wrote:
> Please use all listed layouts.
> And I think putting migration code into L153 seems better.

Done.

https://codereview.chromium.org/178343005/diff/230001/chromeos/ime/component_...
File chromeos/ime/component_extension_ime_manager.cc (right):

https://codereview.chromium.org/178343005/diff/230001/chromeos/ime/component_...
chromeos/ime/component_extension_ime_manager.cc:15: const char*
kLoginLayoutWhitelist[] = {
On 2014/02/28 08:00:00, Seigo Nonaka wrote:
> nit: Could you add comments what this whitelist is used for.

Done.

https://codereview.chromium.org/178343005/diff/230001/chromeos/ime/component_...
chromeos/ime/component_extension_ime_manager.cc:18: "be",
On 2014/02/28 08:00:00, Seigo Nonaka wrote:
> making unique?

Done.

[Seigo Nonaka, 2014-02-28 11:21:22]

lgtm

lgtm with addressing my latest comment.

I recommend you to get review from yukishiino.

https://codereview.chromium.org/178343005/diff/230001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/178343005/diff/230001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:1305:
TEST_F(InputMethodManagerImplTest, MigrateXkbInputMethodTest) {
I meant that I would like you to add new test case about backward migration
case, that is converting from extension ime id to  premitive xkb:* ID.


On 2014/02/28 08:24:19, Shu Chen wrote:
> On 2014/02/28 08:00:00, Seigo Nonaka wrote:
> > If you keep backward migration code in MigrateXkbInputMethodTest, I think
it
> is
> > nice to add unit test for that, just in case.
> > 
> > Also this test will be broken if the hard coded flag is flipped for
backward
> > migration.
> > 
> > I recommend you to add
> > void SetUseWrappedExtensionKeyboardLayoutsFlagForTesting(bool
use_extension)
> > function to extension_ime_util.h and introduce test with setting flag
> > explicitly.
> 
> Done.

[Shu Chen, 2014-02-28 15:15:15]

https://codereview.chromium.org/178343005/diff/230001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/178343005/diff/230001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl_unittest.cc:1305:
TEST_F(InputMethodManagerImplTest, MigrateXkbInputMethodTest) {
On 2014/02/28 11:21:23, Seigo Nonaka wrote:
> I meant that I would like you to add new test case about backward migration
> case, that is converting from extension ime id to  premitive xkb:* ID.
> 
> 
> On 2014/02/28 08:24:19, Shu Chen wrote:
> > On 2014/02/28 08:00:00, Seigo Nonaka wrote:
> > > If you keep backward migration code in MigrateXkbInputMethodTest, I think
it
> > is
> > > nice to add unit test for that, just in case.
> > > 
> > > Also this test will be broken if the hard coded flag is flipped for
backward
> > > migration.
> > > 
> > > I recommend you to add
> > > void SetUseWrappedExtensionKeyboardLayoutsFlagForTesting(bool
use_extension)
> > > function to extension_ime_util.h and introduce test with setting flag
> > > explicitly.
> > 
> > Done.
> 

Done.

[Shu Chen, 2014-03-03 04:04:57]

Yuki, pls review my latest patch set. I've fixed the brower_tests failures.

Thanks,
Shu

[Nikita (slow), 2014-03-03 04:33:28]

lgtm

[Yuki, 2014-03-03 05:59:06]

Nona and I had a discussion about that the component extensions are not
available on chromeos_chrome on linux and in case of testing.  chromeos_chrome
on linux is quite important, and our conclusion is that it's better to support
(some of) component extensions on chromeos_chrome on linux.

Our idea is that:

1. Place component extensions code in the tree.  At least we'd like en-us
keyboard extension to be checked in the tree just for chromeos_chrome and
testing.  Nona may have an idea where we should put them.

2. Change the hard-coded paths in
chrome/browser/chromeos/input_method/component_extension_ime_manager_impl.cc
("/usr/share/chromeos-assets/...") to a relative paths to the chrome executable
or the resource bundle when it's chromeos_chrome on linux or unittests (i.e. not
real chromebooks), so we can find component extensions prepared at step 1.

We think, with this way, we can get rid of many issues that are caused by
absence of component extensions, and chromeos_chrome on linux will be closer to
production environments.

What do you guys think?

If this CL works well without any component extension installed, we can go
first.  Otherwise, we may need to get component extensions ready on
chromeos_chrome on linux first.

https://codereview.chromium.org/178343005/diff/300001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right):

https://codereview.chromium.org/178343005/diff/300001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:331: if
(!component_extension_ime_manager_->IsInitialized() && (
nit: I'd recommend to break a line just before the last '('.
The condition is A && (B || C), but it looks like
  A &&
  B ||
  C
and it's a little confusing.  B and C are on the same indent level as A.
I think the following style is better.
  A &&
  (B ||
   C)
If you prefer '(' at the end, then better to add an indent just before B.

https://codereview.chromium.org/178343005/diff/300001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_util.cc (right):

https://codereview.chromium.org/178343005/diff/300001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_util.cc:627: // Removes the
duplicates.
This code looks a little complicated and it's not that clear that there is no
off-by-one error, etc.  It's not clear what |t| means, too.

Can we simplify the code?  For example,
  std::vector new_ids;
  for (i : input_method_ids) {
    if (new_ids.find(*i) == new_ids.end())
      new_ids.push_back(*i);
  }
  input_method_ids.swap(new_ids);

find() hides the inner-most loop in your code, so the above code has 1 loop + 1
conditional.

https://codereview.chromium.org/178343005/diff/300001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_display_host_impl.cc (right):

https://codereview.chromium.org/178343005/diff/300001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_display_host_impl.cc:162:
chromeos::input_method::InputMethodManager* manager =
You no longer need this line.

https://codereview.chromium.org/178343005/diff/300001/chromeos/ime/component_...
File chromeos/ime/component_extension_ime_manager.cc (right):

https://codereview.chromium.org/178343005/diff/300001/chromeos/ime/component_...
chromeos/ime/component_extension_ime_manager.cc:58: bool WhitelistContains(const
char* s) {
Why don't you use std::set and its find() instead of implementing binary search
for your self?

https://codereview.chromium.org/178343005/diff/300001/chromeos/ime/component_...
chromeos/ime/component_extension_ime_manager.cc:73: bool
IsInLoginLayoutWhitelist(std::vector<std::string>& layouts) {
Use a const reference, otherwise a pointer type.

https://codereview.chromium.org/178343005/diff/300001/chromeos/ime/component_...
chromeos/ime/component_extension_ime_manager.cc:207: std::vector<std::string>&
layouts =
Can you make this a const reference?
If you cannot, the style guide recommends to use a pointer type so it's clear
that you'll change this object.

https://codereview.chromium.org/178343005/diff/300001/chromeos/ime/component_...
chromeos/ime/component_extension_ime_manager.cc:225: // fallback
InputMethodDescriptor to the result for testing purpose.
Could you add a TODO comment?
This code block should be removed once the component extensions are ready for
testing and chromeos_chrome builds.

https://codereview.chromium.org/178343005/diff/300001/chromeos/ime/extension_...
File chromeos/ime/extension_ime_util.cc (right):

https://codereview.chromium.org/178343005/diff/300001/chromeos/ime/extension_...
chromeos/ime/extension_ime_util.cc:23: bool
use_wrapped_extension_keyboard_layouts = true;
Could you rename this to g_use_wrapped_... because this is a global variable.

[Shu Chen, 2014-03-03 08:24:37]

https://codereview.chromium.org/178343005/diff/300001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right):

https://codereview.chromium.org/178343005/diff/300001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:331: if
(!component_extension_ime_manager_->IsInitialized() && (
On 2014/03/03 05:59:06, Yuki wrote:
> nit: I'd recommend to break a line just before the last '('.
> The condition is A && (B || C), but it looks like
>   A &&
>   B ||
>   C
> and it's a little confusing.  B and C are on the same indent level as A.
> I think the following style is better.
>   A &&
>   (B ||
>    C)
> If you prefer '(' at the end, then better to add an indent just before B.

Done.

https://codereview.chromium.org/178343005/diff/300001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_util.cc (right):

https://codereview.chromium.org/178343005/diff/300001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_util.cc:627: // Removes the
duplicates.
On 2014/03/03 05:59:06, Yuki wrote:
> This code looks a little complicated and it's not that clear that there is no
> off-by-one error, etc.  It's not clear what |t| means, too.
> 
> Can we simplify the code?  For example,
>   std::vector new_ids;
>   for (i : input_method_ids) {
>     if (new_ids.find(*i) == new_ids.end())
>       new_ids.push_back(*i);
>   }
>   input_method_ids.swap(new_ids);
> 
> find() hides the inner-most loop in your code, so the above code has 1 loop +
1
> conditional.

The code does inline dup removing, without introducing a new vector.
To make the code more readable, I've changed it as you preferred.

https://codereview.chromium.org/178343005/diff/300001/chromeos/ime/component_...
File chromeos/ime/component_extension_ime_manager.cc (right):

https://codereview.chromium.org/178343005/diff/300001/chromeos/ime/component_...
chromeos/ime/component_extension_ime_manager.cc:58: bool WhitelistContains(const
char* s) {
On 2014/03/03 05:59:06, Yuki wrote:
> Why don't you use std::set and its find() instead of implementing binary
search
> for your self?

At the time of writing this code, I didn't find a good place to write code for
initializing the std::set.

But anyway, let's do lazy init.

https://codereview.chromium.org/178343005/diff/300001/chromeos/ime/component_...
chromeos/ime/component_extension_ime_manager.cc:73: bool
IsInLoginLayoutWhitelist(std::vector<std::string>& layouts) {
On 2014/03/03 05:59:06, Yuki wrote:
> Use a const reference, otherwise a pointer type.

Done.

https://codereview.chromium.org/178343005/diff/300001/chromeos/ime/component_...
chromeos/ime/component_extension_ime_manager.cc:207: std::vector<std::string>&
layouts =
On 2014/03/03 05:59:06, Yuki wrote:
> Can you make this a const reference?
> If you cannot, the style guide recommends to use a pointer type so it's clear
> that you'll change this object.

Done.

https://codereview.chromium.org/178343005/diff/300001/chromeos/ime/extension_...
File chromeos/ime/extension_ime_util.cc (right):

https://codereview.chromium.org/178343005/diff/300001/chromeos/ime/extension_...
chromeos/ime/extension_ime_util.cc:23: bool
use_wrapped_extension_keyboard_layouts = true;
On 2014/03/03 05:59:06, Yuki wrote:
> Could you rename this to g_use_wrapped_... because this is a global variable.

Done.

[Yuki, 2014-03-03 09:10:07]

https://codereview.chromium.org/178343005/diff/320001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_util.cc (right):

https://codereview.chromium.org/178343005/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_util.cc:290: if (pos >= 0)
You must compare |pos| with std::string::npos.
See http://en.cppreference.com/w/cpp/string/basic_string/find

https://codereview.chromium.org/178343005/diff/320001/chromeos/ime/component_...
File chromeos/ime/component_extension_ime_manager.cc (right):

https://codereview.chromium.org/178343005/diff/320001/chromeos/ime/component_...
chromeos/ime/component_extension_ime_manager.cc:60: std::set<std::string>
g_login_layout_map;
Global instances of a non-trivial type are forbidden in chromium code.  Can you
make this a member of ComponentExtensionIMEManager or something?

By the way, the variable name is map while the type is set.

https://codereview.chromium.org/178343005/diff/320001/chromeos/ime/component_...
chromeos/ime/component_extension_ime_manager.cc:68: return
g_login_layout_map.count(s) > 0;
You don't need to count(), why don't you find()?

[Shu Chen, 2014-03-03 09:27:03]

https://codereview.chromium.org/178343005/diff/320001/chrome/browser/chromeos...
File chrome/browser/chromeos/input_method/input_method_util.cc (right):

https://codereview.chromium.org/178343005/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/input_method/input_method_util.cc:290: if (pos >= 0)
On 2014/03/03 09:10:08, Yuki wrote:
> You must compare |pos| with std::string::npos.
> See http://en.cppreference.com/w/cpp/string/basic_string/find

Done.

https://codereview.chromium.org/178343005/diff/320001/chromeos/ime/component_...
File chromeos/ime/component_extension_ime_manager.cc (right):

https://codereview.chromium.org/178343005/diff/320001/chromeos/ime/component_...
chromeos/ime/component_extension_ime_manager.cc:60: std::set<std::string>
g_login_layout_map;
On 2014/03/03 09:10:08, Yuki wrote:
> Global instances of a non-trivial type are forbidden in chromium code.  Can
you
> make this a member of ComponentExtensionIMEManager or something?
> 
> By the way, the variable name is map while the type is set.

Done.

https://codereview.chromium.org/178343005/diff/320001/chromeos/ime/component_...
chromeos/ime/component_extension_ime_manager.cc:68: return
g_login_layout_map.count(s) > 0;
On 2014/03/03 09:10:08, Yuki wrote:
> You don't need to count(), why don't you find()?

Done.

[Yuki, 2014-03-04 04:04:44]

Nits only.

https://codereview.chromium.org/178343005/diff/380001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_display_host_impl.cc (right):

https://codereview.chromium.org/178343005/diff/380001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_display_host_impl.cc:162:
chromeos::input_method::InputMethodManager* manager =
Remove this line, otherwise use a different name other than |manager|.

https://codereview.chromium.org/178343005/diff/380001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc (right):

https://codereview.chromium.org/178343005/diff/380001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:429:
input_methods->at(i).id());
Please do not use std::vector::at() even though the original code used it.

https://codereview.chromium.org/178343005/diff/380001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:437:
CreateInputMethodsEntry(input_methods->at(i), current_input_method_id));
Please change this line, too.  Using std::vector::at() doesn't comply with our
policy that we don't use exceptions or exception handling in our codebase.

https://codereview.chromium.org/178343005/diff/380001/chromeos/ime/extension_...
File chromeos/ime/extension_ime_util.h (right):

https://codereview.chromium.org/178343005/diff/380001/chromeos/ime/extension_...
chromeos/ime/extension_ime_util.h:36: const std::string CHROMEOS_EXPORT
GetInputMethodIDByKeyboardLayout(
const in |const std::string| is meaningless.  If you have to return an object
(not a reference), then const doesn't make much sense.

https://codereview.chromium.org/178343005/diff/380001/chromeos/ime/extension_...
chromeos/ime/extension_ime_util.h:67: bool CHROMEOS_EXPORT
SetUseWrappedExtensionKeyboardLayoutsFlagForTesting(
Please consider to make this a class named ScopedFoo.
See
https://code.google.com/p/chromium/codesearch#search/&q=%5CbScoped%5B%5E%5Cs%...
for example.

base::AutoReset in base/auto_reset.h may be useful.

I'm okay to define a new class here.  I don't think it's worth making a new file
just to define the helper ScopedXxx class.

https://codereview.chromium.org/178343005/diff/380001/chromeos/ime/extension_...
chromeos/ime/extension_ime_util.h:73: const std::string CHROMEOS_EXPORT
MaybeGetLegacyXkbId(
Ditto for const in |const std::string|.

[Shu Chen, 2014-03-04 04:45:02]

https://codereview.chromium.org/178343005/diff/380001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_display_host_impl.cc (right):

https://codereview.chromium.org/178343005/diff/380001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_display_host_impl.cc:162:
chromeos::input_method::InputMethodManager* manager =
On 2014/03/04 04:04:45, Yuki wrote:
> Remove this line, otherwise use a different name other than |manager|.

Done.

https://codereview.chromium.org/178343005/diff/380001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc (right):

https://codereview.chromium.org/178343005/diff/380001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:429:
input_methods->at(i).id());
On 2014/03/04 04:04:45, Yuki wrote:
> Please do not use std::vector::at() even though the original code used it.

Done.

https://codereview.chromium.org/178343005/diff/380001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:437:
CreateInputMethodsEntry(input_methods->at(i), current_input_method_id));
On 2014/03/04 04:04:45, Yuki wrote:
> Please change this line, too.  Using std::vector::at() doesn't comply with our
> policy that we don't use exceptions or exception handling in our codebase.

Done.

https://codereview.chromium.org/178343005/diff/380001/chromeos/ime/extension_...
File chromeos/ime/extension_ime_util.h (right):

https://codereview.chromium.org/178343005/diff/380001/chromeos/ime/extension_...
chromeos/ime/extension_ime_util.h:36: const std::string CHROMEOS_EXPORT
GetInputMethodIDByKeyboardLayout(
On 2014/03/04 04:04:45, Yuki wrote:
> const in |const std::string| is meaningless.  If you have to return an object
> (not a reference), then const doesn't make much sense.

Done.

https://codereview.chromium.org/178343005/diff/380001/chromeos/ime/extension_...
chromeos/ime/extension_ime_util.h:67: bool CHROMEOS_EXPORT
SetUseWrappedExtensionKeyboardLayoutsFlagForTesting(
On 2014/03/04 04:04:45, Yuki wrote:
> Please consider to make this a class named ScopedFoo.
> See
>
https://code.google.com/p/chromium/codesearch#search/&q=%255CbScoped%255B%255...
> for example.
> 
> base::AutoReset in base/auto_reset.h may be useful.
> 
> I'm okay to define a new class here.  I don't think it's worth making a new
file
> just to define the helper ScopedXxx class.

Done.

https://codereview.chromium.org/178343005/diff/380001/chromeos/ime/extension_...
chromeos/ime/extension_ime_util.h:73: const std::string CHROMEOS_EXPORT
MaybeGetLegacyXkbId(
On 2014/03/04 04:04:45, Yuki wrote:
> Ditto for const in |const std::string|.

Done.

[Yuki, 2014-03-04 05:57:27]

lgtm with trivial nits.

https://codereview.chromium.org/178343005/diff/400001/chromeos/ime/extension_...
File chromeos/ime/extension_ime_util.cc (right):

https://codereview.chromium.org/178343005/diff/400001/chromeos/ime/extension_...
chromeos/ime/extension_ime_util.cc:108:
ScopedUseExtensionKeyboardFlagForTesting(bool newFlag)
nit: s/newFlag/new_flag/

https://codereview.chromium.org/178343005/diff/400001/chromeos/ime/extension_...
File chromeos/ime/extension_ime_util.h (right):

https://codereview.chromium.org/178343005/diff/400001/chromeos/ime/extension_...
chromeos/ime/extension_ime_util.h:75: explicit
ScopedUseExtensionKeyboardFlagForTesting(bool newFlag);
nit: s/newFlag/new_flag/

https://codereview.chromium.org/178343005/diff/400001/chromeos/ime/extension_...
chromeos/ime/extension_ime_util.h:76: private:
nit: better to put an empty line just before private:.

[Shu Chen, 2014-03-04 06:43:37]

https://codereview.chromium.org/178343005/diff/400001/chromeos/ime/extension_...
File chromeos/ime/extension_ime_util.cc (right):

https://codereview.chromium.org/178343005/diff/400001/chromeos/ime/extension_...
chromeos/ime/extension_ime_util.cc:108:
ScopedUseExtensionKeyboardFlagForTesting(bool newFlag)
On 2014/03/04 05:57:28, Yuki wrote:
> nit: s/newFlag/new_flag/

Done.

https://codereview.chromium.org/178343005/diff/400001/chromeos/ime/extension_...
File chromeos/ime/extension_ime_util.h (right):

https://codereview.chromium.org/178343005/diff/400001/chromeos/ime/extension_...
chromeos/ime/extension_ime_util.h:75: explicit
ScopedUseExtensionKeyboardFlagForTesting(bool newFlag);
On 2014/03/04 05:57:28, Yuki wrote:
> nit: s/newFlag/new_flag/

Done.

https://codereview.chromium.org/178343005/diff/400001/chromeos/ime/extension_...
chromeos/ime/extension_ime_util.h:76: private:
On 2014/03/04 05:57:28, Yuki wrote:
> nit: better to put an empty line just before private:.

Done.

[Shu Chen, 2014-03-04 06:44:21]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2014-03-04 06:45:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shuchen@chromium.org/178343005/420001

[Shu Chen, 2014-03-04 07:28:05]

The CQ bit was unchecked by shuchen@chromium.org

[Shu Chen, 2014-03-04 09:06:59]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2014-03-04 09:07:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shuchen@chromium.org/178343005/450001

[commit-bot: I haz the power, 2014-03-04 17:03:45]

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

[commit-bot: I haz the power, 2014-03-04 17:03:46]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[Shu Chen, 2014-03-04 23:09:38]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2014-03-04 23:10:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shuchen@chromium.org/178343005/450001

[commit-bot: I haz the power, 2014-03-05 01:12:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shuchen@chromium.org/178343005/450001

[commit-bot: I haz the power, 2014-03-05 06:02:02]

Change committed as 254948