https://codereview.appspot.com/7678045/diff/30025/build.xml
File build.xml (right):
https://codereview.appspot.com/7678045/diff/30025/build.xml#newcode1228
build.xml:1228: <include name="**/caja/tracing.js"/>
I think tracing should have its own package (caja/tracing/tracing.js,
caja/tracing/TracingRewriter.java,
caja/tracing/TracingRewriterMain.java).
(I agree that this does not entirely match the existing package
structure, but I think it's better and will help with disentangling
things in the future.)
https://codereview.appspot.com/7678045/diff/30025/build.xml#newcode1834
build.xml:1834: <echo>Next step will succeed if you don't have Graphviz
installed</echo>
wrong change
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/demos/playground/client/policy.js
File src/com/google/caja/demos/playground/client/policy.js (right):
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/demos/playground/client/policy.js#newcode18
src/com/google/caja/demos/playground/client/policy.js:18:
imports.console = frameGroup.tame(frameGroup.markReadOnlyRecord({
Please add a TODO to add a standardized taming of console somewhere in
Caja. We should have fewer copies of this code.
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/js/PlainModule.java
File src/com/google/caja/parser/js/PlainModule.java (right):
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/js/PlainModule.java#newcode1
src/com/google/caja/parser/js/PlainModule.java:1: // Copyright (C) 2013
Google Inc.
This file appears to be marked as a copy, but not be sufficiently
similar to its original to be relevant.
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/js/PlainModule.java#newcode30
src/com/google/caja/parser/js/PlainModule.java:30: /**
Please document, as its class name is not obvious.
If I understand what this does (basically what the comment inside
render() says), I'm surprised we don't already have something that does
it. Shouldn't the minifier etc. need this functionality already?
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/js/PlainModule.java#newcode66
src/com/google/caja/parser/js/PlainModule.java:66: public final
TokenConsumer makeRenderer(
Why does this belong specifically on this class?
Should be static and package-private?
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/quasiliteral/TracingRewriter.java
File src/com/google/caja/parser/quasiliteral/TracingRewriter.java
(right):
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/quasiliteral/TracingRewriter.java#newcode45
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:45: *
@author [email protected]
Document purpose. If nothing else, at least explain that this is not
part of the cajoler.
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/quasiliteral/TracingRewriter.java#newcode48
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:48:
Don't we usually put private utility methods at the bottom of the file?
(Static _fields_ being at the top, not all static methods.) I would
prefer that style if we're not against it, so that the reader first sees
what this class is actually about.
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/quasiliteral/TracingRewriter.java#newcode49
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:49: private
static StringLiteral nodeEqualsTo(ParseTreeNode n) {
There are many copies of "render Node to string" in our codebase
according to a search for ".makeRenderer(sb". How about putting the
logic as a public static method somewhere, and making this into just an
appending of " = "? There's already a generic one as
CajaTestCase.render, so one could move that outside of tests/.
(I don't know why the Callback<IOException> is here, so maybe that's a
reason.)
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/quasiliteral/TracingRewriter.java#newcode50
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:50:
StringBuffer sb = new StringBuffer();
StringBuilder is preferred when thread-safety is not required.
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/quasiliteral/TracingRewriter.java#newcode118
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:118: ((expr
instanceof Reference )
unnecessary parens (all "comparison" operators are higher precedence
than ?:, and this is common across all C-style syntax I've heard of)
stray whitespace
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/quasiliteral/TracingRewriter.java#newcode126
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:126:
((fc.getIdentifierName() != null)
ditto
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/quasiliteral/TracingRewriter.java#newcode161
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:161:
synopsis="Cajole an UncajoledModule into a PlainModule.",
"Cajole" is wrong
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/quasiliteral/TracingRewriter.java#newcode223
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:223: + "
@fh*;"
This substitution pattern appears not to be used. The parameter can be
omitted.
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/quasiliteral/TracingRewriter.java#newcode339
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:339: + "
return TRACING.@popCallsite('return', new ctor(@actuals*));"
long line
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/plugin/TracingRewriterMain.java
File src/com/google/caja/plugin/TracingRewriterMain.java (right):
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/plugin/TracingRewriterMain.java#newcode58
src/com/google/caja/plugin/TracingRewriterMain.java:58: new UriFetcher()
{
Can we please not have another implementation of UriFetcher? Say, factor
the existing one out of PluginCompilerMain?
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/plugin/TracingRewriterMain.java#newcode74
src/com/google/caja/plugin/TracingRewriterMain.java:74: if
(mq.hasMessageAtLevel(MessageLevel.WARNING)) {
Please factor out this repetition so the code looks as straightforward
as it actually is.
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/plugin/TracingRewriterMain.java#newcode89
src/com/google/caja/plugin/TracingRewriterMain.java:89: mq.addMessage(
Please factor out this repetition so the code looks as straightforward
as it actually is.
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/service/TracingHandler.java
File src/com/google/caja/service/TracingHandler.java (right):
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/service/TracingHandler.java#newcode97
src/com/google/caja/service/TracingHandler.java:97: return null;
AAAAAAAAAAAAAAAAAAAAAAAAAA
Seriously, can we have better error handling here?
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/service/TracingHandler.java#newcode140
src/com/google/caja/service/TracingHandler.java:140: new
Callback<IOException>() {
grumble grumble duplicate code grumble (this one doesn't seem too
obnoxious though)
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/tracing.js
File src/com/google/caja/tracing.js (right):
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/tracing.js#newcode16
src/com/google/caja/tracing.js:16: * @fileoverview The tracing runtime
library.
"Tracing? What's tracing?" This needs to point at, if not some
documentation, then at least where the rest of the code is (i.e.
TracingRewriter) and what the entry point for _using_ it is.
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/tracing.js#newcode44
src/com/google/caja/tracing.js:44: // Exclude 'parent' pointers since
these create cycles.
explain why 'result' is excluded
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/tracing.js#newcode54
src/com/google/caja/tracing.js:54: for (var i = 0; i < a.length; i++) {
use indexOf?
https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/tracing.js#newcode157
src/com/google/caja/tracing.js:157: initialize: initialize,
trailing comma
Please add linting of this file to build.xml, as it would have caught
this.
https://codereview.appspot.com/7678045/diff/30025/tests/com/google/caja/parser/quasiliteral/tracingRewriterTestAssertTraces.js
File
tests/com/google/caja/parser/quasiliteral/tracingRewriterTestAssertTraces.js
(right):
https://codereview.appspot.com/7678045/diff/30025/tests/com/google/caja/parser/quasiliteral/tracingRewriterTestAssertTraces.js#newcode30
tests/com/google/caja/parser/quasiliteral/tracingRewriterTestAssertTraces.js:30:
function stringify(o) {
Since this is almost the same as tracing.js's stringify and they're
working on the same format, maybe export it from tracing.js instead of
duplicating?
https://codereview.appspot.com/7678045/diff/30025/tests/com/google/caja/parser/quasiliteral/tracingRewriterTestAssertTraces.js#newcode33
tests/com/google/caja/parser/quasiliteral/tracingRewriterTestAssertTraces.js:33:
if (k === 'parent' || k === 'result' || k === 'level') { return
undefined; }
Explain why 'level' is excluded.
https://codereview.appspot.com/7678045/diff/30025/tests/com/google/caja/parser/quasiliteral/tracingRewriterTestAssertTraces.js#newcode57
tests/com/google/caja/parser/quasiliteral/tracingRewriterTestAssertTraces.js:57:
Object.getOwnPropertyNames(expected).map(add);
use forEach instead of map
https://codereview.appspot.com/7678045/diff/30025/tests/com/google/caja/parser/quasiliteral/tracingRewriterTestCases.js
File
tests/com/google/caja/parser/quasiliteral/tracingRewriterTestCases.js
(right):
https://codereview.appspot.com/7678045/diff/30025/tests/com/google/caja/parser/quasiliteral/tracingRewriterTestCases.js#newcode24
tests/com/google/caja/parser/quasiliteral/tracingRewriterTestCases.js:24:
return funcExprRE.exec(f.toSource())[1];
toSource is a Mozilla-ism; use toString instead.
https://codereview.appspot.com/7678045/diff/30025/tests/com/google/caja/parser/quasiliteral/tracingRewriterTestCases.js#newcode43
tests/com/google/caja/parser/quasiliteral/tracingRewriterTestCases.js:43:
function () {
no space before (
https://codereview.appspot.com/7678045/diff/30025/tests/com/google/caja/util/RhinoExecutor.java
File tests/com/google/caja/util/RhinoExecutor.java (right):
https://codereview.appspot.com/7678045/diff/30025/tests/com/google/caja/util/RhinoExecutor.java#newcode159
tests/com/google/caja/util/RhinoExecutor.java:159: // Deny reflective
access up front. This should not be triggered due
Why this change?
https://codereview.appspot.com/7678045/diff/30025/tools/postfile.py
File tools/postfile.py (right):
https://codereview.appspot.com/7678045/diff/30025/tools/postfile.py#newcode18
tools/postfile.py:18: Simple HTTP server for saving files from test
harnesses.
If this is to be used with tracing, I think it belongs in bin/ (programs
which are part of Caja) rather than tools/ (programs for developing
Caja).
https://codereview.appspot.com/7678045/
--
---
You received this message because you are subscribed to the Google Groups "Google Caja Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
