http://codereview.appspot.com/14117/diff/1/9 File java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultGuiceModule.java (right):
http://codereview.appspot.com/14117/diff/1/9#newcode139 Line 139: public static class TagRegistryProvider implements Provider<CustomTagRegistry> { if we need any providers just for templating, let's put them in TemplatesModule, not here http://codereview.appspot.com/14117/diff/1/9#newcode146 Line 146: registry.registerHandler(OPENSOCIAL_NAMESPACE, HtmlTagHandler.DEFAULT_TAG_NAME, htmlHandler); Have getNamespace() and getTagName() on a TagHandler API, and instead of binding one-by-one, just have Set<TagHandler> injected into the TagRegistry. Then we just need the provider for Set<TagHandler> http://codereview.appspot.com/14117/diff/1/8 File java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/GadgetHtmlParser.java (right): http://codereview.appspot.com/14117/diff/1/8#newcode98 Line 98: public void parseFragment(String source, DocumentFragment fragment) throws GadgetException { Javadoc. Also "fragment" is a poor name; it identifies the type, but not what it's used for. In this case, don't believe there's any reason to make this a fragment - it can be a Node to which the fragment will be appended. http://codereview.appspot.com/14117/diff/1/8#newcode111 Line 111: fragmentCache.addElement(key, fragment); can't cache the fragment which was passed in; that can be mutated, corrupting the cache http://codereview.appspot.com/14117/diff/1/8#newcode116 Line 116: Document doc = dest.getOwnerDocument(); doc -> destDoc http://codereview.appspot.com/14117/diff/1/7 File java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/nekohtml/NekoHtmlParser.java (right): http://codereview.appspot.com/14117/diff/1/7#newcode64 Line 64: private Document parseFragment(String source) throws SAXException, IOException, GadgetException { this method name is now confusing http://codereview.appspot.com/14117/diff/1/6 File java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/nekohtml/NekoSimplifiedHtmlParser.java (right): http://codereview.appspot.com/14117/diff/1/6#newcode117 Line 117: throw new GadgetException(GadgetException.Code.HTML_PARSE_ERROR, throw new UnsupportedOperationException() more accurately reflects the purpose http://codereview.appspot.com/14117/diff/1/11 File java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/CustomTagHandler.java (right): http://codereview.appspot.com/14117/diff/1/11#newcode27 Line 27: public interface CustomTagHandler { I'd just name it TagHandler http://codereview.appspot.com/14117/diff/1/11#newcode28 Line 28: add getNamespaceUri() and getTagName() http://codereview.appspot.com/14117/diff/1/10 File java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/CustomTagRegistry.java (right): http://codereview.appspot.com/14117/diff/1/10#newcode37 Line 37: public CustomTagRegistry() { recommend injecting the full set of custom tags, and killing registerHandler() http://codereview.appspot.com/14117/diff/1/10#newcode60 Line 60: final String namespaceUri; both should be private http://codereview.appspot.com/14117/diff/1/10#newcode71 Line 71: if (this.getClass() != obj.getClass()) { would write this as: if (this == obj) { return true; } if (!(this instanceof NSName)) { return false; } As written, this code doesn't have the "obj == this" optimization, and will incorrectly throw an NPE if obj == null. http://codereview.appspot.com/14117/diff/1/10#newcode81 Line 81: hash = (namespaceUri + localName).hashCode(); a less expensive approach is just: return (namespaceUri.hashCode() * 37) ^ localName.hashCode() http://codereview.appspot.com/14117/diff/1/15 File java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/HtmlTagHandler.java (right): http://codereview.appspot.com/14117/diff/1/15#newcode28 Line 28: public class HtmlTagHandler extends AbstractTagHandler { Class Javadoc http://codereview.appspot.com/14117/diff/1/15#newcode37 Line 37: private NekoHtmlParser parser; final http://codereview.appspot.com/14117/diff/1/15#newcode51 Line 51: // TODO: What to do here? insert a comment with an error message http://codereview.appspot.com/14117/diff/1/12 File java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/NameTagHandler.java (right): http://codereview.appspot.com/14117/diff/1/12#newcode39 Line 39: String formatted = name.optString("formatted"); "name" could be null; and what if the container doesn't support "formatted"? http://codereview.appspot.com/14117/diff/1/12#newcode41 Line 41: Element root = doc.createElement("b"); TODO: support pluggable markup http://codereview.appspot.com/14117/diff/1/13 File java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/TemplateProcessor.java (right): http://codereview.appspot.com/14117/diff/1/13#newcode279 Line 279: clearSpecialAttributes(element); No, don't - this is the original. If we're in a loop, this removes context for the next pass. http://codereview.appspot.com/14117/diff/1/13#newcode317 Line 317: protected String processString(String expression) { Making this protected isn't enough. TagHandlers need to be definable outside of this package. Also, TagHandlers should be testable without a full TemplateProcessor. So, recommend having a minimal interface that TagHandler uses to call methods on TemplateProcessor; instead of TemplateProcessor implementing it directly, have an inner class that implements it, so TemplateProcessor doesn't have to make any of its API public. http://codereview.appspot.com/14117/diff/1/13#newcode339 Line 339: protected JSONObject processJSON(String expression) { don't think we should have "processXyz()" methods per type. Why not just: T processExpression(String expression, Class<T> type, T defaultValue); http://codereview.appspot.com/14117
