[Chrome Settings UI] Show overruled User Exceptions as strike-through.

chrome/browser/resources/options/content_settings_exceptions_area.js

 // Copyright (c) 2012 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.
 
 cr.define('options.contentSettings', function() {
   /** @const */ var ControlledSettingIndicator =
                     options.ControlledSettingIndicator;
   /** @const */ var InlineEditableItemList = options.InlineEditableItemList;
   /** @const */ var InlineEditableItem = options.InlineEditableItem;
   /** @const */ var ArrayDataModel = cr.ui.ArrayDataModel;
@@ skipping 187 matching lines @@
               this.dataItem.source : null;
 
       if (controlledBy) {
         this.setAttribute('controlled-by', controlledBy);
         this.deletable = false;
         this.editable = false;
       }
 
       if (controlledBy == 'policy' || controlledBy == 'extension') {
         this.querySelector('.row-delete-button').hidden = true;
         var indicator = new ControlledSettingIndicator();

[Bernhard Bauer, 2016/05/19 14:20:40]

This appears to duplicate the code you added. Could we extract this into a
shared method?

[huangs, 2016/05/20 15:16:46]

Done.

         indicator.setAttribute('content-exception', this.contentType);
         // Create a synthetic pref change event decorated as
         // CoreOptionsHandler::CreateValueForPref() does.
         var event = new Event(this.contentType);
         event.value = { controlledBy: controlledBy };
         indicator.handlePrefChange(event);
         this.appendChild(indicator);
       }
 
       // If the exception comes from a hosted app, display the name and the
@@ skipping 119 matching lines @@
      */
     updateEditables: function() {
       this.resetInput();
 
       var settingOption =
           this.select.querySelector('[value=\'' + this.setting + '\']');
       if (settingOption)
         settingOption.selected = true;
     },
 
+    /**
+     * Updates UI to indicate that the exception was overruled by a source.
+     *
+     * @param {string} overruledBy The source that overrules the exception.
+     */
+    setOverruledBy: function(overruledBy) {
+      this.classList.toggle('overruled', !!overruledBy);
+      var textElt = this.querySelector('.exception-pattern .static-text');
+      textElt.setAttribute('title',
+          loadTimeData.getString('exceptionDisabledByPolicy'));
+
+      var indicator = new ControlledSettingIndicator();
+      indicator.setAttribute('content-exception', this.contentType);
+      // Create a synthetic pref change event decorated as
+      // CoreOptionsHandler::CreateValueForPref() does.
+      var event = new Event(this.contentType);
+      event.value = { controlledBy: overruledBy };
+      indicator.handlePrefChange(event);
+      this.appendChild(indicator);

[Bernhard Bauer, 2016/05/18 09:55:13]

What happens if this is called multiple times (e.g. if content settings change)?
Would we just append indicators?

[huangs, 2016/05/18 13:52:49]

If content settings change then the entire list gets regenerated.  However,
relying on call patterns seem kinda fragile.  Please confirm you want
unification code, then I'll add it (extra unused complexity to be defensive).

+    },
+
     /** @override */
     get currentInputIsValid() {
       return this.inputValidityKnown && this.inputIsValid;
     },
 
     /** @override */
     get hasBeenEdited() {
       var livePattern = this.input.value;
       var liveSetting = this.select.value;
       return livePattern != this.pattern || liveSetting != this.setting;
@@ skipping 155 matching lines @@
                                       entry);
       } else {
         var addRowItem = new ExceptionsAddRowListItem(this.contentType,
                                                       this.mode);
         addRowItem.deletable = false;
         return addRowItem;
       }
     },
 
     /**
+     * Updates UI to indicate that user exceptions were overruled by a source.
+     *
+     * @param {string} overruledBy The source that overrules user exceptions.
+     */
+    setOverruledBy: function(overruledBy) {
+      for (var index = 0; index < this.dataModel.length; ++index) {
+        var item = this.getListItemByIndex(index);
+        if (item.dataItem.source == 'preference') {

[Bernhard Bauer, 2016/05/18 09:55:13]

Doesn't this mean that any preference rule is marked as overruled by any
higher-level rule, even if the higher-level rule doesn't apply to the same
origins? E.g., if an extension sets a content setting for google.com, a
preferece rule for yahoo.com would show up as overruled?

[huangs, 2016/05/18 13:52:49]

The "overruled" concept only applies to Policy Defaults and Extension Defaults. 
We don't count exception overrides (inlcuding wildcards).

[Bernhard Bauer, 2016/05/19 14:20:40]

I don't really want to get into a product discussion, but... why don't we?
Historically, we tried not to treat default settings vastly different from
exceptions (default settings are just rules with wildcard patterns).

[huangs, 2016/05/20 15:16:46]

I think treating default as "super wildcard exception" would belong in the
"model" layer.  However, we separate these in the UI, so having a separate
treatment would belong in the "view" and "controller" layer, so maybe there's no
conflict fundamentally?

[Bernhard Bauer, 2016/05/23 13:34:11]

I'm not sure what you are trying to say. This UI will not in all cases where a
rule does not have any effect show it as overruled, which I would consider a
bug. It's true that the UI for a default setting looks different from the UI for
an exception, but
a) that is only the case for user-defined content settings, and this is for
policy- or extension-defined content settings, and
b) the question of whether a rule is effective or not (which is what we want to
show here) is independent of the UI treatment.

I mean, if you want to land this, I won't stop you, but like I said above, to me
this code is not free of bugs, and I would like to see them addressed at least
in the future.

[huangs, 2016/05/24 05:02:00]

By "bug" do you mean:

(1) Code is not doing what is specified, or
(2) The specification is wrong?

My take: The specification is "A User Exception is 'overruled' if it becomes
ineffective due to Policy Default or Extension Default settings.  For
'overruled' user exception, change UI to draw line-through.".

Are you saying that the spec should be: "A User Exception is 'overruled' if
anything, i.e., {Policy, Extension} {Default, Exceptions} make it ineffective. 
For 'overruled' user exception, change UI to..."

If so, then we are disagreeing on (2), and so I'd think "bug" is too strong;
perhaps "incomplete fix"?

If you mean something else, then sorry for misunderstanding!  I'd like to sort
this out before proceeding.

[Bernhard Bauer, 2016/05/24 16:54:34]

Yes, that's what I mean.
What's the difference? I mean, I agree that this change is better than nothing
(which is why I'm not objecting to it in principle), but the part not fixed by
an incomplete fix would be a bug, no?

+          item.setOverruledBy(overruledBy);
+        }
+      }
+    },
+
+    /**
      * Sets the exceptions in the js model.
      *
      * @param {Array<options.Exception>} entries A list of dictionaries of
      *     values, each dictionary represents an exception.
      */
     setExceptions: function(entries) {
       var deleteCount = this.dataModel.length;
 
       if (this.isEditable()) {
         // We don't want to remove the Add New Exception row.
@@ skipping 163 matching lines @@
     }
   };
 
   return {
     ExceptionsListItem: ExceptionsListItem,
     ExceptionsAddRowListItem: ExceptionsAddRowListItem,
     ExceptionsList: ExceptionsList,
     ContentSettingsExceptionsArea: ContentSettingsExceptionsArea,
   };
 });