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.
http://codereview.appspot.com/126062
