Inline... On Tue, Feb 24, 2009 at 8:30 PM, <[email protected]> wrote:
> > http://codereview.appspot.com/14117/diff/4001/3016 > File > > java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/AbstractTagHandler.java > (right): > > http://codereview.appspot.com/14117/diff/4001/3016#newcode35 > Line 35: public AbstractTagHandler(String namespaceUri, String tagName) > { > don't think these need to be properties - just make > getTagName()/getNamespaceUri() abstract. (Doesn't make much of a > difference as long as these are reused instances, but I find it a bit > cleaner) > These are "abstract" in the TagHandler intefrace, but the base class provides an easy way of specifying them. The alternative would be to write these methods as boilerplate in the concrete TagHandler implementations. > > http://codereview.appspot.com/14117/diff/4001/3016#newcode48 > Line 48: protected <T extends Object> T getValueFromTag(Element tag, > String name, > Just <T>: <T extends Object> is redundant. > Fixed (here and in DefaultTemplateProcessor) > > http://codereview.appspot.com/14117/diff/4001/3011 > File > > java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/Evaluator.java > (right): > > http://codereview.appspot.com/14117/diff/4001/3011#newcode24 > Line 24: public interface Evaluator { > for implementing tags that have contents, this interface will need to > handle more than just expression evaluation, so it'll soon need to have > a different name and some more methods (for re-entering the processor) > Removed this. Refactored TemplateProcessor into an interface with evaluate() and processTemplate() - implemented by DefaultTemplateProcessor > > http://codereview.appspot.com/14117/diff/4001/3017 > File > > java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/HtmlTagHandler.java > (right): > > http://codereview.appspot.com/14117/diff/4001/3017#newcode55 > Line 55: } catch (Exception e) { > should catch the exact exception types, not Exception (otherwise, > RuntimeExceptions get caught inappropriately) > Fixed > > http://codereview.appspot.com/14117/diff/4001/3017#newcode57 > Line 57: "Error: " + e.getMessage())); > need to be very careful here - someone could include content > "<script>alert('foo')</script>" - and if that shows up in the exception > message, then that script will get executed. > Error message is now run through NekoSerializer.printEscapedText() > > http://codereview.appspot.com/14117/diff/4001/3012 > File > > java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/TagRegistry.java > (right): > > http://codereview.appspot.com/14117/diff/4001/3012#newcode78 > Line 78: hash = (namespaceUri.hashCode() * 37) ^ localName.hashCode(); > compute aggressively in the constructor (and store as an int, not an > Integer) > Fixed > > http://codereview.appspot.com/14117/diff/4001/3010 > File > > java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/TemplateModule.java > (right): > > http://codereview.appspot.com/14117/diff/4001/3010#newcode49 > Line 49: handlers.add(nameHandler); > handlers = ImmutableSet.of(htmlHandler, nameHandler); > > Done. > http://codereview.appspot.com/14117 >
