Add the ability to filter out extension IMEs from the language settings page.

Add the ability to filter out extension IMEs from the language settings page.

BUG=137640
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157737

[Zachary Kuznia, 2012-09-12 09:07:53]

nona@:  Could you review the changes to Input Method Manager?

jhawkins@:  Could you review the preferences changes?

Thanks,
-Zach

[James Hawkins, 2012-09-12 16:11:57]

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
File chrome/browser/resources/options/language_list.js (right):

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/language_list.js:178: * Clear the selection
nit: s/Clear/Clears/

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
File chrome/browser/resources/options/language_options.html (right):

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/language_options.html:22:
i18n-content="add_button"></button>
nit while you're here: Wrapped element lines should be indented 4 spaces only
(not aligned on attribute name).

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
File chrome/browser/resources/options/language_options.js (right):

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/language_options.js:98:
$('language-options-extension-ime-button').addEventListener('click',
nit: The start of parameter rows must align on the same column.

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/language_options.js:196:
//element.languageCodeSet = {kExtensionImePage: true};
Dead code?

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/language_options.js:203: // Listen to user
clicks.
nit: This comment is superfluous.

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/language_options.js:213:
Preferences.getInstance().addEventListener(this.filteredExtensionImesPref,
nit: The start of parameter rows must align on the same column.

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/language_options.js:598:
chrome.send('inputMethodEnable', [checkbox.inputMethodId]);
Dead code?

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/language_options.js:660: if
(input.inputMethodId.match(/^_ext_ime_/)) {
nit: No braces for single-line blocks.

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/language_options.js:797: for (var i = 0; i <
this.filteredExtensionImes_.length; i++) {
nit: No braces for single-line blocks.

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/language_options.js:804: if
(checkboxes[i].inputMethodId.match(/^_ext_ime_/)) {
nit: No braces for single-line blocks.

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/language_options.js:821: if
(!checkboxes[i].checked) {
nit: No braces for single-line blocks.

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/language_options.js:853: checkboxes[i].checked
= (checkboxes[i].inputMethodId in dictionary);
nit: No braces for single-line blocks.

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/language_options.js:869: if
(checkboxes[i].checked) {
nit: No braces for single-line blocks.

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.h
(right):

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.h:49:
static base::ListValue* GetExtensionImeList();
nit: Document.

[Seigo Nonaka, 2012-09-12 16:50:36]

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/input_method/input_method_manager.h (right):

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/input_method/input_method_manager.h:132: virtual
InputMethodDescriptors* GetInputMethodExtensions() = 0;
Please add function comments.

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/input_method/input_method_manager.h:134: virtual void
SetFilteredExtensionImes(std::vector<std::string>& ids) = 0;
ditto and please use pointer instead.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Refere...

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:370:
InputMethodDescriptors* result = new InputMethodDescriptors;
Caller takes ownership of returned value?
Please leave document in function comment or how about passing
InputMethodDescriptors pointer instead?

[Zachary Kuznia, 2012-09-13 09:28:32]

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/input_method/input_method_manager.h (right):

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/input_method/input_method_manager.h:132: virtual
InputMethodDescriptors* GetInputMethodExtensions() = 0;
On 2012/09/12 16:50:37, Seigo Nonaka wrote:
> Please add function comments.

Done.

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/input_method/input_method_manager.h:134: virtual void
SetFilteredExtensionImes(std::vector<std::string>& ids) = 0;
On 2012/09/12 16:50:37, Seigo Nonaka wrote:
> ditto and please use pointer instead.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Refere...

Done.

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/chromeo...
File chrome/browser/chromeos/input_method/input_method_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/chromeo...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:370:
InputMethodDescriptors* result = new InputMethodDescriptors;
On 2012/09/12 16:50:37, Seigo Nonaka wrote:
> Caller takes ownership of returned value?
> Please leave document in function comment or how about passing
> InputMethodDescriptors pointer instead?

Done.

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
File chrome/browser/resources/options/language_list.js (right):

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/language_list.js:178: * Clear the selection
On 2012/09/12 16:11:58, James Hawkins wrote:
> nit: s/Clear/Clears/

Done.

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
File chrome/browser/resources/options/language_options.html (right):

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/language_options.html:22:
i18n-content="add_button"></button>
On 2012/09/12 16:11:58, James Hawkins wrote:
> nit while you're here: Wrapped element lines should be indented 4 spaces only
> (not aligned on attribute name).

Done.

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
File chrome/browser/resources/options/language_options.js (right):

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/language_options.js:98:
$('language-options-extension-ime-button').addEventListener('click',
On 2012/09/12 16:11:58, James Hawkins wrote:
> nit: The start of parameter rows must align on the same column.

Done.

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/language_options.js:196:
//element.languageCodeSet = {kExtensionImePage: true};
On 2012/09/12 16:11:58, James Hawkins wrote:
> Dead code?

Done.

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/language_options.js:203: // Listen to user
clicks.
On 2012/09/12 16:11:58, James Hawkins wrote:
> nit: This comment is superfluous.

Done.

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/language_options.js:213:
Preferences.getInstance().addEventListener(this.filteredExtensionImesPref,
On 2012/09/12 16:11:58, James Hawkins wrote:
> nit: The start of parameter rows must align on the same column.

Done.

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/language_options.js:598:
chrome.send('inputMethodEnable', [checkbox.inputMethodId]);
On 2012/09/12 16:11:58, James Hawkins wrote:
> Dead code?

Done.

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/language_options.js:660: if
(input.inputMethodId.match(/^_ext_ime_/)) {
On 2012/09/12 16:11:58, James Hawkins wrote:
> nit: No braces for single-line blocks.

Done.

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/language_options.js:797: for (var i = 0; i <
this.filteredExtensionImes_.length; i++) {
On 2012/09/12 16:11:58, James Hawkins wrote:
> nit: No braces for single-line blocks.

Done.

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/language_options.js:804: if
(checkboxes[i].inputMethodId.match(/^_ext_ime_/)) {
On 2012/09/12 16:11:58, James Hawkins wrote:
> nit: No braces for single-line blocks.

Done.

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/language_options.js:821: if
(!checkboxes[i].checked) {
On 2012/09/12 16:11:58, James Hawkins wrote:
> nit: No braces for single-line blocks.

Done.

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/language_options.js:853: checkboxes[i].checked
= (checkboxes[i].inputMethodId in dictionary);
On 2012/09/12 16:11:58, James Hawkins wrote:
> nit: No braces for single-line blocks.

Done.

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/resourc...
chrome/browser/resources/options/language_options.js:869: if
(checkboxes[i].checked) {
On 2012/09/12 16:11:58, James Hawkins wrote:
> nit: No braces for single-line blocks.

Done.

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.h
(right):

https://chromiumcodereview.appspot.com/10907196/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/chromeos/cros_language_options_handler.h:49:
static base::ListValue* GetExtensionImeList();
On 2012/09/12 16:11:58, James Hawkins wrote:
> nit: Document.

Done.

[Seigo Nonaka, 2012-09-13 18:25:20]

lgtm

input_method/* lgtm

http://codereview.chromium.org/10907196/diff/8002/chrome/browser/chromeos/inp...
File chrome/browser/chromeos/input_method/input_method_manager.h (right):

http://codereview.chromium.org/10907196/diff/8002/chrome/browser/chromeos/inp...
chrome/browser/chromeos/input_method/input_method_manager.h:132: // Return a
list of descriptors for all Input Method Extensions.
nit: Returns

[Zachary Kuznia, 2012-09-18 06:25:08]

jhawkins: Ping

[James Hawkins, 2012-09-18 15:47:31]

LGTM with nits.

http://codereview.chromium.org/10907196/diff/8002/chrome/browser/chromeos/inp...
File chrome/browser/chromeos/input_method/input_method_manager_impl.h (right):

http://codereview.chromium.org/10907196/diff/8002/chrome/browser/chromeos/inp...
chrome/browser/chromeos/input_method/input_method_manager_impl.h:149: // These
IMEs are not added to the ime list.
// The list of IMEs that are filtered from the IME list.

http://codereview.chromium.org/10907196/diff/8002/chrome/browser/resources/op...
File chrome/browser/resources/options/language_options.html (right):

http://codereview.chromium.org/10907196/diff/8002/chrome/browser/resources/op...
chrome/browser/resources/options/language_options.html:25:
i18n-content="extension_ime_label"></button>
nit: Wrapped lines must have the closing tag on a new line.

[Zachary Kuznia, 2012-09-19 10:15:50]

https://chromiumcodereview.appspot.com/10907196/diff/8002/chrome/browser/chro...
File chrome/browser/chromeos/input_method/input_method_manager.h (right):

https://chromiumcodereview.appspot.com/10907196/diff/8002/chrome/browser/chro...
chrome/browser/chromeos/input_method/input_method_manager.h:132: // Return a
list of descriptors for all Input Method Extensions.
On 2012/09/13 18:25:21, Seigo Nonaka wrote:
> nit: Returns

Done.

https://chromiumcodereview.appspot.com/10907196/diff/8002/chrome/browser/chro...
File chrome/browser/chromeos/input_method/input_method_manager_impl.h (right):

https://chromiumcodereview.appspot.com/10907196/diff/8002/chrome/browser/chro...
chrome/browser/chromeos/input_method/input_method_manager_impl.h:149: // These
IMEs are not added to the ime list.
On 2012/09/18 15:47:31, James Hawkins wrote:
> // The list of IMEs that are filtered from the IME list.

Done.

https://chromiumcodereview.appspot.com/10907196/diff/8002/chrome/browser/reso...
File chrome/browser/resources/options/language_options.html (right):

https://chromiumcodereview.appspot.com/10907196/diff/8002/chrome/browser/reso...
chrome/browser/resources/options/language_options.html:25:
i18n-content="extension_ime_label"></button>
On 2012/09/18 15:47:31, James Hawkins wrote:
> nit: Wrapped lines must have the closing tag on a new line.

Done.

[commit-bot: I haz the power, 2012-09-19 10:15:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/10907196/15001

[commit-bot: I haz the power, 2012-09-19 10:50:09]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-20 04:41:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/10907196/15001

[commit-bot: I haz the power, 2012-09-20 05:00:55]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-20 08:08:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/10907196/15001

[commit-bot: I haz the power, 2012-09-20 08:30:46]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-20 08:45:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/10907196/33003

[commit-bot: I haz the power, 2012-09-20 10:36:15]

Change committed as 157737