Adding a configuration dialog for Pinyin input method.

Adding a configuration dialog for Pinyin input method.

BUG=crosbug.com/491
BUG=crosbug.com/2623
TEST=manual

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

[satorux1, 2010-04-27 03:41:39]

look good overall, but what about using tags?

[satorux1, 2010-04-27 03:41:43]

looks good overall, but what about using tags?

[satorux1, 2010-04-27 03:42:11]

http://codereview.chromium.org/1694017/diff/1/5
File chrome/browser/chromeos/options/language_pinyin_config_view.cc (right):

http://codereview.chromium.org/1694017/diff/1/5#newcode34
chrome/browser/chromeos/options/language_pinyin_config_view.cc:34: }
You can merge the two loops

http://codereview.chromium.org/1694017/diff/1/5#newcode43
chrome/browser/chromeos/options/language_pinyin_config_view.cc:43: for (size_t i
= 0; i < kNumPinyinBooleanPrefs; ++i) {
Instead of having a loop,  I think you can do:

  const int pref_id = checkbox->tag();
  pinyin_boolean_prefs[pref_id].SetValue(checkbox->checked());

In this way, you don't need to have pinyin_boolean_checkboxes_ as a member
variable.

http://codereview.chromium.org/1694017/diff/1/5#newcode91
chrome/browser/chromeos/options/language_pinyin_config_view.cc:91:
pinyin_boolean_checkboxes_[i]->set_listener(this);
What about setting a tag here?

->set_tag(i)

[Yusuke Sato, 2010-04-27 17:51:02]

Thanks, fixed.

http://codereview.chromium.org/1694017/diff/1/5
File chrome/browser/chromeos/options/language_pinyin_config_view.cc (right):

http://codereview.chromium.org/1694017/diff/1/5#newcode34
chrome/browser/chromeos/options/language_pinyin_config_view.cc:34: }
On 2010/04/27 03:42:11, satorux1 wrote:
> You can merge the two loops

Done.

http://codereview.chromium.org/1694017/diff/1/5#newcode43
chrome/browser/chromeos/options/language_pinyin_config_view.cc:43: for (size_t i
= 0; i < kNumPinyinBooleanPrefs; ++i) {
Good idea. Done.

> In this way, you don't need to have pinyin_boolean_checkboxes_ as a member
> variable.

Let me keep the member variable since it's still used in NotifyPrefChanged().

http://codereview.chromium.org/1694017/diff/1/5#newcode91
chrome/browser/chromeos/options/language_pinyin_config_view.cc:91:
pinyin_boolean_checkboxes_[i]->set_listener(this);
On 2010/04/27 03:42:11, satorux1 wrote:
> What about setting a tag here?
> 
> ->set_tag(i)

Done.

[satorux1, 2010-04-27 17:54:27]

LGTM

[tfarina (gmail-do not use), 2010-04-27 18:14:21]

I know that was already committed :)

http://codereview.chromium.org/1694017/diff/6001/7004
File chrome/browser/chromeos/options/language_pinyin_config_view.cc (right):

http://codereview.chromium.org/1694017/diff/6001/7004#newcode39
chrome/browser/chromeos/options/language_pinyin_config_view.cc:39:
views::Button* sender, const views::Event& event) {
this could be better aligned (readable), as:
...(views::Button* sender,
    const views::Event& even) {
...
}
?

http://codereview.chromium.org/1694017/diff/6001/7004#newcode99
chrome/browser/chromeos/options/language_pinyin_config_view.cc:99: if (type ==
NotificationType::PREF_CHANGED) {
chromium style is to not add {} in single line statements. :)

http://codereview.chromium.org/1694017/diff/6001/7005
File chrome/browser/chromeos/options/language_pinyin_config_view.h (right):

http://codereview.chromium.org/1694017/diff/6001/7005#newcode54
chrome/browser/chromeos/options/language_pinyin_config_view.h:54: //
TODO(yusukes): Support integer prefs if needed.
I guess this belongs to |pinyin_boolean_prefs_|, if so could move it above?
(otherwise it looks it's talking about |contents_|)

http://codereview.chromium.org/1694017/diff/6001/7005#newcode57
chrome/browser/chromeos/options/language_pinyin_config_view.h:57: // A
checkboxes for Pinyin.
nit: The checkboxes?

http://codereview.chromium.org/1694017/diff/6001/7006
File chrome/browser/chromeos/preferences.cc (right):

http://codereview.chromium.org/1694017/diff/6001/7006#newcode1
chrome/browser/chromeos/preferences.cc:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
nit: 2010

http://codereview.chromium.org/1694017/diff/6001/7007
File chrome/browser/chromeos/preferences.h (right):

http://codereview.chromium.org/1694017/diff/6001/7007#newcode1
chrome/browser/chromeos/preferences.h:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
nit: 2010

[satorux1, 2010-04-28 05:37:05]

http://codereview.chromium.org/1694017/diff/6001/7004
File chrome/browser/chromeos/options/language_pinyin_config_view.cc (right):

http://codereview.chromium.org/1694017/diff/6001/7004#newcode99
chrome/browser/chromeos/options/language_pinyin_config_view.cc:99: if (type ==
NotificationType::PREF_CHANGED) {
On 2010/04/27 18:14:21, tfarina wrote:
> chromium style is to not add {} in single line statements. :)

Is it? Could you tell us where it is defined?

[tfarina (gmail-do not use), 2010-04-28 15:44:55]

On 2010/04/28 05:37:05, satorux1 wrote:
> http://codereview.chromium.org/1694017/diff/6001/7004
> File chrome/browser/chromeos/options/language_pinyin_config_view.cc (right):
> 
> http://codereview.chromium.org/1694017/diff/6001/7004#newcode99
> chrome/browser/chromeos/options/language_pinyin_config_view.cc:99: if (type ==
> NotificationType::PREF_CHANGED) {
> On 2010/04/27 18:14:21, tfarina wrote:
> > chromium style is to not add {} in single line statements. :)
> 
> Is it? Could you tell us where it is defined?
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...

"In general, curly braces are not required for single-line statements, but they
are allowed if you like them".

Also if you see some reviews of Peter Kasting and other folks you will see that
they insist on this.