Addressed most feedback and uploaded new patch. One thing I didn't do is change the public name for parseFragment() in GadgetHtmlParser. I believe the name accurately reflects what the method does - parses a fragment of HTML (as opposed to an entire document).
On Mon, Feb 23, 2009 at 7:38 PM, <[email protected]> wrote: > > 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 >
