Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(505)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 6150003: DOMUI: Implement the i18n-options attribute that allows the client to load (Closed)

Created:
9 years, 11 months ago by James Hawkins
Modified:
9 years, 7 months ago
CC:
chromium-reviews, arv (Not doing code reviews)
Visibility:
Public.

Description

DOMUI: Implement the i18n-options attribute that allows the client to load select options directly from the C++ handler. BUG=none TEST=none Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=70812

Patch Set 1 #

Total comments: 2

Patch Set 2 : Review fix. #

Total comments: 2

Patch Set 3 : Review fix 2. #

Total comments: 2

Patch Set 4 : Nit fix. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+57 lines, -95 lines) Patch
M chrome/browser/resources/options/chromeos_language_chewing_options.html View 4 chunks +4 lines, -4 lines 0 comments Download
M chrome/browser/resources/options/chromeos_language_customize_modifier_keys_overlay.html View 3 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/resources/options/chromeos_language_hangul_options.html View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/resources/options/chromeos_language_mozc_options.html View 6 chunks +17 lines, -17 lines 0 comments Download
M chrome/browser/resources/options/chromeos_language_pinyin_options.html View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/resources/options/chromeos_system_options.html View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/resources/options/chromeos_system_options.js View 1 chunk +0 lines, -2 lines 0 comments Download
M chrome/browser/resources/options/clear_browser_data_overlay.html View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/resources/options/clear_browser_data_overlay.js View 1 chunk +0 lines, -4 lines 0 comments Download
M chrome/browser/resources/options/font_settings.html View 5 chunks +12 lines, -3 lines 0 comments Download
M chrome/browser/resources/options/font_settings.js View 2 chunks +3 lines, -21 lines 0 comments Download
M chrome/browser/resources/options/font_settings_ui.js View 1 chunk +0 lines, -14 lines 0 comments Download
M chrome/browser/resources/options/personal_options.html View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/resources/options/personal_options.js View 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/resources/options/pref_ui.js View 2 chunks +0 lines, -23 lines 0 comments Download
M chrome/browser/resources/shared/js/i18n_template.js View 1 2 3 1 chunk +12 lines, -0 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
James Hawkins
9 years, 11 months ago (2011-01-07 21:58:06 UTC) #1
arv (Not doing code reviews)
http://codereview.chromium.org/6150003/diff/1/chrome/browser/resources/shared/js/i18n_template.js File chrome/browser/resources/shared/js/i18n_template.js (right): http://codereview.chromium.org/6150003/diff/1/chrome/browser/resources/shared/js/i18n_template.js#newcode47 chrome/browser/resources/shared/js/i18n_template.js:47: element.appendChild(new Option(values[1], values[0])); I think we should support an ...
9 years, 11 months ago (2011-01-07 22:24:16 UTC) #2
James Hawkins
http://codereview.chromium.org/6150003/diff/1/chrome/browser/resources/shared/js/i18n_template.js File chrome/browser/resources/shared/js/i18n_template.js (right): http://codereview.chromium.org/6150003/diff/1/chrome/browser/resources/shared/js/i18n_template.js#newcode47 chrome/browser/resources/shared/js/i18n_template.js:47: element.appendChild(new Option(values[1], values[0])); On 2011/01/07 22:24:16, arv wrote: > ...
9 years, 11 months ago (2011-01-07 22:41:15 UTC) #3
arv (Not doing code reviews)
http://codereview.chromium.org/6150003/diff/5001/chrome/browser/resources/shared/js/i18n_template.js File chrome/browser/resources/shared/js/i18n_template.js (right): http://codereview.chromium.org/6150003/diff/5001/chrome/browser/resources/shared/js/i18n_template.js#newcode50 chrome/browser/resources/shared/js/i18n_template.js:50: element.appendChild(new Option(values[1], values[0])); Use a var or ?:
9 years, 11 months ago (2011-01-07 23:54:09 UTC) #4
James Hawkins
http://codereview.chromium.org/6150003/diff/5001/chrome/browser/resources/shared/js/i18n_template.js File chrome/browser/resources/shared/js/i18n_template.js (right): http://codereview.chromium.org/6150003/diff/5001/chrome/browser/resources/shared/js/i18n_template.js#newcode50 chrome/browser/resources/shared/js/i18n_template.js:50: element.appendChild(new Option(values[1], values[0])); On 2011/01/07 23:54:09, arv wrote: > ...
9 years, 11 months ago (2011-01-08 00:02:51 UTC) #5
arv (Not doing code reviews)
LGTM with nit fixed http://codereview.chromium.org/6150003/diff/9001/chrome/browser/resources/shared/js/i18n_template.js File chrome/browser/resources/shared/js/i18n_template.js (right): http://codereview.chromium.org/6150003/diff/9001/chrome/browser/resources/shared/js/i18n_template.js#newcode47 chrome/browser/resources/shared/js/i18n_template.js:47: typeof values == 'string' ? ...
9 years, 11 months ago (2011-01-08 00:08:53 UTC) #6
James Hawkins
9 years, 11 months ago (2011-01-08 00:15:51 UTC) #7 
http://codereview.chromium.org/6150003/diff/9001/chrome/browser/resources/sha...
File chrome/browser/resources/shared/js/i18n_template.js (right):

http://codereview.chromium.org/6150003/diff/9001/chrome/browser/resources/sha...
chrome/browser/resources/shared/js/i18n_template.js:47: typeof values ==
'string' ? element.appendChild(new Option(values)) :
On 2011/01/08 00:08:53, arv wrote:
> This was not what I had in mind...
> 
> Either:
> 
> var option;
> if (typeof values == 'string')
>   option = new Option(values);
> else
>   option = new Option(values[1], values[0]);
> element.appendChild(option);
> 
> or
> 
> element.appendChild(typeof values == 'string' ?
>     new Option(values) : new Option(values[1], values[0]))

Done.

Powered by Google App Engine
This is Rietveld 408576698