On Fri, Feb 19, 2010 at 3:26 PM, <[email protected]> wrote: > > 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 { > On 2010/02/19 19:55:06, henry.saputra wrote: > >> I think this is fine since John is using Visitor pattern to manipulate >> > the DOM > >> elements. Maybe changing it to NodeVisitor would be better just in >> > case we need > >> other kind of Visitor. >> > > On 2010/02/18 19:23:44, zhoresh wrote: >> > This is the actual rewriter. Maybe a better name: DomNodeRewriter >> > > > > Fine by me. > > > 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(); > On 2010/02/19 19:52:32, johnfargo wrote: > >> 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. >> > >> > > I guess my question here is why revisit is done on list of nodes and not > just one like visit? > I am not sure I see the advantage, and maybe do revisit on a node might > simplify things.
The follow-up CLs would make this a lot more clear :) To give an example, consider the concatenation operation. In this paradigm, it's implemented by visit(...) reserving all nodes whose previous or next sibling is a same-typed external reference. When revisit(...)ing, it's the job of the Visitor to take *all* the reserved nodes at once and push them together. If revisit(...) simply used a single node (which was the original impl), then the Visitor would have to remember all the nodes it reserved and only perform a modification action when it gets to the last one, and at that it would have to maintain its own list of reserved nodes. Changing to List<Node>, though somewhat odd, easily fixed that. > > > http://codereview.appspot.com/214044/show >
