MD Settings: reduce complexity and overhead of Languages singleton

chrome/browser/resources/settings/languages_page/language_detail_page.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-language-detail-page' is a sub-page for editing
  * an individual language's settings.
  */
 Polymer({
   is: 'settings-language-detail-page',
@@ skipping 12 matching lines @@
      * 'settings-languages' instance.
      * @type {!LanguagesModel|undefined}
      */
     languages: Object,
 
     /**
      * The language to display the details for.
      * @type {!LanguageState|undefined}
      */
     detail: Object,
-
-    /** @private {!LanguageHelper} */
-    languageHelper_: Object,
   },
 
   /** @override */
-  created: function() {
-     this.languageHelper_ = LanguageHelperImpl.getInstance();

[stevenjb, 2016/08/18 16:22:02]

QQ: These changes seem fine, but any particular reason not to just set this to
settings.LanguageHelperImpl.getInstance()? Does the getInstance() result change
after created() is called in tests? Could we move this to ready() or attached()
in that case, or use data binding? I used data binding for networkingPrivate in
the internet code to support a fake implementation in tests, and I am curious
what if any the tradeoffs are.

[michaelpg, 2016/08/18 22:32:27]

Changed to a data-bound  member. The instance wasn't set until the element was
created, and we don't have guaranteed timings for that. Data binding is an
obviously superior solution to ensure the property is set when it needs to be,
and it seems to work perfectly.

-  },
-
-  /** @override */
   ready: function() {
     // In a CrOS multi-user session, the primary user controls the UI language.
     if (this.isSecondaryUser_()) {
       var indicator = this.$.policyIndicator;
       indicator.indicatorType = CrPolicyIndicatorType.PRIMARY_USER;
       indicator.controllingUser = loadTimeData.getString('primaryUserEmail');
     }
 
     // The UI language choice doesn't persist for guests.
     if (cr.isChromeOS &&
         (uiAccountTweaks.UIAccountTweaks.loggedInAsGuest() ||
          uiAccountTweaks.UIAccountTweaks.loggedInAsPublicAccount())) {
       this.$.languageSettings.hidden = true;
     }
   },
 
   /**
    * Checks whether the prospective UI language (the pref that indicates what
    * language to use in Chrome) matches the current language. This pref is only
    * on Chrome OS and Windows; we don't control the UI language elsewhere.
    * @param {string} languageCode The language code identifying a language.
    * @param {string} prospectiveUILanguage The prospective UI language.
    * @return {boolean} True if the given language matches the prospective UI
    *     pref (which may be different from the actual UI language).
    * @private
    */
   isProspectiveUILanguage_: function(languageCode, prospectiveUILanguage) {
     assert(cr.isChromeOS || cr.isWindows);
-    return languageCode == this.languageHelper_.getProspectiveUILanguage();
+    return languageCode ==
+        settings.LanguageHelperImpl.getInstance().getProspectiveUILanguage();

[stevenjb, 2016/08/18 16:22:02]

nit: In C++ we would make getInstance() part of the LanguageHelper interface and
it would return a LanguageHelperImpl class. Can we do the same thing here?
Accessing an Impl class externally is a bit wonky.

[michaelpg, 2016/08/18 22:32:27]

Changed to a data-bound  member.

   },
 
   /**
    * @param {string} languageCode The language code identifying a language.
    * @param {string} prospectiveUILanguage The prospective UI language.
    * @return {boolean} True if the the given language, the prospective UI
    *     language and the actual language all the same.
    * @private
    */
   isCurrentUILanguage_: function(languageCode, prospectiveUILanguage) {
     assert(cr.isChromeOS || cr.isWindows);
     return languageCode == prospectiveUILanguage &&
            languageCode == navigator.language;
   },
 
    /**
    * @param {string} languageCode The language code identifying a language.
    * @param {string} targetLanguageCode The default translate target language.
    * @return {boolean} True if the language code matches the target language.
    * @private
    */
   isTranslateDisabled_: function(languageCode, targetLanguageCode) {
-    return this.languageHelper_.convertLanguageCodeForTranslate(languageCode) ==
-        targetLanguageCode;
+    return settings.LanguageHelperImpl.getInstance()
+        .convertLanguageCodeForTranslate(languageCode) == targetLanguageCode;
   },
 
    /**
    * @param {string} languageCode The language code identifying a language.
    * @param {string} prospectiveUILanguage The prospective UI language.
    * @return {boolean} True if the prospective UI language is set to
    *     |languageCode| but requires a restart to take effect.
    * @private
    */
   isRestartRequired_: function(languageCode, prospectiveUILanguage) {
@@ skipping 36 matching lines @@
     // checkbox when general Chinese is selected.
     return !translateEnabled || languageCode == 'zh';
   },
 
   /**
    * Handler for changes to the translate checkbox.
    * @param {!{target: !{checked: boolean}}} e
    * @private
    */
   onTranslateEnabledChange_: function(e) {
+    var languageHelper = settings.LanguageHelperImpl.getInstance();
     if (e.target.checked)
-      this.languageHelper_.enableTranslateLanguage(this.detail.language.code);
+      languageHelper.enableTranslateLanguage(this.detail.language.code);
     else
-      this.languageHelper_.disableTranslateLanguage(this.detail.language.code);
+      languageHelper.disableTranslateLanguage(this.detail.language.code);
   },
 
   /**
    * Handler for changes to the UI language toggle button.
    * @param {!{target: !{checked: boolean}}} e
    * @private
    */
   onUILanguageChange_: function(e) {
+    var languageHelper = settings.LanguageHelperImpl.getInstance();
     if (e.target.checked) {
-      this.languageHelper_.setUILanguage(this.detail.language.code);
+      languageHelper.setUILanguage(this.detail.language.code);
     } else {
       // Reset the chosen UI language to the actual UI language.
-      this.languageHelper_.resetUILanguage();
+      languageHelper.resetUILanguage();
     }
   },
 
   /**
    * Handler for the restart button.
    * @private
    */
   onRestartTap_: function() {
 <if expr="chromeos">
     settings.LifetimeBrowserProxyImpl.getInstance().logOutAndRestart();
 </if>
 <if expr="not chromeos">
     settings.LifetimeBrowserProxyImpl.getInstance().restart();
 </if>
   },
 });