MD Settings: reduce complexity and overhead of Languages singleton

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

Index: chrome/browser/resources/settings/languages_page/language_detail_page.js
diff --git a/chrome/browser/resources/settings/languages_page/language_detail_page.js b/chrome/browser/resources/settings/languages_page/language_detail_page.js
index d273f5f12b8eeac47cde78c0fe94d137d152e267..574d317b12e2c1020c2f89ff365986c33099c14c 100644
--- a/chrome/browser/resources/settings/languages_page/language_detail_page.js
+++ b/chrome/browser/resources/settings/languages_page/language_detail_page.js
@@ -30,14 +30,6 @@ Polymer({
      * @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 */
@@ -69,7 +61,8 @@ Polymer({
    */
   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.

   },
 
   /**
@@ -92,8 +85,8 @@ Polymer({
    * @private
    */
   isTranslateDisabled_: function(languageCode, targetLanguageCode) {
-    return this.languageHelper_.convertLanguageCodeForTranslate(languageCode) ==
-        targetLanguageCode;
+    return settings.LanguageHelperImpl.getInstance()
+        .convertLanguageCodeForTranslate(languageCode) == targetLanguageCode;
   },
 
    /**
@@ -150,10 +143,11 @@ Polymer({
    * @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);
   },
 
   /**
@@ -162,11 +156,12 @@ Polymer({
    * @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();
     }
   },