Extract language settings methods into a LanguageHelper interface

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

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 /**
  * @fileoverview 'settings-languages' provides convenient access to
  * Chrome's language and input method settings.
  *
  * Instances of this element have a 'languages' property, which reflects the
  * current language settings. The 'languages' property is read-only, meaning
  * hosts using this element cannot change it directly. Instead, changes to
- * language settings should be made using this element's public functions.
+ * language settings should be made using the settings.LanguageHelper singleton.
  *
- * Use two-way binding syntax to propagate changes from child to host, so that
+ * Use upward binding syntax to propagate changes from child to host, so that
  * changes made internally to 'languages' propagate to your host element:
  *
  *     <template>
  *       <settings-languages languages="{{languages}}">
  *       </settings-languages>
  *       <div>[[languages.someProperty]]</div>
  *     </template>
  *
  * @group Chrome Settings Elements
  * @element settings-languages
  */
 
-/** @typedef {{spellCheckEnabled: boolean, translateEnabled: boolean}} */
-var LanguageState;
-
-/**
- * @typedef {{language: !chrome.languageSettingsPrivate.Language,
- *            state: !LanguageState}}
- */
-var LanguageInfo;
-
-/**
- * supportedLanguages: an array of languages, ordered alphabetically.
- * enabledLanguages: an array of enabled language info and state, ordered by
- *     preference.
- * @typedef {{
- *      supportedLanguages: !Array<!chrome.languageSettingsPrivate.Language>,
- *      enabledLanguages: !Array<!LanguageInfo>,
- *      translateTarget: string
- *  }}
- */
-var LanguagesModel;
+cr.exportPath('settings');

[michaelpg, 2015/11/13 05:30:17]

for closure

 
 (function() {
 'use strict';
 
 // Translate server treats some language codes the same.
 // See also: components/translate/core/common/translate_util.cc.
 var kLanguageCodeToTranslateCode = {
   'nb': 'no',
   'fil': 'tl',
   'zh-HK': 'zh-TW',
   'zh-MO': 'zh-TW',
   'zh-SG': 'zh-CN',
 };
 
 // Some ISO 639 language codes have been renamed, e.g. "he" to "iw", but
 // Translate still uses the old versions. TODO(michaelpg): Chrome does too.
 // Follow up with Translate owners to understand the right thing to do.
 var kTranslateLanguageSynonyms = {
   'he': 'iw',
   'jv': 'jw',
 };
 
-/**
- * This element has a reference to the singleton, exposing the singleton's
- * language model to the host of this element as the 'languages' property.
- */
-Polymer({
-  is: 'settings-languages',
-
-  properties: {
-    /**
-     * Singleton element created at startup which provides the languages model.
-     * @type {!Element}
-     */
-    singleton_: {
-      type: Object,
-      value: document.createElement('settings-languages-singleton'),
-    },
-
-    /**
-     * A reference to the languages model from the singleton, exposed as a
-     * read-only property so hosts can bind to it, but not change it.
-     * @type {LanguagesModel|undefined}
-     */
-    languages: {
-      type: Object,
-      notify: true,
-      readOnly: true,
-    },
-  },
-
-  ready: function() {
-    // Set the 'languages' property to reference the singleton's model.
-    this._setLanguages(this.singleton_.languages);
-    // Listen for changes to the singleton's languages property, so we know
-    // when to notify hosts of changes to (our reference to) the property.
-    this.listen(
-        this.singleton_, 'languages-changed', 'singletonLanguagesChanged_');
-  },
-
-  /**
-   * Takes changes reported by the singleton and forwards them to the host,
-   * manually sending a change notification for our 'languages' property (since
-   * it's the same object as the singleton's property, but isn't bound by
-   * Polymer).
-   * @private
-   */
-  singletonLanguagesChanged_: function(e) {
-    // Forward the change notification to the host.
-    this.fire(e.type, e.detail, {bubbles: false});
-  },
-
-  // Forward public methods to the singleton.
-
-  /** @param {string} languageCode */
-  setUILanguage: function(languageCode) {
-    if (cr.isWindows || cr.isChromeOS)
-      this.singleton_.setUILanguage(languageCode);
-  },
-
-  resetUILanguage: function() {
-    if (cr.isWindows || cr.isChromeOS)
-      this.singleton_.resetUILanguage();
-  },
-
-  /** @return {string} */
-  getProspectiveUILanguage: function() {
-    return this.singleton_.getProspectiveUILanguage();
-  },
-
-  /** @param {string} languageCode */
-  enableLanguage: function(languageCode) {
-    this.singleton_.enableLanguage(languageCode);
-  },
-
-  /** @param {string} languageCode */
-  disableLanguage: function(languageCode) {
-    this.singleton_.disableLanguage(languageCode);
-  },
-
-  /** @param {string} languageCode */
-  enableTranslateLanguage: function(languageCode) {
-    this.singleton_.enableTranslateLanguage(languageCode);
-  },
-
-  /** @param {string} languageCode */
-  disableTranslateLanguage: function(languageCode) {
-    this.singleton_.disableTranslateLanguage(languageCode);
-  },
-
-  /**
-   * @param {string} languageCode
-   * @return {boolean}
-   */
-  isEnabled: function(languageCode) {
-    return this.singleton_.isEnabled(languageCode);
-  },
-
-  /**
-   * @param {string} languageCode
-   * @param {boolean} enable
-   */
-  toggleSpellCheck: function(languageCode, enable) {
-    this.singleton_.toggleSpellCheck(languageCode, enable);
-  },
-
-  /**
-   * @param {string} languageCode
-   * @return {string}
-   */
-  convertLanguageCodeForTranslate: function(languageCode) {
-    return this.singleton_.convertLanguageCodeForTranslate(languageCode);
-  },
-});
-
 var preferredLanguagesPrefName = cr.isChromeOS ?
     'settings.language.preferred_languages' : 'intl.accept_languages';
 
 /**
- * Singleton element created when settings-languages is registered.
- * Generates the languages model on start-up, and updates it whenever Chrome's
- * pref store and other settings change. These updates propagate to each
- * <settings-language> instance so that their 'languages' property updates
- * like any other Polymer property.
+ * Singleton element that generates the languages model on start-up and
+ * updates it whenever Chrome's pref store and other settings change. These
+ * updates propagate to each <settings-language> instance so that their
+ * 'languages' property updates like any other Polymer property.
+ * @implements {LanguageSettingsHelper}
  */
-Polymer({
+var SettingsLanguagesSingletonElement = Polymer({
   is: 'settings-languages-singleton',
 
   behaviors: [PrefsBehavior],
 
   properties: {
     /**
      * @type {LanguagesModel|undefined}
      */
     languages: {
       type: Object,
@@ skipping 57 matching lines @@
               resolve();
             });
       }),
     ]).then(function() {
       this.createModel_(languageList, translateTarget);
       this.initialized_ = true;
     }.bind(this));
   },
 
   /**
+   * Updates the list of enabled languages from the preferred languages pref.
+   * @private
+   */
+  preferredLanguagesPrefChanged_: function() {
+    if (!this.initialized_)
+      return;
+    var enabledLanguages =
+        this.getEnabledLanguages_(this.languages.translateTarget);
+
+    // Reset the enabled language map before updating
+    // languages.enabledLanguages.
+    this.enabledLanguageMap_ = {};
+    for (var i = 0; i < enabledLanguages.length; i++) {
+      var languageInfo = enabledLanguages[i];
+      this.enabledLanguageMap_[languageInfo.language.code] = languageInfo;
+    }
+    this.set('languages.enabledLanguages', enabledLanguages);
+  },
+
+  /**
+   * Updates the spellCheckEnabled state of each enabled language.
+   * @private
+   */
+  spellCheckDictionariesPrefChanged_: function() {
+    if (!this.initialized_)
+      return;
+
+    var spellCheckMap = this.makeMapFromArray_(/** @type {!Array<string>} */(
+        this.getPref('spellcheck.dictionaries').value));
+    for (var i = 0; i < this.languages.enabledLanguages.length; i++) {
+      var languageCode = this.languages.enabledLanguages[i].language.code;
+      this.set('languages.enabledLanguages.' + i + '.state.spellCheckEnabled',
+               !!spellCheckMap[languageCode]);
+    }
+  },
+
+  /** @private */
+  translateLanguagesPrefChanged_: function() {
+    if (!this.initialized_)
+      return;
+
+    var translateBlockedPref = this.getPref('translate_blocked_languages');
+    var translateBlockedMap = this.makeMapFromArray_(
+        /** @type {!Array<string>} */(translateBlockedPref.value));
+
+    for (var i = 0; i < this.languages.enabledLanguages.length; i++) {
+      var translateCode = this.convertLanguageCodeForTranslate(
+          this.languages.enabledLanguages[i].language.code);
+      this.set(
+          'languages.enabledLanguages.' + i + '.state.translateEnabled',
+          !translateBlockedMap[translateCode]);
+    }
+  },
+
+  /**
    * Constructs the languages model.
    * @param {!Array<!chrome.languageSettingsPrivate.Language>}
    *     supportedLanguages
    * @param {string} translateTarget Language code of the default translate
    *     target language.
+   * @private
    */
   createModel_: function(supportedLanguages, translateTarget) {
     // Populate the hash map of supported languages.
     for (var i = 0; i < supportedLanguages.length; i++) {
       var language = supportedLanguages[i];
       language.supportsUI = !!language.supportsUI;
       language.supportsTranslate = !!language.supportsTranslate;
       language.supportsSpellcheck = !!language.supportsSpellcheck;
       this.supportedLanguageMap_[language.code] = language;
     }
@@ skipping 33 matching lines @@
     var spellCheckMap = this.makeMapFromArray_(/** @type {!Array<string>} */(
         spellCheckPref.value));
 
     var translateBlockedPref = this.getPref('translate_blocked_languages');
     var translateBlockedMap = this.makeMapFromArray_(
         /** @type {!Array<string>} */(translateBlockedPref.value));
 
     for (var i = 0; i < enabledLanguageCodes.length; i++) {
       var code = enabledLanguageCodes[i];
       var language = this.supportedLanguageMap_[code];
+      // Skip unsupported languages.
       if (!language)
         continue;
-      var state = {};
+      var state = /** @type {LanguageState} */({});
       state.spellCheckEnabled = !!spellCheckMap[code];
       // Translate is considered disabled if this language maps to any translate
       // language that is blocked.
       var translateCode = this.convertLanguageCodeForTranslate(code);
-      state.translateEnabled = language.supportsTranslate &&
+      state.translateEnabled = !!language.supportsTranslate &&
           !translateBlockedMap[translateCode] &&
           translateCode != translateTarget;
-      enabledLanguages.push({language: language, state: state});
+      enabledLanguages.push(/** @type {LanguageInfo} */(
+          {language: language, state: state}));
     }
     return enabledLanguages;
   },
 
   /**
    * Creates an object whose keys are the elements of the list.
    * @param {!Array<string>} list
    * @return {!Object<boolean>}
+   * @private
    */
   makeMapFromArray_: function(list) {
     var map = {};
     for (var i = 0; i < list.length; i++)
       map[list[i]] = true;
     return map;
   },
 
-  /**
-   * Updates the list of enabled languages from the preferred languages pref.
-   * @private
-   * */
-  preferredLanguagesPrefChanged_: function() {
-    if (!this.initialized_)
-      return;
-
-    var enabledLanguages =
-        this.getEnabledLanguages_(this.languages.translateTarget);
-    // Reset the enabled language map. Do this before notifying of the change
-    // via languages.enabledLanguages.
-    this.enabledLanguageMap_ = {};
-    for (var i = 0; i < enabledLanguages.length; i++) {
-      var languageInfo = enabledLanguages[i];
-      this.enabledLanguageMap_[languageInfo.language.code] = languageInfo;
-    }
-    this.set('languages.enabledLanguages', enabledLanguages);
-  },
-
-  /**
-   * Updates the spellCheckEnabled state of each enabled language.
-   * @private
-   */
-  spellCheckDictionariesPrefChanged_: function() {
-    if (!this.initialized_)
-      return;
-
-    var spellCheckMap = this.makeMapFromArray_(/** @type {!Array<string>} */(
-        this.getPref('spellcheck.dictionaries').value));
-    for (var i = 0; i < this.languages.enabledLanguages.length; i++) {
-      var languageCode = this.languages.enabledLanguages[i].language.code;
-      this.set('languages.enabledLanguages.' + i + '.state.spellCheckEnabled',
-               !!spellCheckMap[languageCode]);
-    }
-  },
-
-  translateLanguagesPrefChanged_: function() {
-    if (!this.initialized_)
-      return;
-
-    var translateBlockedPref = this.getPref('translate_blocked_languages');
-    var translateBlockedMap = this.makeMapFromArray_(
-        /** @type {!Array<string>} */(translateBlockedPref.value));
-
-    for (var i = 0; i < this.languages.enabledLanguages.length; i++) {
-      var translateCode = this.convertLanguageCodeForTranslate(
-          this.languages.enabledLanguages[i].language.code);
-      this.set(
-          'languages.enabledLanguages.' + i + '.state.translateEnabled',
-          !translateBlockedMap[translateCode]);
-    }
-  },
-
-  /**
-   * Deletes the given item from the pref at the given key if the item is found.
-   * Asserts if the pref itself is not found or is not an Array type.
-   * @param {string} key
-   * @param {*} item
-   */
-  deletePrefItem_: function(key, item) {
-    assert(this.getPref(key).type == chrome.settingsPrivate.PrefType.LIST);
-    this.arrayDelete('prefs.' + key + '.value', item);
-  },
+  // LanguageSettingsHelper implementation.
+  // TODO(michaelpg): replace duplicate docs with @override once b/24294625
+  // is fixed.
 
   /**
    * Windows and Chrome OS only: Sets the prospective UI language to the chosen
    * language. This dosen't affect the actual UI language until a restart.
    * @param {string} languageCode
    */
   setUILanguage: function(languageCode) {
+    assert(cr.isWindows || cr.isChromeOS);
     chrome.send('setUILanguage', [languageCode]);
   },
 
   /**
    * Windows and Chrome OS only: Resets the prospective UI language back to the
    * actual UI language.
    */
   resetUILanguage: function() {
+    assert(cr.isWindows || cr.isChromeOS);
     chrome.send('setUILanguage', [navigator.language]);
   },
 
   /**
    * 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.
-   * @private
    */
   getProspectiveUILanguage: function() {
     return /** @type {string} */(this.getPref('intl.app_locale').value) ||
         navigator.language;
   },
 
   /**
+   * @param {string} languageCode
+   * @return {boolean} True if the language is enabled.
+   */
+  isLanguageEnabled: function(languageCode) {
+    return !!this.enabledLanguageMap_[languageCode];
+  },
+
+  /**
    * Enables the language, making it available for spell check and input.
    * @param {string} languageCode
    */
   enableLanguage: function(languageCode) {
     if (!CrSettingsPrefs.isInitialized)
       return;
 
     var languageCodes =
         this.getPref(preferredLanguagesPrefName).value.split(',');
     if (languageCodes.indexOf(languageCode) > -1)
       return;
     languageCodes.push(languageCode);
     chrome.languageSettingsPrivate.setLanguageList(languageCodes);
     this.disableTranslateLanguage(languageCode);
   },
 
   /**
    * Disables the language.
    * @param {string} languageCode
    */
   disableLanguage: function(languageCode) {
     if (!CrSettingsPrefs.isInitialized)
       return;
 
-    // Cannot disable the UI language.
-    assert(languageCode != this.getProspectiveUILanguage());
+    assert(this.canDisableLanguage(languageCode));
 
-    // Cannot disable the only enabled language.
+    // Remove the language from spell check.
+    this.arrayDelete('prefs.spellcheck.dictionaries.value', languageCode);
+
+    // Remove the language from preferred languages.
     var languageCodes =
         this.getPref(preferredLanguagesPrefName).value.split(',');
-    assert(languageCodes.length > 1);
-
-    // Remove the language from spell check.
-    this.deletePrefItem_('spellcheck.dictionaries', languageCode);
-
     var languageIndex = languageCodes.indexOf(languageCode);
     if (languageIndex == -1)
       return;
     languageCodes.splice(languageIndex, 1);
     chrome.languageSettingsPrivate.setLanguageList(languageCodes);
     this.enableTranslateLanguage(languageCode);
   },
 
   /**
-   * @param {string} languageCode
-   * @return {boolean} True if the language is enabled.
+   * @param {string} languageCode Language code for an enabled language.
+   * @return {boolean}
    */
-  isEnabled: function(languageCode) {
-    return !!this.enabledLanguageMap_[languageCode];
+  canDisableLanguage: function(languageCode) {
+    // Cannot disable the prospective UI language.
+    if (languageCode == this.getProspectiveUILanguage())
+      return false;
+
+    // Cannot disable the only enabled language.
+    if (this.languages.enabledLanguages.length == 1)
+      return false;
+
+    return true;
   },
 
   /**
    * Enables translate for the given language by removing the translate
    * language from the blocked languages preference.
    * @param {string} languageCode
    */
   enableTranslateLanguage: function(languageCode) {
     languageCode = this.convertLanguageCodeForTranslate(languageCode);
     this.arrayDelete('prefs.translate_blocked_languages.value', languageCode);
@@ skipping 29 matching lines @@
       this.arrayDelete('prefs.spellcheck.dictionaries.value', languageCode);
     }
   },
 
   /**
    * Converts the language code for translate. There are some differences
    * between the language set the Translate server uses and that for
    * Accept-Language.
    * @param {string} languageCode
    * @return {string} The converted language code.
-   * @private
    */
   convertLanguageCodeForTranslate: function(languageCode) {
     if (languageCode in kLanguageCodeToTranslateCode)
       return kLanguageCodeToTranslateCode[languageCode];
 
     var main = languageCode.split('-')[0];
     if (main == 'zh') {
       // In Translate, general Chinese is not used, and the sub code is
       // necessary as a language code for the Translate server.
       return languageCode;
     }
     if (main in kTranslateLanguageSynonyms)
       return kTranslateLanguageSynonyms[main];
 
     return main;
   },
+
+  /**
+   * @param {string} languageCode
+   * @return {!chrome.languageSettingsPrivate.Language|undefined}
+   */
+  getLanguage: function(languageCode) {
+    return this.supportedLanguageMap_[languageCode];
+  },
+});
+
+/**
+ * A reference to the singleton under the guise of a LanguageSettingsHelper
+ * implementation. This provides a limited API but implies the singleton
+ * should not be used directly for data binding.
+ */
+settings.LanguageHelper = SettingsLanguagesSingletonElement;

[Dan Beam, 2015/11/18 18:06:01]

why not cr.exportPath() here?

[michaelpg, 2015/11/22 00:22:40]

Why cr.exportPath() here?

[Dan Beam, 2015/11/23 20:14:10]

as low as possible

[michaelpg, 2015/11/23 20:26:56]

Oh, I interpreted "why not cr.exportPath() here?" as "Why not change this line
here to use cr.exportPath to define settings.LanguageHelper?" Given your latest
response, now I think you mean that line 27 (cr.exportPath('settings')) should
be down here, because you prefer it be called as late as possible. Correct?

[Dan Beam, 2015/11/23 20:59:50]

yes

+cr.addSingletonGetter(settings.LanguageHelper);

[Dan Beam, 2015/11/18 18:06:01]

do you need to addSingletonGetter() to the settings. namespaced version?

[michaelpg, 2015/11/22 00:22:40]

Not if we make settings.LanguageHelper point to the singleton itself. Done.

[Dan Beam, 2015/11/23 20:14:10]

why is this better than addSingletonGetter?  why do you hate committing code?

[michaelpg, 2015/11/23 20:26:56]

Sorry, but: Huh? When you asked if I "needed" to addSingletonGetter(), I assumed
that meant it would be preferable if I didn't unless I had to. And I don't have
to, as I demonstrated in the updated CL.

Now you're asking why my update is better than addSingletonGetter. So, I'm
confused.

I will now attempt to answer your questions directly because the
update-feedback-update cycle isn't getting us anywhere.

1. do you need to addSingletonGetter() to the settings. namespaced version?

No. This uses addSingletonGetter() to lazily create the LanguageHelper when
needed while allowing other objects to reference it without worrying about
whether it has been created yet. It's a common pattern as I understand it, but
it is not a necessity.

2. why is this better than addSingletonGetter?

Well, setting and accessing it directly is a bit less verbose.

3. why do you hate committing code?

....?

[Dan Beam, 2015/11/23 20:59:50]

You were doing this:

  cr.addSingletonGetter(settings.Blah);

I wanted:

  cr.addSingletonGetter(Blah);

because the Chrome-specific closure compilation pass is fragile when it comes to
type detection and this is how most modules do it:

  cr.define('namespace', function() {
    function Blah() {}
    cr.addSingletonGetter(Blah);
    return {Blah: Blah};
  });
"a bit less" is generally not a good idea to add a new variation to the code
base, IMO
continuing to make new paradigms will slow down my approval as I need to
understand what's *so much better* about how your new way does things that
you're adding a permutation to the codebase (and therefore muddying the waters
for future folks to know which way to go).

[michaelpg, 2015/11/24 20:27:13]

OK, I changed it to what you wanted originally. I had no intention of creating
my own paradigm.

+
+/**
+ * This element has a reference to the singleton, exposing the singleton's
+ * |languages| model to the host of this element.
+ */
+Polymer({
+  is: 'settings-languages',
+
+  properties: {
+    /**
+     * Singleton element created at startup which provides the languages model.
+     * @type {!Element}
+     */
+    singleton_: {
+      type: Object,
+      value: settings.LanguageHelper.getInstance(),
+    },
+
+    /**
+     * A reference to the languages model from the singleton, exposed as a
+     * read-only property so hosts can bind to it, but not change it.
+     * @type {LanguagesModel|undefined}
+     */
+    languages: {
+      type: Object,
+      notify: true,
+      readOnly: true,
+    },
+  },
+
+  ready: function() {
+    // Set the 'languages' property to reference the singleton's model.
+    this._setLanguages(this.singleton_.languages);

[Dan Beam, 2015/11/18 18:06:01]

why are you calling a private Polymer method?

[michaelpg, 2015/11/22 00:22:40]

this is how you set a readOnly property.

[Dan Beam, 2015/11/23 20:14:10]

Acknowledged. (though mildly odd to me, guess it's kind of like @synthesize in
Cocoa)

+    // Listen for changes to the singleton's languages property, so we know
+    // when to notify hosts of changes to (our reference to) the property.
+    this.listen(
+        this.singleton_, 'languages-changed', 'singletonLanguagesChanged_');
+  },
+
+  /**
+   * Takes changes reported by the singleton and forwards them to the host,
+   * manually sending a change notification for our 'languages' property (since
+   * it's the same object as the singleton's property, but isn't bound by
+   * Polymer).
+   * @private
+   */
+  singletonLanguagesChanged_: function(e) {
+    // Forward the change notification to the host.
+    this.fire(e.type, e.detail, {bubbles: false});
+  },
 });
 })();