Language settings: Fix Restart and Remove buttons shown at wrong times

Language settings: Fix Restart and Remove buttons shown at wrong times

Language settings makes the faulty assumption that the app locale
(navigator.language) will match the the preferred app locale (intl.app_locale).

For some languages, we map a language code to a different locale. For example,
en-ZA is mapped to en-GB when getting the application locale, so the preference
won't match the actual locale used. See l10n_util::CheckAndResolveLocale for
other wacky examples.

Stop using navigator.language when the intl.app_locale pref is available. Also
move more logic into the model and stop checking intl.app_locale in the UI.
Use a more defensive check for the Restart button: show if the locale preference
has changed from its initial value.

BUG=561227, 649588, 633661
R=stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/4cb2126002cd187035ba90ee13ebda77e16d81f6
Cr-Commit-Position: refs/heads/master@{#438403}

[michaelpg, 2016-12-13 05:04:52]

Description was changed from

==========
Language settings: Fix Restart and Remove buttons shown at wrong times

Language settings makes the faulty assumption that the app locale
(navigator.language) will match the the preferred app locale (intl.app_locale).

For some languages, we map a language code to a different locale. For example,
en-ZA is mapped to en-GB when getting the application locale, so the preference
won't match the actual locale used. See l10n_util::CheckAndResolveLocale for
other wacky examples.

Stop using navigator.language when the intl.app_locale pref is available. Also
move more logic into the model and stop checkingc intl.app_locale in the UI.
Use a more defensive check for the Restart button: show if the locale preference
has changed from its initial value.

BUG=561227,649588,633661
R=stevenjb@chromium.org
==========

to

==========
Language settings: Fix Restart and Remove buttons shown at wrong times

Language settings makes the faulty assumption that the app locale
(navigator.language) will match the the preferred app locale (intl.app_locale).

For some languages, we map a language code to a different locale. For example,
en-ZA is mapped to en-GB when getting the application locale, so the preference
won't match the actual locale used. See l10n_util::CheckAndResolveLocale for
other wacky examples.

Stop using navigator.language when the intl.app_locale pref is available. Also
move more logic into the model and stop checkingc intl.app_locale in the UI.
Use a more defensive check for the Restart button: show if the locale preference
has changed from its initial value.

BUG=561227,649588,633661
R=stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[michaelpg, 2016-12-13 05:31:14]

Description was changed from

==========
Language settings: Fix Restart and Remove buttons shown at wrong times

Language settings makes the faulty assumption that the app locale
(navigator.language) will match the the preferred app locale (intl.app_locale).

For some languages, we map a language code to a different locale. For example,
en-ZA is mapped to en-GB when getting the application locale, so the preference
won't match the actual locale used. See l10n_util::CheckAndResolveLocale for
other wacky examples.

Stop using navigator.language when the intl.app_locale pref is available. Also
move more logic into the model and stop checkingc intl.app_locale in the UI.
Use a more defensive check for the Restart button: show if the locale preference
has changed from its initial value.

BUG=561227,649588,633661
R=stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Language settings: Fix Restart and Remove buttons shown at wrong times

Language settings makes the faulty assumption that the app locale
(navigator.language) will match the the preferred app locale (intl.app_locale).

For some languages, we map a language code to a different locale. For example,
en-ZA is mapped to en-GB when getting the application locale, so the preference
won't match the actual locale used. See l10n_util::CheckAndResolveLocale for
other wacky examples.

Stop using navigator.language when the intl.app_locale pref is available. Also
move more logic into the model and stop checking intl.app_locale in the UI.
Use a more defensive check for the Restart button: show if the locale preference
has changed from its initial value.

BUG=561227,649588,633661
R=stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[michaelpg, 2016-12-13 05:33:09]

fun one for ya...

https://codereview.chromium.org/2573643005/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/languages_page/languages_page.js (left):

https://codereview.chromium.org/2573643005/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/languages_page/languages_page.js:144: if
(languageState.language.code == navigator.language &&
Simplifying this because it leads to bugs and it's kind of silly to allow you to
uncheck "Show Chrome OS in this language".

[stevenjb, 2016-12-13 18:53:09]

Fun indeed. I won't pretend I completely followed all of this... :)

https://codereview.chromium.org/2573643005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/languages.js (right):

https://codereview.chromium.org/2573643005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages.js:191:
prospectiveUILanguage || navigator.language;
nit: I had to look up 'navigator.language', maybe use window.navigator.language?

https://codereview.chromium.org/2573643005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages.js:213: return;
Maybe make 'languages' an argument to this observer method so that a) we don't
need this check, and b) this gets called if this.languages gets set after
prospectiveUILanguage?

https://codereview.chromium.org/2573643005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages.js:381: code !=
prospectiveUILanguage &&
This will be undefined on osx, right? Won't this always be true then? Maybe
that's OK, but I think it would be more clear if we made that explicit on non
win/cros.

https://codereview.chromium.org/2573643005/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/languages_handler.cc (right):

https://codereview.chromium.org/2573643005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/languages_handler.cc:59: locale = locale.size()
? locale :
if (locale.empty()) locale = ...

[michaelpg, 2016-12-14 01:13:27]

Well, hopefully it's easier to follow now than it was before, not harder..

https://codereview.chromium.org/2573643005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/languages.js (right):

https://codereview.chromium.org/2573643005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages.js:191:
prospectiveUILanguage || navigator.language;
On 2016/12/13 18:53:09, stevenjb wrote:
> nit: I had to look up 'navigator.language', maybe use
window.navigator.language?

Done.

https://codereview.chromium.org/2573643005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages.js:213: return;
On 2016/12/13 18:53:09, stevenjb wrote:
> Maybe make 'languages' an argument to this observer method so that a) we don't
> need this check, and b) this gets called if this.languages gets set after
> prospectiveUILanguage?

Done.

https://codereview.chromium.org/2573643005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages.js:381: code !=
prospectiveUILanguage &&
On 2016/12/13 18:53:09, stevenjb wrote:
> This will be undefined on osx, right? Won't this always be true then? Maybe
> that's OK, but I think it would be more clear if we made that explicit on non
> win/cros.
> 

Added !prospectiveUILanguage. There's already more cr.isWindows/cr.isChromeOS in
the code than I'd like...

https://codereview.chromium.org/2573643005/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/languages_handler.cc (right):

https://codereview.chromium.org/2573643005/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/languages_handler.cc:59: locale = locale.size()
? locale :
On 2016/12/13 18:53:09, stevenjb wrote:
> if (locale.empty()) locale = ...

Done.

[stevenjb, 2016-12-14 01:19:50]

It's definitely less unclear :) The changes look good, it's just a complex
system is all. I *think* this is robust, I'm just not promising that I didn't
miss anything :)

LGTM

https://codereview.chromium.org/2573643005/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/languages_page/languages.js (right):

https://codereview.chromium.org/2573643005/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/languages_page/languages.js:381: code !=
prospectiveUILanguage &&
On 2016/12/14 01:13:27, michaelpg wrote:
> On 2016/12/13 18:53:09, stevenjb wrote:
> > This will be undefined on osx, right? Won't this always be true then? Maybe
> > that's OK, but I think it would be more clear if we made that explicit on
non
> > win/cros.
> > 
> 
> Added !prospectiveUILanguage. There's already more cr.isWindows/cr.isChromeOS
in
> the code than I'd like...

Understood. It just wasn't clear that on platforms where prospectiveUILanguage
is undefined that this clause is expected to be true. It is more clear now.

[michaelpg, 2016-12-14 01:27:13]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2016-12-14 01:27:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 02:46:56]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481678833113220,
"parent_rev": "b08f062c195683b9afd68efeeac309f837e21db9", "commit_rev":
"c23bb5932f4a68fa7a7c887c1bcfe032ed3461ab"}

[commit-bot: I haz the power, 2016-12-14 02:47:23]

Description was changed from

==========
Language settings: Fix Restart and Remove buttons shown at wrong times

Language settings makes the faulty assumption that the app locale
(navigator.language) will match the the preferred app locale (intl.app_locale).

For some languages, we map a language code to a different locale. For example,
en-ZA is mapped to en-GB when getting the application locale, so the preference
won't match the actual locale used. See l10n_util::CheckAndResolveLocale for
other wacky examples.

Stop using navigator.language when the intl.app_locale pref is available. Also
move more logic into the model and stop checking intl.app_locale in the UI.
Use a more defensive check for the Restart button: show if the locale preference
has changed from its initial value.

BUG=561227,649588,633661
R=stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Language settings: Fix Restart and Remove buttons shown at wrong times

Language settings makes the faulty assumption that the app locale
(navigator.language) will match the the preferred app locale (intl.app_locale).

For some languages, we map a language code to a different locale. For example,
en-ZA is mapped to en-GB when getting the application locale, so the preference
won't match the actual locale used. See l10n_util::CheckAndResolveLocale for
other wacky examples.

Stop using navigator.language when the intl.app_locale pref is available. Also
move more logic into the model and stop checking intl.app_locale in the UI.
Use a more defensive check for the Restart button: show if the locale preference
has changed from its initial value.

BUG=561227,649588,633661
R=stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2573643005
==========

[commit-bot: I haz the power, 2016-12-14 02:47:24]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-14 02:50:22]

Description was changed from

==========
Language settings: Fix Restart and Remove buttons shown at wrong times

Language settings makes the faulty assumption that the app locale
(navigator.language) will match the the preferred app locale (intl.app_locale).

For some languages, we map a language code to a different locale. For example,
en-ZA is mapped to en-GB when getting the application locale, so the preference
won't match the actual locale used. See l10n_util::CheckAndResolveLocale for
other wacky examples.

Stop using navigator.language when the intl.app_locale pref is available. Also
move more logic into the model and stop checking intl.app_locale in the UI.
Use a more defensive check for the Restart button: show if the locale preference
has changed from its initial value.

BUG=561227,649588,633661
R=stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2573643005
==========

to

==========
Language settings: Fix Restart and Remove buttons shown at wrong times

Language settings makes the faulty assumption that the app locale
(navigator.language) will match the the preferred app locale (intl.app_locale).

For some languages, we map a language code to a different locale. For example,
en-ZA is mapped to en-GB when getting the application locale, so the preference
won't match the actual locale used. See l10n_util::CheckAndResolveLocale for
other wacky examples.

Stop using navigator.language when the intl.app_locale pref is available. Also
move more logic into the model and stop checking intl.app_locale in the UI.
Use a more defensive check for the Restart button: show if the locale preference
has changed from its initial value.

BUG=561227,649588,633661
R=stevenjb@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/4cb2126002cd187035ba90ee13ebda77e16d81f6
Cr-Commit-Position: refs/heads/master@{#438403}
==========

[commit-bot: I haz the power, 2016-12-14 02:50:23]

Patchset 3 (id:??) landed as
https://crrev.com/4cb2126002cd187035ba90ee13ebda77e16d81f6
Cr-Commit-Position: refs/heads/master@{#438403}