MD Settings: Languages; limit platform specific code to the respective platform.

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' handles Chrome's language and input
  * method settings. The 'languages' property, which reflects the current
  * language settings, must not be changed directly. Instead, changes to
  * language settings should be made using the LanguageHelper APIs provided by
  * this class via languageHelper.
@@ skipping 64 matching lines @@
       type: Object,
       value: function() {
         return new PromiseResolver();
       },
     },
 
     /** @type {!LanguageSettingsPrivate} */
     languageSettingsPrivate: Object,
 
     /** @type {!InputMethodPrivate} */
     inputMethodPrivate: Object,

[michaelpg, 2017/04/18 05:03:35]

Wondering if this and the input method properties below could be if'd out as
well.

[dpapad, 2017/04/18 18:31:20]

I am addressing those in my next CL, which hides them behind a proxy, still WIP,
but you can take a look https://codereview.chromium.org/2827553002 (or you can
wait until I send it out later today).

 
     /**
      * Hash map of supported languages by language codes for fast lookup.
      * @private {!Map<string, !chrome.languageSettingsPrivate.Language>}
      */
     supportedLanguageMap_: {
       type: Object,
       value: function() { return new Map(); },
     },
 
@@ skipping 71 matching lines @@
     // Get the language list.
     promises[1] = new Promise(function(resolve) {
       this.languageSettingsPrivate.getLanguageList(resolve);
     }.bind(this));
 
     // Get the translate target language.
     promises[2] = new Promise(function(resolve) {
       this.languageSettingsPrivate.getTranslateTargetLanguage(resolve);
     }.bind(this));
 
     if (cr.isChromeOS) {

[michaelpg, 2017/04/18 05:03:35]

this stuff is platform specific for the same reasons -- should it be grit too?

[dpapad, 2017/04/18 18:31:20]

The platform specific code and the generic code are too tangled here. The
|promises| array indices 3 and 4 are assigned in ChromeOS, but then they are
used at line 207 regardless (guessing results[3] and results[4] are undefined in
other platforms).

Either way, this refactoring does not contribute directly to my effort of
deflaking the tests, so I'd rather leave for later.

[michaelpg, 2017/04/18 22:47:58]

Oof, you're right, and to make things worse the conditional below means another
promise is added on Windows in the 3'rd slot, at line 193.

I verified (with tommy's help) that it doesn't affect how createModel_ is called
on Windows, since the promise doesn't return anything. But we reeeally should
clear that up. Can you add a TODO here? I'd be happy to handle it once the dust
settles from your patches.

       promises[3] = new Promise(function(resolve) {
         this.languageSettingsPrivate.getInputMethodLists(function(lists) {
           resolve(lists.componentExtensionImes.concat(
               lists.thirdPartyExtensionImes));
         });
       }.bind(this));
 
       promises[4] = new Promise(function(resolve) {
         this.inputMethodPrivate.getCurrentInputMethod(resolve);
       }.bind(this));
@@ skipping 288 matching lines @@
 
   // LanguageHelper implementation.
   // TODO(michaelpg): replace duplicate docs with @override once b/24294625
   // is fixed.
 
   /** @return {!Promise} */
   whenReady: function() {
     return this.resolver_.promise;
   },
 
+// <if expr="chromeos or is_win">
   /**
    * Sets the prospective UI language to the chosen language. This won't affect
    * the actual UI language until a restart.
    * @param {string} languageCode
    */
   setProspectiveUILanguage: function(languageCode) {
-    assert(cr.isChromeOS || cr.isWindows);

[michaelpg, 2017/04/18 05:03:35]

I used runtime checks instead of grit because that came up in the original
reviews. Is grit what we prefer now for webui? (maybe my original understanding
was incorrect in the first place)

[dpapad, 2017/04/18 18:31:20]

AFAIK, GRIT is preferred. This guarantees for example that our bundled JS file
is as small as possible for any given platform, since it does not include code
only meant to run in other platforms.

     chrome.send('setProspectiveUILanguage', [languageCode]);

[michaelpg, 2017/04/18 05:03:35]

Does it makes sense to #ifdef these handlers in C++ now?

[dpapad, 2017/04/18 18:31:20]

Done.

   },
 
   /**
    * True if the prospective UI language was changed from its starting value.
    * @return {boolean}
    */
   requiresRestart: function() {
     return this.originalProspectiveUILanguage_ !=
         this.languages.prospectiveUILanguage;
   },
+// </if>
 
   /**
    * @param {string} languageCode
    * @return {boolean} True if the language is enabled.
    */
   isLanguageEnabled: function(languageCode) {
     return this.enabledLanguageSet_.has(languageCode);
   },
 
   /**
@@ skipping 205 matching lines @@
   },
 
   /**
    * @param {string} languageCode
    * @return {!chrome.languageSettingsPrivate.Language|undefined}
    */
   getLanguage: function(languageCode) {
     return this.supportedLanguageMap_.get(languageCode);
   },
 
-  /**
-   * @param {string} id
-   * @return {!chrome.languageSettingsPrivate.InputMethod|undefined}
-   */
-  getInputMethod: function(id) {
-    assert(cr.isChromeOS);
-    return this.supportedInputMethodMap_.get(id);
-  },
-
+// <if expr="chromeos">
   /** @param {string} id */
   addInputMethod: function(id) {
-    assert(cr.isChromeOS);
     if (!this.supportedInputMethodMap_.has(id))
       return;
     this.languageSettingsPrivate.addInputMethod(id);
   },
 
   /** @param {string} id */
   removeInputMethod: function(id) {
-    assert(cr.isChromeOS);
     if (!this.supportedInputMethodMap_.has(id))
       return;
     this.languageSettingsPrivate.removeInputMethod(id);
   },
 
   /** @param {string} id */
   setCurrentInputMethod: function(id) {
-    assert(cr.isChromeOS);
     this.inputMethodPrivate.setCurrentInputMethod(id);
   },
 
   /**
    * @param {string} languageCode
    * @return {!Array<!chrome.languageSettingsPrivate.InputMethod>}
    */
   getInputMethodsForLanguage: function(languageCode) {
     return this.languageInputMethods_.get(languageCode) || [];
   },
 
   /**
    * @param {!chrome.languageSettingsPrivate.InputMethod} inputMethod
    * @return {boolean}
    */
   isComponentIme: function(inputMethod) {
-    assert(cr.isChromeOS);
     return inputMethod.id.startsWith('_comp_');
   },
 
   /** @param {string} id Input method ID. */
   openInputMethodOptions: function(id) {
-    assert(cr.isChromeOS);
     this.inputMethodPrivate.openOptionsPage(id);
   },
 
   /** @param {string} id New current input method ID. */
   onInputMethodChanged_: function(id) {
-    assert(cr.isChromeOS);
     this.set('languages.inputMethods.currentId', id);
   },
 
   /** @param {string} id Added input method ID. */
   onInputMethodAdded_: function(id) {
-    assert(cr.isChromeOS);
     this.updateEnabledInputMethods_();
   },
 
   /** @param {string} id Removed input method ID. */
   onInputMethodRemoved_: function(id) {
-    assert(cr.isChromeOS);
     this.updateEnabledInputMethods_();
   },
+// </if>
 });
 })();