Add document.body and document.documentElement to DOMita

[ihab.awad, 2008-11-24 23:16:44]

Just sending my comments on Linter.java right away; the rest to come.

http://codereview.appspot.com/9661/diff/59/223
File src/com/google/caja/tools/Linter.java (right):

http://codereview.appspot.com/9661/diff/59/223#newcode164
Line 164: private static void lint(
This seems to be redoing a portion of the static (scope) analysis that we are
already doing in the Cajita and Valija rewriters. Should this not be just
another Rewriter? See http://codereview.appspot.com/9466 [*] for a CL I started
working on to do this in the context of modularizing our JS. I suspect we should
merge our efforts in some way...?

[*] Ignore experimental/... changes in this CL.

[MikeSamuel, 2008-11-24 23:29:28]

http://codereview.appspot.com/9661/diff/59/223
File src/com/google/caja/tools/Linter.java (right):

http://codereview.appspot.com/9661/diff/59/223#newcode164
Line 164: private static void lint(
This scope analysis is not meant to be correct according to ES rules, but to
place additional restrictions.

It enforces a kind of block scoping that disallows redeclaration except when the
declarations occurs in disjoint loops.

And I don't see why a Rewriter is appropriate.  The Linter doesn't rewrite.  It
only inspects.

[ihab.awad, 2008-11-25 19:20:34]

LGTM (comments are suggestions or questions) except for the double 'var'
allowance in Linter.java, which should be removed if Linter.java is checked in.

http://codereview.appspot.com/9661/diff/59/224
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/9661/diff/59/224#newcode2557
Line 2557: }
It's LGTM but just f-my-i, why the s/enforce/if.../? For less performance
overhead?

http://codereview.appspot.com/9661/diff/59/219
File src/com/google/caja/plugin/domita.js (right):

http://codereview.appspot.com/9661/diff/59/219#newcode765
Line 765: this.editable___));
Indent++

http://codereview.appspot.com/9661/diff/59/219#newcode1774
Line 1774: var tameBodyElement = new TamePseudoElement(
Can we move TamePseudoElement to be before TameHTMLDocument, so we're not
relying on the forward declarations? Fwiw, the order of declaration confused me.

http://codereview.appspot.com/9661/diff/59/219#newcode1774
Line 1774: var tameBodyElement = new TamePseudoElement(
Where does tameBodyElement acquire the behavior of the EventTarget interface?
Should it? Should that be part of TamePseudoElement?

http://codereview.appspot.com/9661/diff/59/219#newcode1893
Line 1893: tagName, tameDoc, childNodesMaker, parentNodeMaker, innerHTMLMaker,
Can we use a different term than 'maker'? I think that, Caja-wide, we should
reserve the term 'maker' for the E-style modularity abstraction since it's
likely to become increasingly useful....

http://codereview.appspot.com/9661/diff/59/219#newcode1893
Line 1893: tagName, tameDoc, childNodesMaker, parentNodeMaker, innerHTMLMaker,
The 'maker' variables are just attribute getters, right? Why are they not
assigned to the relevant get*() name and exportFields()-ed?

http://codereview.appspot.com/9661/diff/59/219#newcode1935
Line 1935: // TODO(mikesamuel): Needs implementation
This should only be defined for the HTML document, and not for any other Pseudo
Element, right?

http://codereview.appspot.com/9661/diff/59/219#newcode2027
Line 2027: >>>>>>> .r3014
Note diff artifact.

http://codereview.appspot.com/9661/diff/59/223
File src/com/google/caja/tools/Linter.java (right):

http://codereview.appspot.com/9661/diff/59/223#newcode164
Line 164: private static void lint(
Looking at the code that this linter lints, it seems like the number of places
where we've taken advantage of it is minimal. That all said, with the proviso
that the work done here should be rolled into whatever module compilation thing
we come up with in the future, we can include it with no obvious ill effect.

We should think carefully before relying too much on the @provides / @requires
convention, since it does not provide per-instance isolation -- something
important for Caja that's not a concern in other webapps.

If we check in this file, it should be without the special case for loop
variable 'var's. These semantics are not part of ES-anything and so, in some
sense, define a new language. One that is better than current ES, I agree -- who
doesn't want block level lexical scoping? -- but different nevertheless.

Also: the double use of 'var' in the loops is only safe if there is an
initializer in the 2nd loop. It's not *immediately* clear to me from the code
below whether this is checked and the very fact that I have to wonder about it
makes me think that we really don't need this extra complexity.

In the future, if we want, we can implement an ES3.1 -> ES-plain rewriter that
allows us to use 'let' and 'const'.

http://codereview.appspot.com/9661/diff/59/211
File tests/com/google/caja/plugin/domita_test_untrusted.html (right):

http://codereview.appspot.com/9661/diff/59/211#newcode169
Line 169: * @param skeleton an acyclic Object whose own properties describe
aspects
I would say it's a JSON object.

http://codereview.appspot.com/9661/diff/59/211#newcode174
Line 174: function assertDomTree(skeleton, node, opt_stack) {
Very nice!

http://codereview.appspot.com/9661/diff/59/211#newcode175
Line 175: var stack = opt_stack || '(#root:' + node.nodeName + ')';
Not a big deal, but I have doubts about whether I would be able to read 'stack'
in an error message. Can the function instead just construct an output that it
is identical to the JSON 'skeleton' as it walks through, and barf when it finds
something wrong? Given the assertions, it quits on the 1st failure anyway, so
it's not necessary for the barf errors to be inlined into the JSON in a clean
way; just output the JSON verified so far and a big barfage message, and quit.

http://codereview.appspot.com/9661/diff/59/211#newcode182
Line 182: case 'nodeType': assertEquals(msg, v, node.nodeType); break;
I recommend linewrapping all the 'case' bodies. If this function is supposed to
be self-documenting (the 'switch' documents the accepted keys), it should be
easy to scan; the indentation would help considerably imho.

[MikeSamuel, 2008-11-25 23:17:14]

Separated out the linter stuff into http://codereview.appspot.com/9674/show

[MikeSamuel, 2008-11-25 23:30:05]

http://codereview.appspot.com/9661/diff/59/224
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/9661/diff/59/224#newcode2557
Line 2557: }
On 2008/11/25 19:20:34, ihab.awad wrote:
> It's LGTM but just f-my-i, why the s/enforce/if.../? For less performance
> overhead?

The enforce stuff constructs the message string regardless of whether the
condition is false or not.

This is bad performance wise, but it also means that if coercing an argument to
a string raises an exception, then guard will raise that exception regardless of
the condition.

This was a problem when a node constructor hit a guard before fully constructing
the node.

http://codereview.appspot.com/9661/diff/59/219
File src/com/google/caja/plugin/domita.js (right):

http://codereview.appspot.com/9661/diff/59/219#newcode765
Line 765: this.editable___));
Fixed parenthesization

http://codereview.appspot.com/9661/diff/59/219#newcode1774
Line 1774: var tameBodyElement = new TamePseudoElement(
That's not in there yet.

http://codereview.appspot.com/9661/diff/59/219#newcode1774
Line 1774: var tameBodyElement = new TamePseudoElement(
On 2008/11/25 19:20:34, ihab.awad wrote:
> Can we move TamePseudoElement to be before TameHTMLDocument, so we're not
> relying on the forward declarations? Fwiw, the order of declaration confused
me.

Done.

http://codereview.appspot.com/9661/diff/59/219#newcode1893
Line 1893: tagName, tameDoc, childNodesMaker, parentNodeMaker, innerHTMLMaker,
On 2008/11/25 19:20:34, ihab.awad wrote:
> The 'maker' variables are just attribute getters, right? Why are they not
> assigned to the relevant get*() name and exportFields()-ed?

Because that would prevent exportField from being done to the prototypes.

http://codereview.appspot.com/9661/diff/59/219#newcode1893
Line 1893: tagName, tameDoc, childNodesMaker, parentNodeMaker, innerHTMLMaker,
maker -> getter

http://codereview.appspot.com/9661/diff/59/219#newcode1935
Line 1935: // TODO(mikesamuel): Needs implementation
On 2008/11/25 19:20:34, ihab.awad wrote:
> This should only be defined for the HTML document, and not for any other
Pseudo
> Element, right?

Correct.  Removed

http://codereview.appspot.com/9661/diff/59/219#newcode2027
Line 2027: >>>>>>> .r3014
Fixed.

http://codereview.appspot.com/9661/diff/59/211
File tests/com/google/caja/plugin/domita_test_untrusted.html (right):

http://codereview.appspot.com/9661/diff/59/211#newcode169
Line 169: * @param skeleton an acyclic Object whose own properties describe
aspects
On 2008/11/25 19:20:34, ihab.awad wrote:
> I would say it's a JSON object.

It's not.  See the value of sameAs in the use case below.

http://codereview.appspot.com/9661/diff/59/211#newcode182
Line 182: case 'nodeType': assertEquals(msg, v, node.nodeType); break;
On 2008/11/25 19:20:34, ihab.awad wrote:
> I recommend linewrapping all the 'case' bodies. If this function is supposed
to
> be self-documenting (the 'switch' documents the accepted keys), it should be
> easy to scan; the indentation would help considerably imho.

Done.

[MikeSamuel, 2008-12-02 23:02:31]

@3062