Modify Safe HTML Maker to optionally emit HTML as part of the JS.

1. Modify SafeHtmlMaker to optionally emit its HTML as part of the JS, as calls on the HTML emitter. This makes it possible to cajole some HTML to pure JS. This change envisions the following scenarios:

  a. Cajoling HTML->HTML is mainly for containers that
     compose their HTML pages server-side and wish to
     optimize startup latency.

  b. Cajoling HTML->JS is mainly for containers that
     dynamically embed plugins and wish to use the
     simplicity of the module system, or the cross site
     capability of <script> loading.

2. Updated the cajoling service (and some upstream components) to allow a new Caja module format -- one that provides a JSONP-style callback function. So in addition to the classic:

  ___.loadModule({...});

we can now emit:

  customCallback(___.prepareModule({...}));

where "customCallback" is something the client asked for. As an immediate use for this, a future change will take advantage of this in the <script> module loader, so that the <script> module loader will no longer be so fragile when container pages change the default module handler.

[ihab.awad, 2009-10-07 00:12:22]

Adding jasvir as reviewer.

[ihab.awad, 2009-10-07 22:19:15]

Ready for review. My intention is for mikesamuel to focus on the SafeHtmlMaker
(and related) stuff, and metaweta to focus on the cajoling service stuff. But of
course you folks are welcome to cross-check each other' chunks if you have time.
:)

[metaweta, 2009-10-08 23:26:00]

LGTM

http://codereview.appspot.com/126062/diff/2028/3041
File src/com/google/caja/plugin/templates/SafeHtmlMaker.java (right):

http://codereview.appspot.com/126062/diff/2028/3041#newcode60
Line 60: * The {@link #make()} method of this class produces two pieces of
content: safe
Add as much commentary on this as you can, please.

http://codereview.appspot.com/126062/diff/2028/3049
File src/com/google/caja/service/CajaArguments.java (right):

http://codereview.appspot.com/126062/diff/2028/3049#newcode59
Line 59: * The Caja language transform that is being requested. Valid values
are:
Maybe point out that the Cajita transform only applies to .js files, while the
valija transform only applies to .html (though it can refer to .css and .js)?

http://codereview.appspot.com/126062/diff/2028/3044
File src/com/google/caja/service/ContentHandler.java (right):

http://codereview.appspot.com/126062/diff/2028/3044#newcode56
Line 56: * @param checker Used to check whheter two content-types are compatible
whether

[MikeSamuel, 2009-10-09 02:31:44]

http://codereview.appspot.com/126062/diff/2028/3038
File src/com/google/caja/parser/js/CajoledModule.java (right):

http://codereview.appspot.com/126062/diff/2028/3038#newcode48
Line 48: (Expression) QuasiBuilder.substV("___.loadModule");
= should start the line.  Please don't add a dependency from parser.js to
QuasiBuilder.  Just do
    Operator.create(Operator.MEMBER_ACCESS, ...);

http://codereview.appspot.com/126062/diff/2028/3038#newcode109
Line 109: Expression expr = callbackFunction == DEFAULT_MODULE_CALLBACK
Is == the right thing to do here?

http://codereview.appspot.com/126062/diff/2028/3038#newcode128
Line 128: Appendable out, Callback<IOException> exHandler) {
Please comment this method.  Maybe a pointer to where things like
originalSources and callbackFunction are described.

http://codereview.appspot.com/126062/diff/2028/3038#newcode182
Line 182: Appendable out,
What is the reason for all the non-standard rendering code in this class?

http://codereview.appspot.com/126062/diff/2028/3038#newcode191
Line 191: if (callbackFunction != DEFAULT_MODULE_CALLBACK) {
Again, is != right here?

http://codereview.appspot.com/126062/diff/2028/3038#newcode196
Line 196: hout.append("\n");
What is hout?  Does it go through the normal printer?

If so, why is ___.prepareModule one token?
If not, why are ___.prepareModule and ( different chunks?

http://codereview.appspot.com/126062/diff/2028/3038#newcode233
Line 233: if (callbackFunction != DEFAULT_MODULE_CALLBACK) {
ditto

http://codereview.appspot.com/126062/diff/2028/3039
File src/com/google/caja/plugin/PluginMeta.java (right):

http://codereview.appspot.com/126062/diff/2028/3039#newcode28
Line 28: private boolean emitHtmlAsJs;
Do we want to emit html as js or only emit js?

http://codereview.appspot.com/126062/diff/2028/3039#newcode65
Line 65: public boolean isEmitHtmlAsJs() { return emitHtmlAsJs; }
isEmit -> shouldEmit or emit

http://codereview.appspot.com/126062/diff/2028/3040
File src/com/google/caja/plugin/html-emitter.js (right):

http://codereview.appspot.com/126062/diff/2028/3040#newcode35
Line 35: }
Please put this function with the other public entry points.

emit* functions should not clobber existing content.
Maybe check whether there are children, and if so, create a DIV, inject the
HTML, and then extract the children into base.

http://codereview.appspot.com/126062/diff/2028/3040#newcode253
Line 253: this.emitStatic = emitStatic;
Plese put in alphabetic order below discard.

http://codereview.appspot.com/126062/diff/2028/3032
File tests/com/google/caja/parser/quasiliteral/ModuleFormatTest.java (right):

http://codereview.appspot.com/126062/diff/2028/3032#newcode133
Line 133: (Expression) QuasiBuilder.substV("foo.bar.baz"));
indentation
or maybe just use the render method on CajaTestCase.

http://codereview.appspot.com/126062/diff/2028/3032#newcode135
Line 135: ParseTreeNode reparsedModule =
operators should appear at the beginning of a line when the line is split.

http://codereview.appspot.com/126062/diff/2028/3032#newcode138
Line 138: .children().get(0).children().get(0).children().get(0);
It's not clear what this is testing.

http://codereview.appspot.com/126062/diff/2028/3032#newcode152
Line 152: (Expression) QuasiBuilder.substV("foo.bar.baz"),
Maybe jsExpr(fromString(...)) instead of QuasiBuilder.substV

http://codereview.appspot.com/126062/diff/2028/3032#newcode158
Line 158: .children().get(0).children().get(0).children().get(0);
ditto

http://codereview.appspot.com/126062/diff/2028/3033
File tests/com/google/caja/plugin/html-emitter-test.html (right):

http://codereview.appspot.com/126062/diff/2028/3033#newcode33
Line 33: <blockquote id="base1"></blockquote>
Indentation

http://codereview.appspot.com/126062/diff/2028/3033#newcode83
Line 83: emitter___.emitStatic('<p>Hello world</p>');
</p> => <\/p>

http://codereview.appspot.com/126062/diff/2028/3033#newcode86
Line 86: '<p>Hello world</p>',
ditto.

http://codereview.appspot.com/126062/diff/2028/3034
File tests/com/google/caja/plugin/templates/TemplateCompilerTest.java (right):

http://codereview.appspot.com/126062/diff/2028/3034#newcode342
Line 342: htmlFragment(fromString("<span/>")),
Why is this span here?

http://codereview.appspot.com/126062/diff/2028/3034#newcode346
Line 346: + "  var el___; var emitter___ = IMPORTS___.htmlEmitter___;"
Please put each statement on its own line.

[ihab.awad, 2009-10-13 22:21:17]

New snapshot uploaded.

http://codereview.appspot.com/126062/diff/2028/3038
File src/com/google/caja/parser/js/CajoledModule.java (right):

http://codereview.appspot.com/126062/diff/2028/3038#newcode48
Line 48: (Expression) QuasiBuilder.substV("___.loadModule");
On 2009/10/09 02:31:44, MikeSamuel wrote:
> = should start the line.  Please don't add a dependency from parser.js to
> QuasiBuilder.  Just do
>     Operator.create(Operator.MEMBER_ACCESS, ...);

There are a lot of QuasiBuilder usages in this file and they make things lots
more readable. That said, I understand the need for not birds' nestifying our
dependencies. I'm not sure what to do ....

If you insist ;) then I'll remove the quasis entirely. Otherwise, can we maybe
add a TODO to move QuasiBuilder to the parser/ or parser/js/ package, since
after all it's just a thin layer on top of the parser and JS ParseTreeNodes?

Anyway -- I added the TODO and let me know what you think now.

http://codereview.appspot.com/126062/diff/2028/3038#newcode109
Line 109: Expression expr = callbackFunction == DEFAULT_MODULE_CALLBACK
On 2009/10/09 02:31:44, MikeSamuel wrote:
> Is == the right thing to do here?

Yes to the extent that DEFAULT_MODULE_CALLBACK is private and used as an
internal signal that the default output should be produced.

What this does bring up, though, is that for this class as written, if someone
calls:

  render(rc, expr("___.loadModule"));

  produces => "___.loadModule(___.prepareModule({...}))"

whereas if they call:

  render(rc);

  produces => "___.loadModule({...})"

which may be confusing but doing otherwise would require doing deep equality on
the passed-in expression, and in general seems like more magic than necessary.

Anyway I'm going to refactor this; see how you like the new version.

http://codereview.appspot.com/126062/diff/2028/3038#newcode128
Line 128: Appendable out, Callback<IOException> exHandler) {
On 2009/10/09 02:31:44, MikeSamuel wrote:
> Please comment this method.  Maybe a pointer to where things like
> originalSources and callbackFunction are described.

Done.

http://codereview.appspot.com/126062/diff/2028/3038#newcode182
Line 182: Appendable out,
On 2009/10/09 02:31:44, MikeSamuel wrote:
> What is the reason for all the non-standard rendering code in this class?

Because, to make the debugging metadata format work, the number of lines in the
top of the output text must be precisely defined. I try to delegate to default
renderers whenever possible, and take control of only what is necessary to
ensure the format is supported.

http://codereview.appspot.com/126062/diff/2028/3038#newcode191
Line 191: if (callbackFunction != DEFAULT_MODULE_CALLBACK) {
On 2009/10/09 02:31:44, MikeSamuel wrote:
> Again, is != right here?

Refactored out.

http://codereview.appspot.com/126062/diff/2028/3038#newcode196
Line 196: hout.append("\n");
On 2009/10/09 02:31:44, MikeSamuel wrote:
> What is hout?  Does it go through the normal printer?
> 
> If so, why is ___.prepareModule one token?
> If not, why are ___.prepareModule and ( different chunks?

It's not a token stream. The chunks were broken out for readability. Refactored
anyway; see new version.

http://codereview.appspot.com/126062/diff/2028/3038#newcode233
Line 233: if (callbackFunction != DEFAULT_MODULE_CALLBACK) {
On 2009/10/09 02:31:44, MikeSamuel wrote:
> ditto

ditto

http://codereview.appspot.com/126062/diff/2028/3039
File src/com/google/caja/plugin/PluginMeta.java (right):

http://codereview.appspot.com/126062/diff/2028/3039#newcode28
Line 28: private boolean emitHtmlAsJs;
On 2009/10/09 02:31:44, MikeSamuel wrote:
> Do we want to emit html as js or only emit js?

Sounds clearer your way ... done.

http://codereview.appspot.com/126062/diff/2028/3039#newcode65
Line 65: public boolean isEmitHtmlAsJs() { return emitHtmlAsJs; }
On 2009/10/09 02:31:44, MikeSamuel wrote:
> isEmit -> shouldEmit or emit

-> isOnlyJsEmitted.

http://codereview.appspot.com/126062/diff/2028/3040
File src/com/google/caja/plugin/html-emitter.js (right):

http://codereview.appspot.com/126062/diff/2028/3040#newcode35
Line 35: }
On 2009/10/09 02:31:44, MikeSamuel wrote:
> Please put this function with the other public entry points.
> 
> emit* functions should not clobber existing content.

Rght now, emitStatic is called in exactly one place: setting the statically
declared markup in the target container.

The execution model of the original content is that, upon startup, it has in its
Domita virtual "document" the nodes declared in its static markup -- no less,
but also no more. Changing that behavior is a gotcha.

If this all makes sense so far, then expediency would favor the simplest
possible implementation. Perhaps with a doc comment that says, "the existing
HTML will be clobbered, and any container that finds this surprising is breaking
the contract presented to the original content".

What am I missing?

> Maybe check whether there are children, and if so, create a DIV, inject the
> HTML, and then extract the children into base.

Pending clarification.

http://codereview.appspot.com/126062/diff/2028/3040#newcode253
Line 253: this.emitStatic = emitStatic;
On 2009/10/09 02:31:44, MikeSamuel wrote:
> Plese put in alphabetic order below discard.

Done.

http://codereview.appspot.com/126062/diff/2028/3041
File src/com/google/caja/plugin/templates/SafeHtmlMaker.java (right):

http://codereview.appspot.com/126062/diff/2028/3041#newcode60
Line 60: * The {@link #make()} method of this class produces two pieces of
content: safe
On 2009/10/08 23:26:01, metaweta wrote:
> Add as much commentary on this as you can, please.

Done.

http://codereview.appspot.com/126062/diff/2028/3049
File src/com/google/caja/service/CajaArguments.java (right):

http://codereview.appspot.com/126062/diff/2028/3049#newcode59
Line 59: * The Caja language transform that is being requested. Valid values
are:
On 2009/10/08 23:26:01, metaweta wrote:
> Maybe point out that the Cajita transform only applies to .js files, while
the
> valija transform only applies to .html (though it can refer to .css and .js)?

Done.

http://codereview.appspot.com/126062/diff/2028/3044
File src/com/google/caja/service/ContentHandler.java (right):

http://codereview.appspot.com/126062/diff/2028/3044#newcode56
Line 56: * @param checker Used to check whheter two content-types are compatible
On 2009/10/08 23:26:01, metaweta wrote:
> whether

Done.

http://codereview.appspot.com/126062/diff/2028/3032
File tests/com/google/caja/parser/quasiliteral/ModuleFormatTest.java (right):

http://codereview.appspot.com/126062/diff/2028/3032#newcode133
Line 133: (Expression) QuasiBuilder.substV("foo.bar.baz"));
On 2009/10/09 02:31:44, MikeSamuel wrote:
> indentation
> or maybe just use the render method on CajaTestCase.

Can't *quite* use that (since I need the special callbackExpression form of
render()), but did something similar.

http://codereview.appspot.com/126062/diff/2028/3032#newcode135
Line 135: ParseTreeNode reparsedModule =
On 2009/10/09 02:31:44, MikeSamuel wrote:
> operators should appear at the beginning of a line when the line is split.

Done.

http://codereview.appspot.com/126062/diff/2028/3032#newcode138
Line 138: .children().get(0).children().get(0).children().get(0);
On 2009/10/09 02:31:44, MikeSamuel wrote:
> It's not clear what this is testing.

Done.

http://codereview.appspot.com/126062/diff/2028/3032#newcode152
Line 152: (Expression) QuasiBuilder.substV("foo.bar.baz"),
On 2009/10/09 02:31:44, MikeSamuel wrote:
> Maybe jsExpr(fromString(...)) instead of QuasiBuilder.substV

Done.

http://codereview.appspot.com/126062/diff/2028/3032#newcode158
Line 158: .children().get(0).children().get(0).children().get(0);
On 2009/10/09 02:31:44, MikeSamuel wrote:
> ditto

Done.

http://codereview.appspot.com/126062/diff/2028/3033
File tests/com/google/caja/plugin/html-emitter-test.html (right):

http://codereview.appspot.com/126062/diff/2028/3033#newcode33
Line 33: <blockquote id="base1"></blockquote>
On 2009/10/09 02:31:44, MikeSamuel wrote:
> Indentation

Done.

http://codereview.appspot.com/126062/diff/2028/3033#newcode83
Line 83: emitter___.emitStatic('<p>Hello world</p>');
On 2009/10/09 02:31:44, MikeSamuel wrote:
> </p> => <\/p>

Done.

http://codereview.appspot.com/126062/diff/2028/3033#newcode86
Line 86: '<p>Hello world</p>',
On 2009/10/09 02:31:44, MikeSamuel wrote:
> ditto.

Done.

http://codereview.appspot.com/126062/diff/2028/3034
File tests/com/google/caja/plugin/templates/TemplateCompilerTest.java (right):

http://codereview.appspot.com/126062/diff/2028/3034#newcode342
Line 342: htmlFragment(fromString("<span/>")),
On 2009/10/09 02:31:44, MikeSamuel wrote:
> Why is this span here?

Because SafeHtmlMaker, as modified, returns an empty <span> for the static HTML
if it is asked to emit only JS. Why, you ask? Well....

SafeHtmlMaker emits a Pair containing a Node and a list of JS Blocks. For the
Node, I could either return something benign (like the <span>) or return null. I
chose to return the <span> to guard against NPEs in clients that may assume
there is always HTML anyway -- in other words, the output will "work" regardless
of how it's used.

http://codereview.appspot.com/126062/diff/2028/3034#newcode346
Line 346: + "  var el___; var emitter___ = IMPORTS___.htmlEmitter___;"
On 2009/10/09 02:31:44, MikeSamuel wrote:
> Please put each statement on its own line.

Done.

[MikeSamuel, 2009-10-15 19:31:59]

http://codereview.appspot.com/126062/diff/2028/3034
File tests/com/google/caja/plugin/templates/TemplateCompilerTest.java (right):

http://codereview.appspot.com/126062/diff/2028/3034#newcode342
Line 342: htmlFragment(fromString("<span/>")),
On 2009/10/13 22:21:17, ihab.awad wrote:
> On 2009/10/09 02:31:44, MikeSamuel wrote:
> > Why is this span here?
> 
> Because SafeHtmlMaker, as modified, returns an empty <span> for the static
HTML
> if it is asked to emit only JS. Why, you ask? Well....
> 
> SafeHtmlMaker emits a Pair containing a Node and a list of JS Blocks. For the
> Node, I could either return something benign (like the <span>) or return null.
I
> chose to return the <span> to guard against NPEs in clients that may assume
> there is always HTML anyway -- in other words, the output will "work"
regardless
> of how it's used.

Please return an empty document fragment instead.

[ihab.awad, 2009-10-16 16:23:41]

Done & re-snapshotted.

What else?

http://codereview.appspot.com/126062/diff/2028/3034
File tests/com/google/caja/plugin/templates/TemplateCompilerTest.java (right):

http://codereview.appspot.com/126062/diff/2028/3034#newcode342
Line 342: htmlFragment(fromString("<span/>")),
On 2009/10/15 19:31:59, MikeSamuel wrote:
> On 2009/10/13 22:21:17, ihab.awad wrote:
> > On 2009/10/09 02:31:44, MikeSamuel wrote:
> > > Why is this span here?
> > 
> > Because SafeHtmlMaker, as modified, returns an empty <span> for the static
> HTML
> > if it is asked to emit only JS. Why, you ask? Well....
> > 
> > SafeHtmlMaker emits a Pair containing a Node and a list of JS Blocks. For
the
> > Node, I could either return something benign (like the <span>) or return
null.
> I
> > chose to return the <span> to guard against NPEs in clients that may assume
> > there is always HTML anyway -- in other words, the output will "work"
> regardless
> > of how it's used.
> 
> Please return an empty document fragment instead.
> 

Done.

[metaweta, 2009-10-19 20:05:19]

LGTM

http://codereview.appspot.com/126062/diff/4001/3092
File src/com/google/caja/service/ContentHandler.java (right):

http://codereview.appspot.com/126062/diff/4001/3092#newcode56
Line 56: * @param checker Used to check wheter two content-types are compatible
whether (again)

[MikeSamuel, 2009-10-19 22:36:04]

http://codereview.appspot.com/126062/diff/4001/3089
File src/com/google/caja/plugin/templates/SafeHtmlMaker.java (right):

http://codereview.appspot.com/126062/diff/4001/3089#newcode135
Line 135: * has only a few, shallowly nested {@code <script>} nodes with few
siblings.
This documentation is great.  Thanks much.

http://codereview.appspot.com/126062/diff/4001/3089#newcode180
Line 180: quasiStmt("/*@synthetic*/'placeholder';");
operator should appear at beginning of line.

TODO: Is the fact that this node is synthetic significant?
How can a string literal by synthetic?  Only Identifiers and
FunctionConstructors can be synthetic.

http://codereview.appspot.com/126062/diff/4001/3089#newcode364
Line 364: emitStatement(EMIT_STATIC_HTML_PLACEHOLDER);
Instead of emitting a placeholder, can we just emit the static HTML when we're
packaging stuff up?

So instead of modifying this class, can the stage that invokes this, check the
meta, and do
  if (meta.isOnlyJsEmitted) {
    String staticHtml = render(html);
    if (!"".equals(staticHtml)) {
      jobs.add("IMPORTS___.htmlEmitter___.emitStatic(...)")
    }
  } else {
    jobs.add(htmlJob);
  }

[MikeSamuel, 2009-10-19 22:42:55]

http://codereview.appspot.com/126062/diff/4001/3088
File src/com/google/caja/plugin/html-emitter.js (right):

http://codereview.appspot.com/126062/diff/4001/3088#newcode75
Line 75: base.innerHTML = htmlString;
As discussed, I'm fine with clobbering, but maybe we could fail fast if there is
content.
Something like
  // TODO: we could append the HTML but no clients need that yet.
  if (base.firstChild) { throw new Error(...); }

[ihab.awad, 2009-10-22 21:26:22]

mikesamuel, note my question about your suggested refactoring.

http://codereview.appspot.com/126062/diff/4001/3088
File src/com/google/caja/plugin/html-emitter.js (right):

http://codereview.appspot.com/126062/diff/4001/3088#newcode75
Line 75: base.innerHTML = htmlString;
On 2009/10/19 22:42:55, MikeSamuel wrote:
> As discussed, I'm fine with clobbering, but maybe we could fail fast if there
is
> content.
> Something like
>   // TODO: we could append the HTML but no clients need that yet.
>   if (base.firstChild) { throw new Error(...); }

Done.

http://codereview.appspot.com/126062/diff/4001/3089
File src/com/google/caja/plugin/templates/SafeHtmlMaker.java (right):

http://codereview.appspot.com/126062/diff/4001/3089#newcode135
Line 135: * has only a few, shallowly nested {@code <script>} nodes with few
siblings.
On 2009/10/19 22:36:04, MikeSamuel wrote:
> This documentation is great.  Thanks much.

Cool.

http://codereview.appspot.com/126062/diff/4001/3089#newcode180
Line 180: quasiStmt("/*@synthetic*/'placeholder';");
On 2009/10/19 22:36:04, MikeSamuel wrote:
> operator should appear at beginning of line.

Done.

> TODO: Is the fact that this node is synthetic significant?
> How can a string literal by synthetic?  Only Identifiers and
> FunctionConstructors can be synthetic.

Syntheticity was superfluous; removed.

http://codereview.appspot.com/126062/diff/4001/3089#newcode364
Line 364: emitStatement(EMIT_STATIC_HTML_PLACEHOLDER);
On 2009/10/19 22:36:04, MikeSamuel wrote:
> Instead of emitting a placeholder, can we just emit the static HTML when
we're
> packaging stuff up?
> 
> So instead of modifying this class, can the stage that invokes this, check
the
> meta, and do
>   if (meta.isOnlyJsEmitted) {
>     String staticHtml = render(html);
>     if (!"".equals(staticHtml)) {
>       jobs.add("IMPORTS___.htmlEmitter___.emitStatic(...)")
>     }
>   } else {
>     jobs.add(htmlJob);
>   }

This is a great idea, but it's also a pretty major refactoring. Can we add a
TODO or a refactoring bug, or is the current design really egregiously horrible
for some reason?

http://codereview.appspot.com/126062/diff/4001/3092
File src/com/google/caja/service/ContentHandler.java (right):

http://codereview.appspot.com/126062/diff/4001/3092#newcode56
Line 56: * @param checker Used to check wheter two content-types are compatible
On 2009/10/19 20:05:19, metaweta wrote:
> whether (again)

Done.

[MikeSamuel, 2009-10-22 21:33:48]

http://codereview.appspot.com/126062/diff/4001/3089
File src/com/google/caja/plugin/templates/SafeHtmlMaker.java (right):

http://codereview.appspot.com/126062/diff/4001/3089#newcode364
Line 364: emitStatement(EMIT_STATIC_HTML_PLACEHOLDER);
Why is it more complicated than:

Index: src/com/google/caja/plugin/stages/CompileHtmlStage.java
===================================================================
--- src/com/google/caja/plugin/stages/CompileHtmlStage.java     (revision 3825)
+++ src/com/google/caja/plugin/stages/CompileHtmlStage.java     (working copy)
@@ -19,7 +19,9 @@
 import com.google.caja.parser.AncestorChain;
 import com.google.caja.parser.css.CssTree;
 import com.google.caja.parser.html.DomParser;
+import com.google.caja.parser.html.Nodes;
 import com.google.caja.parser.js.Block;
+import com.google.caja.parser.quasiliteral.QuasiBuilder;
 import com.google.caja.plugin.Dom;
 import com.google.caja.plugin.Job;
 import com.google.caja.plugin.Jobs;
@@ -85,7 +87,16 @@
       Pair<Node, List<Block>> htmlAndJs = tc.getSafeHtml(
           DomParser.makeDocument(null, null));

-      jobs.getJobs().add(new Job(AncestorChain.instance(new
Dom(htmlAndJs.a))));
+      boolean onlyJs = false;
+      if (onlyJs) {
+        jobs.getJobs().add(new Job(AncestorChain.instance(QuasiBuilder.substV(
+            "IMPORTS___.htmlEmitter___.emitStatic(@html);",
+            "html", Nodes.render(htmlAndJs.a)
+            ))));
+      } else {
+        jobs.getJobs().add(
+            new Job(AncestorChain.instance(new Dom(htmlAndJs.a))));
+      }
       for (Block bl : htmlAndJs.b) {
         jobs.getJobs().add(new Job(AncestorChain.instance(bl)));
       }

[ihab.awad, 2009-10-27 21:09:16]

New snapshot.

[MikeSamuel, 2009-10-27 22:02:35]

http://codereview.appspot.com/126062/diff/9001/10012
File src/com/google/caja/plugin/stages/CompileHtmlStage.java (right):

http://codereview.appspot.com/126062/diff/9001/10012#newcode117
Line 117: "html", renderDomAsJsStringLiteral(node)));
LGTM

http://codereview.appspot.com/126062/diff/9001/10012#newcode127
Line 127: });
There is a Concatenator(StringBuilder) ctor.

http://codereview.appspot.com/126062/diff/9001/10012#newcode128
Line 128: Nodes.render(node, new RenderContext(tc).withEmbeddable(true));
OK

http://codereview.appspot.com/126062/diff/9001/10012#newcode129
Line 129: return new StringLiteral(
Prefer StringLiteral.valueOf(Nodes.getFilePositionFor(node), stringBuilder);

http://codereview.appspot.com/126062/diff/9001/10003
File tests/com/google/caja/plugin/stages/CompileHtmlStageTest.java (right):

http://codereview.appspot.com/126062/diff/9001/10003#newcode25
Line 25: public final class CompileHtmlStageTest extends PipelineStageTestCase {
Ok, this was the result of an {svn cp} so the testcases to the left were not
deleted.

http://codereview.appspot.com/126062/diff/9001/10003#newcode31
Line 31: Job.JobType.JAVASCRIPT));
Short.  Sweet.  Spiffy.

http://codereview.appspot.com/126062/diff/9001/10003#newcode41
Line 41: Job.JobType.JAVASCRIPT),
cool

http://codereview.appspot.com/126062/diff/9001/10003#newcode44
Line 44: + "  throw 'Translated code must never be executed';\n"
Ok, this is the paranoid guff at the top of TranslatedCode.render()

http://codereview.appspot.com/126062/diff/9001/10009
File tests/com/google/caja/util/TestUtil.java (right):

http://codereview.appspot.com/126062/diff/9001/10009#newcode19
Line 19: import com.google.caja.lexer.TokenConsumer;
Not used

http://codereview.appspot.com/126062/diff/9001/10009#newcode30
Line 30: import com.google.caja.reporting.RenderContext;
Not used

[ihab.awad, 2009-10-27 22:24:47]

http://codereview.appspot.com/126062/diff/9001/10012
File src/com/google/caja/plugin/stages/CompileHtmlStage.java (right):

http://codereview.appspot.com/126062/diff/9001/10012#newcode127
Line 127: });
On 2009/10/27 22:02:35, MikeSamuel wrote:
> There is a Concatenator(StringBuilder) ctor.

Done.

http://codereview.appspot.com/126062/diff/9001/10012#newcode129
Line 129: return new StringLiteral(
On 2009/10/27 22:02:35, MikeSamuel wrote:
> Prefer StringLiteral.valueOf(Nodes.getFilePositionFor(node), stringBuilder);

Done.

http://codereview.appspot.com/126062/diff/9001/10009
File tests/com/google/caja/util/TestUtil.java (right):

http://codereview.appspot.com/126062/diff/9001/10009#newcode19
Line 19: import com.google.caja.lexer.TokenConsumer;
On 2009/10/27 22:02:35, MikeSamuel wrote:
> Not used

Done.

http://codereview.appspot.com/126062/diff/9001/10009#newcode30
Line 30: import com.google.caja.reporting.RenderContext;
On 2009/10/27 22:02:35, MikeSamuel wrote:
> Not used

Done.

[MikeSamuel, 2009-10-27 22:28:40]

LGTM

[ihab.awad, 2009-10-27 22:40:12]

@3828