Implement "Language Switcher" for Chromium OS.

Implement "Language Switcher" for Chromium OS. This change enables users to switch their IME (input method) by clicking a menu button on the status area. Basic structure of the code is almost the same as power_menu_button and power_library.

Demo: http://dev.chromium.org/chromium-os/chromiumos-design-docs/text-input/demos

language_library.{cc,h}: UI-libcros glue. boilerplate code.
language_menu_button.{cc,h}: A button on the status area and its drop-down menu. Implements app/menus/menu_model.h interface.
status_area_view.{cc,h}: Put the language button on the status area.

BUG=494
TEST=Start Chromium OS. Click the menu button on the status area. Then verify that all IMEs you configured (via ibus-setup command, for now) is listed in the drop down menu.


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

[Yusuke Sato, 2009-12-08 06:59:56]

Charlie,
Can I ask you to review this change briefly? I've implemented a status area
button for IME/Keyboard switching (as we discussed when I was in MV) and the
structure of the code is very similar to yours, power button.

Demo movie of the button is available:
http://dev.chromium.org/chromium-os/chromiumos-design-docs/text-input/demos

Satoru:
Please review this.

[satorux1, 2009-12-08 12:20:32]

http://codereview.chromium.org/449050/diff/6001/7004
File chrome/browser/chromeos/language_library.h (right):

http://codereview.chromium.org/449050/diff/6001/7004#newcode43
chrome/browser/chromeos/language_library.h:43: void
ChangeLanguage(LanguageCategory category, const std::string& id);
Could you add some comment about the parameters?

http://codereview.chromium.org/449050/diff/6001/7001
File chrome/browser/chromeos/language_menu_button.cc (right):

http://codereview.chromium.org/449050/diff/6001/7001#newcode37
chrome/browser/chromeos/language_menu_button.cc:37: std::string
FormatInputLanguage(
function comment is missing.

http://codereview.chromium.org/449050/diff/6001/7001#newcode134
chrome/browser/chromeos/language_menu_button.cc:134: return true;
 comment about why always true?

http://codereview.chromium.org/449050/diff/6001/7001#newcode141
chrome/browser/chromeos/language_menu_button.cc:141: void
LanguageMenuButton::HighlightChangedTo(int index) {
Why does this do nothing? not implemented?

http://codereview.chromium.org/449050/diff/6001/7001#newcode214
chrome/browser/chromeos/language_menu_button.cc:214: current_language_ =
*current_language;
If we just want a copy of this, could we use "const InputLanguage&" instead?

http://codereview.chromium.org/449050/diff/6001/7002
File chrome/browser/chromeos/language_menu_button.h (right):

http://codereview.chromium.org/449050/diff/6001/7002#newcode18
chrome/browser/chromeos/language_menu_button.h:18: // The power menu button in
the status area.
pwer menu button -> language menu button ?

http://codereview.chromium.org/449050/diff/6001/7002#newcode19
chrome/browser/chromeos/language_menu_button.h:19: // This class will handle
getting the power status and populating the menu.
the class does not deal with power status. :)

[Yusuke Sato, 2009-12-09 06:50:09]

http://codereview.chromium.org/449050/diff/6001/7004
File chrome/browser/chromeos/language_library.h (right):

http://codereview.chromium.org/449050/diff/6001/7004#newcode43
chrome/browser/chromeos/language_library.h:43: void
ChangeLanguage(LanguageCategory category, const std::string& id);
On 2009/12/08 12:20:32, satorux1 wrote:
> Could you add some comment about the parameters?

Done.

http://codereview.chromium.org/449050/diff/6001/7001
File chrome/browser/chromeos/language_menu_button.cc (right):

http://codereview.chromium.org/449050/diff/6001/7001#newcode37
chrome/browser/chromeos/language_menu_button.cc:37: std::string
FormatInputLanguage(
On 2009/12/08 12:20:32, satorux1 wrote:
> function comment is missing.

Done.

http://codereview.chromium.org/449050/diff/6001/7001#newcode97
chrome/browser/chromeos/language_menu_button.cc:97: return false;
This function should return true!! Fixed and added comment that explains why
this should return true.

(views for Chrome OS does not use this interface though.)

http://codereview.chromium.org/449050/diff/6001/7001#newcode134
chrome/browser/chromeos/language_menu_button.cc:134: return true;
On 2009/12/08 12:20:32, satorux1 wrote:
>  comment about why always true? 

Done.

http://codereview.chromium.org/449050/diff/6001/7001#newcode141
chrome/browser/chromeos/language_menu_button.cc:141: void
LanguageMenuButton::HighlightChangedTo(int index) {
On 2009/12/08 12:20:32, satorux1 wrote:
> Why does this do nothing? not implemented?

Since we have nothing to do (and actually, views for Chrome OS does not support
this interface yet). Added comment.

http://codereview.chromium.org/449050/diff/6001/7001#newcode214
chrome/browser/chromeos/language_menu_button.cc:214: current_language_ =
*current_language;
On 2009/12/08 12:20:32, satorux1 wrote:
> If we just want a copy of this, could we use "const InputLanguage&" instead?

Done.

http://codereview.chromium.org/449050/diff/6001/7002
File chrome/browser/chromeos/language_menu_button.h (right):

http://codereview.chromium.org/449050/diff/6001/7002#newcode18
chrome/browser/chromeos/language_menu_button.h:18: // The power menu button in
the status area.
Oops, fixed.

http://codereview.chromium.org/449050/diff/6001/7002#newcode19
chrome/browser/chromeos/language_menu_button.h:19: // This class will handle
getting the power status and populating the menu.
On 2009/12/08 12:20:32, satorux1 wrote:
> the class does not deal with power status. :)

Done.

[satorux1, 2009-12-09 08:21:33]

LGTM.

http://codereview.chromium.org/449050/diff/7015/7016
File chrome/browser/chromeos/language_menu_button.cc (right):

http://codereview.chromium.org/449050/diff/7015/7016#newcode224
chrome/browser/chromeos/language_menu_button.cc:224: // Don't delete
|current_language|. We don't have the ownership of the object.
You can delete the comment. It's no longer a pointer.

[Yusuke Sato, 2009-12-09 08:25:04]

http://codereview.chromium.org/449050/diff/7015/7016
File chrome/browser/chromeos/language_menu_button.cc (right):

http://codereview.chromium.org/449050/diff/7015/7016#newcode224
chrome/browser/chromeos/language_menu_button.cc:224: // Don't delete
|current_language|. We don't have the ownership of the object.
On 2009/12/09 08:21:34, satorux1 wrote:
> You can delete the comment. It's no longer a pointer.

Done.

[satorux1, 2009-12-09 12:08:00]

LGTM.


On 2009/12/09 08:25:04, Yusuke Sato wrote:
> http://codereview.chromium.org/449050/diff/7015/7016
> File chrome/browser/chromeos/language_menu_button.cc (right):
> 
> http://codereview.chromium.org/449050/diff/7015/7016#newcode224
> chrome/browser/chromeos/language_menu_button.cc:224: // Don't delete
> |current_language|. We don't have the ownership of the object.
> On 2009/12/09 08:21:34, satorux1 wrote:
> > You can delete the comment. It's no longer a pointer.
> 
> Done.

[Charlie Lee, 2009-12-11 00:41:31]

Sorry for the delayed reply. Some comments below.

http://codereview.chromium.org/449050/diff/10001/10004
File chrome/browser/chromeos/language_library.cc (right):

http://codereview.chromium.org/449050/diff/10001/10004#newcode98
chrome/browser/chromeos/language_library.cc:98: LOG(ERROR) << "No observer!";
Why are you doing this check? It's ok for no one to observe, right?

http://codereview.chromium.org/449050/diff/10001/10005
File chrome/browser/chromeos/language_library.h (right):

http://codereview.chromium.org/449050/diff/10001/10005#newcode25
chrome/browser/chromeos/language_library.h:25: virtual void
LanguageChanged(const InputLanguage& current_language) = 0;
Return LanguageLibrary here and have observers call to get the current language.
(see network_library)

http://codereview.chromium.org/449050/diff/10001/10005#newcode39
chrome/browser/chromeos/language_library.h:39: // *never* returns NULL even if
the cros.so library is not found.
If it does not return NULL, what does it return? Please add comment.

http://codereview.chromium.org/449050/diff/10001/10005#newcode42
chrome/browser/chromeos/language_library.h:42: // Changes the current IME engine
to |name| and enable IME (when |category|
what's |name| here referring to? Should it be |id| instead?

http://codereview.chromium.org/449050/diff/10001/10005#newcode58
chrome/browser/chromeos/language_library.h:58: void* object, InputLanguage*
current_language);
change to:
const InputLanguage& current_language
(see network_library for example)

http://codereview.chromium.org/449050/diff/10001/10005#newcode65
chrome/browser/chromeos/language_library.h:65: void
UpdateCurrentLanguage(InputLanguage* current_language);
see comment above.

http://codereview.chromium.org/449050/diff/10001/10002
File chrome/browser/chromeos/language_menu_button.cc (right):

http://codereview.chromium.org/449050/diff/10001/10002#newcode27
chrome/browser/chromeos/language_menu_button.cc:27: if (language_list->empty())
{
add comment that if language_list is empty, then there's no separator. so
"configure ime" should be at index 0.

http://codereview.chromium.org/449050/diff/10001/10002#newcode28
chrome/browser/chromeos/language_menu_button.cc:28: DCHECK_EQ(index, 0);
instead of this. maybe just:

return index==0;

http://codereview.chromium.org/449050/diff/10001/10002#newcode31
chrome/browser/chromeos/language_menu_button.cc:31: if
(static_cast<size_t>(index) == language_list->size() + 1) {
Maybe just simplify to:

return static_cast<size_t>(index) == language_list->size() + 1;

http://codereview.chromium.org/449050/diff/10001/10002#newcode49
chrome/browser/chromeos/language_menu_button.cc:49: formatted += " (Layout)";
Is this string shown to the user? If so, it needs to be localized.

http://codereview.chromium.org/449050/diff/10001/10002#newcode52
chrome/browser/chromeos/language_menu_button.cc:52: formatted += " (IME)";
same as above.

http://codereview.chromium.org/449050/diff/10001/10002#newcode224
chrome/browser/chromeos/language_menu_button.cc:224: LOG(INFO) <<
"LanguageChanged: " << name;
Please remove this logging. Or make it DLOG.

http://codereview.chromium.org/449050/diff/10001/10003
File chrome/browser/chromeos/language_menu_button.h (right):

http://codereview.chromium.org/449050/diff/10001/10003#newcode53
chrome/browser/chromeos/language_menu_button.h:53: // Update the power icon
depending on the power status.
fix comment.

http://codereview.chromium.org/449050/diff/10001/10006
File chrome/browser/chromeos/status_area_view.cc (right):

http://codereview.chromium.org/449050/diff/10001/10006#newcode18
chrome/browser/chromeos/status_area_view.cc:18: #include
"chrome/browser/chromeos/language_menu_button.h"
nit: put this above network_menu_button so it's sorted

http://codereview.chromium.org/449050/diff/10001/10006#newcode207
chrome/browser/chromeos/status_area_view.cc:207: // Buttons have built in
padding, but clock doesn't.
update this comment.

[Charlie Lee, 2009-12-11 00:48:53]

After you check in the cros changes in chromeos, don't forget to modify
cros_deps/DEPS with the new git hash and add it to this change.

[Yusuke Sato, 2009-12-11 15:00:37]

Thanks for the detailed review! Fixed all.

http://codereview.chromium.org/449050/diff/10001/10004
File chrome/browser/chromeos/language_library.cc (right):

http://codereview.chromium.org/449050/diff/10001/10004#newcode98
chrome/browser/chromeos/language_library.cc:98: LOG(ERROR) << "No observer!";
On 2009/12/11 00:41:31, chocobo wrote:
> Why are you doing this check? It's ok for no one to observe, right?

Yes, removed the lines.

http://codereview.chromium.org/449050/diff/10001/10005
File chrome/browser/chromeos/language_library.h (right):

http://codereview.chromium.org/449050/diff/10001/10005#newcode25
chrome/browser/chromeos/language_library.h:25: virtual void
LanguageChanged(const InputLanguage& current_language) = 0;
On 2009/12/11 00:41:31, chocobo wrote:
> Return LanguageLibrary here and have observers call to get the current
language.
> (see network_library)

Done. Moved the |current_language_| member variable in the language menu button
class to the LanguageLibrary class.

http://codereview.chromium.org/449050/diff/10001/10005#newcode39
chrome/browser/chromeos/language_library.h:39: // *never* returns NULL even if
the cros.so library is not found.
On 2009/12/11 00:41:31, chocobo wrote:
> If it does not return NULL, what does it return? Please add comment.

Done.

http://codereview.chromium.org/449050/diff/10001/10005#newcode42
chrome/browser/chromeos/language_library.h:42: // Changes the current IME engine
to |name| and enable IME (when |category|
On 2009/12/11 00:41:31, chocobo wrote:
> what's |name| here referring to? Should it be |id| instead?

Oops, replaced |name| with |id|.

http://codereview.chromium.org/449050/diff/10001/10005#newcode58
chrome/browser/chromeos/language_library.h:58: void* object, InputLanguage*
current_language);
I chose InputLanguage* and passed in |current_language| object on heap so that
libcros can ensure that the object is never destructed while it's being used.
However, I found that allocating the object on heap is unnecessary since
NewRunnableMethod()  copies the object by value. Thanks.

Changed the argument type to const reference. I've also changed
src/platform/cros/ so it passes a InputLanguage object on stack to this function
by reference.

http://codereview.chromium.org/449050/diff/10001/10005#newcode65
chrome/browser/chromeos/language_library.h:65: void
UpdateCurrentLanguage(InputLanguage* current_language);
On 2009/12/11 00:41:31, chocobo wrote:
> see comment above.

Done.

http://codereview.chromium.org/449050/diff/10001/10002
File chrome/browser/chromeos/language_menu_button.cc (right):

http://codereview.chromium.org/449050/diff/10001/10002#newcode27
chrome/browser/chromeos/language_menu_button.cc:27: if (language_list->empty())
{
On 2009/12/11 00:41:31, chocobo wrote:
> add comment that if language_list is empty, then there's no separator. so
> "configure ime" should be at index 0.

Done.

http://codereview.chromium.org/449050/diff/10001/10002#newcode28
chrome/browser/chromeos/language_menu_button.cc:28: DCHECK_EQ(index, 0);
On 2009/12/11 00:41:31, chocobo wrote:
> instead of this. maybe just:
> 
> return index==0;

Done.

http://codereview.chromium.org/449050/diff/10001/10002#newcode31
chrome/browser/chromeos/language_menu_button.cc:31: if
(static_cast<size_t>(index) == language_list->size() + 1) {
On 2009/12/11 00:41:31, chocobo wrote:
> Maybe just simplify to:
> 
> return static_cast<size_t>(index) == language_list->size() + 1;

Done.

http://codereview.chromium.org/449050/diff/10001/10002#newcode49
chrome/browser/chromeos/language_menu_button.cc:49: formatted += " (Layout)";
Let me add a comment "TODO(yusukes): Localize string" since the design of the
language switcher is not finalized yet.

http://codereview.chromium.org/449050/diff/10001/10002#newcode52
chrome/browser/chromeos/language_menu_button.cc:52: formatted += " (IME)";
same as above too.

http://codereview.chromium.org/449050/diff/10001/10002#newcode224
chrome/browser/chromeos/language_menu_button.cc:224: LOG(INFO) <<
"LanguageChanged: " << name;
Removed.

http://codereview.chromium.org/449050/diff/10001/10003
File chrome/browser/chromeos/language_menu_button.h (right):

http://codereview.chromium.org/449050/diff/10001/10003#newcode53
chrome/browser/chromeos/language_menu_button.h:53: // Update the power icon
depending on the power status.
On 2009/12/11 00:41:31, chocobo wrote:
> fix comment.

Done.

http://codereview.chromium.org/449050/diff/10001/10006
File chrome/browser/chromeos/status_area_view.cc (right):

http://codereview.chromium.org/449050/diff/10001/10006#newcode18
chrome/browser/chromeos/status_area_view.cc:18: #include
"chrome/browser/chromeos/language_menu_button.h"
On 2009/12/11 00:41:31, chocobo wrote:
> nit: put this above network_menu_button so it's sorted

Done.

http://codereview.chromium.org/449050/diff/10001/10006#newcode207
chrome/browser/chromeos/status_area_view.cc:207: // Buttons have built in
padding, but clock doesn't.
On 2009/12/11 00:41:31, chocobo wrote:
> update this comment.

Done.

[Charlie Lee, 2009-12-11 20:12:49]

LGTM, but please send me another review, when the cros changes are in and you
have added cros_deps/DEPS to this change. thanks.

[Yusuke Sato, 2009-12-12 06:18:44]

Sure. I'll send you a DEPS patch later.

--Yusuke

On Sat, Dec 12, 2009 at 5:12 AM,  <chocobo@chromium.org> wrote:
> LGTM, but please send me another review, when the cros changes are in and
> you
> have added cros_deps/DEPS to this change. thanks.
>
> http://codereview.chromium.org/449050
>