Thanks for the review! Comments provided here. Patch coming in a moment.
http://codereview.appspot.com/214044/diff/1/3
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java
(right):
http://codereview.appspot.com/214044/diff/1/3#newcode53
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java:53:
public interface Visitor {
My main goal is to distinguish between the top-level Rewriters we have
(Gadget, Request, Content [Gadget+Request], Image) and these
sub-components. I chose Visitor since this pattern is pretty typical for
DOM operations and Visitor is typical the name used. Any names you like
that don't include "Rewriter"?
http://codereview.appspot.com/214044/diff/1/3#newcode65
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java:65:
* modify unreserved nodes. Other append and delete operations are
acceptable
On 2010/02/18 19:23:44, zhoresh wrote:
I am not sure I understand "do not modify unreserved node", is that
imply MODIFY
will be returned only by revisit?
No, it just means that revisit(...) only modifies nodes that are
reserved/revisited. In other words, nothing like this:
reservedNode.getNextSibling().getNextSibling().appendChild(...);
Added some commentary to this effect.
We might be also limiting ourself with the limitation of delete/move
of one
visitor and only in revisit.
How would you implement optimization of joining multiple script
blocks, and then
run them through closer compiler?
Separate rewriters. One joins, and the next compiles. They wouldn't be
part of a single DomWalker.Rewriter - this framework isn't meant to be
comprehensive or to replace the entire Rewriter mechanism, just to
supply some helpers for common operations.
http://codereview.appspot.com/214044/diff/1/3#newcode103
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java:103:
public static class Rewriter implements GadgetRewriter, RequestRewriter
{
Kept Rewriter as a subclass for now since DomWalker is intended as a
namespace (and renaming would lose comments for the benefit of a "."
while adding verbosity to Visitor decls).
http://codereview.appspot.com/214044/diff/1/3#newcode120
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java:120:
return ImmutableList.<Visitor>of();
Good idea, done.
http://codereview.appspot.com/214044/diff/1/3#newcode128
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java:128:
DomWalker.rewrite(visitors != null ? visitors :
On 2010/02/18 19:23:44, zhoresh wrote:
Just call makeVisitors
Done.
http://codereview.appspot.com/214044/diff/1/3#newcode136
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java:136:
return DomWalker.rewrite(visitors != null ? visitors :
On 2010/02/18 19:23:44, zhoresh wrote:
Same as above
Done.
http://codereview.appspot.com/214044/diff/1/3#newcode143
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java:143:
public static boolean rewrite(List<Visitor> visitors, Gadget gadget,
MutableContent content)
I was playing around with class structure for a while and made it static
to allow others to call directly. With the above changes to Rewriter,
now made private and non-static.
http://codereview.appspot.com/214044/diff/1/3#newcode145
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java:145:
Map<Visitor, List<Node>> reservations = Maps.newHashMap();
Hm, interesting. Doing that would ensure that revisiting order is the
same as the first-reserved order, which seems a little arbitrary to me.
I've changed the code to revisit in visitors-order, which seems more
useful. Thoughts?
On 2010/02/18 19:23:44, zhoresh wrote:
Change to List< Pair<Node, Visitor>>
this way you promise revisiting order to be same as scan order.
http://codereview.appspot.com/214044/diff/1/3#newcode156
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java:156:
for (int i = 0; !nodeReserved && !treeReserved && i < visitors.size();
++i) {
With slight modification, done.
On 2010/02/18 19:23:44, zhoresh wrote:
visitors is a list, so use foreach loop:
for (Visitor visitor : visitors) {
...
if (nodeReserved || treeReserved) break;
}
http://codereview.appspot.com/214044/diff/1/3#newcode187
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java:187:
toVisit.add(child);
DFS sounds good, but Stack is generally frowned-upon since it extends
the similarly generally-frowned-upon Vector, for which all access is
synchronized.
Ideally we'd use Deque, but we're still in Java 1.5-land. I've changed
to a direct LinkedList using addFirst and removeFirst methods.
http://codereview.appspot.com/214044/show
