Chromium Code Reviews

Issue 3080026: Implement "Display Chrome OS in this language" button. (Closed)

Created:
10 years, 4 months ago by satorux1
Modified:
9 years, 7 months ago
Reviewers:
arv (Not doing code reviews)
CC:
chromium-reviews, dhg, arv (Not doing code reviews), davemoore+watch_chromium.org, ben+cc_chromium.org
Visibility:
Public.

Description

Implement "Display Chrome OS in this language" button. Hopefully, the "Languages and Input" language dialog will be mostly functional with this change, although we need to polish it more. TEST=manually BUG=chromium-os:4573 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=55026

Patch Set 1 #

Total comments: 6

Patch Set 2 : address comments #

Unified diffs Side-by-side diffs Stats (+121 lines, -10 lines)
M chrome/browser/chromeos/dom_ui/language_options_handler.h View 1 chunk +11 lines, -2 lines 0 comments
M chrome/browser/chromeos/dom_ui/language_options_handler.cc View 3 chunks +38 lines, -1 line 0 comments
M chrome/browser/resources/options/chromeos_language_options.js View 4 chunks +67 lines, -7 lines 0 comments
M chrome/browser/resources/options/options_page.css View 1 chunk +5 lines, -0 lines 0 comments

Messages

Total messages: 3 (0 generated)
satorux1
Erik, Thank you for your help, now the "Language and Input" page is mostly functional.
10 years, 4 months ago (2010-08-04 10:57:59 UTC) #1
arv (Not doing code reviews)
LGTM http://codereview.chromium.org/3080026/diff/1/4 File chrome/browser/resources/options/chromeos_language_options.js (right): http://codereview.chromium.org/3080026/diff/1/4#newcode143 chrome/browser/resources/options/chromeos_language_options.js:143: uiLanguageButton.textContent = ( useless parentheses http://codereview.chromium.org/3080026/diff/1/4#newcode147 chrome/browser/resources/options/chromeos_language_options.js:147: uiLanguageButton.onclick ...
10 years, 4 months ago (2010-08-04 17:39:13 UTC) #2
satorux1
10 years, 4 months ago (2010-08-06 00:37:50 UTC) #3 
The change has been checked in but forgot to send this...

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

http://codereview.chromium.org/3080026/diff/1/4#newcode143
chrome/browser/resources/options/chromeos_language_options.js:143:
uiLanguageButton.textContent = (
On 2010/08/04 17:39:13, arv wrote:
> useless parentheses

Done.

http://codereview.chromium.org/3080026/diff/1/4#newcode147
chrome/browser/resources/options/chromeos_language_options.js:147:
uiLanguageButton.onclick = function(e) {}
On 2010/08/04 17:39:13, arv wrote:
> noop

Done. Changed it to:

        // Remove the event listner.                                            
        uiLanguageButton.onclick = undefined;

Not sure if this is a preferable way. I'll change it if you have a better idea
in a separate patch.

http://codereview.chromium.org/3080026/diff/1/4#newcode351
chrome/browser/resources/options/chromeos_language_options.js:351:
console.log(localStrings.getString('restart_required'));
On 2010/08/04 17:39:13, arv wrote:
> I would at least do alert since that is user visible.

Done.

Powered by Google App Engine