Custom Tag support in Server-side templates

Added HtmlCustomTag (from Templates spec) and NameCustomTag (from OSML spec)

Test for CustomTagHandler mechanism and for HtmlCustomTag.

Fragment parsing and caching in GadgetHtmlParser/NekoHtmlParser (Simplified Neko parser doesn't yet support fragment parsing)

[awiner, 2009-02-24 00:38:43]

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);

[awiner, 2009-02-25 01:30:17]

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)

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.

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)

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)

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.

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)

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);