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

chrome/browser/resources/options/language_options.js

Index: chrome/browser/resources/options/language_options.js
diff --git a/chrome/browser/resources/options/language_options.js b/chrome/browser/resources/options/language_options.js
index 9ab66bf8d28c61ad51d22c6668bfd9241de5b865..8b0116fe7e2fa2449566d0209092932be5e436f7 100644
--- a/chrome/browser/resources/options/language_options.js
+++ b/chrome/browser/resources/options/language_options.js
@@ -23,46 +23,47 @@ cr.define('options', function() {
 
   /**
    * 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';
 
@@ -141,7 +142,15 @@ cr.define('options', function() {
      * @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:
@@ -187,8 +196,16 @@ cr.define('options', function() {
       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_ =
@@ -240,11 +257,19 @@ cr.define('options', function() {
 
       // 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) {
@@ -427,7 +452,7 @@ cr.define('options', function() {
       this.updateSelectedLanguageName_(languageCode);
 
       if (!cr.isMac)
-        this.updateSpellCheckLanguageButton_(languageCode);
+        this.updateSpellCheckLanguageControls_(languageCode);
 
       if (cr.isChromeOS)
         this.updateInputMethodList_(languageCode);
@@ -604,56 +629,70 @@ cr.define('options', function() {
      * @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;
+        }
+
+        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 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;
       } else {
         spellCheckLanguageMessage.textContent =
             loadTimeData.getString('cannotBeUsedForSpellChecking');
-        showMutuallyExclusiveNodes(
-            [spellCheckLanguageButton, spellCheckLanguageMessage], 1);
+        spellCheckLanguageMessage.hidden = false;
       }
     },
 
@@ -919,8 +958,8 @@ cr.define('options', function() {
      * @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;
      },
@@ -943,14 +982,36 @@ cr.define('options', function() {
      */
     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
@@ -978,6 +1039,31 @@ cr.define('options', function() {
     },
 
     /**
+     * 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.
@@ -1276,7 +1362,7 @@ cr.define('options', function() {
       if (!cr.isMac &&
           languageCode ==
               $('language-options-list').getSelectedLanguageCode()) {
-        this.updateSpellCheckLanguageButton_(languageCode);
+        this.updateSpellCheckLanguageControls_(languageCode);
       }
     },
 
@@ -1292,7 +1378,7 @@ cr.define('options', function() {
       if (!cr.isMac &&
           languageCode ==
               $('language-options-list').getSelectedLanguageCode()) {
-        this.updateSpellCheckLanguageButton_(languageCode);
+        this.updateSpellCheckLanguageControls_(languageCode);
       }
     },
 
@@ -1309,7 +1395,7 @@ cr.define('options', function() {
       if (!cr.isMac &&
           languageCode ==
               $('language-options-list').getSelectedLanguageCode()) {
-        this.updateSpellCheckLanguageButton_(languageCode);
+        this.updateSpellCheckLanguageControls_(languageCode);
       }
     },