An optimization pass that annotates array acceses so that CajitaRewriter can optimize them.

This change

- Implements an optimization pass between ConsolidateCode and
  ValidateJavascript that folds converts foo[bar] to foo[+bar] when
  bar is provably a publicly accessible array member regardless of
  what foo is.
- Adds a convenience link for the crypto-AES benchmark to the testbed,
  and makes the game of life button into a similar convenience link.
- Adds tests that measure the performance improvement on the
  array-heavy crypto-AES benchmark.
- Adds tests to browser-expectations that allow us to quickly
  determine whether a particular interpreter behaves correctly around
  numeric operators.

Unresolved questions:
- A previous implementation turned off the optimization when debug
  mode was off so that cajita-debugmode.js could give proper
  stacktraces for expressions like (a = null, a[0]).  Since
  CajitaRewriter now does the job of optimizing, do we need to turn
  off the array index optimizations in CajitaRewriter when
  pluginMeta.getDebugMode() is true?

[MikeSamuel, 2008-12-16 20:08:09]

ping

[MarkM, 2008-12-20 00:22:30]

Review still very incomplete, but publishing now so we can overlap answers with
more questions.

Sorry, but I hadn't seen till now that there's a more recent patchset. All
further comments will be on that one.

http://codereview.appspot.com/8703/diff/1/15
File src/com/google/caja/opensocial/applet/example-aes.js (right):

http://codereview.appspot.com/8703/diff/1/15#newcode37
Line 37: * AES Cipher function: encrypt 'input' with Rijndael algorithm
Why is this in src/../opensocial/applet instead of third_party or
tests/../demos/benchmarks ?

http://codereview.appspot.com/8703/diff/1/11
File src/com/google/caja/opensocial/applet/example-life.js (right):

http://codereview.appspot.com/8703/diff/1/11#newcode1
Line 1: <!-- Cellular automaton gadget. This code is in the public domain. -->
Why is this in src/../opensocial/applet instead of third_party or
tests/../demos/benchmarks ?

http://codereview.appspot.com/8703/diff/1/14
File src/com/google/caja/opensocial/applet/xmlhttp.js (right):

http://codereview.appspot.com/8703/diff/1/14#newcode17
Line 17: * An XMLHttpRequest wrapper.
Seems strange to do all this work to create a safe wrapper only for use by the
testbed. Is there any overlap with Ihab's work to create a tamed XMLHttpRequest
wrapper? Should there be?

http://codereview.appspot.com/8703/diff/1/14#newcode51
Line 51: // http://isaacschlueter.com/2006/10/msie-memory-leaks/
TODO(erights): Follow these links and learn enough about XMLHttpRequest problems
in current browsers to be able to comment. The current review comments on this
CL lack this, and so are incomplete.

http://codereview.appspot.com/8703/diff/1/14#newcode63
Line 63: // Make sure that xml http response processing does not interleave
Omigod! Asynchronous response processing might interleave otherwise? (Surely
browser implementors would not do anything so silly? ;))

But it seems this module is only providing synchronous access. For synchronous
access, why do we want the responseClosure to run the callback/handler
asynchronously? (Or, if we do want it to run it asyncronously, why don't we want
to make an asynchronous request?)

http://codereview.appspot.com/8703/diff/1/20
File src/com/google/caja/parser/AncestorChain.java (right):

http://codereview.appspot.com/8703/diff/1/20#newcode11
Line 11: public final class AncestorChain<T extends ParseTreeNode> {
(echoing verbal conversation) As we transition to immutable ASTs, we should be
able to painlessly share subtrees. However (as you just pointed out to me), if
the same parent contains the same child in two positions, the current
AncestorChain structure becomes inadequate to record context. 

How about adding an "int index" instance variable, such that, if parent != null,
then parent.children()[index] == node, but the former says which appearance of
node is being considered? If so, the constructor for the parent-present case
only needs (parent, index).

Many of the convenience methods below would become either unnecessary or more
trivial; as well as more accurate. For example, isFirstSibling() becomes simply
"index == 0".

http://codereview.appspot.com/8703/diff/1/20#newcode26
Line 26: public static <T extends ParseTreeNode> AncestorChain<T> instance(T
node) {
Why does this class have both public constructors and public static factory
methods that trivially wrap these constructors?

http://codereview.appspot.com/8703/diff/1/17
File src/com/google/caja/parser/quasiliteral/opt/ArrayIndexOptimization.java
(right):

http://codereview.appspot.com/8703/diff/1/17#newcode47
Line 47: *   known to be an {@code Array} or that all publicly visible members
of
As we've discussed verbally, this documentation is confusing. The actual
optimization does not care at all whether the object being indexed is an array,
nor does it care about any properties other than numbers and "length" -- neither
of which are specific to arrays.

http://codereview.appspot.com/8703/diff/1/17#newcode54
Line 54: *   {@code NaN} instead of the name {@code undefined}.  This is
documented as
Certainly it should be so documented, just as we should actually have a cajoler
spec. Perhaps future tense + TODO?

http://codereview.appspot.com/8703/diff/1/17#newcode56
Line 56: *     var o = { 0: 'a', NaN: 'b', undefined: 'c' };
Based on the code which follows, this should be "var a = ..."

http://codereview.appspot.com/8703/diff/1/17#newcode63
Line 63: *   use-before-initialization.
If this is a dynamic check, I suspect we'd lose all the gains this CL is trying
to gain. If it's a static check, it can't be accurate. If you agree, please
remove this line.

http://codereview.appspot.com/8703/diff/1/17#newcode92
Line 92: || key instanceof NumberLiteral)) {
Unindent this by one space, so that it lines up with the "(" of "(key"

http://codereview.appspot.com/8703/diff/1/17#newcode121
Line 121: static boolean doesVarReferenceArrayMember(
Again, the issue isn't "ArrayMember", but rather, does it reference either a
number, a stringified number or 'length'. All misleading references to "Array
member" and such should be replaced by an appropriate phrase (I wish I had a
short suggestion.)

If we want to generalize: Does it reference an unconditionally whitelisted
property name? (Though I still have no short suggestion.)

http://codereview.appspot.com/8703/diff/1/18
File src/com/google/caja/parser/quasiliteral/opt/OptimizationPass.java (right):

http://codereview.appspot.com/8703/diff/1/18#newcode65
Line 65: // Find all the sites we can apply an optimization too.
"too" should be "to"

http://codereview.appspot.com/8703/diff/1/18#newcode116
Line 116: protected abstract boolean isOptimizationSemanticsPreserving(
(Echoes verbal discussion) Since an OptimizationPass describes multiple possible
optimizations that may apply to a given candidate AncestorChain, this API for
determining if a given *candidate optimization* would apply isn't adequate,
since it doesn't indicate which possible optimization is being considered.

Something more like our normal rewriter structure may be more appropriate: where
a rewriter mostly consists of a sequence of candidate Rule objects, each of
which has an input and out quasi-string, but where arbitrary code is associated
with each individual input-output pair, rather than with the rewriter pass as a
whole.

http://codereview.appspot.com/8703/diff/1/18#newcode162
Line 162: for (Pair<String, String> optimization : optimizations) {
From this loop, I'd guess that the answer to my question on OptimizationPassTest
is that only the first applicable match is applied to the input AST, independent
of whether either 1) the input matched other rules as well or 2) the output
matches any rules. I think property #1 is desirable. #2 may not be, in order to
combine optimizations, but this probably doesn't yet matter.

http://codereview.appspot.com/8703/diff/1/16
File src/com/google/caja/parser/quasiliteral/opt/ScopeTree.java (right):

http://codereview.appspot.com/8703/diff/1/16#newcode37
Line 37: private final AncestorChain<?> root;
Since we have a constructor which has parameters for initializing these first
three instance the order of instance variable decls should agree with the
constructor parameter order.

http://codereview.appspot.com/8703/diff/1/16#newcode112
Line 112: for (ScopeTree t : children) {
If this loop fails to find a match, this code will fall through to "return
this;". But musn't it find a match? If it doesn't find a match, isn't that an
internal error -- most probably a precondition violation?

http://codereview.appspot.com/8703/diff/1/16#newcode120
Line 120: return Collections.unmodifiableList(children);
Why not do "children = Collections.unmodifiableList(children);" at the end of
the constructor, in which case this can simply "return children;"?

http://codereview.appspot.com/8703/diff/1/7
File tests/com/google/caja/parser/quasiliteral/opt/OptimizationPassTest.java
(right):

http://codereview.appspot.com/8703/diff/1/7#newcode32
Line 32: public void testMultipleOptimizations() throws Exception {
Two untested cases where the the OptimizationPass spec is unclear:
* What if the .a portion of multiple rules match a given specimen AST node?
* What if the result of transforming with one rule matches the .a of another
rule?

[ihab.awad, 2009-02-20 05:49:07]

Just one little thing I found when reading the CL.

http://codereview.appspot.com/8703/diff/1401/1424
File src/com/google/caja/util/CapturingReader.java (right):

http://codereview.appspot.com/8703/diff/1401/1424#newcode65
Line 65: delegate.close();
Is this redundant with the close() below?

[MikeSamuel, 2009-03-05 02:21:19]

http://codereview.appspot.com/8703/diff/1401/1424
File src/com/google/caja/util/CapturingReader.java (right):

http://codereview.appspot.com/8703/diff/1401/1424#newcode65
Line 65: delegate.close();
On 2009/02/20 05:49:07, ihab.awad wrote:
> Is this redundant with the close() below?

Yes.

[MikeSamuel, 2009-04-25 01:49:14]

http://codereview.appspot.com/8703/diff/1/15
File src/com/google/caja/opensocial/applet/example-aes.js (right):

http://codereview.appspot.com/8703/diff/1/15#newcode37
Line 37: * AES Cipher function: encrypt 'input' with Rijndael algorithm
On 2008/12/20 00:22:30, MarkM wrote:
> Why is this in src/../opensocial/applet instead of third_party or
> tests/../demos/benchmarks ?

So that it can be loaded by the testbed.

http://codereview.appspot.com/8703/diff/1/14
File src/com/google/caja/opensocial/applet/xmlhttp.js (right):

http://codereview.appspot.com/8703/diff/1/14#newcode17
Line 17: * An XMLHttpRequest wrapper.
On 2008/12/20 00:22:30, MarkM wrote:
> Seems strange to do all this work to create a safe wrapper only for use by the
> testbed. Is there any overlap with Ihab's work to create a tamed
XMLHttpRequest
> wrapper? Should there be?

This is used by the container, so shouldn't be tamed.

http://codereview.appspot.com/8703/diff/1/14#newcode51
Line 51: // http://isaacschlueter.com/2006/10/msie-memory-leaks/
On 2008/12/20 00:22:30, MarkM wrote:
> TODO(erights): Follow these links and learn enough about XMLHttpRequest
problems
> in current browsers to be able to comment. The current review comments on this
> CL lack this, and so are incomplete.

I don't follow.
This implementation should not have the leak problems described by those
articles.

http://codereview.appspot.com/8703/diff/1/20
File src/com/google/caja/parser/AncestorChain.java (right):

http://codereview.appspot.com/8703/diff/1/20#newcode11
Line 11: public final class AncestorChain<T extends ParseTreeNode> {
On 2008/12/20 00:22:30, MarkM wrote:
> (echoing verbal conversation) As we transition to immutable ASTs, we should be
> able to painlessly share subtrees. However (as you just pointed out to me), if
> the same parent contains the same child in two positions, the current
> AncestorChain structure becomes inadequate to record context. 
> 
> How about adding an "int index" instance variable, such that, if parent !=
null,
> then parent.children()[index] == node, but the former says which appearance of
> node is being considered? If so, the constructor for the parent-present case
> only needs (parent, index).
> 
> Many of the convenience methods below would become either unnecessary or more
> trivial; as well as more accurate. For example, isFirstSibling() becomes
simply
> "index == 0".

I realized those methods were only used in one place, so I got rid of them.

http://codereview.appspot.com/8703/diff/1/20#newcode26
Line 26: public static <T extends ParseTreeNode> AncestorChain<T> instance(T
node) {
On 2008/12/20 00:22:30, MarkM wrote:
> Why does this class have both public constructors and public static factory
> methods that trivially wrap these constructors?

History.  Changed to use factory methods consistently.

http://codereview.appspot.com/8703/diff/1/17
File src/com/google/caja/parser/quasiliteral/opt/ArrayIndexOptimization.java
(right):

http://codereview.appspot.com/8703/diff/1/17#newcode47
Line 47: *   known to be an {@code Array} or that all publicly visible members
of
On 2008/12/20 00:22:30, MarkM wrote:
> As we've discussed verbally, this documentation is confusing. The actual
> optimization does not care at all whether the object being indexed is an
array,
> nor does it care about any properties other than numbers and "length" --
neither
> of which are specific to arrays.

Done.

http://codereview.appspot.com/8703/diff/1/17#newcode54
Line 54: *   {@code NaN} instead of the name {@code undefined}.  This is
documented as
On 2008/12/20 00:22:30, MarkM wrote:
> Certainly it should be so documented, just as we should actually have a
cajoler
> spec. Perhaps future tense + TODO?

Done.

http://codereview.appspot.com/8703/diff/1/17#newcode56
Line 56: *     var o = { 0: 'a', NaN: 'b', undefined: 'c' };
On 2008/12/20 00:22:30, MarkM wrote:
> Based on the code which follows, this should be "var a = ..."

Done.

http://codereview.appspot.com/8703/diff/1/17#newcode63
Line 63: *   use-before-initialization.
On 2008/12/20 00:22:30, MarkM wrote:
> If this is a dynamic check, I suspect we'd lose all the gains this CL is
trying
> to gain. If it's a static check, it can't be accurate. If you agree, please
> remove this line.

I disagree.  We can do conservative and fairly accurate use-before
initialization checks, and simply decline to optimize when we can't.
In the common idiom:
  for (var i = 0; i < array.length; ++i) {
    f(arr[i]);
  }
we can definitively say that the i in f(arr[i]) is not read before i has been
initialized.  If f aliases eval, and arr[0] contains 'i = undefined', then we
can't guarantee that i is a number, but we can say definitively that it has been
initialized.

http://codereview.appspot.com/8703/diff/1/17#newcode92
Line 92: || key instanceof NumberLiteral)) {
On 2008/12/20 00:22:30, MarkM wrote:
> Unindent this by one space, so that it lines up with the "(" of "(key"

Done.

http://codereview.appspot.com/8703/diff/1/17#newcode121
Line 121: static boolean doesVarReferenceArrayMember(
On 2008/12/20 00:22:30, MarkM wrote:
> Again, the issue isn't "ArrayMember", but rather, does it reference either a
> number, a stringified number or 'length'. All misleading references to "Array
> member" and such should be replaced by an appropriate phrase (I wish I had a
> short suggestion.)

Done.  Used the term "visible property" and documented it above.  Should this
class change to "VisiblePropertyOptimization?"

> If we want to generalize: Does it reference an unconditionally whitelisted
> property name? (Though I still have no short suggestion.)

http://codereview.appspot.com/8703/diff/1/18
File src/com/google/caja/parser/quasiliteral/opt/OptimizationPass.java (right):

http://codereview.appspot.com/8703/diff/1/18#newcode65
Line 65: // Find all the sites we can apply an optimization too.
On 2008/12/20 00:22:30, MarkM wrote:
> "too" should be "to"

Done.

http://codereview.appspot.com/8703/diff/1/18#newcode116
Line 116: protected abstract boolean isOptimizationSemanticsPreserving(
On 2008/12/20 00:22:30, MarkM wrote:
> (Echoes verbal discussion) Since an OptimizationPass describes multiple
possible
> optimizations that may apply to a given candidate AncestorChain, this API for
> determining if a given *candidate optimization* would apply isn't adequate,
> since it doesn't indicate which possible optimization is being considered.
> 
> Something more like our normal rewriter structure may be more appropriate:
where
> a rewriter mostly consists of a sequence of candidate Rule objects, each of
> which has an input and out quasi-string, but where arbitrary code is
associated
> with each individual input-output pair, rather than with the rewriter pass as
a
> whole.

That sounds like a good idea, but I'd rather do that reorg in another CL.  How
about I check this in without hooking it up, and rework it in a follow-on CL?

http://codereview.appspot.com/8703/diff/1/16
File src/com/google/caja/parser/quasiliteral/opt/ScopeTree.java (right):

http://codereview.appspot.com/8703/diff/1/16#newcode37
Line 37: private final AncestorChain<?> root;
On 2008/12/20 00:22:30, MarkM wrote:
> Since we have a constructor which has parameters for initializing these first
> three instance the order of instance variable decls should agree with the
> constructor parameter order.

Done.

http://codereview.appspot.com/8703/diff/1/16#newcode112
Line 112: for (ScopeTree t : children) {
On 2008/12/20 00:22:30, MarkM wrote:
> If this loop fails to find a match, this code will fall through to "return
> this;". But musn't it find a match? If it doesn't find a match, isn't that an
> internal error -- most probably a precondition violation? 

Done.

http://codereview.appspot.com/8703/diff/1/16#newcode120
Line 120: return Collections.unmodifiableList(children);
On 2008/12/20 00:22:30, MarkM wrote:
> Why not do "children = Collections.unmodifiableList(children);" at the end of
> the constructor, in which case this can simply "return children;"?

Done.

http://codereview.appspot.com/8703/diff/1/7
File tests/com/google/caja/parser/quasiliteral/opt/OptimizationPassTest.java
(right):

http://codereview.appspot.com/8703/diff/1/7#newcode32
Line 32: public void testMultipleOptimizations() throws Exception {
On 2008/12/20 00:22:30, MarkM wrote:
> Two untested cases where the the OptimizationPass spec is unclear:
> * What if the .a portion of multiple rules match a given specimen AST node?
> * What if the result of transforming with one rule matches the .a of another
> rule?

These might be easier to test once I switch to a more rewriter like style.

[metaweta, 2009-04-25 03:38:31]

http://codereview.appspot.com/8703/diff/1/17
File src/com/google/caja/parser/quasiliteral/opt/ArrayIndexOptimization.java
(right):

http://codereview.appspot.com/8703/diff/1/17#newcode63
Line 63: *   use-before-initialization.
On 2009/04/25 01:49:14, MikeSamuel wrote:
> On 2008/12/20 00:22:30, MarkM wrote:
> > If this is a dynamic check, I suspect we'd lose all the gains this CL is
> trying
> > to gain. If it's a static check, it can't be accurate. If you agree, please
> > remove this line.
> 
> I disagree.  We can do conservative and fairly accurate use-before
> initialization checks, and simply decline to optimize when we can't.
> In the common idiom:
>   for (var i = 0; i < array.length; ++i) {
>     f(arr[i]);
>   }
> we can definitively say that the i in f(arr[i]) is not read before i has been
> initialized.  If f aliases eval, and arr[0] contains 'i = undefined', then we
> can't guarantee that i is a number, but we can say definitively that it has
been
> initialized.

What about if it says "delete i;" ?

[MikeSamuel, 2009-05-20 05:20:18]

Reworked to run before the Cajita Rewriter and got rid of OptimizationPass.

See especially changes to ArrayOptimization, ArrayOptimizationTest, and
CajitaRewriter.



On 2009/04/25 03:38:31, metaweta wrote:
> http://codereview.appspot.com/8703/diff/1/17
> File src/com/google/caja/parser/quasiliteral/opt/ArrayIndexOptimization.java
> (right):
> 
> http://codereview.appspot.com/8703/diff/1/17#newcode63
> Line 63: *   use-before-initialization.
> On 2009/04/25 01:49:14, MikeSamuel wrote:
> > On 2008/12/20 00:22:30, MarkM wrote:
> > > If this is a dynamic check, I suspect we'd lose all the gains this CL is
> > trying
> > > to gain. If it's a static check, it can't be accurate. If you agree,
please
> > > remove this line.
> > 
> > I disagree.  We can do conservative and fairly accurate use-before
> > initialization checks, and simply decline to optimize when we can't.
> > In the common idiom:
> >   for (var i = 0; i < array.length; ++i) {
> >     f(arr[i]);
> >   }
> > we can definitively say that the i in f(arr[i]) is not read before i has
been
> > initialized.  If f aliases eval, and arr[0] contains 'i = undefined', then
we
> > can't guarantee that i is a number, but we can say definitively that it has
> been
> > initialized.
> 
> What about if it says "delete i;" ?

[MikeSamuel, 2009-05-20 20:42:47]

Mark is going to take over this change.

[MarkM, 2009-08-05 20:36:19]

Although we can't make full use of this CL because of issue 1093, and although
the current state of this CL may be a bit stale because of our recent fixes to
close issue 1093, it is still worth proceeding with this as a non-default option
so that we can get experience playing with and measuring it. Once FF 3.5 falls
of the Yahoo A-Grade list, we should then able to change this default with more
confidence.

The following comments are far from a full review. Just some initial reactions
as I get my feet wet in this CL again.

http://codereview.appspot.com/8703/diff/21523/22538
File src/com/google/caja/demos/applet/example-aes.js (right):

http://codereview.appspot.com/8703/diff/21523/22538#newcode454
Line 454: // (mikesamuel): added timing dump to output.
Except for this addition, I did not review this file. Given its origin and
purpose, I do not believe it needs to be reviewed.

http://codereview.appspot.com/8703/diff/21523/22534
File src/com/google/caja/demos/applet/example-life.js (right):

http://codereview.appspot.com/8703/diff/21523/22534#newcode1
Line 1: <!-- Cellular automaton gadget. This code is in the public domain. -->
Origin of this code should still be attributed.

http://codereview.appspot.com/8703/diff/21523/22536
File src/com/google/caja/demos/applet/index.html (right):

http://codereview.appspot.com/8703/diff/21523/22536#newcode253
Line 253: To get started, you can load <a
href="javascript:loadExampleInto('example-life.js',
document.forms['cajolerForm.1'])">Conway's game of life</a>, <a
href="javascript:loadExampleInto('example-aes.js',
document.forms['cajolerForm.1'])">SunSpider crypto-AES</a>.
Break up into shorter lines if possible.

http://codereview.appspot.com/8703/diff/21523/22537
File src/com/google/caja/demos/applet/xmlhttp.js (right):

http://codereview.appspot.com/8703/diff/21523/22537#newcode17
Line 17: * An XMLHttpRequest wrapper.
Does the applet still need its own wrapper, or can it reuse one?

http://codereview.appspot.com/8703/diff/21523/22521
File src/com/google/caja/parser/quasiliteral/CajitaRewriter.java (right):

http://codereview.appspot.com/8703/diff/21523/22521#newcode1019
Line 1019: if (!preventOptimizations) {
My own style is that an if-then-else should always test a positive condition
unless there's some reason not to swap its then and else parts. In this case,
since the general style of rules is that the "return NONE;" is the fall through
case, I would keep the "!" in the conditional but remove the "else {" at the
end.

http://codereview.appspot.com/8703/diff/21523/22521#newcode2433
Line 2433: RewriteMode mode) {
There are many places where one of the CajitaRewriter constructors are called,
where this extra parameter causes them to add a RewriteMode.NORMAL. Rather than
require this for the default case, should we instead provide a modeless override
here that adds the default ourselves? This would give us a single place to
control what that default should be. Note that this interacts with the 1093
issues noted elsewhere.

http://codereview.appspot.com/8703/diff/21523/22521#newcode2437
Line 2437: this.preventOptimizations = mode == RewriteMode.DEBUG;
Is "preventOptimizations" tied to "RewriteMode.DEBUG" because otherwise this
optimization would interfere with debug mode execution?

http://codereview.appspot.com/8703/diff/21523/22521#newcode2446
Line 2446: public static boolean hasNumericResult(Expression e) {
Good refactoring.

http://codereview.appspot.com/8703/diff/21523/22517
File src/com/google/caja/parser/quasiliteral/opt/ArrayIndexOptimization.java
(right):

http://codereview.appspot.com/8703/diff/21523/22517#newcode50
Line 50: *   are visible properties of {@code Object}.
Should add link to bug 1093 together with caveat that this optimization
therefore must not be used when the target includes such browsers. Practically,
this means we can't make this optimization the default until FF 3.5 falls off
the Yahoo A-Grade list, which will be a while. But we can still make it a
non-default option so that we can play with it in the meantime.

http://codereview.appspot.com/8703/diff/21523/22529
File src/com/google/caja/plugin/PluginCompiler.java (right):

http://codereview.appspot.com/8703/diff/21523/22529#newcode134
Line 134: stages.add(new OptimizeJavascriptStage());
Must not make this the default until FF 3.5 falls off Yahoo A-Grade list. (Yes,
I know this CL was submitted before issue 1093.)

http://codereview.appspot.com/8703/diff/21523/22530
File src/com/google/caja/plugin/PluginCompilerMain.java (right):

http://codereview.appspot.com/8703/diff/21523/22530#newcode215
Line 215: } finally {
Does reversing the order in which the catch block and the finally block runs
matter? If not, I agree that the new way is better style.

[metaweta, 2011-12-16 17:40:53]

On 2009/08/05 20:36:19, MarkM wrote:
> Although we can't make full use of this CL because of issue 1093, and although
> the current state of this CL may be a bit stale because of our recent fixes to
> close issue 1093, it is still worth proceeding with this as a non-default
option
> so that we can get experience playing with and measuring it. Once FF 3.5 falls
> of the Yahoo A-Grade list, we should then able to change this default with
more
> confidence.
> 
> The following comments are far from a full review. Just some initial reactions
> as I get my feet wet in this CL again.
> 
> http://codereview.appspot.com/8703/diff/21523/22538
> File src/com/google/caja/demos/applet/example-aes.js (right):
> 
> http://codereview.appspot.com/8703/diff/21523/22538#newcode454
> Line 454: // (mikesamuel): added timing dump to output.
> Except for this addition, I did not review this file. Given its origin and
> purpose, I do not believe it needs to be reviewed.
> 
> http://codereview.appspot.com/8703/diff/21523/22534
> File src/com/google/caja/demos/applet/example-life.js (right):
> 
> http://codereview.appspot.com/8703/diff/21523/22534#newcode1
> Line 1: <!-- Cellular automaton gadget. This code is in the public domain. -->
> Origin of this code should still be attributed.
> 
> http://codereview.appspot.com/8703/diff/21523/22536
> File src/com/google/caja/demos/applet/index.html (right):
> 
> http://codereview.appspot.com/8703/diff/21523/22536#newcode253
> Line 253: To get started, you can load <a
> href="javascript:loadExampleInto('example-life.js',
> document.forms['cajolerForm.1'])">Conway's game of life</a>, <a
> href="javascript:loadExampleInto('example-aes.js',
> document.forms['cajolerForm.1'])">SunSpider crypto-AES</a>.
> Break up into shorter lines if possible.
> 
> http://codereview.appspot.com/8703/diff/21523/22537
> File src/com/google/caja/demos/applet/xmlhttp.js (right):
> 
> http://codereview.appspot.com/8703/diff/21523/22537#newcode17
> Line 17: * An XMLHttpRequest wrapper.
> Does the applet still need its own wrapper, or can it reuse one?
> 
> http://codereview.appspot.com/8703/diff/21523/22521
> File src/com/google/caja/parser/quasiliteral/CajitaRewriter.java (right):
> 
> http://codereview.appspot.com/8703/diff/21523/22521#newcode1019
> Line 1019: if (!preventOptimizations) {
> My own style is that an if-then-else should always test a positive condition
> unless there's some reason not to swap its then and else parts. In this case,
> since the general style of rules is that the "return NONE;" is the fall
through
> case, I would keep the "!" in the conditional but remove the "else {" at the
> end.
> 
> http://codereview.appspot.com/8703/diff/21523/22521#newcode2433
> Line 2433: RewriteMode mode) {
> There are many places where one of the CajitaRewriter constructors are called,
> where this extra parameter causes them to add a RewriteMode.NORMAL. Rather
than
> require this for the default case, should we instead provide a modeless
override
> here that adds the default ourselves? This would give us a single place to
> control what that default should be. Note that this interacts with the 1093
> issues noted elsewhere.
> 
> http://codereview.appspot.com/8703/diff/21523/22521#newcode2437
> Line 2437: this.preventOptimizations = mode == RewriteMode.DEBUG;
> Is "preventOptimizations" tied to "RewriteMode.DEBUG" because otherwise this
> optimization would interfere with debug mode execution?
> 
> http://codereview.appspot.com/8703/diff/21523/22521#newcode2446
> Line 2446: public static boolean hasNumericResult(Expression e) {
> Good refactoring.
> 
> http://codereview.appspot.com/8703/diff/21523/22517
> File src/com/google/caja/parser/quasiliteral/opt/ArrayIndexOptimization.java
> (right):
> 
> http://codereview.appspot.com/8703/diff/21523/22517#newcode50
> Line 50: *   are visible properties of {@code Object}.
> Should add link to bug 1093 together with caveat that this optimization
> therefore must not be used when the target includes such browsers.
Practically,
> this means we can't make this optimization the default until FF 3.5 falls off
> the Yahoo A-Grade list, which will be a while. But we can still make it a
> non-default option so that we can play with it in the meantime.
> 
> http://codereview.appspot.com/8703/diff/21523/22529
> File src/com/google/caja/plugin/PluginCompiler.java (right):
> 
> http://codereview.appspot.com/8703/diff/21523/22529#newcode134
> Line 134: stages.add(new OptimizeJavascriptStage());
> Must not make this the default until FF 3.5 falls off Yahoo A-Grade list.
(Yes,
> I know this CL was submitted before issue 1093.)
> 
> http://codereview.appspot.com/8703/diff/21523/22530
> File src/com/google/caja/plugin/PluginCompilerMain.java (right):
> 
> http://codereview.appspot.com/8703/diff/21523/22530#newcode215
> Line 215: } finally {
> Does reversing the order in which the catch block and the finally block runs
> matter? If not, I agree that the new way is better style.

Please close this review.

[metaweta, 2012-02-28 20:37:26]

On 2011/12/16 17:40:53, metaweta wrote:
> Please close this review.

Ping.

[metaweta, 2012-03-27 00:04:37]

On 2012/02/28 20:37:26, metaweta wrote:
> On 2011/12/16 17:40:53, metaweta wrote:
> > Please close this review.
> 
> Ping.

ping

[metaweta, 2012-09-06 17:59:06]

On 2012/03/27 00:04:37, metaweta wrote:
> On 2012/02/28 20:37:26, metaweta wrote:
> > On 2011/12/16 17:40:53, metaweta wrote:
> > > Please close this review.
> > 
> > Ping.
> 
> ping

ping