Upgrade to Neko 1.9.13

[awiner, 2009-09-18 21:54:40]

http://codereview.appspot.com/120050/diff/1/27
File
../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/DefaultHtmlSerializer.java
(right):

http://codereview.appspot.com/120050/diff/1/27#newcode105
Line 105: private Element replaceElement(Element elem) {
Some Javadoc (e.g., explaining why this exists).
I'd call it "substituteElement", perhaps.

http://codereview.appspot.com/120050/diff/1/27#newcode110
Line 110: replacement.setAttribute("type", scriptType);
it looks as though the parsing code doesn't remove the "type" attribute.

http://codereview.appspot.com/120050/diff/1/7
File
../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/GadgetHtmlParser.java
(right):

http://codereview.appspot.com/120050/diff/1/7#newcode54
Line 54: // Bi-map of OpenSocial tags to their script type attribute values.
Should be Javadoc, not //

http://codereview.appspot.com/120050/diff/1/18
File
../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/nekohtml/NekoSimplifiedHtmlParser.java
(right):

http://codereview.appspot.com/120050/diff/1/18#newcode68
Line 68: public NekoSimplifiedHtmlParser(DOMImplementation documentFactory) {
indent here through line 166 looks off by two

http://codereview.appspot.com/120050/diff/1/18#newcode113
Line 113: HTMLScanner htmlScanner = new HTMLScanner();
some comments showing the order of filters would be helpful

http://codereview.appspot.com/120050/diff/1/18#newcode119
Line 119: if (config.getFeature("http://xml.org/sax/features/namespaces")) {
This feature should always be on, so you could lose the else block.

http://codereview.appspot.com/120050/diff/1/18#newcode164
Line 164: protected boolean isElementImportant(QName qName) {
method could be killed, ditto all references to the "false" cases (e.g.
startUnimportantElement())

http://codereview.appspot.com/120050/diff/1/18#newcode173
Line 173: protected class OSMLDocumentFilter extends DefaultFilter {
static?

http://codereview.appspot.com/120050/diff/1/18#newcode490
Line 490: protected final void appendChild(Node node) {
Could be private now - was only exposed for SocialMarkupHtmlParser

http://codereview.appspot.com/120050/diff/1/31
File
../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/TemplateRewriter.java
(right):

http://codereview.appspot.com/120050/diff/1/31#newcode152
Line 152: DomUtil.getElementsByTagNameCaseInsensitive(content.getDocument(),
why not document.getElementsByTagName, as in the pipeline code?

http://codereview.appspot.com/120050/diff/1/11
File
../trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/parse/nekohtml/NekoSimplifiedHtmlParserTest.java
(right):

http://codereview.appspot.com/120050/diff/1/11#newcode37
Line 37: * Test for the social markup parser.
comment a bit out-of-date

[louiscryan, 2009-09-19 00:05:01]

http://codereview.appspot.com/120050/diff/1/27
File
../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/DefaultHtmlSerializer.java
(right):

http://codereview.appspot.com/120050/diff/1/27#newcode105
Line 105: private Element replaceElement(Element elem) {
On 2009/09/18 21:54:40, awiner wrote:
> Some Javadoc (e.g., explaining why this exists).
> I'd call it "substituteElement", perhaps.

Done.

http://codereview.appspot.com/120050/diff/1/27#newcode110
Line 110: replacement.setAttribute("type", scriptType);
On 2009/09/18 21:54:40, awiner wrote:
> it looks as though the parsing code doesn't remove the "type" attribute.

Fixed.

http://codereview.appspot.com/120050/diff/1/7
File
../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/GadgetHtmlParser.java
(right):

http://codereview.appspot.com/120050/diff/1/7#newcode54
Line 54: // Bi-map of OpenSocial tags to their script type attribute values.
On 2009/09/18 21:54:40, awiner wrote:
> Should be Javadoc, not //

Done.

http://codereview.appspot.com/120050/diff/1/18
File
../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/nekohtml/NekoSimplifiedHtmlParser.java
(right):

http://codereview.appspot.com/120050/diff/1/18#newcode68
Line 68: public NekoSimplifiedHtmlParser(DOMImplementation documentFactory) {
On 2009/09/18 21:54:40, awiner wrote:
> indent here through line 166 looks off by two

Done.

http://codereview.appspot.com/120050/diff/1/18#newcode119
Line 119: if (config.getFeature("http://xml.org/sax/features/namespaces")) {
On 2009/09/18 21:54:40, awiner wrote:
> This feature should always be on, so you could lose the else block.

Done.

http://codereview.appspot.com/120050/diff/1/18#newcode164
Line 164: protected boolean isElementImportant(QName qName) {
On 2009/09/18 21:54:40, awiner wrote:
> method could be killed, ditto all references to the "false" cases (e.g.
> startUnimportantElement())

Done and done

http://codereview.appspot.com/120050/diff/1/18#newcode173
Line 173: protected class OSMLDocumentFilter extends DefaultFilter {
On 2009/09/18 21:54:40, awiner wrote:
> static?

Done.

http://codereview.appspot.com/120050/diff/1/18#newcode490
Line 490: protected final void appendChild(Node node) {
On 2009/09/18 21:54:40, awiner wrote:
> Could be private now - was only exposed for SocialMarkupHtmlParser

Done.

http://codereview.appspot.com/120050/diff/1/31
File
../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/TemplateRewriter.java
(right):

http://codereview.appspot.com/120050/diff/1/31#newcode152
Line 152: DomUtil.getElementsByTagNameCaseInsensitive(content.getDocument(),
On 2009/09/18 21:54:40, awiner wrote:
> why not document.getElementsByTagName, as in the pipeline code?

Done.

http://codereview.appspot.com/120050/diff/1/11
File
../trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/parse/nekohtml/NekoSimplifiedHtmlParserTest.java
(right):

http://codereview.appspot.com/120050/diff/1/11#newcode37
Line 37: * Test for the social markup parser.
On 2009/09/18 21:54:40, awiner wrote:
> comment a bit out-of-date

Done.

[louiscryan, 2009-09-19 00:06:14]

Update

[louiscryan, 2009-09-29 19:20:24]

A more thorough patch

[awiner, 2009-09-29 21:23:03]

http://codereview.appspot.com/120050/diff/2005/2024
File
../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/nekohtml/NekoSimplifiedHtmlParser.java
(right):

http://codereview.appspot.com/120050/diff/2005/2024#newcode409
Line 409: * tag. This doesnt quite work together as without this fix it will
still introduce head tags
would be good to refer to a NekoHtml bug number, so we know when we can kill
this workaround

[louiscryan, 2009-09-29 22:23:34]

http://codereview.appspot.com/120050/diff/2005/2024
File
../trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/nekohtml/NekoSimplifiedHtmlParser.java
(right):

http://codereview.appspot.com/120050/diff/2005/2024#newcode409
Line 409: * tag. This doesnt quite work together as without this fix it will
still introduce head tags
On 2009/09/29 21:23:03, awiner wrote:
> would be good to refer to a NekoHtml bug number, so we know when we can kill
> this workaround

Done.