[MD settings] content category policy

chrome/browser/resources/settings/site_settings/site_settings_category.js

Index: chrome/browser/resources/settings/site_settings/site_settings_category.js
diff --git a/chrome/browser/resources/settings/site_settings/site_settings_category.js b/chrome/browser/resources/settings/site_settings/site_settings_category.js
index 63cc5ace2c53df64ad00e0278f10d3d4eb37921d..1c3b296ef8860340377c3cb8b374d471202a782b 100644
--- a/chrome/browser/resources/settings/site_settings/site_settings_category.js
+++ b/chrome/browser/resources/settings/site_settings/site_settings_category.js
@@ -13,15 +13,36 @@ Polymer({
   behaviors: [SiteSettingsBehavior, WebUIListenerBehavior],
 
   properties: {
+    /** @private {chrome.settingsPrivate.PrefObject} */
+    fakePref_: {
+      type: Object,
+      value: function() {
+        return /** @type {chrome.settingsPrivate.PrefObject} */({});
+      },
+    },
+
     /**
-     * Represents the state of the main toggle shown for the category. For
-     * example, the Location category can be set to Block/Ask so false, in that
-     * case, represents Block and true represents Ask.
+     * Cookies and Flash settings have a sub-preference that is used to mimic a
+     * tri-state value.
+     * @private {chrome.settingsPrivate.PrefObject}
      */
-    categoryEnabled: Boolean,
+    fakeSubPref_: {
+      type: Object,
+      value: function() {
+        return /** @type {chrome.settingsPrivate.PrefObject} */({});
+      },
+    },

[Dan Beam, 2016/11/29 05:17:14]

why is this needed?

[dschuyler, 2016/11/29 22:28:12]

This is a more generic way of handling the flashAskFirst_
and cookiesSessionOnly_ member variables. Having a sub-pref
allows any category to have a tri-state value without adding
a special variable for each one and hand-working it into
the code. At the moment, Flash and Cookies are the only two
with tri-states. I would not be surprised if one or two others
became tri-states and this sub-pref will make supporting them
easier.

+
+    /** @private {!DefaultContentSetting} */
+    currentSetting_: {

[Dan Beam, 2016/11/29 05:17:14]

can this be default_ or like categoryDefault_?

[dschuyler, 2016/11/29 22:28:12]

How about priorDefaultContentSetting_?

+      type: Object,
+      value: function() {
+        return /** @type {DefaultContentSetting} */({});
+      },
+    },
 
     /**
-     * The site that was selected by the user in the dropdown list.
+     * The site that is passed down to the site list.
      * @type {SiteException}
      */
     selectedSite: {
@@ -31,54 +52,30 @@ Polymer({
 
     /**
      * The description to be shown next to the slider.
+     * @private
      */
     sliderDescription_: String,
-
-    /**
-     * Used only for the Flash to persist the Ask First checkbox state.
-     * Defaults to true, as the checkbox should be checked unless the user
-     * has explicitly unchecked it or has the ALLOW setting on Flash.
-     */
-    flashAskFirst_: {
-      type: Boolean,
-      value: true,
-    },
-
-    /**
-     * Used only for Cookies to keep track of the Session Only state.
-     * Defaults to true, as the checkbox should be checked unless the user
-     * has explicitly unchecked it or has the ALLOW setting on Cookies.
-     */
-    cookiesSessionOnly_: {
-      type: Boolean,
-      value: true,
-    },

[Dan Beam, 2016/11/29 05:17:14]

ugh, why were specific options in the generic class?  that doesn't make any
sense to me.

[dschuyler, 2016/11/29 22:28:12]

Not sure.

[Dan Beam, 2016/12/01 00:10:54]

you reviewed one of these
https://codereview.chromium.org/2316633002/diff/20001/chrome/browser/resource...

tommycli@ added the other
https://codereview.chromium.org/2280233002/diff/60001/chrome/browser/resource...

can't we just do something like <content select=".extra-options"> or something?

   },
 
   observers: [
     'onCategoryChanged_(category)',
+    'onChangePermissionControl_(category, fakePref_.value)',
+    'onChangePermissionControl_(category, fakeSubPref_.value)',
   ],
 
   ready: function() {
     this.addWebUIListener('contentSettingCategoryChanged',
-        this.defaultValueForCategoryChanged_.bind(this));
-  },
-
-  /**
-   * Called when the default value for a category has been changed.
-   * @param {number} category The category that changed.
-   * @private
-   */
-  defaultValueForCategoryChanged_: function(category) {
-    if (category == this.category)
-      this.onCategoryChanged_();
+        this.onCategoryChanged_.bind(this));
   },
 
   /**
    * A handler for changing the default permission value for a content type.
    * @private
    */
-  onChangePermissionControl_: function(event) {
+  onChangePermissionControl_: function() {
+    if (!this.category)
+      return;
+
     switch (this.category) {
       case settings.ContentSettingsTypes.BACKGROUND_SYNC:
       case settings.ContentSettingsTypes.IMAGES:
@@ -89,7 +86,7 @@ Polymer({
         // "Allowed" vs "Blocked".
         this.browserProxy.setDefaultValueForContentType(
             this.category,
-            this.categoryEnabled ?
+            this.fakePref_.value ?
                 settings.PermissionValues.ALLOW :
                 settings.PermissionValues.BLOCK);
         break;
@@ -102,16 +99,18 @@ Polymer({
         // "Ask" vs "Blocked".
         this.browserProxy.setDefaultValueForContentType(
             this.category,
-            this.categoryEnabled ?
+            this.fakePref_.value ?
                 settings.PermissionValues.ASK :
                 settings.PermissionValues.BLOCK);
         break;
       case settings.ContentSettingsTypes.COOKIES:
         // This category is tri-state: "Allow", "Block", "Keep data until
         // browser quits".
+        if (this.fakeSubPref_.value === undefined)
+          break;
         var value = settings.PermissionValues.BLOCK;
-        if (this.categoryEnabled) {
-          value = this.cookiesSessionOnly_ ?
+        if (this.fakePref_.value) {
+          value = this.fakeSubPref_.value ?
               settings.PermissionValues.SESSION_ONLY :
               settings.PermissionValues.ALLOW;
         }
@@ -119,9 +118,11 @@ Polymer({
         break;
       case settings.ContentSettingsTypes.PLUGINS:
         // This category is tri-state: "Allow", "Block", "Ask before running".
+        if (this.fakeSubPref_.value === undefined)
+          break;
         var value = settings.PermissionValues.BLOCK;
-        if (this.categoryEnabled) {
-          value = this.flashAskFirst_ ?
+        if (this.fakePref_.value) {
+          value = this.fakeSubPref_.value ?
               settings.PermissionValues.IMPORTANT_CONTENT :
               settings.PermissionValues.ALLOW;
         }
@@ -133,16 +134,69 @@ Polymer({
   },
 
   /**
+   * Update the fake pref value from the content settings.
+   * @param {!DefaultContentSetting} update
+   * @private
+   */
+  updateFakePrefs_: function(update) {
+    // Early out if there is no actual change.
+    if (this.currentSetting_.setting == update.setting &&
+        this.currentSetting_.source == update.source)
+      return;
+    this.currentSetting_ = update;
+
+    // TODO(dschuyler): The concept of using fake prefs (here and elsewhere)
+    // should be rethought. This is considered okay in a pinch, but not a
+    // great way to get the policy controls. See http://crbug.com/666164
+    var basePref = {
+      'key': 'fakeDefaultSettingPref',
+      'type': 'BOOLEAN',
+    };
+    if (update.source !== undefined &&
+        update.source != ContentSettingProvider.PREFERENCE) {
+      basePref.enforcement = chrome.settingsPrivate.Enforcement.ENFORCED;
+      basePref.controlledBy =
+          update.source == ContentSettingProvider.EXTENSION ?
+          chrome.settingsPrivate.ControlledBy.EXTENSION :
+          chrome.settingsPrivate.ControlledBy.USER_POLICY;
+    }
+
+    var prefValue = this.computeIsSettingEnabled(update.setting);
+    // The fakePref_ must be replaced (rather than just value changes) so that
+    // observers will be notified of the change.
+    this.fakePref_ = /** @type {chrome.settingsPrivate.PrefObject} */(
+        Object.assign({'value': prefValue}, basePref));

[Dan Beam, 2016/11/29 05:17:14]

why are you using Object.assign() rather than just

basePref.value = true;
...
this.fakePref_ = basePref;

[dschuyler, 2016/11/29 22:28:12]

basePref is used twice, once here and 
again on line 184. If I set value directly it will (I believe)
become the single instance of value that will 
be seen by fakePref and fakeSubPref. I'm looking to
have two separate instances of value with a separate 
pref referencing each value.

+
+    if (this.category == settings.ContentSettingsTypes.PLUGINS ||
+        this.category == settings.ContentSettingsTypes.COOKIES) {
+      var subPrefValue = false;
+      if (this.category == settings.ContentSettingsTypes.PLUGINS &&
+          update.setting == settings.PermissionValues.IMPORTANT_CONTENT) {
+        subPrefValue = true;
+      } else if (this.category == settings.ContentSettingsTypes.COOKIES &&
+          update.setting == settings.PermissionValues.SESSION_ONLY) {
+        subPrefValue = true;
+      }
+
+      // The fakeSubPref_ must be replaced (rather than just value changes) so
+      // that observers will be notified of the change.
+      this.fakeSubPref_ = /** @type {chrome.settingsPrivate.PrefObject} */(
+          Object.assign({'value': subPrefValue}, basePref));
+    }
+  },
+
+  /**
    * Handles changes to the category pref and the |category| member variable.
    * @private
    */
   onCategoryChanged_: function() {
     settings.SiteSettingsPrefsBrowserProxyImpl.getInstance()
         .getDefaultValueForContentType(
-            this.category).then(function(setting) {
-              this.categoryEnabled = this.computeIsSettingEnabled(setting);
+            this.category).then(function(defaultContentSetting) {
+              this.updateFakePrefs_(defaultContentSetting);
 
               // Flash only shows ALLOW or BLOCK descriptions on the slider.
+              var setting = defaultContentSetting.setting;
               var sliderSetting = setting;
               if (this.category == settings.ContentSettingsTypes.PLUGINS &&
                   setting == settings.PermissionValues.IMPORTANT_CONTENT) {
@@ -154,21 +208,6 @@ Polymer({
               }
               this.sliderDescription_ =
                   this.computeCategoryDesc(this.category, sliderSetting, true);
-
-              if (this.category == settings.ContentSettingsTypes.PLUGINS) {
-                // The checkbox should only be cleared when the Flash setting
-                // is explicitly set to ALLOW.
-                if (setting == settings.PermissionValues.ALLOW)
-                  this.flashAskFirst_ = false;
-                if (setting == settings.PermissionValues.IMPORTANT_CONTENT)
-                  this.flashAskFirst_ = true;
-              } else if (
-                  this.category == settings.ContentSettingsTypes.COOKIES) {
-                if (setting == settings.PermissionValues.ALLOW)
-                  this.cookiesSessionOnly_ = false;
-                else if (setting == settings.PermissionValues.SESSION_ONLY)
-                  this.cookiesSessionOnly_ = true;
-              }
             }.bind(this));
   },