[IME] Removes the duplicated IMEs in chrome://settings/languages, and support async component IMEs …

[IME] Removes the duplicated IMEs in chrome://settings/languages, and support async component IMEs loading in oobe network screen, and fix the wrong indicator issue.

BUG=345604, 349829
TEST=None
TBR=nona@chromium.org

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

[Shu Chen, 2014-03-07 06:21:23]

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

[Seigo Nonaka, 2014-03-07 07:21:20]

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

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_util.cc:135: const
_XkbConfigItem kXkbIndicators[] = {
nit: Please leave comment this is related to crbug.com/232260

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

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:392:
descriptors = comp_manager->GetXkbIMEAsInputMethodDescriptor();
What's happened if component extension manager is not initialized?

https://codereview.chromium.org/190033005/diff/10001/chromeos/ime/component_e...
File chromeos/ime/component_extension_ime_manager.cc (right):

https://codereview.chromium.org/190033005/diff/10001/chromeos/ime/component_e...
chromeos/ime/component_extension_ime_manager.cc:97: FOR_EACH_OBSERVER(Observer,
observers_, OnInitialized());
Why NotifyInitialized function is needed?
It is natural for me to call this in Initialize function.

[Shu Chen, 2014-03-07 07:30:29]

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

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_util.cc:135: const
_XkbConfigItem kXkbIndicators[] = {
On 2014/03/07 07:21:21, Seigo Nonaka wrote:
> nit: Please leave comment this is related to crbug.com/232260

Done.

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

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:392:
descriptors = comp_manager->GetXkbIMEAsInputMethodDescriptor();
On 2014/03/07 07:21:21, Seigo Nonaka wrote:
> What's happened if component extension manager is not initialized?

If it's not initialized, descriptors will be an empty vector.
Thus, options::CrosLanguageOptionsHandler::GetUILanguageList() will get only
en-US.
But that is ok, because soon later the callback OnInitialized() will fill in all
other languages and keyboards.

https://codereview.chromium.org/190033005/diff/10001/chromeos/ime/component_e...
File chromeos/ime/component_extension_ime_manager.cc (right):

https://codereview.chromium.org/190033005/diff/10001/chromeos/ime/component_e...
chromeos/ime/component_extension_ime_manager.cc:97: FOR_EACH_OBSERVER(Observer,
observers_, OnInitialized());
On 2014/03/07 07:21:21, Seigo Nonaka wrote:
> Why NotifyInitialized function is needed?
> It is natural for me to call this in Initialize function.

Please refer to line 419 of input_method_manager_impl.cc.
It needs to make sure the observers to handle OnInitialized() and can get the
input method descriptors from InputMethodUtil.

[Yuki, 2014-03-07 07:47:04]

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

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:254: if
(component_extension_ime_manager_->IsInitialized())
We don't need this conditional.  LoadNecessaryComponentExtensions() itself
checks whether the manager is initialized or not.

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:419:
component_extension_ime_manager_->NotifyInitialized();
In general, This sort of methods (NotifyFoobar()) should be called from the
notifier itself (Thus could be private: methods).  If you need to call Notify()
outside of the object, it's a wrong design, I think.

Having said that, we're short in time, let's put a TODO comment that this method
call should be moved into ComponentExtensionIMEManager.

As a second best solution, please put this call inside
LoadNecessaryComponentExtensions().  There are two calls to
LoadNecessaryComponentExtensions(), and we should notify in either case,
shouldn't we?

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:432:
std::vector<std::string> unfiltered_input_method_ids =
Just a nit:

std::vector unfiltered_ids;  // empty vector
unfiltered_ids.swap(active_ids_);

is much more efficient because typical implementations of swap() never copy
members while the assignment operator does copy.  (Typical implementations of
vector assignment don't do copy-on-write).

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

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_util.cc:129: struct
_XkbConfigItem {
Do not start with the underscore followed by a capital letter.  It's reserved by
the C and C++ standard.  Also our style guide doesn't allow it.

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_util.cc:135: const
_XkbConfigItem kXkbIndicators[] = {
Static initialization of non-trivial types are prohibited by our policy.
const char* const is okay in this case.  You may want another type to have pairs
of layout name and indicator in addition to XkbConfigItem.

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_util.cc:555: return NULL;
nit: Fix the indent.  4 spaces => 2 spaces.

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

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:466:
std::string current_input_method_id = manager->GetCurrentInputMethod().id();
nit: Use a const reference for |current_input_method_id|.

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:500:
std::string us_keyboard_id =
nit: Use a const reference.

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:501:
extension_ime_util::GetInputMethodIDByKeyboardLayout(kUSlayout);
nit: kUsLayout or kUSLayout.  L in Layout must be capital.  Not your fault
though.

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:502: if
(input_methods_added.count(us_keyboard_id) == 0) {
nit: Use find() rather than count() if the number doesn't matter.

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/network_screen_handler.h (right):

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.h:70: // Handles
change of the language, the parameter force_refresh can control
nit: s/force_refresh/|force_refresh|/
We use |code_snippet_or_symbol_name| style.

https://codereview.chromium.org/190033005/diff/10001/chromeos/ime/component_e...
File chromeos/ime/component_extension_ime_manager.cc (right):

https://codereview.chromium.org/190033005/diff/10001/chromeos/ime/component_e...
chromeos/ime/component_extension_ime_manager.cc:214:
input_method::InputMethodDescriptors descriptors =
Make this a const reference.  It's a good practice in general.  (Although no
effect in this case.)

https://codereview.chromium.org/190033005/diff/10001/chromeos/ime/component_e...
File chromeos/ime/component_extension_ime_manager.h (right):

https://codereview.chromium.org/190033005/diff/10001/chromeos/ime/component_e...
chromeos/ime/component_extension_ime_manager.h:69: virtual void OnInitialized()
= 0;
This is not your fault, but this is a very bad name.
If possible, could you rename this to something more specific?  For example,
OnImeComponentExtensionInitialized().

It's too bad and confusing that NetworkScreenHandler::OnInitialized() overrides
this method.  NetworkScreenHandler::OnInitialized() looks like it's called when
NetworkScreenHandler is initialized.

There are 1 unittest and CrosLanguageOptionsHandler that override this method. 
So shouldn't be too much work.

https://codereview.chromium.org/190033005/diff/10001/chromeos/ime/component_e...
chromeos/ime/component_extension_ime_manager.h:80: // Notifies the observes for
the component extension IMEs are initialized.
typo: s/observes/observers/

[Seigo Nonaka, 2014-03-07 07:50:04]

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

https://codereview.chromium.org/190033005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:438: LOG(ERROR)
<< "------NetworkScreenHandler::OnInitialized";
Please remove debug code.

https://codereview.chromium.org/190033005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:447:
this->core_oobe_actor_->ReloadContent(localized_strings);
Question: does ReloadContent cause display blink at bootstrap?
If so, I recommend you to call JavaScript function instead of reloading whole
page.

This is what chrome://settings/ page does.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

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

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

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:254: if
(component_extension_ime_manager_->IsInitialized())
On 2014/03/07 07:47:04, Yuki wrote:
> We don't need this conditional.  LoadNecessaryComponentExtensions() itself
> checks whether the manager is initialized or not.

Done.

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:419:
component_extension_ime_manager_->NotifyInitialized();
On 2014/03/07 07:47:04, Yuki wrote:
> In general, This sort of methods (NotifyFoobar()) should be called from the
> notifier itself (Thus could be private: methods).  If you need to call
Notify()
> outside of the object, it's a wrong design, I think.
> 
> Having said that, we're short in time, let's put a TODO comment that this
method
> call should be moved into ComponentExtensionIMEManager.
> 
> As a second best solution, please put this call inside
> LoadNecessaryComponentExtensions().  There are two calls to
> LoadNecessaryComponentExtensions(), and we should notify in either case,
> shouldn't we?

This NotifyInitialized() should be called only once. If move to
LoadNecessaryComponentExtensions() then there will be chances to call it twice.
I would prefer to add TODO comment for now.

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:432:
std::vector<std::string> unfiltered_input_method_ids =
On 2014/03/07 07:47:04, Yuki wrote:
> Just a nit:
> 
> std::vector unfiltered_ids;  // empty vector
> unfiltered_ids.swap(active_ids_);
> 
> is much more efficient because typical implementations of swap() never copy
> members while the assignment operator does copy.  (Typical implementations of
> vector assignment don't do copy-on-write).

Done.

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

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_util.cc:129: struct
_XkbConfigItem {
On 2014/03/07 07:47:04, Yuki wrote:
> Do not start with the underscore followed by a capital letter.  It's reserved
by
> the C and C++ standard.  Also our style guide doesn't allow it.

Done.

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_util.cc:135: const
_XkbConfigItem kXkbIndicators[] = {
On 2014/03/07 07:47:04, Yuki wrote:
> Static initialization of non-trivial types are prohibited by our policy.
> const char* const is okay in this case.  You may want another type to have
pairs
> of layout name and indicator in addition to XkbConfigItem.

Sorry I forgot to remove the type _XkbConfigItem.

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_util.cc:555: return NULL;
On 2014/03/07 07:47:04, Yuki wrote:
> nit: Fix the indent.  4 spaces => 2 spaces.

Done.

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

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:466:
std::string current_input_method_id = manager->GetCurrentInputMethod().id();
On 2014/03/07 07:47:04, Yuki wrote:
> nit: Use a const reference for |current_input_method_id|.

Done.

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:500:
std::string us_keyboard_id =
On 2014/03/07 07:47:04, Yuki wrote:
> nit: Use a const reference.

Done.

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:501:
extension_ime_util::GetInputMethodIDByKeyboardLayout(kUSlayout);
On 2014/03/07 07:47:04, Yuki wrote:
> nit: kUsLayout or kUSLayout.  L in Layout must be capital.  Not your fault
> though.

Done.

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:502: if
(input_methods_added.count(us_keyboard_id) == 0) {
On 2014/03/07 07:47:04, Yuki wrote:
> nit: Use find() rather than count() if the number doesn't matter.

Done.

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/network_screen_handler.h (right):

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.h:70: // Handles
change of the language, the parameter force_refresh can control
On 2014/03/07 07:47:04, Yuki wrote:
> nit: s/force_refresh/|force_refresh|/
> We use |code_snippet_or_symbol_name| style.

Done. I've removed this.

https://codereview.chromium.org/190033005/diff/10001/chromeos/ime/component_e...
File chromeos/ime/component_extension_ime_manager.cc (right):

https://codereview.chromium.org/190033005/diff/10001/chromeos/ime/component_e...
chromeos/ime/component_extension_ime_manager.cc:214:
input_method::InputMethodDescriptors descriptors =
On 2014/03/07 07:47:04, Yuki wrote:
> Make this a const reference.  It's a good practice in general.  (Although no
> effect in this case.)

Done.

https://codereview.chromium.org/190033005/diff/10001/chromeos/ime/component_e...
File chromeos/ime/component_extension_ime_manager.h (right):

https://codereview.chromium.org/190033005/diff/10001/chromeos/ime/component_e...
chromeos/ime/component_extension_ime_manager.h:69: virtual void OnInitialized()
= 0;
On 2014/03/07 07:47:04, Yuki wrote:
> This is not your fault, but this is a very bad name.
> If possible, could you rename this to something more specific?  For example,
> OnImeComponentExtensionInitialized().
> 
> It's too bad and confusing that NetworkScreenHandler::OnInitialized()
overrides
> this method.  NetworkScreenHandler::OnInitialized() looks like it's called
when
> NetworkScreenHandler is initialized.
> 
> There are 1 unittest and CrosLanguageOptionsHandler that override this method.

> So shouldn't be too much work.

Done.

https://codereview.chromium.org/190033005/diff/10001/chromeos/ime/component_e...
chromeos/ime/component_extension_ime_manager.h:80: // Notifies the observes for
the component extension IMEs are initialized.
On 2014/03/07 07:47:04, Yuki wrote:
> typo: s/observes/observers/

Done.

[Shu Chen, 2014-03-07 08:25:31]

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

https://codereview.chromium.org/190033005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:438: LOG(ERROR)
<< "------NetworkScreenHandler::OnInitialized";
On 2014/03/07 07:50:05, Seigo Nonaka wrote:
> Please remove debug code.

Done.

https://codereview.chromium.org/190033005/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/network_screen_handler.cc:447:
this->core_oobe_actor_->ReloadContent(localized_strings);
On 2014/03/07 07:50:05, Seigo Nonaka wrote:
> Question: does ReloadContent cause display blink at bootstrap?
> If so, I recommend you to call JavaScript function instead of reloading whole
> page.
> 
> This is what chrome://settings/ page does.
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

ReloadContent() methods will just call a JS function named "reloadContent" in
the page.

[Yuki, 2014-03-07 09:02:27]

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

https://codereview.chromium.org/190033005/diff/10001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_manager_impl.cc:419:
component_extension_ime_manager_->NotifyInitialized();
On 2014/03/07 08:24:09, Shu Chen wrote:
> On 2014/03/07 07:47:04, Yuki wrote:
> > In general, This sort of methods (NotifyFoobar()) should be called from the
> > notifier itself (Thus could be private: methods).  If you need to call
> Notify()
> > outside of the object, it's a wrong design, I think.
> > 
> > Having said that, we're short in time, let's put a TODO comment that this
> method
> > call should be moved into ComponentExtensionIMEManager.
> > 
> > As a second best solution, please put this call inside
> > LoadNecessaryComponentExtensions().  There are two calls to
> > LoadNecessaryComponentExtensions(), and we should notify in either case,
> > shouldn't we?
> 
> This NotifyInitialized() should be called only once. If move to
> LoadNecessaryComponentExtensions() then there will be chances to call it
twice.
> I would prefer to add TODO comment for now.

Then, could you add a flag inside ComponentExtensionIMEManager like
|is_initialized_|?  Like:
  if (is_initialized_ && was_initialization_notified_) ...

[Shu Chen, 2014-03-07 09:11:16]

On 2014/03/07 09:02:27, Yuki wrote:
>
https://codereview.chromium.org/190033005/diff/10001/chrome/browser/chromeos/...
> File chrome/browser/chromeos/input_method/input_method_manager_impl.cc
(right):
> 
>
https://codereview.chromium.org/190033005/diff/10001/chrome/browser/chromeos/...
> chrome/browser/chromeos/input_method/input_method_manager_impl.cc:419:
> component_extension_ime_manager_->NotifyInitialized();
> On 2014/03/07 08:24:09, Shu Chen wrote:
> > On 2014/03/07 07:47:04, Yuki wrote:
> > > In general, This sort of methods (NotifyFoobar()) should be called from
the
> > > notifier itself (Thus could be private: methods).  If you need to call
> > Notify()
> > > outside of the object, it's a wrong design, I think.
> > > 
> > > Having said that, we're short in time, let's put a TODO comment that this
> > method
> > > call should be moved into ComponentExtensionIMEManager.
> > > 
> > > As a second best solution, please put this call inside
> > > LoadNecessaryComponentExtensions().  There are two calls to
> > > LoadNecessaryComponentExtensions(), and we should notify in either case,
> > > shouldn't we?
> > 
> > This NotifyInitialized() should be called only once. If move to
> > LoadNecessaryComponentExtensions() then there will be chances to call it
> twice.
> > I would prefer to add TODO comment for now.
> 
> Then, could you add a flag inside ComponentExtensionIMEManager like
> |is_initialized_|?  Like:
>   if (is_initialized_ && was_initialization_notified_) ...

Done.

[Yuki, 2014-03-07 09:13:26]

lgtm.  Thanks for working on the issues.

[Shu Chen, 2014-03-07 09:15:51]

Nikita, can you please review this cl?

This cl is an urgent fix for crbug.com/349829.

Thanks,
Shu

[Nikita (slow), 2014-03-07 09:26:59]

+Alexander, please review */chromeos/* code.

[Nikita (slow), 2014-03-07 09:28:09]

On 2014/03/07 09:26:59, Nikita Kostylev wrote:
> +Alexander, please review */chromeos/* code.

Since this fixes dev-blocker bug please review by EOD.

[Alexander Alekseev, 2014-03-07 13:19:31]

lgtm

If this is urgent and translation cannot be done now,
please create another CL.

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

https://codereview.chromium.org/190033005/diff/50001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_util.cc:131: const char* const
kXkbIndicators[][2] = {
Will this be translated?

[Shu Chen, 2014-03-07 13:22:25]

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

https://codereview.chromium.org/190033005/diff/50001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_util.cc:131: const char* const
kXkbIndicators[][2] = {
On 2014/03/07 13:19:31, alemate wrote:
> Will this be translated?

Thanks for your quick review.

There is no need to translate these strings. It won't change the original
behaviors.

Please refer to
https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/ime/input....
I was just partially copying that map.

[Seigo Nonaka, 2014-03-10 02:16:32]

lgtm, I'm sorry for my late reply.

[Shu Chen, 2014-03-10 04:30:22]

The CQ bit was checked by shuchen@chromium.org

[Shu Chen, 2014-03-10 04:30:22]

The CQ bit was unchecked by shuchen@chromium.org

[Shu Chen, 2014-03-10 04:30:33]

The CQ bit was checked by shuchen@chromium.org

[commit-bot: I haz the power, 2014-03-10 04:30:43]

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

[commit-bot: I haz the power, 2014-03-10 06:19:41]

Change committed as 255879