[MD settings] Data driven content setting subtext

chrome/browser/resources/settings/site_settings_page/site_settings_page.js

Index: chrome/browser/resources/settings/site_settings_page/site_settings_page.js
diff --git a/chrome/browser/resources/settings/site_settings_page/site_settings_page.js b/chrome/browser/resources/settings/site_settings_page/site_settings_page.js
index 69701bf6cce10f83c60f14244e14a78ec611c81f..601d9e304066ec234ab20229aa23a6052058c7df 100644
--- a/chrome/browser/resources/settings/site_settings_page/site_settings_page.js
+++ b/chrome/browser/resources/settings/site_settings_page/site_settings_page.js
@@ -7,6 +7,7 @@
  * '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',
 
@@ -60,6 +61,21 @@ Polymer({
   },
 
   /**
+   * @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
    */
@@ -68,10 +84,7 @@ Polymer({
         category).then(function(defaultValue) {
           this.set(
               'default_.' + Polymer.CaseMap.dashToCamelCase(category),
-              this.computeCategoryDesc(
-                  category,
-                  defaultValue.setting,
-                  /*showRecommendation=*/false));
+              defaultValue.setting);
         }.bind(this));
   },
 
@@ -84,12 +97,9 @@ Polymer({
     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);
   },
 
   /**