Enables the user to select multiple languages for spellchecking (UI)

chrome/browser/resources/options/language_options.js

 // Copyright (c) 2012 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.
 
 // TODO(kochi): Generalize the notification as a component and put it
 // in js/cr/ui/notification.js .
 
 cr.define('options', function() {
   /** @const */ var Page = cr.ui.pageManager.Page;
   /** @const */ var PageManager = cr.ui.pageManager.PageManager;
   /** @const */ var LanguageList = options.LanguageList;
   /** @const */ var ThirdPartyImeConfirmOverlay =
       options.ThirdPartyImeConfirmOverlay;
 
   /**
    * Spell check dictionary download status.
    * @type {Enum}
    */
   /** @const*/ var DOWNLOAD_STATUS = {
     IN_PROGRESS: 1,
     FAILED: 2
   };
 
   /**
    * The preference is a boolean that enables/disables spell checking.
-   * @type {string}
-   * @const
+   * @const {string}

[please use gerrit instead, 2015/06/05 17:50:05]

Unless some Chrome tool or bot told you to do it this way, it's best to not
modify notations here. There's a server that periodically runs Closure compiler
(I think) on these files and will bug you if you're loosey goosey with your
comment conventions.

[Julius, 2015/06/05 21:38:32]

I changed this because a comment on Klemen's patch mentioned to change it so I
just changed it wherever it showed up. I guess this is the newer way of doing it
(https://developers.google.com/closure/compiler/docs/js-for-compiler )? I could
change other similar comment styles in the file to match it or just change these
all back.

[please use gerrit instead, 2015/06/06 01:42:12]

Sounds good. let's leave it as is.

    */
   var ENABLE_SPELL_CHECK_PREF = 'browser.enable_spellchecking';
 
   /**
    * The preference is a CSV string that describes preload engines
    * (i.e. active input methods).
-   * @type {string}
-   * @const
+   * @const {string}
    */
   var PRELOAD_ENGINES_PREF = 'settings.language.preload_engines';
 
   /**
    * The preference that lists the extension IMEs that are enabled in the
    * language menu.
-   * @type {string}
-   * @const
+   * @const {string}
    */
   var ENABLED_EXTENSION_IME_PREF = 'settings.language.enabled_extension_imes';
 
   /**
    * The preference that lists the languages which are not translated.
-   * @type {string}
-   * @const
+   * @const {string}
    */
   var TRANSLATE_BLOCKED_LANGUAGES_PREF = 'translate_blocked_languages';
 
   /**
    * The preference key that is a string that describes the spell check
    * dictionary language, like "en-US".
-   * @type {string}
-   * @const
+   * @const {string}
    */
   var SPELL_CHECK_DICTIONARY_PREF = 'spellcheck.dictionary';
 
   /**
+   * The preference key that describes the spell check dictionary languages
+   * currently selected (as a comma separated string, like "en-US,sl-SI").
+   * @const {string}

[please use gerrit instead, 2015/06/05 17:50:05]

Best to following existing conventions of this file.

@type {string}
@const

[Julius, 2015/06/05 21:38:32]

See previous comment.

+   */
+  var SPELL_CHECK_DICTIONARIES_PREF = 'spellcheck.dictionaries';
+
+  /**
    * The preference that indicates if the Translate feature is enabled.
-   * @type {string}
-   * @const
+   * @const {string}
    */
   var ENABLE_TRANSLATE = 'translate.enabled';
 
   /////////////////////////////////////////////////////////////////////////////
   // LanguageOptions class:
 
   /**
    * Encapsulated handling of ChromeOS language options page.
    * @constructor
    * @extends {cr.ui.pageManager.Page}
@@ skipping 58 matching lines @@
      * @private
      */
     translateSupportedLanguages_: [],
 
     /**
      * The preference is a string that describes the spell check dictionary
      * language, like "en-US".
      * @type {string}
      * @private
      */
-    spellCheckDictionary_: '',
+    spellCheckLanguage_: '',

[please use gerrit instead, 2015/06/05 17:50:05]

I thinks this renaming is not necessary and makes the code more confusing. Can
you undo it? (You can keep the rename if you can explain/justify it.)

[Julius, 2015/06/05 21:38:32]

I changed it because in Klemen's patch someone mentioned some confusion about
spellCheckDictionary being a dictionary. I'll change it back though because
looking through all other code it's known that the "dictionary" refers to the
language name, and not an actual data structure comprising the words in the
language.

+
+    /**
+     * The dictionary of currently selected spell check dictionary languages,
+     * like {"en-US": true, "sl-SI": true}.
+     * @type {Object}
+     * @private
+     */
+    spellCheckLanguages_: {},

[please use gerrit instead, 2015/06/05 17:50:05]

If you undo rename of spellCheckDictionary_, then this var should be named
spellCheckDictionaries_.

[please use gerrit instead, 2015/06/05 17:50:05]

Can you look through Klemen's review and see whether he had a good justification
for using a dictionary instead of a list here?

[Julius, 2015/06/05 21:38:32]

Done.

[Julius, 2015/06/05 21:38:33]

Dan Beam had a write-up about it here (
https://codereview.chromium.org/654653002/diff/400001/chrome/browser/resource...
). Initially I thought a list might be better too, but I think since we don't
care about an order and it's faster to check if something is in an object this
is the way to go.

 
     /**
      * The map of language code to input method IDs, like:
      * {'ja': ['mozc', 'mozc-jp'], 'zh-CN': ['pinyin'], ...}
      * @type {Object}
      * @private
      */
     languageCodeToInputMethodIdsMap_: {},
 
     /**
@@ skipping 25 matching lines @@
         this.initializeLanguageCodeToInputMethodIdsMap_();
       }
 
       var checkbox = $('offer-to-translate-in-this-language');
       checkbox.addEventListener('click',
           this.handleOfferToTranslateCheckboxClick_.bind(this));
 
       Preferences.getInstance().addEventListener(
           TRANSLATE_BLOCKED_LANGUAGES_PREF,
           this.handleTranslateBlockedLanguagesPrefChange_.bind(this));
-      Preferences.getInstance().addEventListener(SPELL_CHECK_DICTIONARY_PREF,
+
+      if (loadTimeData.getBoolean('enableMultilingualSpellChecker')) {
+        Preferences.getInstance().addEventListener(
+            SPELL_CHECK_DICTIONARIES_PREF,
+            this.handleSpellCheckDictionariesPrefChange_.bind(this));
+      } else {
+        Preferences.getInstance().addEventListener(SPELL_CHECK_DICTIONARY_PREF,
           this.handleSpellCheckDictionaryPrefChange_.bind(this));
+      }
+
       Preferences.getInstance().addEventListener(ENABLE_TRANSLATE,
           this.handleEnableTranslatePrefChange_.bind(this));
       this.translateSupportedLanguages_ =
           loadTimeData.getValue('translateSupportedLanguages');
 
       // Set up add button.
       var onclick = function(e) {
         // Add the language without showing the overlay if it's specified in
         // the URL hash (ex. lang_add=ja).  Used for automated testing.
         var match = document.location.hash.match(/\blang_add=([\w-]+)/);
@@ skipping 31 matching lines @@
         // Handle spell check enable/disable.
         if (!cr.isMac) {
           Preferences.getInstance().addEventListener(
               ENABLE_SPELL_CHECK_PREF,
               this.updateEnableSpellCheck_.bind(this));
         }
       }
 
       // Handle clicks on "Use this language for spell checking" button.
       if (!cr.isMac) {
-        var spellCheckLanguageButton = getRequiredElement(
-            'language-options-spell-check-language-button');
-        spellCheckLanguageButton.addEventListener(
-            'click',
-            this.handleSpellCheckLanguageButtonClick_.bind(this));
+        if (loadTimeData.getBoolean('enableMultilingualSpellChecker')) {
+          var spellCheckLanguageCheckbox = getRequiredElement(
+              'language-options-spellcheck-language-checkbox');
+          spellCheckLanguageCheckbox.addEventListener(
+              'click',
+              this.handleSpellCheckLanguageCheckboxClick_.bind(this));
+        } else {
+          var spellCheckLanguageButton = getRequiredElement(
+              'language-options-spellcheck-language-button');
+          spellCheckLanguageButton.addEventListener(
+              'click',
+              this.handleSpellCheckLanguageButtonClick_.bind(this));
+        }
       }
 
       if (cr.isChromeOS) {
         $('language-options-ui-restart-button').onclick = function() {
           chrome.send('uiLanguageRestart');
         };
       }
 
       $('language-confirm').onclick =
           PageManager.closeOverlay.bind(PageManager);
@@ skipping 162 matching lines @@
       }
 
       this.updateOfferToTranslateCheckbox_(languageCode);
 
       if (cr.isWindows || cr.isChromeOS)
         this.updateUiLanguageButton_(languageCode);
 
       this.updateSelectedLanguageName_(languageCode);
 
       if (!cr.isMac)
-        this.updateSpellCheckLanguageButton_(languageCode);
+        this.updateSpellCheckLanguageControls_(languageCode);
 
       if (cr.isChromeOS)
         this.updateInputMethodList_(languageCode);
 
       this.updateLanguageListInAddLanguageOverlay_();
     },
 
     /**
      * Handles languageOptionsList's save event.
      * @param {Event} e Save event.
@@ skipping 156 matching lines @@
         showMutuallyExclusiveNodes(
             [uiLanguageButton, uiLanguageMessage, uiLanguageNotification], 1);
       }
     },
 
     /**
      * Updates the spell check language button.
      * @param {string} languageCode Language code (ex. "fr").
      * @private
      */
-    updateSpellCheckLanguageButton_: function(languageCode) {
+    updateSpellCheckLanguageControls_: function(languageCode) {
       var spellCheckLanguageSection = $('language-options-spellcheck');
       var spellCheckLanguageButton =
-          $('language-options-spell-check-language-button');
+          $('language-options-spellcheck-language-button');
+      var spellCheckLanguageCheckboxDiv =
+          $('language-options-spellcheck-language-checkbox-div');
+      var spellCheckLanguageCheckbox =
+          $('language-options-spellcheck-language-checkbox');
       var spellCheckLanguageMessage =
-          $('language-options-spell-check-language-message');
+          $('language-options-spellcheck-language-message');
       var dictionaryDownloadInProgress =
           $('language-options-dictionary-downloading-message');
       var dictionaryDownloadFailed =
           $('language-options-dictionary-download-failed-message');
       var dictionaryDownloadFailHelp =
           $('language-options-dictionary-download-fail-help-message');
+
       spellCheckLanguageSection.hidden = false;
       spellCheckLanguageMessage.hidden = true;
       spellCheckLanguageButton.hidden = true;
+      spellCheckLanguageCheckboxDiv.hidden = true;
       dictionaryDownloadInProgress.hidden = true;
       dictionaryDownloadFailed.hidden = true;
       dictionaryDownloadFailHelp.hidden = true;
 
-      if (languageCode == this.spellCheckDictionary_) {
-        if (!(languageCode in this.spellcheckDictionaryDownloadStatus_)) {
-          spellCheckLanguageMessage.textContent =
-              loadTimeData.getString('isUsedForSpellChecking');
-          showMutuallyExclusiveNodes(
-              [spellCheckLanguageButton, spellCheckLanguageMessage], 1);
-        } else if (this.spellcheckDictionaryDownloadStatus_[languageCode] ==
-                       DOWNLOAD_STATUS.IN_PROGRESS) {
-          dictionaryDownloadInProgress.hidden = false;
-        } else if (this.spellcheckDictionaryDownloadStatus_[languageCode] ==
-                       DOWNLOAD_STATUS.FAILED) {
-          spellCheckLanguageSection.hidden = true;
-          dictionaryDownloadFailed.hidden = false;
-          if (this.spellcheckDictionaryDownloadFailures_ > 1)
-            dictionaryDownloadFailHelp.hidden = false;
+      spellCheckLanguageCheckbox.checked = false;
+
+      if (!languageCode)
+        return;
+
+      if (languageCode in loadTimeData.getValue('spellCheckLanguageCodeSet')) {
+        if (loadTimeData.getBoolean('enableMultilingualSpellChecker')) {
+          spellCheckLanguageCheckbox.languageCode = languageCode;
+          spellCheckLanguageCheckbox.checked =
+            this.spellCheckLanguages_.hasOwnProperty(languageCode);
+          spellCheckLanguageCheckboxDiv.hidden = false;
+        } else if (languageCode == this.spellCheckLanguage_) {
+          if (!(languageCode in this.spellcheckDictionaryDownloadStatus_)) {
+            spellCheckLanguageMessage.textContent =
+                loadTimeData.getString('isUsedForSpellChecking');
+            spellCheckLanguageMessage.hidden = false;
+          }
+        } else {
+          spellCheckLanguageButton.textContent =
+              loadTimeData.getString('useThisForSpellChecking');
+          spellCheckLanguageButton.hidden = false;
+          spellCheckLanguageButton.languageCode = languageCode;
         }
-      } else if (languageCode in
-          loadTimeData.getValue('spellCheckLanguageCodeSet')) {
-        spellCheckLanguageButton.textContent =
-            loadTimeData.getString('useThisForSpellChecking');
-        showMutuallyExclusiveNodes(
-            [spellCheckLanguageButton, spellCheckLanguageMessage], 0);
-        spellCheckLanguageButton.languageCode = languageCode;
-      } else if (!languageCode) {
-        spellCheckLanguageButton.hidden = true;
-        spellCheckLanguageMessage.hidden = true;
+
+        switch (this.spellcheckDictionaryDownloadStatus_[languageCode]) {
+          case DOWNLOAD_STATUS.IN_PROGRESS:
+            dictionaryDownloadInProgress.hidden = false;
+            break;
+          case DOWNLOAD_STATUS.FAILED:
+            showMutuallyExclusiveNodes(
+              [spellCheckLanguageSection, dictionaryDownloadFailed], 1);
+            if (this.spellcheckDictionaryDownloadFailures_ > 1)
+              dictionaryDownloadFailHelp.hidden = false;
+            break;
+        }
       } else {
         spellCheckLanguageMessage.textContent =
             loadTimeData.getString('cannotBeUsedForSpellChecking');
-        showMutuallyExclusiveNodes(
-            [spellCheckLanguageButton, spellCheckLanguageMessage], 1);
+        spellCheckLanguageMessage.hidden = false;
       }
     },
 
     /**
      * Updates the checkbox for stopping translation.
      * @param {string} languageCode Language code (ex. "fr").
      * @private
      */
     updateOfferToTranslateCheckbox_: function(languageCode) {
       var div = $('language-options-offer-to-translate');
@@ skipping 245 matching lines @@
       return (!cr.isChromeOS ||
               this.canDeleteLanguage_(languageCode));
     },
 
     /**
      * Handles browse.enable_spellchecking change.
      * @param {Event} e Change event.
      * @private
      */
     updateEnableSpellCheck_: function(e) {
-       var value = !$('enable-spell-check').checked;
-       $('language-options-spell-check-language-button').disabled = value;
+       var value = !$('enable-spellcheck').checked;
+       $('language-options-spellcheck-language-button').disabled = value;
        if (!cr.isMac)
          $('edit-dictionary-button').hidden = value;
      },
 
     /**
      * Handles translateBlockedLanguagesPref change.
      * @param {Event} e Change event.
      * @private
      */
     handleTranslateBlockedLanguagesPrefChange_: function(e) {
       this.translateBlockedLanguages_ = e.value.value;
       this.updateOfferToTranslateCheckbox_(
           $('language-options-list').getSelectedLanguageCode());
     },
 
     /**
      * Handles spellCheckDictionaryPref change.
      * @param {Event} e Change event.
      * @private
      */
     handleSpellCheckDictionaryPrefChange_: function(e) {
       var languageCode = e.value.value;

[please use gerrit instead, 2015/06/05 17:50:05]

Inline this variable into the next statement.

[Julius, 2015/06/05 21:38:32]

Done.

-      this.spellCheckDictionary_ = languageCode;
+      this.spellCheckLanguage_ = languageCode;
       if (!cr.isMac) {

[please use gerrit instead, 2015/06/05 17:50:05]

Make both handleSpellCheckDictionaryPrefChange_() and 
handleSpellCheckDictionariesPrefChange_() do either one of "if (!cr.isMac) { do
work }" or "if (cr.isMac) return". The difference is jarring.

[Julius, 2015/06/05 21:38:32]

Done.

-        this.updateSpellCheckLanguageButton_(
+        this.updateSpellCheckLanguageControls_(
             $('language-options-list').getSelectedLanguageCode());
       }
     },
 
     /**
+     * Updates spellcheck dictionary UI (checkboxes, buttons, and labels) when
+     * preferences change.
+     * @param {Event} e. Preference change event where e.value.value is the

[please use gerrit instead, 2015/06/05 17:50:05]

No period (.) after the first mention of the variable name |e|.

[Julius, 2015/06/05 21:38:32]

Done.

+     * comma separated list of languages currently used for spellchecking.
+     * @private
+     */
+    handleSpellCheckDictionariesPrefChange_: function(e) {
+      if (cr.isMac)
+        return;
+
+      var commaSeparatedLanguageCodes = e.value.value;

[please use gerrit instead, 2015/06/05 17:50:05]

Inline this variable into the next statement.

[Julius, 2015/06/05 21:38:32]

Done.

+      var languageCodesSplit = commaSeparatedLanguageCodes.split(',');
+
+      this.spellCheckLanguages_ = {};
+      for (var i = 0; i < languageCodesSplit.length; i++)

[please use gerrit instead, 2015/06/05 17:50:05]

Easier to write this?

for (var i in languageCodesSplit)
  this.spellCheckLanguages_[languageCodesSplit[i]] = true;

[Julius, 2015/06/05 21:38:32]

This isn't the right way to iterate over array elements. for-in loops, in
Javascript, iterate over object properties and it goes down the prototype chain
so it actually picks up array indices too. The style guide also says to use a
normal for loop to iterate over arrays (
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#for-in_...
). I'll leave it as it is.

+        this.spellCheckLanguages_[languageCodesSplit[i]] = true;
+
+      this.updateSpellCheckLanguageControls_(
+          $('language-options-list').getSelectedLanguageCode());
+    },
+
+    /**
      * Handles translate.enabled change.
      * @param {Event} e Change event.
      * @private
      */
     handleEnableTranslatePrefChange_: function(e) {
       var enabled = e.value.value;
       this.enableTranslate_ = enabled;
       this.updateOfferToTranslateCheckbox_(
           $('language-options-list').getSelectedLanguageCode());
     },
 
     /**
      * Handles spellCheckLanguageButton click.
      * @param {Event} e Click event.
      * @private
      */
     handleSpellCheckLanguageButtonClick_: function(e) {
       var languageCode = e.target.languageCode;
       // Save the preference.
       Preferences.setStringPref(SPELL_CHECK_DICTIONARY_PREF,
                                 languageCode, true);
       chrome.send('spellCheckLanguageChange', [languageCode]);
       chrome.send('coreOptionsUserMetricsAction',
                   ['Options_Languages_SpellCheck']);
     },
 
     /**
+     * Updates the spellcheck.dictionaries preference with the currently
+     * selected language codes.
+     * @param {Event} e Click event. e.target represents the "Use this language
+     * for spellchecking"
+     * @private
+     */
+    handleSpellCheckLanguageCheckboxClick_: function(e) {
+      var languageCode = e.target.languageCode;
+
+      if (e.target.checked)
+        this.spellCheckLanguages_[languageCode] = true;
+      else
+        delete this.spellCheckLanguages_[languageCode];
+
+      var languageCodes = [];
+      for (var currentLanguageCode in this.spellCheckLanguages_) {
+        if (this.spellCheckLanguages_.hasOwnProperty(currentLanguageCode))

[please use gerrit instead, 2015/06/05 17:50:05]

Are you able to iterate over the elements in this.spellCheckLanguages_ instead?

[Julius, 2015/06/05 21:38:33]

Yep, this is the standard way of iterating over keys in javascript.

+          languageCodes.push(currentLanguageCode);
+      }
+
+      Preferences.setStringPref(SPELL_CHECK_DICTIONARIES_PREF,
+                                languageCodes.join(','), true);
+    },
+
+    /**
      * Checks whether it's possible to remove the language specified by
      * languageCode and returns true if possible. This function returns false
      * if the removal causes the number of preload engines to be zero.
      *
      * @param {string} languageCode Language code (ex. "fr").
      * @return {boolean} Returns true on success.
      * @private
      */
     canDeleteLanguage_: function(languageCode) {
       // First create the set of engines to be removed from input methods
@@ skipping 278 matching lines @@
      * @param {string} languageCode The language of the dictionary that just
      *     began downloading.
      * @private
      */
     onDictionaryDownloadBegin_: function(languageCode) {
       this.spellcheckDictionaryDownloadStatus_[languageCode] =
           DOWNLOAD_STATUS.IN_PROGRESS;
       if (!cr.isMac &&
           languageCode ==
               $('language-options-list').getSelectedLanguageCode()) {
-        this.updateSpellCheckLanguageButton_(languageCode);
+        this.updateSpellCheckLanguageControls_(languageCode);
       }
     },
 
     /**
      * A handler for when dictionary for |languageCode| successfully downloaded.
      * @param {string} languageCode The language of the dictionary that
      *     succeeded downloading.
      * @private
      */
     onDictionaryDownloadSuccess_: function(languageCode) {
       delete this.spellcheckDictionaryDownloadStatus_[languageCode];
       this.spellcheckDictionaryDownloadFailures_ = 0;
       if (!cr.isMac &&
           languageCode ==
               $('language-options-list').getSelectedLanguageCode()) {
-        this.updateSpellCheckLanguageButton_(languageCode);
+        this.updateSpellCheckLanguageControls_(languageCode);
       }
     },
 
     /**
      * A handler for when dictionary for |languageCode| fails to download.
      * @param {string} languageCode The language of the dictionary that failed
      *     to download.
      * @private
      */
     onDictionaryDownloadFailure_: function(languageCode) {
       this.spellcheckDictionaryDownloadStatus_[languageCode] =
           DOWNLOAD_STATUS.FAILED;
       this.spellcheckDictionaryDownloadFailures_++;
       if (!cr.isMac &&
           languageCode ==
               $('language-options-list').getSelectedLanguageCode()) {
-        this.updateSpellCheckLanguageButton_(languageCode);
+        this.updateSpellCheckLanguageControls_(languageCode);
       }
     },
 
     /**
      * Converts the language code for Translation. There are some differences
      * between the language set for Translation and that for Accept-Language.
      * @param {string} languageCode The language code like 'fr'.
      * @return {string} The converted language code.
      * @private
      */
@@ skipping 51 matching lines @@
 
   LanguageOptions.onDictionaryDownloadFailure = function(languageCode) {
     LanguageOptions.getInstance().onDictionaryDownloadFailure_(languageCode);
   };
 
   // Export
   return {
     LanguageOptions: LanguageOptions
   };
 });