Cajole to HTML

Rewrote HTML Compiler and HTML Sanitizer to output Javascript
and static HTML instead of just Javascript.

http://code.google.com/p/google-caja/issues/detail?id=294
Issue 294:   Caja produces pages that don't work if Javascript is disabled
    The problem is that, in order to preserve execution order
    properties, we emit HTML (and CSS?) as Javascript that builds the
    page.

    We need to also (optionally) output just the HTML and CSS in a
    <noscript> tag.

This makes two major changes:
  (1) Replaces HtmlSanitizer and HtmlCompiler with a mechanism that
      outputs a static HTML chunk and then reworks it to add dynamic
      attributes.
  (2) Added an ID suffix to PluginMeta so that containers which can
      avoid suffix overlap can move more rewriting to the static side.
      The new HtmlSanitizer and HtmlCompiler use that when rewriting
      attributes, and when present, no javascript is needed to emit
      CSS so a noscript version of a gadget will be properly styled.

The new Sanitizer and Compiler are based on the scheme described in
http://google-caja.googlecode.com/svn/trunk/doc/html/templates/TemplateCompiler.html
and can be extended to implement the entire template scheme.


This change was adopted by Ihab

[Jasvir, 2009-05-22 04:50:50]

The review is not yet complete but Ihab and I had some queries...

http://codereview.appspot.com/61041/diff/3465/1923
File build.xml (right):

http://codereview.appspot.com/61041/diff/3465/1923#newcode512
Line 512: <include name="**/caja/plugin/templates/*.json"/>
Why is there json here?  Its not included in the src

http://codereview.appspot.com/61041/diff/3465/1879
File src/com/google/caja/lang/css/css-extensions-whitelist.json (right):

http://codereview.appspot.com/61041/diff/3465/1879#newcode7
Line 7: "border-radius", "content", "filter", "font-stretch",
"-moz-border-radius",
Why are we adding this in this CL?

http://codereview.appspot.com/61041/diff/3465/1888
File src/com/google/caja/opensocial/DefaultGadgetRewriter.java (right):

http://codereview.appspot.com/61041/diff/3465/1888#newcode166
Line 166: Document doc = dom.getOwnerDocument();
Why did you inline this method call?

http://codereview.appspot.com/61041/diff/3465/1888#newcode197
Line 197: return null;
Why are you removing this?

http://codereview.appspot.com/61041/diff/3465/1882
File src/com/google/caja/parser/html/AbstractElementStack.java (right):

http://codereview.appspot.com/61041/diff/3465/1882#newcode115
Line 115: if (rootPos.startCharInFile() <= endPos.startCharInFile()) {
weird.  Under what circumstances is rootPos.sCIF > endPos.sCIF?

http://codereview.appspot.com/61041/diff/3465/1881
File src/com/google/caja/parser/html/XmlElementStack.java (right):

http://codereview.appspot.com/61041/diff/3465/1881#newcode174
Line 174: if (rootStart.startCharInFile() <= endOfDocument.startCharInFile()) {
Under what circumstances is rootStart.sCIF > endOfDocument.sCIF?

http://codereview.appspot.com/61041/diff/3465/1886
File src/com/google/caja/parser/js/TranslatedCode.java (right):

http://codereview.appspot.com/61041/diff/3465/1886#newcode67
Line 67: rc.getOut().consume("/* Start translated code */");
Why was this change made?  Previously translated code if it got rendered
directly would result in at least a syntax error - now this would get rendered
and execute.

http://codereview.appspot.com/61041/diff/3465/1887
File src/com/google/caja/parser/js/UncajoledModule.java (right):

http://codereview.appspot.com/61041/diff/3465/1887#newcode84
Line 84: rc.getOut().consume("/* Start Uncajoled Module */");
Ditto.

http://codereview.appspot.com/61041/diff/3465/1880
File src/com/google/caja/parser/quasiliteral/CajitaRewriter.java (right):

http://codereview.appspot.com/61041/diff/3465/1880#newcode574
Line 574: for (Statement c : ((Block) node).children()) {
Safe in the absence of let and const constructs.

http://codereview.appspot.com/61041/diff/3465/1898
File src/com/google/caja/plugin/BuildServiceImplementation.java (right):

http://codereview.appspot.com/61041/diff/3465/1898#newcode216
Line 216: // Write out as HTML if the output file has the right extension.
I don't think we should switch on the extension.  Can the call provide an
explicit output mimetypes and switch on that?

http://codereview.appspot.com/61041/diff/3465/1892
File src/com/google/caja/plugin/CssRuleRewriter.java (right):

http://codereview.appspot.com/61041/diff/3465/1892#newcode48
Line 48: private static final String GADGET_ID_PLACEHOLDER = "___GADGET___";
Ensure switching does not occur on meta.getIdClass being equal to
GADGET_ID_PLACEHOLDER or ___GADGET___

http://codereview.appspot.com/61041/diff/3465/1895
File src/com/google/caja/plugin/Dom.java (right):

http://codereview.appspot.com/61041/diff/3465/1895#newcode58
Line 58: .replace("\\", "\\\\").replace("\n", "\\n").replace("\r", "\\r");
What is the basis for this escaping?  Is this escaping safe everywhere?

http://codereview.appspot.com/61041/diff/3465/1891
File src/com/google/caja/plugin/ExtractedHtmlContent.java (right):

http://codereview.appspot.com/61041/diff/3465/1891#newcode33
Line 33: public static final Block extractedScriptFor(Element el) {
This & the following method should be like:

    setExtractedScriptFor(el)
    getExtractedScriptFor(el, js)

or some other such convention, with the same name for the 1st formal parameter,
to make them consistent and clear that they are duals of one another

http://codereview.appspot.com/61041/diff/3465/1910
File src/com/google/caja/plugin/templates/SafeHtmlMaker.java (right):

http://codereview.appspot.com/61041/diff/3465/1910#newcode115
Line 115: Pair<Node, List<Block>> make(List<Node> roots, List<Statement>
handlers) {
If make is only ever used once and is tied to scriptsPerNode initialized above,
why not pass the roots and handlers in the constructor?

http://codereview.appspot.com/61041/diff/3465/1875
File tests/com/google/caja/plugin/HtmlEmitterTest.java (right):

http://codereview.appspot.com/61041/diff/3465/1875#newcode159
Line 159: StringBuilder sb = new StringBuilder();
Why are you rendering individual statements instead of sticking the statements
in a block and asking it to render itself?

http://codereview.appspot.com/61041/diff/3465/1864
File tests/com/google/caja/plugin/templates/RenamingTest.java (right):

http://codereview.appspot.com/61041/diff/3465/1864#newcode16
Line 16: 
What class is this test supposed to test?

[MikeSamuel, 2009-05-22 20:42:54]

http://codereview.appspot.com/61041/diff/3465/1923
File build.xml (right):

http://codereview.appspot.com/61041/diff/3465/1923#newcode512
Line 512: <include name="**/caja/plugin/templates/*.json"/>
On 2009/05/22 04:50:51, jasvir wrote:
> Why is there json here?  Its not included in the src

Cruft.  In he version with the intermediate templating language, there are
schemas for that language.

http://codereview.appspot.com/61041/diff/3465/1879
File src/com/google/caja/lang/css/css-extensions-whitelist.json (right):

http://codereview.appspot.com/61041/diff/3465/1879#newcode7
Line 7: "border-radius", "content", "filter", "font-stretch",
"-moz-border-radius",
This was meant to be in one of the smaller CLs that I split out, but I must've
forgotten this bit.
These properties are used by the calendar demo.

http://codereview.appspot.com/61041/diff/3465/1888
File src/com/google/caja/opensocial/DefaultGadgetRewriter.java (right):

http://codereview.appspot.com/61041/diff/3465/1888#newcode166
Line 166: Document doc = dom.getOwnerDocument();
On 2009/05/22 04:50:51, jasvir wrote:
> Why did you inline this method call?

The name rewriteContent was inappropriate and I couldn't think of a better name
at the time.

http://codereview.appspot.com/61041/diff/3465/1888#newcode197
Line 197: return null;
On 2009/05/22 04:50:51, jasvir wrote:
> Why are you removing this?

Because another piece of code checks the null return value and adds the exact
same message that UriCallbackException does.

It's not the responsibility of the PluginEnvironment to issue messages.  It's
the caller that should decide whether and how to report on an inability to
fetch.

http://codereview.appspot.com/61041/diff/3465/1882
File src/com/google/caja/parser/html/AbstractElementStack.java (right):

http://codereview.appspot.com/61041/diff/3465/1882#newcode115
Line 115: if (rootPos.startCharInFile() <= endPos.startCharInFile()) {
This should not be in this CL, but I believe that the Localizer, which replaces
<ihtml:msg> with a message from a message bundle runs into this problem when
reordering placeholders.

http://codereview.appspot.com/61041/diff/3465/1886
File src/com/google/caja/parser/js/TranslatedCode.java (right):

http://codereview.appspot.com/61041/diff/3465/1886#newcode67
Line 67: rc.getOut().consume("/* Start translated code */");
Part of the contract of render is that it produces JS.  It should not produce
invalid JS, hence the FIXME.

http://codereview.appspot.com/61041/diff/3465/1880
File src/com/google/caja/parser/quasiliteral/CajitaRewriter.java (right):

http://codereview.appspot.com/61041/diff/3465/1880#newcode574
Line 574: for (Statement c : ((Block) node).children()) {
On 2009/05/22 04:50:51, jasvir wrote:
> Safe in the absence of let and const constructs.

Yep.  This is not necessary, but it seems that we produce a lot of unnecessary
blocks and noops, and those are implementation details that clutter up our
tests.

http://codereview.appspot.com/61041/diff/3465/1898
File src/com/google/caja/plugin/BuildServiceImplementation.java (right):

http://codereview.appspot.com/61041/diff/3465/1898#newcode216
Line 216: // Write out as HTML if the output file has the right extension.
Any reasons why not?  Switching on the output extension seems a lot like using
indentation to specify block structure in python -- you're using a high value
signal that people shouldn't violate anyway.

We could do a lot of things.

http://codereview.appspot.com/61041/diff/3465/1895
File src/com/google/caja/plugin/Dom.java (right):

http://codereview.appspot.com/61041/diff/3465/1895#newcode58
Line 58: .replace("\\", "\\\\").replace("\n", "\\n").replace("\r", "\\r");
On 2009/05/22 04:50:51, jasvir wrote:
> What is the basis for this escaping?  Is this escaping safe everywhere?

The goal is to produce readable debug info.

http://codereview.appspot.com/61041/diff/3465/1910
File src/com/google/caja/plugin/templates/SafeHtmlMaker.java (right):

http://codereview.appspot.com/61041/diff/3465/1910#newcode115
Line 115: Pair<Node, List<Block>> make(List<Node> roots, List<Statement>
handlers) {
On 2009/05/22 04:50:51, jasvir wrote:
> If make is only ever used once and is tied to scriptsPerNode initialized
above,
> why not pass the roots and handlers in the constructor?

No reason not to.

http://codereview.appspot.com/61041/diff/3465/1875
File tests/com/google/caja/plugin/HtmlEmitterTest.java (right):

http://codereview.appspot.com/61041/diff/3465/1875#newcode159
Line 159: StringBuilder sb = new StringBuilder();
On 2009/05/22 04:50:51, jasvir wrote:
> Why are you rendering individual statements instead of sticking the statements
> in a block and asking it to render itself?

Because blocks add unnecessary brackets and indenting.

http://codereview.appspot.com/61041/diff/3465/1864
File tests/com/google/caja/plugin/templates/RenamingTest.java (right):

http://codereview.appspot.com/61041/diff/3465/1864#newcode16
Line 16: 
On 2009/05/22 04:50:51, jasvir wrote:
> What class is this test supposed to test?

One that is not in this CL.  Renaming does alpha renaming of template variables.

[Jasvir, 2009-05-27 21:21:15]

LGTM

http://codereview.appspot.com/61041/diff/3465/1886
File src/com/google/caja/parser/js/TranslatedCode.java (right):

http://codereview.appspot.com/61041/diff/3465/1886#newcode67
Line 67: rc.getOut().consume("/* Start translated code */");
Making the first line a throw would be both safe and valid javascript.  Can we
change it to that?

http://codereview.appspot.com/61041/diff/3465/1910
File src/com/google/caja/plugin/templates/SafeHtmlMaker.java (right):

http://codereview.appspot.com/61041/diff/3465/1910#newcode116
Line 116: js.clear();
Why call js.clear if SafeHtmlMaker is only to be called once?

http://codereview.appspot.com/61041/diff/3465/1910#newcode263
Line 263: /** Define bits needed by the emitter calls and the attribute fixup.
*/
This comment is copied from start() above and is incorrect for finish()

http://codereview.appspot.com/61041/diff/3465/1910#newcode291
Line 291: "scriptBody", /*new UncajoledModule*/(script),
The commented out "new UncajoledModule" wrapper is presumably for when we
deconsolidate the consolidate javascript stage.

http://codereview.appspot.com/61041/diff/3465/1910#newcode361
Line 361: id = a;
Please either assert that id is previously null (having been removed by a prior
stage) or comment that multiple ids are ok and the last one takes precedence.

http://codereview.appspot.com/61041/diff/3465/1913
File src/com/google/caja/plugin/templates/TemplateCompiler.java (right):

http://codereview.appspot.com/61041/diff/3465/1913#newcode107
Line 107: * the value is an expression specify the dynamic text value.
s/specify/specifying/

Can you reformat this comment so the nesting is clearer.

http://codereview.appspot.com/61041/diff/3465/1913#newcode127
Line 127: List<? extends Node> ihtmlRoots,
Under what circumstances are there more than 1 element in ihtmlRoots.

http://codereview.appspot.com/61041/diff/3465/1913#newcode191
Line 191: HtmlSchema schema = htmlSchema;
Why do you alias schema here?

http://codereview.appspot.com/61041/diff/3465/1913#newcode192
Line 192: if (!schema.isElementAllowed(elName)) { return; }
Under what circumstances does this get executed?  Shouldn't the
TemplateSanitizer remove unallowed elements?

http://codereview.appspot.com/61041/diff/3465/1913#newcode246
Line 246: && "".equals(child.getNodeValue().replaceAll("[\r\n]+[ \t]*", ""));
This leaves whitespace without a leading eol character intact.  Is this leading
whitespace significant in a document fragment?

http://codereview.appspot.com/61041/diff/3465/1913#newcode478
Line 478: * nodes may have {@code afterScript0___} classes added so that a
client
Stale comment.

http://codereview.appspot.com/61041/diff/3465/1913#newcode481
Line 481: * The output will not will not contain SCRIPT nodes corresponding to
inline
s/will not//

http://codereview.appspot.com/61041/diff/3465/1913#newcode482
Line 482: * HTML.  This could be repaired by changing {@link SafeHtmlMaker} to
include
Repaired implies there is something broken with the current scheme.  The (only?)
observable difference between the original and the current is on introspection,
a program would find that the script blocks it was expecting are missing.

http://codereview.appspot.com/61041/diff/3465/1913#newcode485
Line 485: public Pair<Node, List<Block>> getSafeHtml(Document doc) {
Clarify doc is only used as a factory for DOM nodes.

http://codereview.appspot.com/61041/diff/3465/1913#newcode485
Line 485: public Pair<Node, List<Block>> getSafeHtml(Document doc) {
TemplateCompiler.inspect must be called prior to this method.  Why not call
inspect in this method itself and simplify the public interface for this class?

http://codereview.appspot.com/61041/diff/1939/3565
File src/com/google/caja/plugin/templates/SafeHtmlMaker.java (right):

http://codereview.appspot.com/61041/diff/1939/3565#newcode68
Line 68: *   <dt>Bones</dt>
A bone may reference a node in the skeleton which is a span constructed as a
marker.

http://codereview.appspot.com/61041/diff/1939/3565#newcode70
Line 70: *     scripts.</dd>
Clarification: A script in a bone is to be run immediately after the node the
bone points to.

[ihab.awad, 2009-05-29 05:11:04]

I am not the owner of this issue and hence cannot edit it. Therefore, I created
a new issue:

  http://codereview.appspot.com/63168

the 1st patch set of 63168 is identical to the last patch set of 61041. The 2nd
patch set of 63168 is changes in response to comments, as noted here. Further
work should continue on 63168.

http://codereview.appspot.com/61041/diff/3465/1879
File src/com/google/caja/lang/css/css-extensions-whitelist.json (right):

http://codereview.appspot.com/61041/diff/3465/1879#newcode7
Line 7: "border-radius", "content", "filter", "font-stretch",
"-moz-border-radius",
On 2009/05/22 20:42:54, MikeSamuel wrote:
> This was meant to be in one of the smaller CLs that I split out, but I must've
> forgotten this bit.
> These properties are used by the calendar demo.
> 

Ok, will keep.

http://codereview.appspot.com/61041/diff/3465/1886
File src/com/google/caja/parser/js/TranslatedCode.java (right):

http://codereview.appspot.com/61041/diff/3465/1886#newcode67
Line 67: rc.getOut().consume("/* Start translated code */");
On 2009/05/27 21:21:15, jasvir wrote:
> Making the first line a throw would be both safe and valid javascript.  Can we
> change it to that?
> 

Changed to emit a JavaScript 'throw'.

http://codereview.appspot.com/61041/diff/3465/1887
File src/com/google/caja/parser/js/UncajoledModule.java (right):

http://codereview.appspot.com/61041/diff/3465/1887#newcode84
Line 84: rc.getOut().consume("/* Start Uncajoled Module */");
On 2009/05/22 04:50:51, jasvir wrote:
> Ditto.

Right back atcha.

http://codereview.appspot.com/61041/diff/3465/1880
File src/com/google/caja/parser/quasiliteral/CajitaRewriter.java (right):

http://codereview.appspot.com/61041/diff/3465/1880#newcode574
Line 574: for (Statement c : ((Block) node).children()) {
On 2009/05/22 20:42:54, MikeSamuel wrote:
> On 2009/05/22 04:50:51, jasvir wrote:
> > Safe in the absence of let and const constructs.
> 
> Yep.  This is not necessary, but it seems that we produce a lot of unnecessary
> blocks and noops, and those are implementation details that clutter up our
> tests.
> 

I agree, actually. The intermediate blocks create crufty dependencies between
the rewriter structure and the test cases.

That said, to achieve the same effect yet also keep the code as simple as
possible, it would be nice if we had an elegant way to handle these. E.g., maybe
we need Yet Another Non Rendering Node (something like ParseTreeNodeContainer
that is a Statement) which we can nest, but which gets flattened out either via
a final pipeline stage or just during rendering.

http://codereview.appspot.com/61041/diff/3465/1898
File src/com/google/caja/plugin/BuildServiceImplementation.java (right):

http://codereview.appspot.com/61041/diff/3465/1898#newcode216
Line 216: // Write out as HTML if the output file has the right extension.
On 2009/05/22 20:42:54, MikeSamuel wrote:
> Any reasons why not?  Switching on the output extension seems a lot like using
> indentation to specify block structure in python -- you're using a high value
> signal that people shouldn't violate anyway.
> 
> We could do a lot of things.
> 

Leaving as-is for this CL. To revisit discussion later.

http://codereview.appspot.com/61041/diff/3465/1892
File src/com/google/caja/plugin/CssRuleRewriter.java (right):

http://codereview.appspot.com/61041/diff/3465/1892#newcode48
Line 48: private static final String GADGET_ID_PLACEHOLDER = "___GADGET___";
On 2009/05/22 04:50:51, jasvir wrote:
> Ensure switching does not occur on meta.getIdClass being equal to
> GADGET_ID_PLACEHOLDER or ___GADGET___

Ensured during code review.

http://codereview.appspot.com/61041/diff/3465/1895
File src/com/google/caja/plugin/Dom.java (right):

http://codereview.appspot.com/61041/diff/3465/1895#newcode58
Line 58: .replace("\\", "\\\\").replace("\n", "\\n").replace("\r", "\\r");
On 2009/05/22 20:42:54, MikeSamuel wrote:
> On 2009/05/22 04:50:51, jasvir wrote:
> > What is the basis for this escaping?  Is this escaping safe everywhere?
> 
> The goal is to produce readable debug info.
> 

Ok.

http://codereview.appspot.com/61041/diff/3465/1891
File src/com/google/caja/plugin/ExtractedHtmlContent.java (right):

http://codereview.appspot.com/61041/diff/3465/1891#newcode33
Line 33: public static final Block extractedScriptFor(Element el) {
On 2009/05/22 04:50:51, jasvir wrote:
> This & the following method should be like:
> 
>     setExtractedScriptFor(el)
>     getExtractedScriptFor(el, js)
> 
> or some other such convention, with the same name for the 1st formal
parameter,
> to make them consistent and clear that they are duals of one another

Done.

http://codereview.appspot.com/61041/diff/3465/1910
File src/com/google/caja/plugin/templates/SafeHtmlMaker.java (right):

http://codereview.appspot.com/61041/diff/3465/1910#newcode115
Line 115: Pair<Node, List<Block>> make(List<Node> roots, List<Statement>
handlers) {
On 2009/05/22 20:42:54, MikeSamuel wrote:
> On 2009/05/22 04:50:51, jasvir wrote:
> > If make is only ever used once and is tied to scriptsPerNode initialized
> above,
> > why not pass the roots and handlers in the constructor?
> 
> No reason not to.
> 

Done.

http://codereview.appspot.com/61041/diff/3465/1910#newcode116
Line 116: js.clear();
On 2009/05/27 21:21:15, jasvir wrote:
> Why call js.clear if SafeHtmlMaker is only to be called once?

On second thoughts, given that make() is a public method, it ensures at least
some modicum of consistency upon repeated invocation.

This stuff really lends itself to a more functional style, but Java is not
friendly to that (e.g. no nested function definitions) so one has to define a
*class* containing the necessary state, and then the whole lifecycle of the
class's instance becomes an issue where it really should never have been ....

Given the constraints, I'd say this should remain as-is for now.

http://codereview.appspot.com/61041/diff/3465/1910#newcode263
Line 263: /** Define bits needed by the emitter calls and the attribute fixup.
*/
On 2009/05/27 21:21:15, jasvir wrote:
> This comment is copied from start() above and is incorrect for finish()

Done.

http://codereview.appspot.com/61041/diff/3465/1910#newcode291
Line 291: "scriptBody", /*new UncajoledModule*/(script),
On 2009/05/27 21:21:15, jasvir wrote:
> The commented out "new UncajoledModule" wrapper is presumably for when we
> deconsolidate the consolidate javascript stage.

Done.

http://codereview.appspot.com/61041/diff/3465/1910#newcode361
Line 361: id = a;
On 2009/05/27 21:21:15, jasvir wrote:
> Please either assert that id is previously null (having been removed by a
prior
> stage) or comment that multiple ids are ok and the last one takes precedence.

Actually the *first* one takes precedence because the HTML parser removes extra
ones (and warns). Added comment to that effect, and an "assert id == null" just
for sanity checking in testing.

http://codereview.appspot.com/61041/diff/3465/1913
File src/com/google/caja/plugin/templates/TemplateCompiler.java (right):

http://codereview.appspot.com/61041/diff/3465/1913#newcode107
Line 107: * the value is an expression specify the dynamic text value.
On 2009/05/27 21:21:15, jasvir wrote:
> s/specify/specifying/
> 
> Can you reformat this comment so the nesting is clearer.
> 

Done.

http://codereview.appspot.com/61041/diff/3465/1913#newcode191
Line 191: HtmlSchema schema = htmlSchema;
On 2009/05/27 21:21:15, jasvir wrote:
> Why do you alias schema here?

Done.

http://codereview.appspot.com/61041/diff/3465/1913#newcode192
Line 192: if (!schema.isElementAllowed(elName)) { return; }
On 2009/05/27 21:21:15, jasvir wrote:
> Under what circumstances does this get executed?  Shouldn't the
> TemplateSanitizer remove unallowed elements?

Yes, the TemplateSanitizer *does* remove disallowed elements. Changed this to an
"assert" and removed <object> tags from the TemplateCompilerTest. The
TemplateSanitizer tests already include stuff to ensure invalid tags are
removed.

http://codereview.appspot.com/61041/diff/3465/1913#newcode246
Line 246: && "".equals(child.getNodeValue().replaceAll("[\r\n]+[ \t]*", ""));
On 2009/05/27 21:21:15, jasvir wrote:
> This leaves whitespace without a leading eol character intact.  Is this
leading
> whitespace significant in a document fragment?

Added a TODO to investigate. I suspect this is not a critical issue -- certainly
cannot lead to any exploits -- and its use is localized to children of
DocumentFragments.

http://codereview.appspot.com/61041/diff/3465/1913#newcode478
Line 478: * nodes may have {@code afterScript0___} classes added so that a
client
On 2009/05/27 21:21:15, jasvir wrote:
> Stale comment.

Done.

http://codereview.appspot.com/61041/diff/3465/1913#newcode481
Line 481: * The output will not will not contain SCRIPT nodes corresponding to
inline
On 2009/05/27 21:21:15, jasvir wrote:
> s/will not//

Done.

http://codereview.appspot.com/61041/diff/3465/1913#newcode482
Line 482: * HTML.  This could be repaired by changing {@link SafeHtmlMaker} to
include
On 2009/05/27 21:21:15, jasvir wrote:
> Repaired implies there is something broken with the current scheme.  The
(only?)
> observable difference between the original and the current is on
introspection,
> a program would find that the script blocks it was expecting are missing.

Done.

http://codereview.appspot.com/61041/diff/3465/1913#newcode485
Line 485: public Pair<Node, List<Block>> getSafeHtml(Document doc) {
On 2009/05/27 21:21:15, jasvir wrote:
> Clarify doc is only used as a factory for DOM nodes.

Done.

http://codereview.appspot.com/61041/diff/3465/1913#newcode485
Line 485: public Pair<Node, List<Block>> getSafeHtml(Document doc) {
On 2009/05/27 21:21:15, jasvir wrote:
> TemplateCompiler.inspect must be called prior to this method.  Why not call
> inspect in this method itself and simplify the public interface for this
class?

Done.

http://codereview.appspot.com/61041/diff/3465/1875
File tests/com/google/caja/plugin/HtmlEmitterTest.java (right):

http://codereview.appspot.com/61041/diff/3465/1875#newcode159
Line 159: StringBuilder sb = new StringBuilder();
On 2009/05/22 20:42:54, MikeSamuel wrote:
> On 2009/05/22 04:50:51, jasvir wrote:
> > Why are you rendering individual statements instead of sticking the
statements
> > in a block and asking it to render itself?
> 
> Because blocks add unnecessary brackets and indenting.
> 

Makes sense; left as-is.

http://codereview.appspot.com/61041/diff/3465/1864
File tests/com/google/caja/plugin/templates/RenamingTest.java (right):

http://codereview.appspot.com/61041/diff/3465/1864#newcode16
Line 16: 
On 2009/05/22 20:42:54, MikeSamuel wrote:
> On 2009/05/22 04:50:51, jasvir wrote:
> > What class is this test supposed to test?
> 
> One that is not in this CL.  Renaming does alpha renaming of template
variables.

Ok, leaving as-is.

http://codereview.appspot.com/61041/diff/1939/3565
File src/com/google/caja/plugin/templates/SafeHtmlMaker.java (right):

http://codereview.appspot.com/61041/diff/1939/3565#newcode68
Line 68: *   <dt>Bones</dt>
On 2009/05/27 21:21:16, jasvir wrote:
> A bone may reference a node in the skeleton which is a span constructed as a
> marker.

See below.

http://codereview.appspot.com/61041/diff/1939/3565#newcode70
Line 70: *     scripts.</dd>
On 2009/05/27 21:21:16, jasvir wrote:
> Clarification: A script in a bone is to be run immediately after the node the
> bone points to.

I think we've determined that the scripts are actually JS expressions that
generate *all* the attributes and even some tag names, so I don't think this
remark is valid any longer.

[ihab.awad, 2009-05-29 16:24:22]

Update: This change is now moved to:

   http://codereview.appspot.com/67126