Refactor Domado property definitions.

Previously, Domado internally used a property definition helper called
'definePropertiesAwesomely' which wrapped getters/setters so that they
get an additional argument which is the property name. We now replace
'definePropertiesAwesomely' with 'Props.define', where Props also
contains functions for generating property descriptors. Props.define
accepts, in addition to property descriptors, functions which generate
property descriptors given context ('env') information including the
property name, the object (thus being able to provide setOwn-like
override functionality), and the applicable Confidence.

Including the Confidence in the context also means that PropertyTaming
(renamed PT) does not need to be explicitly instantiated for a
particular Confidence; thus the vocabulary used for amplifying access is
not dependent on the the particular type being defined.

The context-based design of Props is also intended to be of future use
in refactoring the canvas 2D context taming to prototypical style while
keeping its short taming definitions.

User-visible changes:
* There may be more cases where read-only inherited properties can be
  overridden by assignment on instances.

Additional changes:
* Nearly all methods are now defined using Props.define rather than as
  TameFoo.prototype.bar = nodeAmp(function(privates) { ... }).
  Reordered so accessor definitions come before method definitions.
* Introduced PT.ROView to express non-writable tamed properties with
  a value transformation.
* browser-test-case.js no longer defines a guest '$' function (because
  document.getElementById ends up being an accessor property, which
  can't be retrieved without using an inES5Mode conditional) and tests
  which used it now define it internally.
* window's reflexive properties are defined in its constructor rather
  than separately.

@r5420

[felix8a, 2013-05-16 23:26:39]

I haven't figured out yet how to get solid results, but so far I'm not seeing
any noticeable runtime time or space difference with and without this patch, and
examining the code I'm not seeing anything that looks to me like it will reduce
runtime cost. What's the benefit you're expecting to get?

[kpreid2, 2013-05-16 23:35:18]

On 2013/05/16 23:26:39, felix8a wrote:
> I haven't figured out yet how to get solid results, but so far I'm not seeing
> any noticeable runtime time or space difference with and without this patch,
and
> examining the code I'm not seeing anything that looks to me like it will
reduce
> runtime cost. What's the benefit you're expecting to get?

The remarks about performance improvement were more along the lines of “well, it
*ought* to be faster” — by one less function call per property access
(specifically, the removal of definePropertiesAwesomely's getter/setter
wrappers). So, disregard that. I have removed performance claims from the
description.

This is really a refactoring for maintainability (and preparation for further
improvements), not optimization.

(On the upside, I'm glad to hear it isn't slower. I tried to design it so the
cost of additional abstraction was in startup time only.)

[felix8a, 2013-05-16 23:36:57]

On 2013/05/16 23:35:18, kpreid2 wrote:
> On 2013/05/16 23:26:39, felix8a wrote:
> > I haven't figured out yet how to get solid results, but so far I'm not
seeing
> > any noticeable runtime time or space difference with and without this patch,
> and
> > examining the code I'm not seeing anything that looks to me like it will
> reduce
> > runtime cost. What's the benefit you're expecting to get?
> 
> The remarks about performance improvement were more along the lines of “well,
it
> *ought* to be faster” — by one less function call per property access
> (specifically, the removal of definePropertiesAwesomely's getter/setter
> wrappers). So, disregard that. I have removed performance claims from the
> description.
> 
> This is really a refactoring for maintainability (and preparation for further
> improvements), not optimization.
> 
> (On the upside, I'm glad to hear it isn't slower. I tried to design it so the
> cost of additional abstraction was in startup time only.)

ah, ok. so far the refactoring looks ok to me, will finish reading it in a bit.

[kpreid2, 2013-05-20 23:01:55]

ping

[felix8a, 2013-05-21 19:43:50]

frankly I'm finding the new code much more confusing, and it's taking me a while
to convince myself it's doing the right thing. I think I'd be a little less
confused if instead of replacing Object.defineProperties with a function that
eventually calls Object.defineProperty, this were structured as something like
   Object.defineProperties(o, functionThatDoesTransformations(...,
propertydesc))
but that might not really help my core confusion, which is that I have trouble
modelling in my mind where 'env' comes from and where it's going.

after several reading sessions, I think I have a solid model of what's going on,
but I'm not very confident I can spot errors.

I'm about halfway done with domado.js, it'll take me about another day to
finish. I know this is blocking other work, so if you want a faster response
maybe corner someone to look over it in person?

https://codereview.appspot.com/9447044/diff/1/src/com/google/caja/plugin/doma...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/9447044/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:684: ?
opt_confidence.amplifying.bind(opt_confidence) : undefined
maybe hoist this out of the loop?

https://codereview.appspot.com/9447044/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:693: // Make a copy so that we can mutate
desc, and validate.
is copying necessary here? the only mutation is desc.enumerable for the firefox
workaround, and it's harmless if that mutation is persistent. also it looks to
me like desc is always derived from an object literal that isn't retained after
the call to define()

https://codereview.appspot.com/9447044/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:774: * The function should be a function
literal and will have innocuous()
"should be a function literal" is an odd constraint since whether or not a
function is a literal is unobservable, and I don't see why it matters?

[kpreid2, 2013-05-21 21:37:27]

Previously, Domado internally used a property definition helper called
'definePropertiesAwesomely' which wrapped getters/setters so that they
get an additional argument which is the property name. We now replace
'definePropertiesAwesomely' with 'Props.define', where Props also
contains functions for generating property descriptors. Props.define
accepts, in addition to property descriptors, functions which generate
property descriptors given context ('env') information including the
property name, the object (thus being able to provide setOwn-like
override functionality), and the applicable Confidence.

Including the Confidence in the context also means that PropertyTaming
(renamed PT) does not need to be explicitly instantiated for a
particular Confidence; thus the vocabulary used for amplifying access is
not dependent on the the particular type being defined.

The context-based design of Props is also intended to be of future use
in refactoring the canvas 2D context taming to prototypical style while
keeping its short taming definitions.

User-visible changes:
* There may be more cases where read-only inherited properties can be
  overridden by assignment on instances.

Additional changes:
* Nearly all methods are now defined using Props.define rather than as
  TameFoo.prototype.bar = nodeAmp(function(privates) { ... }).
  Reordered so accessor definitions come before method definitions.
* Introduced PT.ROView to express non-writable tamed properties with
  a value transformation.
* browser-test-case.js no longer defines a guest '$' function (because
  document.getElementById ends up being an accessor property, which
  can't be retireved without using an inES5Mode conditional) and tests
  which used it now define it internally.
* window's reflexive properties are defined in its constructor rather
  than separately.

[kpreid2, 2013-05-21 21:40:50]

New snapshot: changes as described and conflict resolution.

On 2013/05/21 19:43:50, felix8a wrote:
> frankly I'm finding the new code much more confusing, and it's taking me a
> while to convince myself it's doing the right thing.

That sounds like a bug to be fixed, and I'd like to do so rather than have you
work harder to understand it. One of the problems with Domado is that it has a
lot of structure and invariants that are not made sufficiently explicit.

Do you think additional documentation in some appropriate location would help?

Can you think of any other code changes to help clarify, beyond what I have now
done and describe below?

> I think I'd be a little less
> confused if instead of replacing Object.defineProperties with a function that
> eventually calls Object.defineProperty, this were structured as something like
>    Object.defineProperties(o, functionThatDoesTransformations(...,
> propertydesc))
> but that might not really help my core confusion, which is that I have trouble
> modelling in my mind where 'env' comes from and where it's going.

I could say that 'env' is all the bits of context for a given property
definition (what object, what the property name is, confidence, etc.) that would
be obvious to the programmer reading an ordinary defineProperties in
well-written code, but aren't actually available programmatically without the
'env' mechanism.

I have now refactored define() like so:

function define(object, opt_confidence, propSpecs) {
  var makeEnv = makeEnvOuter(object, opt_confidence);
  for (var prop in propSpecs) {
    var desc = specToDesc(makeEnv(prop), propSpecs[prop]);
    if (desc !== NO_PROPERTY) {
      Object.defineProperty(object, prop, desc);
    }
  }
}

That is, the "context" part is split out into makeEnv and clearly distinct from
the "transform property descs" part, and specToDesc takes care of all the
transformations. I think this helps make it clearer what define is doing by way
of sticking to high-level code; do you agree?

Also, would it be better to name it 'context' instead of 'env'?

> after several reading sessions, I think I have a solid model of what's going
> on, but I'm not very confident I can spot errors.
> 
> I'm about halfway done with domado.js, it'll take me about another day to
> finish. I know this is blocking other work, so if you want a faster response
> maybe corner someone to look over it in person?

That would have to be Ihab, and he can volunteer if he wants, but otherwise I'd
rather make sure this is understandable *without* having me explaining it in
person.

https://codereview.appspot.com/9447044/diff/1/src/com/google/caja/plugin/doma...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/9447044/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:684: ?
opt_confidence.amplifying.bind(opt_confidence) : undefined
On 2013/05/21 19:43:50, felix8a wrote:
> maybe hoist this out of the loop?

Done.

https://codereview.appspot.com/9447044/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:693: // Make a copy so that we can mutate
desc, and validate.
On 2013/05/21 19:43:50, felix8a wrote:
> is copying necessary here? the only mutation is desc.enumerable for the
> firefox workaround, and it's harmless if that mutation is persistent. also
> it looks to me like desc is always derived from an object literal that isn't
> retained after the call to define()

In previous work, I have been bitten by mutating property descriptors. I would
want strong evidence that it's valuable before I step away from observably
functional style.

https://codereview.appspot.com/9447044/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:774: * The function should be a function
literal and will have innocuous()
On 2013/05/21 19:43:50, felix8a wrote:
> "should be a function literal" is an odd constraint since whether or not a
> function is a literal is unobservable, and I don't see why it matters?

It is a deliberate style constraint rather than a semantic one. The actual point
is that these are things of which exactly one should be applied to a function,
and they mutate the function: if you *can't* write the arg as a function
literal, then something is suspicious.

I've rewritten the comments to make this more clear and added a similar comment
to innocuous().

[felix8a, 2013-05-22 21:26:40]

I'm about halfway through domado.js now, the env stuff is ok, the rest is just
slogging through all the changes that don't align in diff.

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:729: * where descriptor() passes plain
property descriptors through and can
this was basically my core confusion. descriptor() is not a real function. this
is not an actual description of what define() does. it's a simplification, and
the injection of env is not a trivial addition, because it captures state in a
nontrivial way, and it took me a while to decide that it's doing it correctly.
for instance, if I come at this code with a question like, "is it possible for
TameElement.getAttribute to receive the wrong privates", it takes me a while to
reach an answer, because of how env is passed around, and I'm not that confident
of my answer, because the chain of reasoning is too long to keep in my head all
at once.

now that I've looked at this several times, I've internalized a model of what
it's doing, so it's ok now, but it took a while to get there, and I'm not that
confident that I'd recognize if some future change has security implications.
I'm not sure how to fix that, so I'm ok with this for now.

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:3791: contains:
Props.cond(containsAvailable,
nontrivial Props.cond use is hard to read. maybe instead of cond(test, then,
else) make it cond(test1, then1, test2, then2, ...)?

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:3855: var textAccessor =
Props.rename('nodeValue', PT.filterProp(identity,
'rename' seems like the wrong verb. maybe something like 'useProp' or 'alias'
instead?

[felix8a, 2013-05-23 09:13:08]

ok, done reading domado.js.  lgtm++

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:393: get: cajaVM.constFunc(function
setOwnGetter() { return value; }),
innocuous()?

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:611: * As a matter of style, fn should
always be a function literal; think of this
this seems to be not true, since plainMethod calls innocuous(f). the f incoming
to plainMethod is supposedly a function literal too, but why wouldn't something
else also want to call plainMethod with a function that isn't a literal?

it's not really clear to me what's the distinction you're trying to make here.
what type of function should have constFunc() called instead of innocuous()? are
there calls to constFunc in this file that shouldn't be calls to innocuous?

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:2720: var NP_tameDescendant =
Props.markPropMaker(function(env) {
rename to PT_

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:3537: toString: Props.overridable(false,
cajaVM.constFunc(function() {
innocuous()?

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:4198: blur: tameVoidEdit,
"blur is a tameVoidEdit property" is a mysterious statement. how about call it
something like PT_noArgMethod. and likewise tameNodeEdit maybe should be
PT_noArgMethodReturningNode.

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:4310: // TODO(kpreid): This is a
non-standard method and looks like a relic
much older than domado. it was introduced in r797 and nobody used it.
https://code.google.com/p/google-caja/source/detail?r=797
I'm ok with deleting it.

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:4603: toString: cajaVM.constFunc(function()
{
innocuous

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:6586: toString: Props.overridable(false,
cajaVM.constFunc(function() {
innocuous()

[kpreid2, 2013-05-23 17:00:21]

Previously, Domado internally used a property definition helper called
'definePropertiesAwesomely' which wrapped getters/setters so that they
get an additional argument which is the property name. We now replace
'definePropertiesAwesomely' with 'Props.define', where Props also
contains functions for generating property descriptors. Props.define
accepts, in addition to property descriptors, functions which generate
property descriptors given context ('env') information including the
property name, the object (thus being able to provide setOwn-like
override functionality), and the applicable Confidence.

Including the Confidence in the context also means that PropertyTaming
(renamed PT) does not need to be explicitly instantiated for a
particular Confidence; thus the vocabulary used for amplifying access is
not dependent on the the particular type being defined.

The context-based design of Props is also intended to be of future use
in refactoring the canvas 2D context taming to prototypical style while
keeping its short taming definitions.

User-visible changes:
* There may be more cases where read-only inherited properties can be
  overridden by assignment on instances.

Additional changes:
* Nearly all methods are now defined using Props.define rather than as
  TameFoo.prototype.bar = nodeAmp(function(privates) { ... }).
  Reordered so accessor definitions come before method definitions.
* Introduced PT.ROView to express non-writable tamed properties with
  a value transformation.
* browser-test-case.js no longer defines a guest '$' function (because
  document.getElementById ends up being an accessor property, which
  can't be retireved without using an inES5Mode conditional) and tests
  which used it now define it internally.
* window's reflexive properties are defined in its constructor rather
  than separately.

[kpreid2, 2013-05-23 17:05:52]

Submitting since you said LGTM++, but Props.cond and Props.rename remain issues;
please comment and I'll fix appropriately in a separate change.

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:393: get: cajaVM.constFunc(function
setOwnGetter() { return value; }),
On 2013/05/23 09:13:08, felix8a wrote:
> innocuous()?

Eeh — not sure if it's the right thing here. But done.

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:611: * As a matter of style, fn should
always be a function literal; think of this
On 2013/05/23 09:13:08, felix8a wrote:
> this seems to be not true, since plainMethod calls innocuous(f). the f
> incoming to plainMethod is supposedly a function literal too, but why
> wouldn't something else also want to call plainMethod with a function
> that isn't a literal?

Can I plead distinction between 'framework internal' and 'user' code here?

> it's not really clear to me what's the distinction you're trying to make
> here. what type of function should have constFunc() called instead of
> innocuous()?

One which is not directly exposed to the guest, but we are defending out of
general capability style / strong modularity / defense in depth.

I agree that innocuous is possibly incoherent. I invented it while introducing
the nodeAmp thing with the notion that I could categorize all
exposed-to-the-guest functions and have test-scan-guest check that all found
functions fall into a suitable category, but I found that there are lots of
indirectly-visible functions that Domado itself isn't managing.

I'd like to keep it around for a bit and get a chance to try to, as a standalone
change, make the scanner actually do that checking.

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:729: * where descriptor() passes plain
property descriptors through and can
On 2013/05/22 21:26:40, felix8a wrote:
> this was basically my core confusion. descriptor() is not a real function.
this
> is not an actual description of what define() does.

OK, changed it to use the real specToDesc, but kept the vague "can also
construct" language because being more specific ends up being duplicating the
code.

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:2720: var NP_tameDescendant =
Props.markPropMaker(function(env) {
On 2013/05/23 09:13:08, felix8a wrote:
> rename to PT_

No, PT is generic, NP is for nodes. Naming might need work, but I'd like to do
that separately.

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:3537: toString: Props.overridable(false,
cajaVM.constFunc(function() {
On 2013/05/23 09:13:08, felix8a wrote:
> innocuous()?

Done.

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:3537: toString: Props.overridable(false,
cajaVM.constFunc(function() {
On 2013/05/23 09:13:08, felix8a wrote:
> innocuous()?

Done.

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:3791: contains:
Props.cond(containsAvailable,
On 2013/05/22 21:26:40, felix8a wrote:
> nontrivial Props.cond use is hard to read. maybe instead of cond(test,
> then, else) make it cond(test1, then1, test2, then2, ...)?

Now it feels like we're heading in the direction of Greenspun's tenth rule.

Should we just use ?: and expose Props.NO_PROPERTY, either for all cases, or for
the complex ones and keep Props.cond in two-arg form?

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:3855: var textAccessor =
Props.rename('nodeValue', PT.filterProp(identity,
On 2013/05/22 21:26:40, felix8a wrote:
> 'rename' seems like the wrong verb. maybe something like 'useProp' or 'alias'
> instead?

I agree the names here are bad, but I haven't been able to think of better ones.
Further suggestions welcome.

'useProp' is uncommunicative to me. 'alias' is better, but I already use 'alias'
for something else! Also, 'rename' was the name for this concept in the old
PropertyTaming as well.

Maybe something like 'altTargetProp' or 'altFeralProp'?

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:4198: blur: tameVoidEdit,
On 2013/05/23 09:13:08, felix8a wrote:
> "blur is a tameVoidEdit property" is a mysterious statement. how about call it
> something like PT_noArgMethod. and likewise tameNodeEdit maybe should be
> PT_noArgMethodReturningNode.

Done, but with:
NP_noArgEditVoidMethod
NP_noArgEditMethodReturningNode

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:4310: // TODO(kpreid): This is a
non-standard method and looks like a relic
On 2013/05/23 09:13:08, felix8a wrote:
> much older than domado. it was introduced in r797 and nobody used it.
> https://code.google.com/p/google-caja/source/detail?r=797
> I'm ok with deleting it.

OK, but not in this change.

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:4603: toString: cajaVM.constFunc(function()
{
On 2013/05/23 09:13:08, felix8a wrote:
> innocuous

Done.

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:6586: toString: Props.overridable(false,
cajaVM.constFunc(function() {
On 2013/05/23 09:13:08, felix8a wrote:
> innocuous()

Done.

[felix8a, 2013-05-23 19:07:24]

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:3791: contains:
Props.cond(containsAvailable,
On 2013/05/23 17:05:52, kpreid2 wrote:
> On 2013/05/22 21:26:40, felix8a wrote:
> > nontrivial Props.cond use is hard to read. maybe instead of cond(test,
> > then, else) make it cond(test1, then1, test2, then2, ...)?
> 
> Now it feels like we're heading in the direction of Greenspun's tenth rule.
> 
> Should we just use ?: and expose Props.NO_PROPERTY, either for all cases, or
for
> the complex ones and keep Props.cond in two-arg form?

ok, I'm fine with that

https://codereview.appspot.com/9447044/diff/8001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:3855: var textAccessor =
Props.rename('nodeValue', PT.filterProp(identity,
On 2013/05/23 17:05:52, kpreid2 wrote:
> On 2013/05/22 21:26:40, felix8a wrote:
> > 'rename' seems like the wrong verb. maybe something like 'useProp' or
'alias'
> > instead?
> 
> I agree the names here are bad, but I haven't been able to think of better
ones.
> Further suggestions welcome.
> 
> 'useProp' is uncommunicative to me. 'alias' is better, but I already use
'alias'
> for something else! Also, 'rename' was the name for this concept in the old
> PropertyTaming as well.
> 
> Maybe something like 'altTargetProp' or 'altFeralProp'?

retargetTo? actAs?