First cut at adding the Languages and Input options sub page.

First cut at adding Chromium OS's "Languages and Input" options sub page.

This is more like a boilerplate place holder.
Will add the contents in a separate change.

BUG=chromiumo-os:4573
TEST=manually

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

[dhg, 2010-07-09 16:02:59]

http://codereview.chromium.org/2958003/diff/1/2
File chrome/browser/chromeos/dom_ui/language_options_handler.cc (right):

http://codereview.chromium.org/2958003/diff/1/2#newcode22
chrome/browser/chromeos/dom_ui/language_options_handler.cc:22:
localized_strings->SetString(
On all of these, the first param should be on the line above (see c++ style
guide Function_Calls).  the first param is only supposed to be on the next line
when the function name is too long.

http://codereview.chromium.org/2958003/diff/1/4
File chrome/browser/dom_ui/core_options_handler.cc (right):

http://codereview.chromium.org/2958003/diff/1/4#newcode57
chrome/browser/dom_ui/core_options_handler.cc:57:
l10n_util::GetString(IDS_OPTIONS_SETTINGS_LANGUAGES_DIALOG_TITLE));
This should be moved to your handler.

http://codereview.chromium.org/2958003/diff/1/7
File chrome/browser/resources/options/chromeos_language_options.html (right):

http://codereview.chromium.org/2958003/diff/1/7#newcode2
chrome/browser/resources/options/chromeos_language_options.html:2: <h1
i18n-content="languagePage"></h1>
Intentionally left blank?

http://codereview.chromium.org/2958003/diff/1/8
File chrome/browser/resources/options/chromeos_language_options.js (right):

http://codereview.chromium.org/2958003/diff/1/8#newcode3
chrome/browser/resources/options/chromeos_language_options.js:3: // found in the
LICENSE file.
I Don't think you need this class at all.  You should be able to just create an
OptionsPage.   Take a look at what labs is doing.

[arv (Not doing code reviews), 2010-07-09 17:04:32]

FYI

http://codereview.chromium.org/2958003/diff/1/2
File chrome/browser/chromeos/dom_ui/language_options_handler.cc (right):

http://codereview.chromium.org/2958003/diff/1/2#newcode9
chrome/browser/chromeos/dom_ui/language_options_handler.cc:9: #include
"chrome/browser/chromeos/input_method/input_method_util.h"
I don't think you are using this one?

http://codereview.chromium.org/2958003/diff/1/3
File chrome/browser/chromeos/dom_ui/language_options_handler.h (right):

http://codereview.chromium.org/2958003/diff/1/3#newcode8
chrome/browser/chromeos/dom_ui/language_options_handler.h:8: #include
"base/values.h"
You don't need to include this here. You can Just declare DictionaryValue since
you are only passing it in as an argument to a function.

[satorux1, 2010-07-13 11:56:30]

Thank you for the code review. I've addressed all comments. Please take another
look.

http://codereview.chromium.org/2958003/diff/1/2
File chrome/browser/chromeos/dom_ui/language_options_handler.cc (right):

http://codereview.chromium.org/2958003/diff/1/2#newcode9
chrome/browser/chromeos/dom_ui/language_options_handler.cc:9: #include
"chrome/browser/chromeos/input_method/input_method_util.h"
On 2010/07/09 17:04:32, arv wrote:
> I don't think you are using this one?

Done.

http://codereview.chromium.org/2958003/diff/1/2#newcode22
chrome/browser/chromeos/dom_ui/language_options_handler.cc:22:
localized_strings->SetString(
On 2010/07/09 16:03:00, dhg wrote:
> On all of these, the first param should be on the line above (see c++ style
> guide Function_Calls).  the first param is only supposed to be on the next
line
> when the function name is too long.

Done.

http://codereview.chromium.org/2958003/diff/1/3
File chrome/browser/chromeos/dom_ui/language_options_handler.h (right):

http://codereview.chromium.org/2958003/diff/1/3#newcode8
chrome/browser/chromeos/dom_ui/language_options_handler.h:8: #include
"base/values.h"
On 2010/07/09 17:04:32, arv wrote:
> You don't need to include this here. You can Just declare DictionaryValue
since
> you are only passing it in as an argument to a function.

Done.

http://codereview.chromium.org/2958003/diff/1/4
File chrome/browser/dom_ui/core_options_handler.cc (right):

http://codereview.chromium.org/2958003/diff/1/4#newcode57
chrome/browser/dom_ui/core_options_handler.cc:57:
l10n_util::GetString(IDS_OPTIONS_SETTINGS_LANGUAGES_DIALOG_TITLE));
On 2010/07/09 16:03:00, dhg wrote:
> This should be moved to your handler.

Done.

http://codereview.chromium.org/2958003/diff/1/7
File chrome/browser/resources/options/chromeos_language_options.html (right):

http://codereview.chromium.org/2958003/diff/1/7#newcode2
chrome/browser/resources/options/chromeos_language_options.html:2: <h1
i18n-content="languagePage"></h1>
On 2010/07/09 16:03:00, dhg wrote:
> Intentionally left blank?

Yes, the contents will be implemented in a separate CL. Added a comment.

http://codereview.chromium.org/2958003/diff/1/8
File chrome/browser/resources/options/chromeos_language_options.js (right):

http://codereview.chromium.org/2958003/diff/1/8#newcode3
chrome/browser/resources/options/chromeos_language_options.js:3: // found in the
LICENSE file.
On 2010/07/09 16:03:00, dhg wrote:
> I Don't think you need this class at all.  You should be able to just create
an
> OptionsPage.   Take a look at what labs is doing.

Removed. I'm expecting that Language Options will be complex (see the current
"Language and Input" dialog), hence we'll likely need a class. But let's add it
when it becomes necessary. :)

[satorux1, 2010-07-14 00:39:54]

Erik,

Could you take another look? David seems to be ooo this week.

On 2010/07/13 11:56:30, satorux1 wrote:
> Thank you for the code review. I've addressed all comments. Please take
another
> look.
> 
> http://codereview.chromium.org/2958003/diff/1/2
> File chrome/browser/chromeos/dom_ui/language_options_handler.cc (right):
> 
> http://codereview.chromium.org/2958003/diff/1/2#newcode9
> chrome/browser/chromeos/dom_ui/language_options_handler.cc:9: #include
> "chrome/browser/chromeos/input_method/input_method_util.h"
> On 2010/07/09 17:04:32, arv wrote:
> > I don't think you are using this one?
> 
> Done.
> 
> http://codereview.chromium.org/2958003/diff/1/2#newcode22
> chrome/browser/chromeos/dom_ui/language_options_handler.cc:22:
> localized_strings->SetString(
> On 2010/07/09 16:03:00, dhg wrote:
> > On all of these, the first param should be on the line above (see c++ style
> > guide Function_Calls).  the first param is only supposed to be on the next
> line
> > when the function name is too long.
> 
> Done.
> 
> http://codereview.chromium.org/2958003/diff/1/3
> File chrome/browser/chromeos/dom_ui/language_options_handler.h (right):
> 
> http://codereview.chromium.org/2958003/diff/1/3#newcode8
> chrome/browser/chromeos/dom_ui/language_options_handler.h:8: #include
> "base/values.h"
> On 2010/07/09 17:04:32, arv wrote:
> > You don't need to include this here. You can Just declare DictionaryValue
> since
> > you are only passing it in as an argument to a function.
> 
> Done.
> 
> http://codereview.chromium.org/2958003/diff/1/4
> File chrome/browser/dom_ui/core_options_handler.cc (right):
> 
> http://codereview.chromium.org/2958003/diff/1/4#newcode57
> chrome/browser/dom_ui/core_options_handler.cc:57:
> l10n_util::GetString(IDS_OPTIONS_SETTINGS_LANGUAGES_DIALOG_TITLE));
> On 2010/07/09 16:03:00, dhg wrote:
> > This should be moved to your handler.
> 
> Done.
> 
> http://codereview.chromium.org/2958003/diff/1/7
> File chrome/browser/resources/options/chromeos_language_options.html (right):
> 
> http://codereview.chromium.org/2958003/diff/1/7#newcode2
> chrome/browser/resources/options/chromeos_language_options.html:2: <h1
> i18n-content="languagePage"></h1>
> On 2010/07/09 16:03:00, dhg wrote:
> > Intentionally left blank?
> 
> Yes, the contents will be implemented in a separate CL. Added a comment.
> 
> http://codereview.chromium.org/2958003/diff/1/8
> File chrome/browser/resources/options/chromeos_language_options.js (right):
> 
> http://codereview.chromium.org/2958003/diff/1/8#newcode3
> chrome/browser/resources/options/chromeos_language_options.js:3: // found in
the
> LICENSE file.
> On 2010/07/09 16:03:00, dhg wrote:
> > I Don't think you need this class at all.  You should be able to just create
> an
> > OptionsPage.   Take a look at what labs is doing.
> 
> Removed. I'm expecting that Language Options will be complex (see the current
> "Language and Input" dialog), hence we'll likely need a class. But let's add
it
> when it becomes necessary. :)

[arv (Not doing code reviews), 2010-07-14 00:50:06]

LGTM

erik



On Tue, Jul 13, 2010 at 17:39,  <satorux@chromium.org> wrote:
> Erik,
>
> Could you take another look? David seems to be ooo this week.
>
> On 2010/07/13 11:56:30, satorux1 wrote:
>>
>> Thank you for the code review. I've addressed all comments. Please take
>
> another
>>
>> look.
>
>> http://codereview.chromium.org/2958003/diff/1/2
>> File chrome/browser/chromeos/dom_ui/language_options_handler.cc (right):
>
>> http://codereview.chromium.org/2958003/diff/1/2#newcode9
>> chrome/browser/chromeos/dom_ui/language_options_handler.cc:9: #include
>> "chrome/browser/chromeos/input_method/input_method_util.h"
>> On 2010/07/09 17:04:32, arv wrote:
>> > I don't think you are using this one?
>
>> Done.
>
>> http://codereview.chromium.org/2958003/diff/1/2#newcode22
>> chrome/browser/chromeos/dom_ui/language_options_handler.cc:22:
>> localized_strings->SetString(
>> On 2010/07/09 16:03:00, dhg wrote:
>> > On all of these, the first param should be on the line above (see c++
>> > style
>> > guide Function_Calls).  the first param is only supposed to be on the
>> > next
>> line
>> > when the function name is too long.
>
>> Done.
>
>> http://codereview.chromium.org/2958003/diff/1/3
>> File chrome/browser/chromeos/dom_ui/language_options_handler.h (right):
>
>> http://codereview.chromium.org/2958003/diff/1/3#newcode8
>> chrome/browser/chromeos/dom_ui/language_options_handler.h:8: #include
>> "base/values.h"
>> On 2010/07/09 17:04:32, arv wrote:
>> > You don't need to include this here. You can Just declare
>> > DictionaryValue
>> since
>> > you are only passing it in as an argument to a function.
>
>> Done.
>
>> http://codereview.chromium.org/2958003/diff/1/4
>> File chrome/browser/dom_ui/core_options_handler.cc (right):
>
>> http://codereview.chromium.org/2958003/diff/1/4#newcode57
>> chrome/browser/dom_ui/core_options_handler.cc:57:
>> l10n_util::GetString(IDS_OPTIONS_SETTINGS_LANGUAGES_DIALOG_TITLE));
>> On 2010/07/09 16:03:00, dhg wrote:
>> > This should be moved to your handler.
>
>> Done.
>
>> http://codereview.chromium.org/2958003/diff/1/7
>> File chrome/browser/resources/options/chromeos_language_options.html
>> (right):
>
>> http://codereview.chromium.org/2958003/diff/1/7#newcode2
>> chrome/browser/resources/options/chromeos_language_options.html:2: <h1
>> i18n-content="languagePage"></h1>
>> On 2010/07/09 16:03:00, dhg wrote:
>> > Intentionally left blank?
>
>> Yes, the contents will be implemented in a separate CL. Added a comment.
>
>> http://codereview.chromium.org/2958003/diff/1/8
>> File chrome/browser/resources/options/chromeos_language_options.js
>> (right):
>
>> http://codereview.chromium.org/2958003/diff/1/8#newcode3
>> chrome/browser/resources/options/chromeos_language_options.js:3: // found
>> in
>
> the
>>
>> LICENSE file.
>> On 2010/07/09 16:03:00, dhg wrote:
>> > I Don't think you need this class at all.  You should be able to just
>> > create
>> an
>> > OptionsPage.   Take a look at what labs is doing.
>
>> Removed. I'm expecting that Language Options will be complex (see the
>> current
>> "Language and Input" dialog), hence we'll likely need a class. But let's
>> add
>
> it
>>
>> when it becomes necessary. :)
>
>
>
> http://codereview.chromium.org/2958003/show
>

[satorux1, 2010-07-14 01:03:55]

Thanks! Will submit once the tree is opened.

On 2010/07/14 00:50:06, arv wrote:
> LGTM
> 
> erik
> 
> 
> 
> On Tue, Jul 13, 2010 at 17:39,  <mailto:satorux@chromium.org> wrote:
> > Erik,
> >
> > Could you take another look? David seems to be ooo this week.
> >
> > On 2010/07/13 11:56:30, satorux1 wrote:
> >>
> >> Thank you for the code review. I've addressed all comments. Please take
> >
> > another
> >>
> >> look.
> >
> >> http://codereview.chromium.org/2958003/diff/1/2
> >> File chrome/browser/chromeos/dom_ui/language_options_handler.cc (right):
> >
> >> http://codereview.chromium.org/2958003/diff/1/2#newcode9
> >> chrome/browser/chromeos/dom_ui/language_options_handler.cc:9: #include
> >> "chrome/browser/chromeos/input_method/input_method_util.h"
> >> On 2010/07/09 17:04:32, arv wrote:
> >> > I don't think you are using this one?
> >
> >> Done.
> >
> >> http://codereview.chromium.org/2958003/diff/1/2#newcode22
> >> chrome/browser/chromeos/dom_ui/language_options_handler.cc:22:
> >> localized_strings->SetString(
> >> On 2010/07/09 16:03:00, dhg wrote:
> >> > On all of these, the first param should be on the line above (see c++
> >> > style
> >> > guide Function_Calls). &nbsp;the first param is only supposed to be on
the
> >> > next
> >> line
> >> > when the function name is too long.
> >
> >> Done.
> >
> >> http://codereview.chromium.org/2958003/diff/1/3
> >> File chrome/browser/chromeos/dom_ui/language_options_handler.h (right):
> >
> >> http://codereview.chromium.org/2958003/diff/1/3#newcode8
> >> chrome/browser/chromeos/dom_ui/language_options_handler.h:8: #include
> >> "base/values.h"
> >> On 2010/07/09 17:04:32, arv wrote:
> >> > You don't need to include this here. You can Just declare
> >> > DictionaryValue
> >> since
> >> > you are only passing it in as an argument to a function.
> >
> >> Done.
> >
> >> http://codereview.chromium.org/2958003/diff/1/4
> >> File chrome/browser/dom_ui/core_options_handler.cc (right):
> >
> >> http://codereview.chromium.org/2958003/diff/1/4#newcode57
> >> chrome/browser/dom_ui/core_options_handler.cc:57:
> >> l10n_util::GetString(IDS_OPTIONS_SETTINGS_LANGUAGES_DIALOG_TITLE));
> >> On 2010/07/09 16:03:00, dhg wrote:
> >> > This should be moved to your handler.
> >
> >> Done.
> >
> >> http://codereview.chromium.org/2958003/diff/1/7
> >> File chrome/browser/resources/options/chromeos_language_options.html
> >> (right):
> >
> >> http://codereview.chromium.org/2958003/diff/1/7#newcode2
> >> chrome/browser/resources/options/chromeos_language_options.html:2: <h1
> >> i18n-content="languagePage"></h1>
> >> On 2010/07/09 16:03:00, dhg wrote:
> >> > Intentionally left blank?
> >
> >> Yes, the contents will be implemented in a separate CL. Added a comment.
> >
> >> http://codereview.chromium.org/2958003/diff/1/8
> >> File chrome/browser/resources/options/chromeos_language_options.js
> >> (right):
> >
> >> http://codereview.chromium.org/2958003/diff/1/8#newcode3
> >> chrome/browser/resources/options/chromeos_language_options.js:3: // found
> >> in
> >
> > the
> >>
> >> LICENSE file.
> >> On 2010/07/09 16:03:00, dhg wrote:
> >> > I Don't think you need this class at all. &nbsp;You should be able to
just
> >> > create
> >> an
> >> > OptionsPage. &nbsp; Take a look at what labs is doing.
> >
> >> Removed. I'm expecting that Language Options will be complex (see the
> >> current
> >> "Language and Input" dialog), hence we'll likely need a class. But let's
> >> add
> >
> > it
> >>
> >> when it becomes necessary. :)
> >
> >
> >
> > http://codereview.chromium.org/2958003/show
> >
>