[MD settings] Data driven content setting subtext

chrome/browser/resources/settings/site_settings_page/site_settings_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.
 
 /**
  * @fileoverview
  * 'settings-site-settings-page' is the settings page containing privacy and
  * security site settings.
  */
+

[tommycli, 2017/01/27 00:36:52]

nit: this was on purpose i assume?

[dschuyler, 2017/01/27 22:29:17]

Acknowledged.

 Polymer({
   is: 'settings-site-settings-page',
 
   behaviors: [SiteSettingsBehavior, WebUIListenerBehavior],
 
   properties: {
     /**
      * An object to bind default value labels to (so they are not in the |this|

[tommycli, 2017/01/27 00:36:52]

Update comment to reflect that fact that they now contain permission values
instead.

[dschuyler, 2017/01/27 22:29:17]

Done.

      * scope). The keys of this object are the values of the
      * settings.ContentSettingsTypes enum.
      * @private
      */
     default_: {
       type: Object,
       value: function() {
         return {};
       },
     },
@@ skipping 25 matching lines @@
     this.addWebUIListener(
         'contentSettingCategoryChanged',
         this.updateDefaultValueLabel_.bind(this));
     this.addWebUIListener(
         'setHandlersEnabled',
         this.updateHandlersEnabled_.bind(this));
     this.browserProxy.observeProtocolHandlersEnabledState();
   },
 
   /**
+   * @param {string} setting Value from settings.PermissionValues.
+   * @param {string} allow Non-block label ('feature X not allowed').
+   * @param {string} block Block label (likely just, 'Blocked').
+   * @param {?string} other Tristate value (maybe, 'session only').
+   * @private
+   */
+  defaultSettingLabel_: function(setting, allow, block, other) {

[tommycli, 2017/01/27 00:36:52]

ALLOW and BLOCK have special meanings in content setting, which doesn't always
match the label params... (sometimes the allow param contains the ask string for
instance).

Maybe it would be clearer to have a function signature like:

function(permissionValue, enabledString, disabledString, otherString)

[dschuyler, 2017/01/27 22:29:17]

I left of the String suffix since iirc, type decorations on vars is undesired.

+    if (setting == settings.PermissionValues.BLOCK)
+      return block;
+    if (other === undefined || setting == settings.PermissionValues.ALLOW)
+      return allow;
+    return other;

[tommycli, 2017/01/27 00:36:52]

A nitpick, but the (other === undefined || setting == ALLOW) pattern looked odd
to me. Maybe for maximal idiot proofness, something like:

if (setting == BLOCK)
  return block;
if (setting == ALLOW)
  return allow;
if (other)
  return other;

return allow;

[dschuyler, 2017/01/27 22:29:17]

I left out the blank line because the whole group is related. I'm a fan of the
blank line after an unrelated condition block -- I'm sayin' this is different.

+  },
+
+  /**
    * @param {string} category The category to update.
    * @private
    */
   updateDefaultValueLabel_: function(category) {
     this.browserProxy.getDefaultValueForContentType(
         category).then(function(defaultValue) {
           this.set(
               'default_.' + Polymer.CaseMap.dashToCamelCase(category),
-              this.computeCategoryDesc(
-                  category,
-                  defaultValue.setting,
-                  /*showRecommendation=*/false));
+              defaultValue.setting);
         }.bind(this));
   },
 
   /**
    * The protocol handlers have a separate enabled/disabled notifier.
    * @param {boolean} enabled
    * @private
    */
   updateHandlersEnabled_: function(enabled) {
     var category = settings.ContentSettingsTypes.PROTOCOL_HANDLERS;
     this.set(
         'default_.' + Polymer.CaseMap.dashToCamelCase(category),
-        this.computeCategoryDesc(
-            category,
-            enabled ?
-                settings.PermissionValues.ALLOW :
-                settings.PermissionValues.BLOCK,
-            /*showRecommendation=*/false));
+        enabled ?
+            settings.PermissionValues.ALLOW :
+            settings.PermissionValues.BLOCK);
   },
 
   /**
    * Navigate to the route specified in the event dataset.
    * @param {!Event} event The tap event.
    * @private
    */
   onTapNavigate_: function(event) {
     var dataSet = /** @type {{route: string}} */(event.currentTarget.dataset);
     settings.navigateTo(settings.Route[dataSet.route]);
   },
 });