MD Settings: change outlinks to actually use <a>

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:
  *
@@ skipping 67 matching lines @@
         3,
         4,
         5,
       ],
     },
 
     /** @private */
     themeSublabel_: String,
 
     /** @private */
+    themeUrl_: String,
+
+    /** @private */
     useSystemTheme_: {
       type: Boolean,
       value: false,  // Can only be true on Linux, but value exists everywhere.
     },
 
     /** @private {!Map<string, string>} */
     focusConfig_: {
       type: Object,
       value: function() {
         var map = new Map();
         map.set(
             settings.Route.FONTS.path,
             '#customize-fonts-subpage-trigger .subpage-arrow');
         return map;
       },
     },
   },
 
   /** @private {?settings.AppearanceBrowserProxy} */
   browserProxy_: null,
 
-  /** @private {string} */
-  themeUrl_: '',
-
   observers: [
     'themeChanged_(prefs.extensions.theme.id.value, useSystemTheme_)',
 
 // <if expr="is_linux and not chromeos">
     // NOTE: this pref only exists on Linux.
     'useSystemThemePrefChanged_(prefs.extensions.theme.use_system.value)',
 // </if>
   ],
 
   created: function() {
@@ skipping 36 matching lines @@
   /** @private */
   onCustomizeFontsTap_: function() {
     settings.navigateTo(settings.Route.FONTS);
   },
 
   /** @private */
   onDisableExtension_: function() {
     this.fire('refresh-pref', 'homepage');
   },
 
   /** @private */

[dschuyler, 2017/04/18 23:22:44]

comment like, "URL for either current theme or the theme gallery."

@return {string}

[scottchen, 2017/04/20 18:41:49]

Acknowledged.

-  onThemesTap_: function() {
-    window.open(this.themeUrl_ || loadTimeData.getString('themesGalleryUrl'));
+  getThemeHref_: function() {
+    return this.themeUrl_ || loadTimeData.getString('themesGalleryUrl');

[scottchen, 2017/04/18 19:12:49]

this.themeUrl_ contains a generated themes id, so I checked for potential XXS
and don't think it's possible with the id that gets returned from our C++ code.

But just to be safe, can a reviewer confirm if you know for sure?

[dpapad, 2017/04/18 21:29:27]

+dbeam: Should we be using
https://cs.chromium.org/chromium/src/ui/webui/resources/js/parse_html_subset....,
or some other way to escape the URL here?

[Dan Beam, 2017/04/18 21:59:49]

i wouldn't worry about it

if you realllly want to:
https://gist.github.com/danbeam/fc99e28f48b6696ddf9d13176fee3c61

[scottchen, 2017/04/18 22:41:51]

Acknowledged.

   },
 
   // <if expr="chromeos">
   /**
    * ChromeOS only.
    * @private
    */
   openWallpaperManager_: function() {
     this.browserProxy_.openWallpaperManager();
   },
@@ skipping 50 matching lines @@
     this.browserProxy_.useSystemTheme();
   },
   // </if>
 
   /**
    * @param {string} themeId
    * @param {boolean} useSystemTheme
    * @private
    */
   themeChanged_: function(themeId, useSystemTheme) {
     if (themeId) {

[dschuyler, 2017/04/18 23:22:44]

If you check the themeId for the themeId_, this looks like a good place to check
the themeId for xxs. (Rather than within getThemeHref_).

       assert(!useSystemTheme);
 
       this.browserProxy_.getThemeInfo(themeId).then(function(info) {
         this.themeSublabel_ = info.name;
       }.bind(this));
 
       this.themeUrl_ = 'https://chrome.google.com/webstore/detail/' + themeId;
       return;
     }
 
@@ skipping 26 matching lines @@
    * @see content::ZoomValuesEqual().
    * @param {number} zoom1
    * @param {number} zoom2
    * @return {boolean}
    * @private
    */
   zoomValuesEqual_: function(zoom1, zoom2) {
     return Math.abs(zoom1 - zoom2) <= 0.001;
   },
 });