Make UI language change work on chromeos.

Make UI language change work on chromeos.

The left hand side value can't be string literal.

BUG=chromium:130715
TEST=manual


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

[kochi, 2012-06-01 07:56:28]

Hi Evan, Dan,

Could you review this?
UI language change was broken since r137067.
http://src.chromium.org/viewvc/chrome?view=rev&revision=137067

The core issue was that left hand side of the assignment
was a string.  So introduced a local variable to hold the state.

Another issue was when the restart button is clicked,
chrome.send.bind(chrome, 'uiLanguageRestart') ends up in
NOTREACHED in cpp_variant.cc:246.  It looks like
it is converting non-existing(?) 1st argument into a vector
and failed.

I'm not sure about what chrome.send.bind() is intended for,
so including Dan who made the change.

[Evan Stade, 2012-06-01 17:21:16]

sorry for breakage. LGTM

https://chromiumcodereview.appspot.com/10442128/diff/1/chrome/browser/resourc...
File chrome/browser/resources/options2/language_options.js (left):

https://chromiumcodereview.appspot.com/10442128/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/language_options.js:116:
chrome.send.bind(chrome, 'uiLanguageRestart');
is this change necessary/related? I don't really care one way or another, just
wondering.

[Dan Beam, 2012-06-01 17:45:13]

https://chromiumcodereview.appspot.com/10442128/diff/1/chrome/browser/resourc...
File chrome/browser/resources/options2/language_options.js (right):

https://chromiumcodereview.appspot.com/10442128/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/language_options.js:43: // For recording the
prospective language (the next locale after relaunch).
/**
 * For recording the prospective language (the next locale after relaunch).
 * @type {?string}
 * @private
 */

https://chromiumcodereview.appspot.com/10442128/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/language_options.js:120:
$('language-options-ui-restart-button').onclick =
you don't need to wrap this

[kochi, 2012-06-02 22:39:14]

Thanks for the reviews!

http://chromiumcodereview.appspot.com/10442128/diff/1/chrome/browser/resource...
File chrome/browser/resources/options2/language_options.js (left):

http://chromiumcodereview.appspot.com/10442128/diff/1/chrome/browser/resource...
chrome/browser/resources/options2/language_options.js:116:
chrome.send.bind(chrome, 'uiLanguageRestart');
On 2012/06/01 17:21:17, Evan Stade wrote:
> is this change necessary/related? I don't really care one way or another, just
> wondering.

This line caused a NOTREACHED(). - so this also fixes the crash on changing
UI language.

http://chromiumcodereview.appspot.com/10442128/diff/1/chrome/browser/resource...
File chrome/browser/resources/options2/language_options.js (right):

http://chromiumcodereview.appspot.com/10442128/diff/1/chrome/browser/resource...
chrome/browser/resources/options2/language_options.js:120:
$('language-options-ui-restart-button').onclick =
On 2012/06/01 17:45:13, Dan Beam wrote:
> you don't need to wrap this

Done.

[Dan Beam, 2012-06-12 01:28:28]

http://codereview.chromium.org/10442128/diff/7001/chrome/browser/resources/op...
File chrome/browser/resources/options2/language_options.js (right):

http://codereview.chromium.org/10442128/diff/7001/chrome/browser/resources/op...
chrome/browser/resources/options2/language_options.js:124:
chrome.send('uiLanguageRestart');
this calls the function, doesn't return a reference to it, revert back to bind
or make an anonymous function expression like you did before (i.e. onclick =
function(){ chrome.send() };).

[kochi, 2012-06-12 06:02:25]

Hi Dan, thanks for the comments.
(I was also OOO last week).

I'm still not sure why chrome.send.bind() cause CHECK
condition, but anyway this works and fixes the issue.

I'll investigate this later.
https://code.google.com/p/chromium/issues/detail?id=132226

http://codereview.chromium.org/10442128/diff/7001/chrome/browser/resources/op...
File chrome/browser/resources/options2/language_options.js (right):

http://codereview.chromium.org/10442128/diff/7001/chrome/browser/resources/op...
chrome/browser/resources/options2/language_options.js:124:
chrome.send('uiLanguageRestart');
Oh, I hope I understand you this time:)

When I tried chrome.send.bind(), it failed with CHECK on
chromeos=1 build on Linux, although it seem to work on
Release build on Chrome OS.  I haven't taken a look into
the details, but avoiding chrome.send.bind() is a
workaround for it.

I'll look into what's happening behind.  I'd like to put this
fix in first.

On 2012/06/12 01:28:28, Dan Beam wrote:
> this calls the function, doesn't return a reference to it, revert back to bind
> or make an anonymous function expression like you did before (i.e. onclick =
> function(){ chrome.send() };).

[Dan Beam, 2012-06-12 06:09:38]

lgtm

my only thoughts to why this wasn't working would be that the "chrome" object
doesn't exist yet (and is bound later).

[commit-bot: I haz the power, 2012-06-12 06:25:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kochi@chromium.org/10442128/11001

[commit-bot: I haz the power, 2012-06-12 08:46:25]

Change committed as 141632