cr-settings-checkbox: Add 'inverted' property.

chrome/browser/resources/settings/checkbox/checkbox.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-checkbox` is a checkbox that controls a supplied preference.
  *
  * Example:
  *      <cr-settings-checkbox pref="{{prefs.settings.enableFoo}}"
  *          label="Enable foo setting." subLabel="(bar also)">
  *      </cr-settings-checkbox>
  *
  * @element cr-settings-checkbox
  */
 Polymer({
   is: 'cr-settings-checkbox',
 
   properties: {
     /**
      * The boolean preference object to control.
      * @type {?chrome.settingsPrivate.PrefObject}
      */
     pref: {
       type: Object,
       notify: true,
+      value: function() { return {}; }

[Jeremy Klein, 2015/06/04 00:49:54]

Why do we need this? Doesn't it break the pref-tracker?

[Oren Blasberg, 2015/06/04 16:10:04]

Oddly, without the initial value, I get an error in checkedChanged_ that
'this.pref' is undefined. I'm not sure I understand why; maybe we can look at it
tomorrow.

Having this initial value doesn't seem to break the pref tracker; could you
touch upon why you think it would?

[Jeremy Klein, 2015/06/04 17:20:16]

Sorry, by "breaking" I mean preventing real errors from being found. If you set
a default here to {}, this line will always be false:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

and so the errors won't get picked up. What's the benefit of the default value?

[Oren Blasberg, 2015/06/04 18:26:11]

Oh, I see. Yes, the validation would always succeed.

There isn't a 'benefit' to having {} here, except that it prevents the error I
mentioned in the previous comment, in checkedChanged_. But I still don't get why
that error happens in the first place, since checkedChanged_ should only be
getting called if the 'checked' actually changes (which I don't think happens on
initialization?)... 

Let's look at this in person later.

+    },
+
+    inverted: {
+      type: Boolean,
+      value: false
+    },
+
+    checked: {
+      type: Boolean,
+      value: false,
+      observer: 'checkedChanged'
     },
 
     label: {
       type: String,
       value: '',
     },
 
     subLabel: {
       type: String,
       value: '',
     },
   },
 
+  observers: [
+    'prefValueChanged(pref.value)'
+  ],
+
   /** @override */
   ready: function() {
     this.$.events.forward(this.$.checkbox, ['change']);
   },
+
+  prefValueChanged: function(prefValue) {

[Jeremy Klein, 2015/06/04 00:49:54]

This and the other 2 new functions can be private.

[Oren Blasberg, 2015/06/04 16:10:04]

Done.

+    if (this.pref) {
+      this.checked = this.computeValue(prefValue);
+    }
+  },
+
+  checkedChanged: function() {
+    this.pref.value = this.computeValue(this.checked);
+  },
+
+  computeValue: function(val) {

[Jeremy Klein, 2015/06/04 00:49:54]

For some reason the name computeValue confuses me because it's used for both
checked and pref.value. Maybe "invertIfNeeded_"?

[Oren Blasberg, 2015/06/04 16:10:04]

Ah ok. It's not actually inverting though; it's just returning a value, so how
about "getPrefValue_" or "getValueToPersist_"?

[Jeremy Klein, 2015/06/04 17:20:16]

The this is that it doesn't only get the prefValue or the value to persist
because you're using it to go both directions:

pref -> this.checked
and this.checked -> pref

[Oren Blasberg, 2015/06/04 18:26:11]

Ok, how about "getUpdatedValue_"?   "getValueBasedOnInversion_"? ...
Because this doesn't actually do inversion, it just returns a bool I am hesitant
to call it "invertIfNeeded_". As you pointed out, the field that gets inverted
(if needed) depends on who called the method.

+    return this.inverted ? !val : val;
+  }
 });