You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by aw...@gmail.com on 2009/02/24 01:38:44 UTC
Custom Tag support in Server-side templates
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
Re: Custom Tag support in Server-side templates
Posted by Lev Epshteyn <le...@google.com>.
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, <aw...@gmail.com> 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
>