Automate reflected properties in SkyElement.

Automate reflected properties in SkyElement.

Now you can use the attributes attribute on <sky-element> that
accepts a set of name:type comma separated pairs to control
the reflected attributes. ex. attributes="size:number"

Each attribute creates a getter and a setter on the element that returns
the correct type and will coerce the string attribute values into the
right type. It also correctly hooks into the data binding system.

A new callback is added: {name}Changed(oldValue, newValue) which will
be invoked whenever the attribute with that name changes and gets the
correctly coerced type values. See the <sky-radio> or <sky-checkbox>
widgets for examples of using the new callback.

Number attributes default to 0, booleans to false, and strings to empty
string. There is no way provided to set a different default, for that you
can use hasAttribute in the created callback to conditionally set a value.
Don't just assign the property there otherwise you'll overwrite the value
from the parser.

Another behavior change from making this work is that now attributeChanged()
is called for each attribute, even when the element is created by the
parser.

Overall this allows a nice simplification to the <sky-button>,
<sky-radio> and <sky-checkbox> widgets.

R=abarth@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/a0235a16d45cfaca40274db73e84bcf6bd25c72d

[esprehn, 2015-01-06 03:51:42]

esprehn@chromium.org changed reviewers:
+ abarth@chromium.org

[abarth-chromium, 2015-01-06 04:02:38]

LGTM

https://codereview.chromium.org/836923002/diff/1/sky/framework/sky-element/sk...
File sky/framework/sky-element/sky-element.sky (right):

https://codereview.chromium.org/836923002/diff/1/sky/framework/sky-element/sk...
sky/framework/sky-element/sky-element.sky:28: if (value == null)
Why handle |null| special but not |undefined|?

https://codereview.chromium.org/836923002/diff/1/sky/framework/sky-element/sk...
sky/framework/sky-element/sky-element.sky:35: function
defineReflectedAttribute(prototype, descriptor, name) {
Why not just pass in the convert function instead of the whole descriptor?

https://codereview.chromium.org/836923002/diff/1/sky/framework/sky-element/sk...
sky/framework/sky-element/sky-element.sky:60: var type = (parts[1] ||
"").trim();
s/""/''/ for consistency

https://codereview.chromium.org/836923002/diff/1/sky/framework/sky-radio/sky-...
File sky/framework/sky-radio/sky-radio.sky (right):

https://codereview.chromium.org/836923002/diff/1/sky/framework/sky-radio/sky-...
sky/framework/sky-radio/sky-radio.sky:55: }
Why add the braces?

[esprehn, 2015-01-06 04:10:24]

https://codereview.chromium.org/836923002/diff/1/sky/framework/sky-element/sk...
File sky/framework/sky-element/sky-element.sky (right):

https://codereview.chromium.org/836923002/diff/1/sky/framework/sky-element/sk...
sky/framework/sky-element/sky-element.sky:28: if (value == null)
On 2015/01/06 at 04:02:38, abarth wrote:
> Why handle |null| special but not |undefined|?

undefined is itself a string, if I special case that you could never have a
string with the value "undefined" which seems bad.

<foo-widget foo="undefined"> would result in the value of foo being "".

The special case for null is because if an attribute is not present
getAttribute() returns null, and you don't want to get back "null", you want ""
then.

https://codereview.chromium.org/836923002/diff/1/sky/framework/sky-element/sk...
sky/framework/sky-element/sky-element.sky:35: function
defineReflectedAttribute(prototype, descriptor, name) {
On 2015/01/06 at 04:02:38, abarth wrote:
> Why not just pass in the convert function instead of the whole descriptor?

Done, originally I had a more complex idea for how to do this, we can expand it
later.

https://codereview.chromium.org/836923002/diff/1/sky/framework/sky-element/sk...
sky/framework/sky-element/sky-element.sky:60: var type = (parts[1] ||
"").trim();
On 2015/01/06 at 04:02:38, abarth wrote:
> s/""/''/ for consistency

done.

https://codereview.chromium.org/836923002/diff/1/sky/framework/sky-radio/sky-...
File sky/framework/sky-radio/sky-radio.sky (right):

https://codereview.chromium.org/836923002/diff/1/sky/framework/sky-radio/sky-...
sky/framework/sky-radio/sky-radio.sky:55: }
On 2015/01/06 at 04:02:38, abarth wrote:
> Why add the braces?

Woops, that was from debugging.

[esprehn, 2015-01-06 04:13:02]

Committed patchset #4 (id:60001) manually as
a0235a16d45cfaca40274db73e84bcf6bd25c72d (presubmit successful).

[abarth-chromium, 2015-01-06 05:17:45]

On 2015/01/06 at 04:10:24, esprehn wrote:
>
https://codereview.chromium.org/836923002/diff/1/sky/framework/sky-element/sk...
> File sky/framework/sky-element/sky-element.sky (right):
> 
>
https://codereview.chromium.org/836923002/diff/1/sky/framework/sky-element/sk...
> sky/framework/sky-element/sky-element.sky:28: if (value == null)
> On 2015/01/06 at 04:02:38, abarth wrote:
> > Why handle |null| special but not |undefined|?
> 
> undefined is itself a string, if I special case that you could never have a
string with the value "undefined" which seems bad.

What you've got here seems fine, but I don't think undefined itself is a string:

$ undefined == "undefined"
false
$ undefined === "undefined"
false

[esprehn, 2015-01-06 05:27:51]

On 2015/01/06 at 05:17:45, abarth wrote:
> On 2015/01/06 at 04:10:24, esprehn wrote:
> >
https://codereview.chromium.org/836923002/diff/1/sky/framework/sky-element/sk...
> > File sky/framework/sky-element/sky-element.sky (right):
> > 
> >
https://codereview.chromium.org/836923002/diff/1/sky/framework/sky-element/sk...
> > sky/framework/sky-element/sky-element.sky:28: if (value == null)
> > On 2015/01/06 at 04:02:38, abarth wrote:
> > > Why handle |null| special but not |undefined|?
> > 
> > undefined is itself a string, if I special case that you could never have a
string with the value "undefined" which seems bad.
> 
> What you've got here seems fine, but I don't think undefined itself is a
string:
> 
> $ undefined == "undefined"
> false
> $ undefined === "undefined"
> false

Heh, seems I'm thinking of a bygone era. I'll change this as you suggested.

[ojan, 2015-01-07 04:30:32]

ojan@chromium.org changed reviewers:
+ ojan@chromium.org

[ojan, 2015-01-07 04:30:32]

not lgtm.

Why do we want reflected attributes? This is one of the things the web got wrong
IMO. We should have properties and attributes and they should stay separate. If
we want an easy way to access attributes, we can add an attrs thing off the
element, so you can do this.attrs.highlight = true.

Blurring the line makes for confusing things like this.id and this.value that we
got rid of. You can never tell if something is an attribute or a property and if
setting it is going to be accidentally expensive.

[esprehn, 2015-01-07 04:42:03]

On 2015/01/07 at 04:30:32, ojan wrote:
> not lgtm.
> 
> Why do we want reflected attributes? This is one of the things the web got
wrong IMO. We should have properties and attributes and they should stay
separate. If we want an easy way to access attributes, we can add an attrs thing
off the element, so you can do this.attrs.highlight = true.
> 
> Blurring the line makes for confusing things like this.id and this.value that
we got rid of. You can never tell if something is an attribute or a property and
if setting it is going to be accidentally expensive.

We should just delete attribute selectors then, it doesn't make any sense to
have data duplicated in both properties and attributes, that just leads to bugs.