[MD settings] theme name shown in sub-label, wallpaper chrome os only

chrome/browser/resources/settings/appearance_page/appearance_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.
 
 /**
  * 'settings-appearance-page' is the settings page containing appearance
  * settings.
  *
  * Example:
  *
  *    <iron-animated-pages>
  *      <settings-appearance-page prefs="{{prefs}}">
  *      </settings-appearance-page>
  *      ... other pages ...
  *    </iron-animated-pages>
  */
 Polymer({
   is: 'settings-appearance-page',
 
+  behaviors: [I18nBehavior],
+
   properties: {
     /**
      * The current active route.
      */
     currentRoute: {
       notify: true,
       type: Object,
     },
 
     /**
@@ skipping 62 matching lines @@
         {value: 110, name: '110%'},
         {value: 125, name: '125%'},
         {value: 150, name: '150%'},
         {value: 175, name: '175%'},
         {value: 200, name: '200%'},
         {value: 300, name: '300%'},
         {value: 400, name: '400%'},
         {value: 500, name: '500%'},
       ],
     },
+
+    /** @private */
+    themeSublabel_: {
+      type: String,
+      value: function() { return this.i18n('chooseFromWebStore'); },

[michaelpg, 2016/03/31 15:48:21]

does this duplicate what happens in themeChanged_, or does that not get called
initially when the value is empty?

[dschuyler, 2016/03/31 19:13:36]

Done.

+    },
   },
 
-  behaviors: [
-    I18nBehavior,
-  ],
-
   observers: [
+    'themeChanged_(prefs.extensions.theme.id.value)',
     'zoomLevelChanged_(defaultZoomLevel_.value)',
   ],
 
   ready: function() {
     this.$.defaultFontSize.menuOptions = this.fontSizeOptions_;
     this.$.pageZoom.menuOptions = this.pageZoomOptions_;
     // TODO(dschuyler): Look into adding a listener for the
     // default zoom percent.
     chrome.settingsPrivate.getDefaultZoomPercent(
         this.zoomPrefChanged_.bind(this));
@@ skipping 15 matching lines @@
    * @param {string} homepage If not using NTP, use this URL.
    * @return {string} The sub-label.
    * @private
    */
   getShowHomeSubLabel_: function(isNtp, homepage) {
     if (isNtp)
       return this.i18n('homePageNtp');
     return homepage || this.i18n('exampleDotCom');
   },
 
-  /**
-   * @param {boolean} enabled Whether the theme reset is available.
-   */
-  setResetThemeEnabled: function(enabled) {
-    this.allowResetTheme_ = enabled;
-  },
-
   /** @private */
   onCustomizeFontsTap_: function() {
     this.$.pages.setSubpageChain(['appearance-fonts']);
   },
 
   /** @private */
   openThemesGallery_: function() {
     window.open(loadTimeData.getString('themesGalleryUrl'));
   },
 
+  /**
+   * ChromeOS only.
+   * @private
+   * */

[michaelpg, 2016/03/31 15:48:21]

nit: extra ' *'

[dschuyler, 2016/03/31 19:13:36]

Done.

+  openWallpaperGallery_: function() {
+    chrome.send('openWallpaperManager');

[michaelpg, 2016/03/31 15:48:21]

does this exist in a settings handler?

[michaelpg, 2016/03/31 15:48:21]

Dan just said to always use a browser proxy instead of chrome.send. I guess that
would also apply to cr.sendWithPromise (and maybe cr.addWebUIListener?) so
that's a larger scoped CL than you should do here.

[Dan Beam, 2016/03/31 17:53:21]

i didn't necesarilly say "always", but I do agree with michaelpg@ that this
class should use a browser proxy when possible.  whether that's before, during,
or after that CL is up to y'all.

Dave: you know my vote as that's what I pre-did:
https://codereview.chromium.org/1845473002/

before hooking up validation:
https://codereview.chromium.org/1849543006/

[dschuyler, 2016/03/31 19:13:36]

Acknowledged.

[dschuyler, 2016/03/31 19:13:36]

Done.

[dschuyler, 2016/03/31 19:13:36]

I'm on board with the conversion.  I'm just doing
it it a different order.

+  },
+
   /** @private */
   resetTheme_: function() {
     chrome.send('resetTheme');
   },
 
+  /**
+   * @param {boolean} enabled Whether the theme reset is available.
+   */
+  setResetThemeEnabled: function(enabled) {

[michaelpg, 2016/03/31 15:48:21]

I assume you have a reason for moving this from one location between two private
functions to another location between two private functions.

[dschuyler, 2016/03/31 19:13:36]

Done.

+    this.allowResetTheme_ = enabled;
+  },
+
   /** @private */
   showFontsPage_: function() {
     return this.currentRoute.subpage[0] == 'appearance-fonts';
   },
 
   /**
+   * @param {string} themeId The theme ID.
+   * @private
+   */
+  themeChanged_: function(themeId) {
+    if (themeId) {
+      var self = this;

[michaelpg, 2016/03/31 15:48:21]

nit: .bind(this) instead, as that's more common?

[dschuyler, 2016/03/31 19:13:36]

Done.

+      chrome.management.get(themeId,
+          function(info) {
+              self.themeSublabel_ = info.name;

[michaelpg, 2016/03/31 15:48:21]

2-space indent

[dschuyler, 2016/03/31 19:13:36]

Done.

+          });
+    } else {
+      this.themeSublabel_ = this.i18n('chooseFromWebStore');
+    }
+  },
+
+  /**
    * @param {number} percent The integer percentage of the page zoom.
    * @private
    */
   zoomPrefChanged_: function(percent) {
     this.set('defaultZoomLevel_.value', percent);
   },
 
   /**
    * @param {number} percent The integer percentage of the page zoom.
    * @private
    */
   zoomLevelChanged_: function(percent) {
     // The |percent| may be undefined on startup.
     if (percent === undefined)
       return;
     chrome.settingsPrivate.setDefaultZoomPercent(percent);
   },
 });