Adds experimental flag --enable-experimental-ime-features.

Adds 2 flags for input method features.

1) --enable-new-korean-ime, which is for new Korean IME based on Google Input Tools.
2) --enable-physical-keyboard-autocorrect, which is for suggestions on typing with physical keyboard.

For enable-physical-keyboard-autocorrect, the extension JS will use inputMethodPrivate.getInputMethodConfig API to read the flag value and turn on the feature accordingly.

BUG=422869
TEST=Verified on linux_chromeos.

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

[Shu Chen, 2014-10-23 05:38:23]

shuchen@chromium.org changed reviewers:
+ isherman@chromium.org, kalman@chromium.org, yukishiino@chromium.org

[Shu Chen, 2014-10-23 05:38:23]

Guys, can you please review this cl? Thanks

yukishiino@: overall IMF changes.

kalman@: chrome/common/extensions/api/input_method_private.json

isherman@: extensions/browser/extension_function_histogram_value.h
                   tools/metrics/histograms/histograms.xml

Thanks!

[Ilya Sherman, 2014-10-23 05:47:31]

https://codereview.chromium.org/676593004/diff/1/extensions/browser/extension...
File extensions/browser/extension_function_histogram_value.h (right):

https://codereview.chromium.org/676593004/diff/1/extensions/browser/extension...
extensions/browser/extension_function_histogram_value.h:971: //
tools/metrics/histograms/histograms.xml.
Please run the script to update histograms.xml.

[Shu Chen, 2014-10-23 06:21:37]

https://codereview.chromium.org/676593004/diff/1/extensions/browser/extension...
File extensions/browser/extension_function_histogram_value.h (right):

https://codereview.chromium.org/676593004/diff/1/extensions/browser/extension...
extensions/browser/extension_function_histogram_value.h:971: //
tools/metrics/histograms/histograms.xml.
On 2014/10/23 05:47:30, Ilya Sherman wrote:
> Please run the script to update histograms.xml.

Done.

[Ilya Sherman, 2014-10-23 06:24:09]

histograms lgtm

[Yuki, 2014-10-23 07:05:43]

1. I don't see any changes about autocorrection for physical keyboards.  Where
are you checking the flag for the autocorrection?

2. I'd recommend to have two separate flags for two totally different features. 
It shouldn't be hard to have two flags.

https://codereview.chromium.org/676593004/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/676593004/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:13860: +        Enable experimental features
for IMEs.
It's better to be more specific and descriptive.  People wouldn't understand
what the experimental features are.

https://codereview.chromium.org/676593004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/input_method_api.cc (right):

https://codereview.chromium.org/676593004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/input_method_api.cc:34: LOG(ERROR) <<
"-----run to here." << isExperimental;
Do not use LOG(ERROR) for non error case.
Probably you forgot to remove test code?

https://codereview.chromium.org/676593004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/input_method_api.h (right):

https://codereview.chromium.org/676593004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/input_method_api.h:21: class
GetConfigFunction : public UIThreadExtensionFunction {
I'm worrying about the class name.  It's extensions::GetConfigFunction.  It
seems too general.  You'd better to give a more specific name.

https://codereview.chromium.org/676593004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/input_method_api.h:33: };
Better to have DISALLOW_COPY_AND_ASSIGN just in case, if you don't have a plan
to copy.

[Shu Chen, 2014-10-23 07:16:01]

> 1. I don't see any changes about autocorrection for physical keyboards.  Where
> are you checking the flag for the autocorrection?
As mentioned in CL description, the flag is exposed to a private API. IME
extension JS will use it to enable/disable certain features.

> 2. I'd recommend to have two separate flags for two totally different
features. 
> It shouldn't be hard to have two flags.
There will be more experimental features coming. I feel we'd better use one
single flag as it's only used for dogfood or something.

[Shu Chen, 2014-10-23 07:19:05]

https://codereview.chromium.org/676593004/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/676593004/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:13860: +        Enable experimental features
for IMEs.
On 2014/10/23 07:05:43, Yuki wrote:
> It's better to be more specific and descriptive.  People wouldn't understand
> what the experimental features are.

Just like IDS_FLAGS_ENABLE_EXPERIMENTAL_INPUT_VIEW_FEATURES_..., it's hard to
list all possible experimental features because they could be easily changed.

I think we don't want to update this description over and over again to reflect
the real experimental features.

https://codereview.chromium.org/676593004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/input_method_api.cc (right):

https://codereview.chromium.org/676593004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/input_method_api.cc:34: LOG(ERROR) <<
"-----run to here." << isExperimental;
On 2014/10/23 07:05:43, Yuki wrote:
> Do not use LOG(ERROR) for non error case.
> Probably you forgot to remove test code?

Done.

https://codereview.chromium.org/676593004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/input_method_api.h (right):

https://codereview.chromium.org/676593004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/input_method_api.h:21: class
GetConfigFunction : public UIThreadExtensionFunction {
On 2014/10/23 07:05:43, Yuki wrote:
> I'm worrying about the class name.  It's extensions::GetConfigFunction.  It
> seems too general.  You'd better to give a more specific name.

Done.

https://codereview.chromium.org/676593004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/input_method_api.h:33: };
On 2014/10/23 07:05:43, Yuki wrote:
> Better to have DISALLOW_COPY_AND_ASSIGN just in case, if you don't have a plan
> to copy.

Done.

[Yuki, 2014-10-23 07:30:13]

On 2014/10/23 07:16:01, Shu Chen wrote:
> > 1. I don't see any changes about autocorrection for physical keyboards. 
Where
> > are you checking the flag for the autocorrection?
> As mentioned in CL description, the flag is exposed to a private API. IME
> extension JS will use it to enable/disable certain features.

It's not mentioned that the flag *will* be used in coming changes.
Please update the description.

It's also a new fact for me that IME extension JS can change the flag.  AFAIK,
regular flags are not changed by the program itself without the user operation.
Could you note this fact, too?


> > 2. I'd recommend to have two separate flags for two totally different
> features. 
> > It shouldn't be hard to have two flags.
> There will be more experimental features coming. I feel we'd better use one
> single flag as it's only used for dogfood or something.

If the flag is meant only for internal dogfooders and developers, please note it
in both of CL description and the code.

[Shu Chen, 2014-10-23 07:41:11]

On 2014/10/23 07:30:13, Yuki wrote:
> On 2014/10/23 07:16:01, Shu Chen wrote:
> > > 1. I don't see any changes about autocorrection for physical keyboards. 
> Where
> > > are you checking the flag for the autocorrection?
> > As mentioned in CL description, the flag is exposed to a private API. IME
> > extension JS will use it to enable/disable certain features.
> 
> It's not mentioned that the flag *will* be used in coming changes.
> Please update the description.
Done.
> 
> It's also a new fact for me that IME extension JS can change the flag.  AFAIK,
> regular flags are not changed by the program itself without the user
operation.
> Could you note this fact, too?
Sorry for being not clear. I meant JS will just read the flag and enable/disable
its internal features (e.g. Whether listen on physical key events for
suggestions).
> 
> 
> > > 2. I'd recommend to have two separate flags for two totally different
> > features. 
> > > It shouldn't be hard to have two flags.
> > There will be more experimental features coming. I feel we'd better use one
> > single flag as it's only used for dogfood or something.
> 
> If the flag is meant only for internal dogfooders and developers, please note
it
> in both of CL description and the code.
Done.

[Yuki, 2014-10-23 07:42:02]

https://codereview.chromium.org/676593004/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/676593004/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:13860: +        Enable experimental features
for IMEs.
On 2014/10/23 07:19:05, Shu Chen wrote:
> On 2014/10/23 07:05:43, Yuki wrote:
> > It's better to be more specific and descriptive.  People wouldn't understand
> > what the experimental features are.
> 
> Just like IDS_FLAGS_ENABLE_EXPERIMENTAL_INPUT_VIEW_FEATURES_..., it's hard to
> list all possible experimental features because they could be easily changed.
> 
> I think we don't want to update this description over and over again to
reflect
> the real experimental features.

I don't think it makes a good excuse.  It *could* sound like that it's more
important for us to be lazy than to care about users.  This is a user visible
description, and users need clear explanations.

I'd still highly recommend to have each separate flag for each feature and make
a clear description for each.  It's simplest, maintainable, easier to read and
good engineering, I believe.  However, if you're going to go with a single giant
flag, please note that the flag is meant only for internal developers and not
designed for external (enthusiastic) users.

[Shu Chen, 2014-10-23 07:53:55]

On 2014/10/23 07:42:02, Yuki wrote:
>
https://codereview.chromium.org/676593004/diff/20001/chrome/app/generated_res...
> File chrome/app/generated_resources.grd (right):
> 
>
https://codereview.chromium.org/676593004/diff/20001/chrome/app/generated_res...
> chrome/app/generated_resources.grd:13860: +        Enable experimental
features
> for IMEs.
> On 2014/10/23 07:19:05, Shu Chen wrote:
> > On 2014/10/23 07:05:43, Yuki wrote:
> > > It's better to be more specific and descriptive.  People wouldn't
understand
> > > what the experimental features are.
> > 
> > Just like IDS_FLAGS_ENABLE_EXPERIMENTAL_INPUT_VIEW_FEATURES_..., it's hard
to
> > list all possible experimental features because they could be easily
changed.
> > 
> > I think we don't want to update this description over and over again to
> reflect
> > the real experimental features.
> 
> I don't think it makes a good excuse.  It *could* sound like that it's more
> important for us to be lazy than to care about users.  This is a user visible
> description, and users need clear explanations.
> 
> I'd still highly recommend to have each separate flag for each feature and
make
> a clear description for each.  It's simplest, maintainable, easier to read and
> good engineering, I believe.  However, if you're going to go with a single
giant
> flag, please note that the flag is meant only for internal developers and not
> designed for external (enthusiastic) users.

I've mentioned "only for internal dogfooders and developers" in
chrome/app/generated_resources.grd and chromeos/chromeos_switches.cc?
Does it sound good to you?

[Yuki, 2014-10-23 08:15:28]

https://codereview.chromium.org/676593004/diff/80001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/676593004/diff/80001/chrome/app/generated_res...
chrome/app/generated_resources.grd:13860: +        Enable experimental features
for IMEs.
Please write the same thing here, too.  Users cannot read the comment.
I'd recommend this comment so that users understand that they're not expected to
use this flag.

[Shu Chen, 2014-10-23 08:54:39]

On 2014/10/23 08:15:28, Yuki wrote:
>
https://codereview.chromium.org/676593004/diff/80001/chrome/app/generated_res...
> File chrome/app/generated_resources.grd (right):
> 
>
https://codereview.chromium.org/676593004/diff/80001/chrome/app/generated_res...
> chrome/app/generated_resources.grd:13860: +        Enable experimental
features
> for IMEs.
> Please write the same thing here, too.  Users cannot read the comment.
> I'd recommend this comment so that users understand that they're not expected
to
> use this flag.

I've split the flag into 2 flags. PTAL?

[Yuki, 2014-10-23 09:05:49]

lgtm.  Looks great to me :)

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

https://codereview.chromium.org/676593004/diff/120001/chrome/app/generated_re...
chrome/app/generated_resources.grd:13860: +        Enable new Korean IME, which
is based on Google Input Tools' HMM engine.
I'm not quite sure if we can write a single quote as is in this file.
Is it really okay?  Or do we need an escape sequence, say "?

[not at google - send to devlin, 2014-10-23 13:59:24]

lgtm

[Shu Chen, 2014-10-23 14:12:53]

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

https://codereview.chromium.org/676593004/diff/120001/chrome/app/generated_re...
chrome/app/generated_resources.grd:13860: +        Enable new Korean IME, which
is based on Google Input Tools' HMM engine.
On 2014/10/23 09:05:49, Yuki wrote:
> I'm not quite sure if we can write a single quote as is in this file.
> Is it really okay?  Or do we need an escape sequence, say "?

I confirm it is ok.

[Shu Chen, 2014-10-23 14:14:00]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2014-10-23 14:14:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/676593004/120001

[commit-bot: I haz the power, 2014-10-23 15:17:33]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-10-23 15:18:04]

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