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

chrome/browser/resources/settings/languages_page/languages.js

Index: chrome/browser/resources/settings/languages_page/languages.js
diff --git a/chrome/browser/resources/settings/languages_page/languages.js b/chrome/browser/resources/settings/languages_page/languages.js
index 790a45fba91d763cbad5c548833d0bc05dae1fdb..50b0c7d4641628c902240121e882d7ae2a4fe0d8 100644
--- a/chrome/browser/resources/settings/languages_page/languages.js
+++ b/chrome/browser/resources/settings/languages_page/languages.js
@@ -121,9 +121,13 @@ Polymer({
       type: Object,
       value: function() { return new Map(); },
     },
+
+    /** @private Prospective UI language when the page was loaded. */
+    originalProspectiveUILanguage_: String,
   },
 
   observers: [
+    'prospectiveUILanguageChanged_(prefs.intl.app_locale.value)',
     'preferredLanguagesPrefChanged_(' +
         'prefs.' + preferredLanguagesPrefName + '.value, languages)',
     'spellCheckDictionariesPrefChanged_(' +
@@ -178,6 +182,16 @@ Polymer({
       }.bind(this));
     }
 
+    if (cr.isWindows || cr.isChromeOS) {
+      // Fetch the starting UI language, which affects which actions should be
+      // enabled.
+      promises.push(cr.sendWithPromise('getProspectiveUILanguage').then(
+          function(prospectiveUILanguage) {
+            this.originalProspectiveUILanguage_ =
+                prospectiveUILanguage || navigator.language;

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

nit: I had to look up 'navigator.language', maybe use window.navigator.language?

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

Done.

+          }.bind(this)));
+    }
+
     Promise.all(promises).then(function(results) {
       this.createModel_(results[1], results[2], results[3], results[4]);
       this.resolver_.resolve();
@@ -190,12 +204,25 @@ Polymer({
   },
 
   /**
+   * Updates the prospective UI language based on the new pref value.
+   * @param {string} prospectiveUILanguage
+   * @private
+   */
+  prospectiveUILanguageChanged_: function(prospectiveUILanguage) {
+    if (!this.languages)
+      return;

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

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?

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

Done.

+
+    this.set('languages.prospectiveUILanguage',
+        prospectiveUILanguage || this.originalProspectiveUILanguage_);
+  },
+
+  /**
    * Updates the list of enabled languages from the preferred languages pref.
    * @private
    */
   preferredLanguagesPrefChanged_: function() {
-    var enabledLanguageStates =
-        this.getEnabledLanguageStates_(this.languages.translateTarget);
+    var enabledLanguageStates = this.getEnabledLanguageStates_(
+        this.languages.translateTarget, this.languages.prospectiveUILanguage);
 
     // Recreate the enabled language set before updating languages.enabled.
     this.enabledLanguageSet_.clear();
@@ -226,8 +253,10 @@ Polymer({
         /** @type {!Array<string>} */(translateBlockedPref.value));
 
     for (var i = 0; i < this.languages.enabled.length; i++) {
-      if (this.languages.enabled[i].language.code == navigator.language)
+      if (this.languages.enabled[i].language.code ==
+          this.languages.prospectiveUILanguage) {
         continue;
+      }
       var translateCode = this.convertLanguageCodeForTranslate(
           this.languages.enabled[i].language.code);
       this.set(
@@ -277,8 +306,16 @@ Polymer({
       }
     }
 
+    var prospectiveUILanguage;
+    if (cr.isChromeOS || cr.isWindows) {
+      prospectiveUILanguage =
+          /** @type {string} */(this.getPref('intl.app_locale').value) ||
+          this.originalProspectiveUILanguage_;
+    }
+
     // Create a list of enabled languages from the supported languages.
-    var enabledLanguageStates = this.getEnabledLanguageStates_(translateTarget);
+    var enabledLanguageStates =
+        this.getEnabledLanguageStates_(translateTarget, prospectiveUILanguage);
     // Populate the hash set of enabled languages.
     for (var languageState of enabledLanguageStates)
       this.enabledLanguageSet_.add(languageState.language.code);
@@ -288,6 +325,10 @@ Polymer({
       enabled: enabledLanguageStates,
       translateTarget: translateTarget,
     });
+
+    if (cr.isChromeOS || cr.isWindows)
+      model.prospectiveUILanguage = prospectiveUILanguage;
+
     if (cr.isChromeOS) {
       model.inputMethods = /** @type {!InputMethodsModel} */({
         supported: supportedInputMethods,
@@ -305,10 +346,12 @@ Polymer({
    * languages list.
    * @param {string} translateTarget Language code of the default translate
    *     target language.
+   * @param {(string|undefined)} prospectiveUILanguage Prospective UI display
+   *     language. Only defined on Windows and Chrome OS.
    * @return {!Array<!LanguageState>}
    * @private
    */
-  getEnabledLanguageStates_: function(translateTarget) {
+  getEnabledLanguageStates_: function(translateTarget, prospectiveUILanguage) {
     assert(CrSettingsPrefs.isInitialized);
 
     var pref = this.getPref(preferredLanguagesPrefName);
@@ -335,7 +378,7 @@ Polymer({
       // language that is blocked.
       var translateCode = this.convertLanguageCodeForTranslate(code);
       languageState.translateEnabled = !!language.supportsTranslate &&
-          code != navigator.language &&
+          code != prospectiveUILanguage &&

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

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.

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

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

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

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.

           !translateBlockedSet.has(translateCode) &&
           translateCode != translateTarget;
       enabledLanguageStates.push(languageState);
@@ -426,26 +469,18 @@ Polymer({
    * the actual UI language until a restart.
    * @param {string} languageCode
    */
-  setUILanguage: function(languageCode) {
+  setProspectiveUILanguage: function(languageCode) {
     assert(cr.isChromeOS || cr.isWindows);
-    chrome.send('setUILanguage', [languageCode]);
-  },
-
-  /** Resets the prospective UI language back to the actual UI language. */
-  resetUILanguage: function() {
-    assert(cr.isChromeOS || cr.isWindows);
-    chrome.send('setUILanguage', [navigator.language]);
+    chrome.send('setProspectiveUILanguage', [languageCode]);
   },
 
   /**
-   * Returns the "prospective" UI language, i.e. the one to be used on next
-   * restart. If the pref is not set, the current UI language is also the
-   * "prospective" language.
-   * @return {string} Language code of the prospective UI language.
+   * True if the prospective UI language was changed from its starting value.
+   * @return {boolean}
    */
-  getProspectiveUILanguage: function() {
-    return /** @type {string} */(this.getPref('intl.app_locale').value) ||
-        navigator.language;
+  requiresRestart: function() {
+    return this.originalProspectiveUILanguage_ !=
+        this.languages.prospectiveUILanguage;
   },
 
   /**
@@ -506,7 +541,7 @@ Polymer({
    */
   canDisableLanguage: function(languageCode) {
     // Cannot disable the prospective UI language.
-    if (languageCode == this.getProspectiveUILanguage())
+    if (languageCode == this.languages.prospectiveUILanguage)
       return false;
 
     // Cannot disable the only enabled language.