Sort input methods names in the language selector by language names.

Sort input methods names in the language selector by language names.

BUG=chromium-os:3867
TEST=manual. ran the unit_test as well.

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=49367

[satorux1, 2010-06-09 04:27:06]

http://codereview.chromium.org/2764001/diff/2001/3001
File chrome/browser/chromeos/options/language_config_view.cc (right):

http://codereview.chromium.org/2764001/diff/2001/3001#newcode981
chrome/browser/chromeos/options/language_config_view.cc:981: language_code_1 =
id_to_language_code_map_.find(s1)->second;
You are looking up the map twice here.

http://codereview.chromium.org/2764001/diff/2001/3001#newcode989
chrome/browser/chromeos/options/language_config_view.cc:989: return s1 < s2;  //
use an input method id as the second sort key.
This worries me. STL sort() can crash if the given comparator does not meet
certain conditions. I'm not sure if this meets the conditions.

I guess you can get rid of the "if" block.

http://codereview.chromium.org/2764001/diff/2001/3003
File chrome/browser/chromeos/options/language_config_view_test.cc (right):

http://codereview.chromium.org/2764001/diff/2001/3003#newcode32
chrome/browser/chromeos/options/language_config_view_test.cc:32:
LanguageConfigView::SortLanguageCodesByNames(&language_codes);  // do nothing
Maybe a bit more descriptive comment is useful.

// Check if this function can handle an empty list.

http://codereview.chromium.org/2764001/diff/2001/3003#newcode55
chrome/browser/chromeos/options/language_config_view_test.cc:55:
LanguageConfigView::SortInputMethodIdsByNames(&input_method_ids);
ditto.

http://codereview.chromium.org/2764001/diff/2001/3003#newcode59
chrome/browser/chromeos/options/language_config_view_test.cc:59:
input_method_ids.push_back("m17n:latn-pre");  // language code is "t"
We'll sort by language names, not codes. Would be nice to have language names in
the comments like

// ja => Japanese

http://codereview.chromium.org/2764001/diff/2001/3003#newcode64
chrome/browser/chromeos/options/language_config_view_test.cc:64:
ASSERT_EQ("m17n:latn-pre",  input_method_ids[2]);  // "t"
Ditto.

[Yusuke Sato, 2010-06-09 09:20:50]

Thanks for the comment. Fixed all and checked unit_test passes.

http://codereview.chromium.org/2764001/diff/2001/3001
File chrome/browser/chromeos/options/language_config_view.cc (right):

http://codereview.chromium.org/2764001/diff/2001/3001#newcode981
chrome/browser/chromeos/options/language_config_view.cc:981: language_code_1 =
id_to_language_code_map_.find(s1)->second;
On 2010/06/09 04:27:07, satorux1 wrote:
> You are looking up the map twice here. 

Done.

http://codereview.chromium.org/2764001/diff/2001/3001#newcode989
chrome/browser/chromeos/options/language_config_view.cc:989: return s1 < s2;  //
use an input method id as the second sort key.
On 2010/06/09 04:27:07, satorux1 wrote:
> This worries me. STL sort() can crash if the given comparator does not meet
> certain conditions. I'm not sure if this meets the conditions.
> 
> I guess you can get rid of the "if" block.

Done.

Instead of comparing s1 and s2, I used std::stable_sort in
SortInputMethodIdsByNames() so that the function maintains the order of input
method ids that belong to the same language (e.g. "mozc" and "xkb:jp::jpn").

Please check the comment in SetInputMethodActivated() too.

http://codereview.chromium.org/2764001/diff/2001/3003
File chrome/browser/chromeos/options/language_config_view_test.cc (right):

http://codereview.chromium.org/2764001/diff/2001/3003#newcode32
chrome/browser/chromeos/options/language_config_view_test.cc:32:
LanguageConfigView::SortLanguageCodesByNames(&language_codes);  // do nothing
On 2010/06/09 04:27:07, satorux1 wrote:
> Maybe a bit more descriptive comment is useful.
> 
> // Check if this function can handle an empty list.

Done.

http://codereview.chromium.org/2764001/diff/2001/3003#newcode55
chrome/browser/chromeos/options/language_config_view_test.cc:55:
LanguageConfigView::SortInputMethodIdsByNames(&input_method_ids);
On 2010/06/09 04:27:07, satorux1 wrote:
> ditto.

Done.

http://codereview.chromium.org/2764001/diff/2001/3003#newcode59
chrome/browser/chromeos/options/language_config_view_test.cc:59:
input_method_ids.push_back("m17n:latn-pre");  // language code is "t"
On 2010/06/09 04:27:07, satorux1 wrote:
> We'll sort by language names, not codes. Would be nice to have language names
in
> the comments like
> 
> // ja => Japanese

Done.

http://codereview.chromium.org/2764001/diff/2001/3003#newcode64
chrome/browser/chromeos/options/language_config_view_test.cc:64:
ASSERT_EQ("m17n:latn-pre",  input_method_ids[2]);  // "t"
On 2010/06/09 04:27:07, satorux1 wrote:
> Ditto.

Done.

[satorux1, 2010-06-09 09:42:37]

LGTM

On 2010/06/09 09:20:50, Yusuke Sato wrote:
> Thanks for the comment. Fixed all and checked unit_test passes.
> 
> http://codereview.chromium.org/2764001/diff/2001/3001
> File chrome/browser/chromeos/options/language_config_view.cc (right):
> 
> http://codereview.chromium.org/2764001/diff/2001/3001#newcode981
> chrome/browser/chromeos/options/language_config_view.cc:981: language_code_1 =
> id_to_language_code_map_.find(s1)->second;
> On 2010/06/09 04:27:07, satorux1 wrote:
> > You are looking up the map twice here. 
> 
> Done.
> 
> http://codereview.chromium.org/2764001/diff/2001/3001#newcode989
> chrome/browser/chromeos/options/language_config_view.cc:989: return s1 < s2; 
//
> use an input method id as the second sort key.
> On 2010/06/09 04:27:07, satorux1 wrote:
> > This worries me. STL sort() can crash if the given comparator does not meet
> > certain conditions. I'm not sure if this meets the conditions.
> > 
> > I guess you can get rid of the "if" block.
> 
> Done.
> 
> Instead of comparing s1 and s2, I used std::stable_sort in
> SortInputMethodIdsByNames() so that the function maintains the order of input
> method ids that belong to the same language (e.g. "mozc" and "xkb:jp::jpn").
> 
> Please check the comment in SetInputMethodActivated() too.
> 
> http://codereview.chromium.org/2764001/diff/2001/3003
> File chrome/browser/chromeos/options/language_config_view_test.cc (right):
> 
> http://codereview.chromium.org/2764001/diff/2001/3003#newcode32
> chrome/browser/chromeos/options/language_config_view_test.cc:32:
> LanguageConfigView::SortLanguageCodesByNames(&language_codes);  // do nothing
> On 2010/06/09 04:27:07, satorux1 wrote:
> > Maybe a bit more descriptive comment is useful.
> > 
> > // Check if this function can handle an empty list.
> 
> Done.
> 
> http://codereview.chromium.org/2764001/diff/2001/3003#newcode55
> chrome/browser/chromeos/options/language_config_view_test.cc:55:
> LanguageConfigView::SortInputMethodIdsByNames(&input_method_ids);
> On 2010/06/09 04:27:07, satorux1 wrote:
> > ditto.
> 
> Done.
> 
> http://codereview.chromium.org/2764001/diff/2001/3003#newcode59
> chrome/browser/chromeos/options/language_config_view_test.cc:59:
> input_method_ids.push_back("m17n:latn-pre");  // language code is "t"
> On 2010/06/09 04:27:07, satorux1 wrote:
> > We'll sort by language names, not codes. Would be nice to have language
names
> in
> > the comments like
> > 
> > // ja => Japanese
> 
> Done.
> 
> http://codereview.chromium.org/2764001/diff/2001/3003#newcode64
> chrome/browser/chromeos/options/language_config_view_test.cc:64:
> ASSERT_EQ("m17n:latn-pre",  input_method_ids[2]);  // "t"
> On 2010/06/09 04:27:07, satorux1 wrote:
> > Ditto.
> 
> Done.