Add tracing rewriter for ease of comparison between runs

Add tracing rewriter for ease of comparison between runs

[kpreid2, 2013-03-12 19:22:06]

(*Not* adding myself as reviewer for the whole thing, just pointing out
something I noticed while taking an interested look.)

https://codereview.appspot.com/7678045/diff/1/src/com/google/caja/plugin/Plug...
File src/com/google/caja/plugin/PluginCompilerMain.java (right):

https://codereview.appspot.com/7678045/diff/1/src/com/google/caja/plugin/Plug...
src/com/google/caja/plugin/PluginCompilerMain.java:334: public static
MessageLevel dumpMessages(
Not necessary? TracingRewriterMain is in the same package.

[ihab.awad, 2013-03-25 03:15:27]

Ready for review when you are! :)

[Jasvir, 2013-04-03 07:13:49]

https://codereview.appspot.com/7678045/diff/12001/build.xml
File build.xml (right):

https://codereview.appspot.com/7678045/diff/12001/build.xml#newcode422
build.xml:422: <succeed message=
Debugging?

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/parser/...
File src/com/google/caja/parser/js/PlainModule.java (right):

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/parser/...
src/com/google/caja/parser/js/PlainModule.java:57: public Object getValue() {
Remind me how this value is used?

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/parser/...
File src/com/google/caja/parser/quasiliteral/TracingRewriter.java (right):

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/parser/...
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:270: "fname",
bindings.get("fname"),
Does this do the right thing when rendering and @fname was null?

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/parser/...
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:294:
substitutes="TRACING.log(@lit + @expr)")
Nice!

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/parser/...
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:312: matches="new
@ctor(@args*)",
Do you need to also handle new complex expr - like new x.y?

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/parser/...
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:422: + " 
TRACING.@pushCallsite('function', @pos, @name);"
Check that pushCallsite and popCallsite don't throw their own exceptions.

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/plugin/...
File src/com/google/caja/plugin/TracingRewriterMain.java (right):

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/TracingRewriterMain.java:75: dumpMessages(mq);
dumpMessages(mq);
return;

...would save you the levels of nesting.

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/service...
File src/com/google/caja/service/HtmlHandler.java (right):

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/service...
src/com/google/caja/service/HtmlHandler.java:102: boolean htmlInline, Writer
output,
Any reason to restrict this to a Writer?

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/tracing.js
File src/com/google/caja/tracing.js (right):

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/tracing...
src/com/google/caja/tracing.js:88: ctx = ctx.parent;
Can this ever result in derefencing undefined?  What happens if you trace a
program consisting just of a "return". (I am trying to ensure that the tracer
does not end up masking bugs in the original program or otherwise changing it's
behavior by generating it's own exceptions).

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/tracing...
src/com/google/caja/tracing.js:101: if (!ctx.log) {
Ah this is not initialized along with the rest of ctx in clear() because it's
supposed to last across clear()s.

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/tracing...
src/com/google/caja/tracing.js:108: var xhr = new XMLHttpRequest();
Consider doing the check for XDomainRequest since this tool would probably come
in really handy on IE where xhr isn't CORS enabled.

[ihab.awad, 2013-08-01 19:47:21]

Add tracing rewriter for ease of comparison between runs

[ihab.awad, 2013-08-01 20:33:40]

https://codereview.appspot.com/7678045/diff/12001/build.xml
File build.xml (right):

https://codereview.appspot.com/7678045/diff/12001/build.xml#newcode422
build.xml:422: <succeed message=
On 2013/04/03 07:13:49, Jasvir wrote:
> Debugging?

Done.

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/parser/...
File src/com/google/caja/parser/js/PlainModule.java (right):

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/parser/...
src/com/google/caja/parser/js/PlainModule.java:57: public Object getValue() {
On 2013/04/03 07:13:49, Jasvir wrote:
> Remind me how this value is used?

It should not be. It's just the inherited member accessor for things like the
"value" of a ParseTreeNode representing a literal, etc.

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/parser/...
File src/com/google/caja/parser/quasiliteral/TracingRewriter.java (right):

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/parser/...
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:270: "fname",
bindings.get("fname"),
On 2013/04/03 07:13:49, Jasvir wrote:
> Does this do the right thing when rendering and @fname was null?

The test cases have a lot of anon functions in them and they work, so yes.

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/parser/...
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:294:
substitutes="TRACING.log(@lit + @expr)")
On 2013/04/03 07:13:49, Jasvir wrote:
> Nice!

Done.

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/parser/...
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:312: matches="new
@ctor(@args*)",
On 2013/04/03 07:13:49, Jasvir wrote:
> Do you need to also handle new complex expr - like new x.y?

Yes, that should work fine given that @ctor is expand()-ed below.

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/parser/...
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:422: + " 
TRACING.@pushCallsite('function', @pos, @name);"
On 2013/04/03 07:13:49, Jasvir wrote:
> Check that pushCallsite and popCallsite don't throw their own exceptions.

Good point. My hope is that these 2 functions are simple enough that they are
too small to fail. Or are things too big to fail these days? I don't know.
Anyway.

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/plugin/...
File src/com/google/caja/plugin/TracingRewriterMain.java (right):

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/TracingRewriterMain.java:75: dumpMessages(mq);
On 2013/04/03 07:13:49, Jasvir wrote:
> dumpMessages(mq);
> return;
> 
> ...would save you the levels of nesting.

Done.

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/service...
File src/com/google/caja/service/HtmlHandler.java (right):

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/service...
src/com/google/caja/service/HtmlHandler.java:102: boolean htmlInline, Writer
output,
On 2013/04/03 07:13:49, Jasvir wrote:
> Any reason to restrict this to a Writer?

Yeah -- because the new-and-improved renderAsJSON would not accept it. Note this
is just a private method, not part of the inherited API of the class.

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/tracing.js
File src/com/google/caja/tracing.js (right):

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/tracing...
src/com/google/caja/tracing.js:88: ctx = ctx.parent;
The tracer always (or is supposed to, any rate) write out programs with a
symmetrical number of pushes and pops. Your hypothetical program is not legal
JavaScript, but for other similar things, the tracer writes out a push and a pop
for the program as a whole.

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/tracing...
src/com/google/caja/tracing.js:101: if (!ctx.log) {
On 2013/04/03 07:13:49, Jasvir wrote:
> Ah this is not initialized along with the rest of ctx in clear() because it's
> supposed to last across clear()s.

Done.

https://codereview.appspot.com/7678045/diff/12001/src/com/google/caja/tracing...
src/com/google/caja/tracing.js:108: var xhr = new XMLHttpRequest();
On 2013/04/03 07:13:49, Jasvir wrote:
> Consider doing the check for XDomainRequest since this tool would probably
come
> in really handy on IE where xhr isn't CORS enabled.

Noted as an enhancement, thank you. The code has lain fallow for too long so I'm
trying to get it checked in 1st.

[ihab.awad, 2013-08-01 20:34:02]

Add tracing rewriter for ease of comparison between runs

[kpreid_google, 2013-08-01 21:48:39]

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/p...
File src/com/google/caja/demos/playground/client/policy.js (right):

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/demos/p...
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/...
File src/com/google/caja/parser/js/PlainModule.java (right):

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/...
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/...
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/...
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/...
File src/com/google/caja/parser/quasiliteral/TracingRewriter.java (right):

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/...
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:45: * @author
ihab.awad@gmail.com
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/...
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/...
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/...
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/...
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/...
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/...
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/...
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/...
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/...
File src/com/google/caja/plugin/TracingRewriterMain.java (right):

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/plugin/...
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/...
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/...
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...
File src/com/google/caja/service/TracingHandler.java (right):

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/service...
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...
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...
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...
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...
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...
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/parse...
File
tests/com/google/caja/parser/quasiliteral/tracingRewriterTestAssertTraces.js
(right):

https://codereview.appspot.com/7678045/diff/30025/tests/com/google/caja/parse...
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/parse...
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/parse...
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/parse...
File tests/com/google/caja/parser/quasiliteral/tracingRewriterTestCases.js
(right):

https://codereview.appspot.com/7678045/diff/30025/tests/com/google/caja/parse...
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/parse...
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/...
File tests/com/google/caja/util/RhinoExecutor.java (right):

https://codereview.appspot.com/7678045/diff/30025/tests/com/google/caja/util/...
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).

[kpreid_google, 2013-09-18 16:55:23]

ping

[ihab.awad, 2013-09-19 21:38:13]

Add tracing rewriter for ease of comparison between runs

[ihab.awad, 2013-09-19 21:38:16]

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"/>
On 2013/08/01 21:48:39, kpreid_google wrote:
> 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.)

Done.

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>
On 2013/08/01 21:48:39, kpreid_google wrote:
> wrong change

Done.

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/demos/p...
File src/com/google/caja/demos/playground/client/policy.js (right):

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/demos/p...
src/com/google/caja/demos/playground/client/policy.js:18: imports.console =
frameGroup.tame(frameGroup.markReadOnlyRecord({
On 2013/08/01 21:48:39, kpreid_google wrote:
> Please add a TODO to add a standardized taming of console somewhere in Caja.
We
> should have fewer copies of this code.

Done.

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/...
File src/com/google/caja/parser/js/PlainModule.java (right):

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/...
src/com/google/caja/parser/js/PlainModule.java:1: // Copyright (C) 2013 Google
Inc.
On 2013/08/01 21:48:39, kpreid_google wrote:
> This file appears to be marked as a copy, but not be sufficiently similar to
its
> original to be relevant.

Yeah, sorry. :(

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/...
src/com/google/caja/parser/js/PlainModule.java:30: /**
On 2013/08/01 21:48:39, kpreid_google wrote:
> 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?

Yeah I dunno, but the minifier is under ancillary/ and is a nontrivial codebase
to plumb. This is just a simple utility that does precisely what I need. :)

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/...
src/com/google/caja/parser/js/PlainModule.java:66: public final TokenConsumer
makeRenderer(
On 2013/08/01 21:48:39, kpreid_google wrote:
> Why does this belong specifically on this class?
> 
> Should be static and package-private?

It's inherited from Renderable....

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/...
File src/com/google/caja/parser/quasiliteral/TracingRewriter.java (right):

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/...
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:45: * @author
ihab.awad@gmail.com
On 2013/08/01 21:48:39, kpreid_google wrote:
> Document purpose. If nothing else, at least explain that this is not part of
the
> cajoler.

Done.

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/...
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:45: * @author
ihab.awad@gmail.com
On 2013/08/01 21:48:39, kpreid_google wrote:
> Document purpose. If nothing else, at least explain that this is not part of
the
> cajoler.

Done.

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/...
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:48: 
Deferred.

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/...
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:49: private static
StringLiteral nodeEqualsTo(ParseTreeNode n) {
Deferred.

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/...
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:50: StringBuffer sb
= new StringBuffer();
On 2013/08/01 21:48:39, kpreid_google wrote:
> StringBuilder is preferred when thread-safety is not required.

Done.

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/...
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:118: ((expr
instanceof Reference )
On 2013/08/01 21:48:39, kpreid_google wrote:
> unnecessary parens (all "comparison" operators are higher precedence than ?:,
> and this is common across all C-style syntax I've heard of)
> stray whitespace

Done.

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/...
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:126:
((fc.getIdentifierName() != null)
On 2013/08/01 21:48:39, kpreid_google wrote:
> ditto

Done.

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/...
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:161:
synopsis="Cajole an UncajoledModule into a PlainModule.",
On 2013/08/01 21:48:39, kpreid_google wrote:
> "Cajole" is wrong

Done.

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/...
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:223: + "  @fh*;"
On 2013/08/01 21:48:39, kpreid_google wrote:
> This substitution pattern appears not to be used. The parameter can be
omitted.

Done.

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/parser/...
src/com/google/caja/parser/quasiliteral/TracingRewriter.java:339: + "    return
TRACING.@popCallsite('return', new ctor(@actuals*));"
On 2013/08/01 21:48:39, kpreid_google wrote:
> long line

Done.

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/plugin/...
File src/com/google/caja/plugin/TracingRewriterMain.java (right):

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/plugin/...
src/com/google/caja/plugin/TracingRewriterMain.java:58: new UriFetcher() {
Deferred.

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/plugin/...
src/com/google/caja/plugin/TracingRewriterMain.java:74: if
(mq.hasMessageAtLevel(MessageLevel.WARNING)) {
Deferred.

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/plugin/...
src/com/google/caja/plugin/TracingRewriterMain.java:89: mq.addMessage(
Deferred.

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/service...
File src/com/google/caja/service/TracingHandler.java (right):

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/service...
src/com/google/caja/service/TracingHandler.java:97: return null;
Deferred.

https://codereview.appspot.com/7678045/diff/30025/src/com/google/caja/service...
src/com/google/caja/service/TracingHandler.java:140: new Callback<IOException>()
{
Deferred.

https://codereview.appspot.com/7678045/diff/30025/tests/com/google/caja/parse...
File
tests/com/google/caja/parser/quasiliteral/tracingRewriterTestAssertTraces.js
(right):

https://codereview.appspot.com/7678045/diff/30025/tests/com/google/caja/parse...
tests/com/google/caja/parser/quasiliteral/tracingRewriterTestAssertTraces.js:30:
function stringify(o) {
Deferred.

https://codereview.appspot.com/7678045/diff/30025/tests/com/google/caja/parse...
tests/com/google/caja/parser/quasiliteral/tracingRewriterTestAssertTraces.js:33:
if (k === 'parent' || k === 'result' || k === 'level') { return undefined; }
Deferred.

https://codereview.appspot.com/7678045/diff/30025/tests/com/google/caja/parse...
tests/com/google/caja/parser/quasiliteral/tracingRewriterTestAssertTraces.js:57:
Object.getOwnPropertyNames(expected).map(add);
On 2013/08/01 21:48:39, kpreid_google wrote:
> use forEach instead of map

Done.

https://codereview.appspot.com/7678045/diff/30025/tests/com/google/caja/parse...
File tests/com/google/caja/parser/quasiliteral/tracingRewriterTestCases.js
(right):

https://codereview.appspot.com/7678045/diff/30025/tests/com/google/caja/parse...
tests/com/google/caja/parser/quasiliteral/tracingRewriterTestCases.js:24: return
funcExprRE.exec(f.toSource())[1];
This code is run under Rhino.

https://codereview.appspot.com/7678045/diff/30025/tests/com/google/caja/parse...
tests/com/google/caja/parser/quasiliteral/tracingRewriterTestCases.js:43:
function () {
On 2013/08/01 21:48:39, kpreid_google wrote:
> no space before (

Done.

https://codereview.appspot.com/7678045/diff/30025/tests/com/google/caja/util/...
File tests/com/google/caja/util/RhinoExecutor.java (right):

https://codereview.appspot.com/7678045/diff/30025/tests/com/google/caja/util/...
tests/com/google/caja/util/RhinoExecutor.java:159: // Deny reflective access up
front.  This should not be triggered due
On 2013/08/01 21:48:39, kpreid_google wrote:
> Why this change?

I don't recall specifically, but it was necessary and seemed safe at the time.

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.
Deferred.

[ihab.awad, 2013-09-19 21:39:14]

Add tracing rewriter for ease of comparison between runs

[ihab.awad, 2013-09-19 21:41:25]

Add tracing rewriter for ease of comparison between runs

[kpreid_google, 2013-09-20 18:19:23]

LGTM

[ihab.awad, 2013-09-21 22:20:03]

Add tracing rewriter for ease of comparison between runs

[ihab.awad, 2013-09-21 22:21:16]

@5601