Support lists in <cr-settings-pref>

chrome/browser/resources/settings/prefs/prefs.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
  * 'cr-settings-prefs' models Chrome settings and preferences, listening for
  * changes to Chrome prefs whitelisted in chrome.settingsPrivate.
  * When changing prefs in this element's 'prefs' property via the UI, this
  * element tries to set those preferences in Chrome. Whether or not the calls to
@@ skipping 31 matching lines @@
 PrefWrapper.prototype.equals = function(other) {
   assert(this.key_ == other.key_);
   return this.value_ == other.value_;
 };
 
 /**
  * @constructor
  * @extends {PrefWrapper}
  * @param {!chrome.settingsPrivate.PrefObject} prefObj
  */
 function ListPrefWrapper(prefObj) {

[Dan Beam, 2015/09/17 06:36:56]

Why wouldn't we just make a DictPrefWrapper?

[michaelpg, 2015/09/17 17:53:47]

These wrappers don't make much sense to me so I'm not sure what a
DictPrefWrapper would look like. If anything I would get rid of the polymorphic
design entirely, or split it between FundamentalPrefWrapper and
ComplexPrefWrapper.

A ListPref could have Dictionaries, and a DictPref could have Lists. It would be
too much to have a ListPrefWrapper containing DictPrefWrappers containing
ListPrefWrappers.

Having a single, generalized, recursive "equals" function is simpler IMO than
having ListEqual, DictionaryEqual, etc. and having them call each other
recursively.

[Dan Beam, 2015/09/17 18:13:31]

that's not a problem
no, it wouldn't
no, it's not as it has every type's equality method baked in plus a whole ton of
if (i'm this type) forks.

[michaelpg, 2015/09/17 22:54:45]

I'm happy to separate out the equality methods, but I maintain that having a
class for each type (essentially, re-creating the JavaScript type system but
without considering prototypes) is, if not infeasible, an excessive amount of
work and introduces redundant time/space usage. Converting an arrays of objects
of arrays to a series of ListValues and DictionaryValues means creating a ton of
pointless Objects just so we can polymorphically test them.

   // Copy the array so changes to prefObj aren't reflected in this.value_.
-  // TODO(michaelpg): Do a deep copy to support nested lists and objects.
-  this.value_ = prefObj.value.slice();
+  this.value_ = PrefWrapper.deepCopy(prefObj.value);
 }
 
   ListPrefWrapper.prototype = {
   __proto__: PrefWrapper.prototype,
 
   /**
    * Tests whether two ListPrefWrapper values contain the same list items.
    * @override
    */
   equals: function(other) {
-    'use strict';
     assert(this.key_ == other.key_);
-    if (this.value_.length != other.value_.length)
-      return false;
-    for (let i = 0; i < this.value_.length; i++) {
-      if (this.value_[i] != other.value_[i])
-        return false;
-    }
-    return true;
+    return PrefWrapper.valuesEquivalent(this.value_, other.value_);
   },
 };
 
 (function() {
   'use strict';
 
+  /**
+   * @param {*} val1 Value to compare.
+   * @param {*} val2 Value to compare with val1.
+   * @return {boolean} True if the values are equivalent (equal
+   *     fundamental values, or objects whose elements are equivalent).
+   */
+  PrefWrapper.valuesEquivalent = function(val1, val2) {

[Dan Beam, 2015/09/17 06:36:56]

var toString = Object.prototype.toString;
if (toString.call(val1) != toString.call(val2))
  return false;

if (val1 == val2)
  return true;

if (val1 === null || typeof val1 != 'object')
  return false;

function len(val) {
  return (Array.isArray(val) ? val : Object.keys(val)).length;
}

if (len(val1) != len(val2))
  return false;

for (var key in val1) {
  if (!PrefWrapper.values equivalent(val1[key], val2[key]))
    return false;
}

return true;

[michaelpg, 2015/09/17 17:53:47]

I assume 80% of the time we'll be comparing booleans or numbers. Is it worth
converting those to strings?

+    if (val1 == val2)

[Dan Beam, 2015/09/17 06:36:56]

Eh probably don't do this without type checking first

[michaelpg, 2015/09/17 17:53:47]

Acknowledged.

[michaelpg, 2015/09/17 22:54:45]

Actually, in my new version I think checking "===" (strict equality, not loose
equality) simplifies the branching and should make the function cheaper on
average.

[Dan Beam, 2015/09/17 23:07:51]

=== is probably OK, yeah

+      return true;
+
+    if (Array.isArray(val1)) {
+      if (!Array.isArray(val2))
+        return false;
+      if (val1.length != val2.length)
+        return false;
+
+      for (var i = 0; i < val1.length; i++) {
+        if (!PrefWrapper.valuesEquivalent(val1[i], val2[i]))
+          return false;
+      }
+      return true;

[Dan Beam, 2015/09/17 18:13:31]

this is the ListPrefWrapper.equals implementation

+    }
+
+    if (typeof val1 == 'object') {

[Dan Beam, 2015/09/17 06:36:56]

What about {}, null?

[michaelpg, 2015/09/17 17:53:47]

{}: "just works", valuesEquivalent({}, {}) == true
null, undefined: added checks

[Dan Beam, 2015/09/17 18:13:31]

no, I meant val1 == {}, val2 == null

[michaelpg, 2015/09/17 22:54:45]

New version handles this case.

+      var props1 = Object.keys(val1);
+      var props2 = Object.keys(val2);
+      if (props1.length != props2.length)
+        return false;
+      for (var prop of props1) {
+        if (!PrefWrapper.valuesEquivalent(val1[prop], val2[prop]))
+          return false;
+      }
+      return true;

[Dan Beam, 2015/09/17 18:13:31]

this is the DictPrefWrapper.equals implementation

[michaelpg, 2015/09/17 22:54:45]

the problem is I don't want a ListPrefWrapper to be composed of
DictPrefWrappers, because a DictPrefWrapper should wrap a pref, not just a
value.

+    }
+
+    return false;
+  };
+
+  /**
+   * @param {*} val Value to copy.
+   * @return {*} A deep copy of the value.
+   */
+  PrefWrapper.deepCopy = function(val) {
+    if (Array.isArray(val)) {
+      var copy = [];
+      for (let el of val)
+        copy.push(PrefWrapper.deepCopy(el));
+      return copy;
+    }
+    if (typeof val == 'object') {

[Dan Beam, 2015/09/17 06:36:56]

null?

[michaelpg, 2015/09/17 17:53:47]

Done.

+      var copy = {};
+      for (let prop in val)
+        copy[prop] = PrefWrapper.deepCopy(val[prop]);
+    }
+    return val;
+  };
+
   Polymer({
     is: 'cr-settings-prefs',
 
     properties: {
       /**
        * Object containing all preferences, for use by Polymer controls.
        */
       prefs: {
         type: Object,
         notify: true,
@@ skipping 44 matching lines @@
       var prefWrapper = this.prefWrappers_[key];
       if (!prefWrapper)
         return;
 
       var prefObj = /** @type {chrome.settingsPrivate.PrefObject} */(
           this.get(key, this.prefs));
 
       // If settingsPrivate already has this value, do nothing. (Otherwise,
       // a change event from settingsPrivate could make us call
       // settingsPrivate.setPref and potentially trigger an IPC loop.)
-      if (prefWrapper.equals(this.createPrefWrapper_(prefObj)))
+      if (PrefWrapper.valuesEquivalent(prefWrapper.value_, prefObj.value))
         return;
 
       this.settingsApi_.setPref(
           key,
           prefObj.value,
           /* pageId */ '',
           /* callback */ this.setPrefCallback_.bind(this, key));
     },
 
     /**
@@ skipping 90 matching lines @@
      * @return {PrefWrapper} An object containing a copy of the PrefObject's
      *     value.
      * @private
      */
     createPrefWrapper_: function(prefObj) {
       return prefObj.type == chrome.settingsPrivate.PrefType.LIST ?
           new ListPrefWrapper(prefObj) : new PrefWrapper(prefObj);
     },
   });
 })();