Fixes uses of cajita's taming API and improves primFreeze speed.

(The following text appears in 'myvn describe' but was not showing here on appspot. I don't know why.

Fixes uses of cajita's taming API and improves primFreeze speed.

Deprecated in ___, in anticipation of removal:
* grantEnumOnly, grantCall, grantGeneric, useApplyHandler, useCallHandler,
  handleGeneric, grantTypedGeneric, grantMutator, useDeleteHandler,
  isXo4aFunc, xo4a. 
Several of these will continue to be used internal to cajita.js, but
should no longer be exposed.

* Changed incorrect uses of ___.func() to use ___.markFuncFreeze().
* Changed incorrect uses of ___.grantCall() to a grant that also
  correctly determines what is read. 
* Changed anachronistic "grantEnumOnly" to "grantEnum". Likewise
  fastpathEnumOnly. 
* grantGeneric -> grantGenericMethod
* grantTypedGeneric -> grantTypedMethod
* grantMutator -> grantMutatingMethod
* handleGeneric -> handleGenericMethod

* ctor -> markCtor
* frozenFunc -> markFuncFreeze
* func -> markFuncOnly (only used by cajoled output)
* xo4a -> markXo4a (only within cajita.js)

primFreeze was typically doing much unneeded work enumerating all
property names with a for-in, in order to delete or override
the per-property *_canSet___ and *_canDelete___ flags. Instead, added
a per-object SLOWFREEZE___ flag that records whether there may be any
*_canSet___ or *_canDelete___ on this object. If not, as is typical,
primFreeze can safely skip the enumeration.

Added more internal diagnostics to catch more accidental bad usage of
the taming API.

Moved most of the logic for creating inert constructors from hidden
tamed constructors from domita.js's inertClassCtor() to cajita.js's
extend(). Refactored some to remove redundancy and make more robust.

Fixes issue 1058 simply by removing the code that overrode the grant*
operations. The CajaRuntimeDebuggingRewriter already rewrites the code
it generates to avoid checking the fastpath flags by just calling the
entry functions directly. Since the internal logic for using the
fastpath is still intact, that should close issue 1058.

[ben, 2009-04-23 20:47:34]

Using this code review to test the usefulness of
http://code.google.com/p/google-caja/wiki/NiceNeighbor, the first issue I hit is
that I have no idea what the relationship between ___.frozenFunc() and
___.grantFunc() is, i.e. when one uses one vs. the other (or both?) and what
they actually mean...

I will defer further review until this is fixed (mostly coz I have a meeting to
go to now).

[BenL, 2009-05-20 20:09:27]

http://codereview.appspot.com/50041/diff/4001/4009
File src/com/google/caja/cajita-debugmode.js (right):

http://codereview.appspot.com/50041/diff/4001/4009#newcode355
Line 355: 'grantEnum', noop,
Perhaps a comment to say why you are not disabling grant{Func,Generic,...)?

Also a comment to say why you are disabling fast-tracking generally.

http://codereview.appspot.com/50041/diff/4001/4011
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/50041/diff/4001/4011#newcode580
Line 580: function deprecate(func, badName, advice) {
Nice.

http://codereview.appspot.com/50041/diff/4001/4015
File src/com/google/caja/demos/calendar/demo-cajoled.html (right):

http://codereview.appspot.com/50041/diff/4001/4015#newcode152
Line 152: ___.ctor(StringInterpolation, Object, 'StringInterpolation');
NiceNeighbour doesn't explain what optSuper and optName are for.

http://codereview.appspot.com/50041/diff/4001/4013
File src/com/google/caja/demos/lolcat-search/search.html (right):

http://codereview.appspot.com/50041/diff/4001/4013#newcode66
Line 66: ___.grantRead(___.sharedImports, 'StringInterpolation');
Why not ___.grantFunc and eliminate ___.frozenFunc above?

http://codereview.appspot.com/50041/diff/4001/4002
File tests/com/google/caja/parser/quasiliteral/DefaultValijaRewriterTest.java
(right):

http://codereview.appspot.com/50041/diff/4001/4002#newcode492
Line 492: "___.grantFunc(testImports, 'assertEquals');" +
Are both the frozenFunc and grantFunc calls needed? NiceNeighbour suggests not.

http://codereview.appspot.com/50041/diff/4001/4003
File tests/com/google/caja/parser/quasiliteral/taming_test.html (right):

http://codereview.appspot.com/50041/diff/4001/4003#newcode88
Line 88: ___.handleGeneric(MyRootNamespace.SubSpace, 'attenuated', function (f)
{
I'm not sure I like the name ... shouldn't this (and friends) be
handleGenericFunc?

[MarkM, 2009-06-26 06:31:54]

Still looking at your first issue below -- the disabling of some taming methods
by cajita-debugmode -- in light of new issue 1058. This first issue is not yet
addressed. All the rest are ready for judgment. Thanks.

http://codereview.appspot.com/50041/diff/4001/4009
File src/com/google/caja/cajita-debugmode.js (right):

http://codereview.appspot.com/50041/diff/4001/4009#newcode355
Line 355: 'grantEnum', noop,
Looking at this disabling again in light of issue 1058.

http://codereview.appspot.com/50041/diff/4001/4015
File src/com/google/caja/demos/calendar/demo-cajoled.html (right):

http://codereview.appspot.com/50041/diff/4001/4015#newcode152
Line 152: ___.ctor(StringInterpolation, Object, 'StringInterpolation');
On 2009/05/20 20:09:27, BenL wrote:
> NiceNeighbour doesn't explain what optSuper and optName are for.

Done.

http://codereview.appspot.com/50041/diff/4001/4013
File src/com/google/caja/demos/lolcat-search/search.html (right):

http://codereview.appspot.com/50041/diff/4001/4013#newcode66
Line 66: ___.grantRead(___.sharedImports, 'StringInterpolation');
On 2009/05/20 20:09:27, BenL wrote:
> Why not ___.grantFunc and eliminate ___.frozenFunc above?

Done.

http://codereview.appspot.com/50041/diff/4001/4002
File tests/com/google/caja/parser/quasiliteral/DefaultValijaRewriterTest.java
(right):

http://codereview.appspot.com/50041/diff/4001/4002#newcode492
Line 492: "___.grantFunc(testImports, 'assertEquals');" +
On 2009/05/20 20:09:27, BenL wrote:
> Are both the frozenFunc and grantFunc calls needed? NiceNeighbour suggests
not.

Done.

http://codereview.appspot.com/50041/diff/4001/4003
File tests/com/google/caja/parser/quasiliteral/taming_test.html (right):

http://codereview.appspot.com/50041/diff/4001/4003#newcode88
Line 88: ___.handleGeneric(MyRootNamespace.SubSpace, 'attenuated', function (f)
{
On 2009/05/20 20:09:27, BenL wrote:
> I'm not sure I like the name ... shouldn't this (and friends) be
> handleGenericFunc?

The naming style used by these taming functions is to use *Func only for marking
things as simple-functions, which are first class in Cajita. All the others
functions for taming methods assume that the function involved is exophoric, and
so shouldn't be accessible from Cajita. Perhaps handleGenericMethod would be
clearer? I think I'll leave this alone.

[ben, 2009-06-26 11:31:21]

I guess I need to see a new patch?

http://codereview.appspot.com/50041/diff/4001/4003
File tests/com/google/caja/parser/quasiliteral/taming_test.html (right):

http://codereview.appspot.com/50041/diff/4001/4003#newcode88
Line 88: ___.handleGeneric(MyRootNamespace.SubSpace, 'attenuated', function (f)
{
On 2009/06/26 06:31:54, MarkM wrote:
> On 2009/05/20 20:09:27, BenL wrote:
> > I'm not sure I like the name ... shouldn't this (and friends) be
> > handleGenericFunc?
> 
> The naming style used by these taming functions is to use *Func only for
marking
> things as simple-functions, which are first class in Cajita. All the others
> functions for taming methods assume that the function involved is exophoric,
and
> so shouldn't be accessible from Cajita. Perhaps handleGenericMethod would be
> clearer? I think I'll leave this alone.

"Generic" doesn't really tell me anything. handleExophoric, perhaps?

[ben, 2009-07-06 10:26:20]

http://codereview.appspot.com/50041/diff/10037/8111
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/50041/diff/10037/8111#newcode465
Line 465: fail('Not writable: (', debugReference(obj), ').', name);
If we're going to call it writeable, should this be handleWrite? Feel free to
just add a FIXME rather than actually make this change.

http://codereview.appspot.com/50041/diff/10037/8111#newcode1073
Line 1073: *             ___.ctor().
It worries me that, AFAICS, we don't (can't?) check that someSuper actually is a
superclass, or that it meets these constraints.

http://codereview.appspot.com/50041/diff/10037/8116
File src/com/google/caja/demos/calendar/demo-cajoled.html (right):

http://codereview.appspot.com/50041/diff/10037/8116#newcode94
Line 94: getImports: ___.frozenFunc(function () {
NiceNeighbor does not explain (to me) the difference between ___.frozenFunc()
and ___.grantFunc(). If the only difference is that one takes a function and the
other takes obj, name, then why such different names?

Also, should it not be ___.freezeFunc()?

[ihab.awad, 2009-07-06 21:11:07]

The domita.js changes look great, thanks! Only minor comments.

http://codereview.appspot.com/50041/diff/10037/8109
File src/com/google/caja/cajita-debugmode.js (left):

http://codereview.appspot.com/50041/diff/10037/8109#oldcode81
Line 81: 
Why can we delete this stuff? Doesn't it do anything useful any more? Do the
debug mode tests work without it?

http://codereview.appspot.com/50041/diff/10037/8109#oldcode359
Line 359: ], 0);
Same question re deleting stuff.

http://codereview.appspot.com/50041/diff/10037/8107
File src/com/google/caja/plugin/domita.js (right):

http://codereview.appspot.com/50041/diff/10037/8107#newcode3243
Line 3243: // called by cajoled code, so we do not use
classUtils.inertClassCtor().
s/classUtils.inertClassCtor/inertCtor/

[MarkM, 2009-07-07 07:56:00]

http://codereview.appspot.com/50041/diff/10037/8109
File src/com/google/caja/cajita-debugmode.js (left):

http://codereview.appspot.com/50041/diff/10037/8109#oldcode81
Line 81: 
On 2009/07/06 21:11:08, ihab.awad wrote:
> Why can we delete this stuff? Doesn't it do anything useful any more? 

No. It was only used by the deleted code below, where this is explained.

> Do the debug mode tests work without it?

Yes.

http://codereview.appspot.com/50041/diff/10037/8109#oldcode81
Line 81: 
On 2009/07/06 21:11:08, ihab.awad wrote:
> Why can we delete this stuff? Doesn't it do anything useful any more? Do the
> debug mode tests work without it?

Done.

http://codereview.appspot.com/50041/diff/10037/8109#oldcode359
Line 359: ], 0);
On 2009/07/06 21:11:08, ihab.awad wrote:
> Same question re deleting stuff.

The purpose of this was to suppress the setting of fastpath flags, so that, for
example, calls would always go through callPub(), which is overriden in
debugmode with extra bookkeeping. However, debug mode also rewrites the
translation so that the fastpath flags aren't checked by the cajoler output
anyway. Therefore, the above short circuiting isn't needed. As issue 1058
explains, this short circuiting didn't actually work, so we're lucky we can
simply delete it.

I wish they were all this easy ;).

http://codereview.appspot.com/50041/diff/10037/8109#oldcode359
Line 359: ], 0);
On 2009/07/06 21:11:08, ihab.awad wrote:
> Same question re deleting stuff.

Done.

http://codereview.appspot.com/50041/diff/10037/8111
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/50041/diff/10037/8111#newcode465
Line 465: fail('Not writable: (', debugReference(obj), ').', name);
On 2009/07/06 10:26:20, ben wrote:
> If we're going to call it writeable, should this be handleWrite? Feel free to
> just add a FIXME rather than actually make this change.

New text near the top of cajita.js:

// FIXME(erights): After the Caja runtime was started, some of Caja's
// ideas for controlling ES3 attribute helped inspire the new ES5
// attribute control APIs. But in the process, different terms were
// adopted for these attributes. The current correspondences:
// ES3          Caja         ES5
// ReadOnly     canSet       writable
// DontEnum     canEnum      enumerable
// DontDelete   canDelete    configurable
// We should consider migrating our terminology towards ES5's,
// especially changing "set" to "write".

http://codereview.appspot.com/50041/diff/10037/8111#newcode465
Line 465: fail('Not writable: (', debugReference(obj), ').', name);
On 2009/07/06 10:26:20, ben wrote:
> If we're going to call it writeable, should this be handleWrite? Feel free to
> just add a FIXME rather than actually make this change.

Done.

http://codereview.appspot.com/50041/diff/10037/8111#newcode1073
Line 1073: *             ___.ctor().
On 2009/07/06 10:26:20, ben wrote:
> It worries me that, AFAICS, we don't (can't?) check that 
> someSuper actually is a superclass, 


It is not that someSuper already was a superclass. As it says earlier above,
"extend() also sets hiddenCtor.prototype to set up the prototype chain so that
[..]". In other words, extend() initializes hiddenCtor so that it represents a
"subclass" of someSuper. I have added text to this effect at the beginning of
the doc-comment.


> or that it meets these constraints.

Actually, the code "someSuper = asCtor(someSuper.prototype.constructor);" below
already checks exactly this condition.

http://codereview.appspot.com/50041/diff/10037/8111#newcode1073
Line 1073: *             ___.ctor().
On 2009/07/06 10:26:20, ben wrote:
> It worries me that, AFAICS, we don't (can't?) check that someSuper actually is
a
> superclass, or that it meets these constraints.
> 

Done.

http://codereview.appspot.com/50041/diff/10037/8116
File src/com/google/caja/demos/calendar/demo-cajoled.html (right):

http://codereview.appspot.com/50041/diff/10037/8116#newcode94
Line 94: getImports: ___.frozenFunc(function () {
On 2009/07/06 10:26:20, ben wrote:
> NiceNeighbor does not explain (to me) the difference between ___.frozenFunc()
> and ___.grantFunc(). If the only difference is that one takes a function and
the
> other takes obj, name, then why such different names?
> 
> Also, should it not be ___.freezeFunc()?

Fixed according to our verbal agreement:

frozenFunc -> markFuncFreeze
ctor -> markCtor
func -> markFuncOnly (only for use by cajoler output)
xo4a -> markXo4a (only inside cajita.js)

http://codereview.appspot.com/50041/diff/10037/8116#newcode94
Line 94: getImports: ___.frozenFunc(function () {
On 2009/07/06 10:26:20, ben wrote:
> NiceNeighbor does not explain (to me) the difference between ___.frozenFunc()
> and ___.grantFunc(). If the only difference is that one takes a function and
the
> other takes obj, name, then why such different names?
> 
> Also, should it not be ___.freezeFunc()?
> 
> 
> 

Done.

http://codereview.appspot.com/50041/diff/10037/8107
File src/com/google/caja/plugin/domita.js (right):

http://codereview.appspot.com/50041/diff/10037/8107#newcode3243
Line 3243: // called by cajoled code, so we do not use
classUtils.inertClassCtor().
On 2009/07/06 21:11:08, ihab.awad wrote:
> s/classUtils.inertClassCtor/inertCtor/

Done.

[ben, 2009-07-07 10:45:59]

I forgot to say, LTGM, modulo naming changes as discussed.

[ben, 2009-07-07 11:07:22]

LGTM, with naming changes.

One final comment, act on it if you see fit: perhaps markFuncOnly should be
markFuncOnly_ to suggest it is internal only?