Gtk languages options page part 1: accept-languages configuration.

Gtk languages options page part 1: accept-languages configuration.

Move accept_language_list to l10n_util, use it for LanguagesPageGtk.

BUG=13524
TEST=You can view, reorder, remove and add accept-languages.

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

[mattm, 2009-08-19 03:59:33]

Wasn't entirely sure where to put the accept_languages_list. Was vacillating
between this and putting them in language_combobox_model.

[Evan Stade, 2009-08-19 04:07:08]

On 2009/08/19 03:59:33, mattm wrote:
> Wasn't entirely sure where to put the accept_languages_list. Was vacillating
> between this and putting them in language_combobox_model.

this looks ok. l10n_util is a pretty much a grab-bag. reviewing now...

[Evan Stade, 2009-08-19 04:21:18]

mostly nits

http://codereview.chromium.org/174043/diff/1/5
File chrome/browser/gtk/options/languages_page_gtk.cc (right):

http://codereview.chromium.org/174043/diff/1/5#newcode88
Line 88: gtk_widget_show_all(dialog_);
can you make this the last line of the function? I'm sure it doesn't matter but
it would feel nicer

http://codereview.chromium.org/174043/diff/1/5#newcode131
Line 131: 
delete blank line

http://codereview.chromium.org/174043/diff/1/5#newcode224
Line 224: // TODO: Spell checker controls.
TODOs want names

http://codereview.chromium.org/174043/diff/1/5#newcode225
Line 225: 
extra line.

http://codereview.chromium.org/174043/diff/1/5#newcode277
Line 277: NOTREACHED();
if nothing is selected, will |list| be NULL or just empty?

http://codereview.chromium.org/174043/diff/1/5#newcode285
Line 285: LOG(INFO) << "kAcceptLanguages: " <<
WideToASCII(accept_languages_.GetValue()).c_str();
debugging line?

http://codereview.chromium.org/174043/diff/1/5#newcode289
Line 289: // XXX
?

http://codereview.chromium.org/174043/diff/1/6
File chrome/browser/gtk/options/languages_page_gtk.h (right):

http://codereview.chromium.org/174043/diff/1/6#newcode5
Line 5: // The languages page of the Languages & languages options dialog, which
contains
80cols

http://codereview.chromium.org/174043/diff/1/6#newcode57
Line 57: // Callbacks XXX
?

[mattm, 2009-08-19 04:42:42]

Updated.  Sorry about the cruft.

http://codereview.chromium.org/174043/diff/1/5
File chrome/browser/gtk/options/languages_page_gtk.cc (right):

http://codereview.chromium.org/174043/diff/1/5#newcode88
Line 88: gtk_widget_show_all(dialog_);
On 2009/08/19 04:21:19, Evan Stade wrote:
> can you make this the last line of the function? I'm sure it doesn't matter
but
> it would feel nicer

Done.

http://codereview.chromium.org/174043/diff/1/5#newcode131
Line 131: 
On 2009/08/19 04:21:19, Evan Stade wrote:
> delete blank line

Done.

http://codereview.chromium.org/174043/diff/1/5#newcode224
Line 224: // TODO: Spell checker controls.
On 2009/08/19 04:21:19, Evan Stade wrote:
> TODOs want names

Done.

http://codereview.chromium.org/174043/diff/1/5#newcode225
Line 225: 
On 2009/08/19 04:21:19, Evan Stade wrote:
> extra line.

Done.

http://codereview.chromium.org/174043/diff/1/5#newcode277
Line 277: NOTREACHED();
On 2009/08/19 04:21:19, Evan Stade wrote:
> if nothing is selected, will |list| be NULL or just empty?

Done.

http://codereview.chromium.org/174043/diff/1/5#newcode285
Line 285: LOG(INFO) << "kAcceptLanguages: " <<
WideToASCII(accept_languages_.GetValue()).c_str();
On 2009/08/19 04:21:19, Evan Stade wrote:
> debugging line?

removed.

http://codereview.chromium.org/174043/diff/1/5#newcode289
Line 289: // XXX
On 2009/08/19 04:21:19, Evan Stade wrote:
> ?

fixed.

http://codereview.chromium.org/174043/diff/1/6
File chrome/browser/gtk/options/languages_page_gtk.h (right):

http://codereview.chromium.org/174043/diff/1/6#newcode5
Line 5: // The languages page of the Languages & languages options dialog, which
contains
On 2009/08/19 04:21:19, Evan Stade wrote:
> 80cols

Done.

http://codereview.chromium.org/174043/diff/1/6#newcode57
Line 57: // Callbacks XXX
On 2009/08/19 04:21:19, Evan Stade wrote:
> ?

Fixed.

[Evan Stade, 2009-08-19 04:44:30]

lgtm!

http://codereview.chromium.org/174043/diff/1014/1019
File chrome/browser/gtk/options/languages_page_gtk.h (right):

http://codereview.chromium.org/174043/diff/1014/1019#newcode51
Line 51: // Get the row number of the first selected row.
or -1 if no row is selected