HTML compaction

1. Removing HTML comments
2. Collapsing whitespace characters

[louiscryan, 2009-03-31 17:02:15]

Looks pretty close.

http://codereview.appspot.com/27104/diff/1/10
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/nekohtml/NekoCompactSerializer.java
(right):

http://codereview.appspot.com/27104/diff/1/10#newcode1
Line 1: // Copyright 2009 Google Inc. All Rights Reserved.
Make sure to use the apache license header

http://codereview.appspot.com/27104/diff/1/10#newcode32
Line 32: Matcher matcher = WHITESPACES.matcher(n.getTextContent());
regexs are not particularly efficient in Java and youre really only stripping
out a very limited set of characters, better to implement as a function.

See org.apache.commons.lang.util.StringUtils and the methods
stripStart, stripEnd, isWhitespace

If string is entirely whitepace you should probably completely suppress it with
the possible exception of a single newline for readability if the original
string contained one.

http://codereview.appspot.com/27104/diff/1/10#newcode52
Line 52: return comment.startsWith("[if ") && comment.endsWith("<![endif]");
does it strictly start with [if or can there be leading space? might be simpler
to just do a containment check.

http://codereview.appspot.com/27104/diff/1/10#newcode56
Line 56: return SPECIAL_TAGS.contains(node.getNodeName());
 node names are not guaranteed to be lowercase, use
node.getNodeName().toLowerCase()
or
Sets.newTreeSet(String.CASE_INSENSITIVE_ORDER)

http://codereview.appspot.com/27104/diff/1/3
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/parse/nekohtml/AbstractParserAndSerializerTest.java
(right):

http://codereview.appspot.com/27104/diff/1/3#newcode1
Line 1: // Copyright 2009 Google Inc. All Rights Reserved.
Apache license

http://codereview.appspot.com/27104/diff/1/3#newcode17
Line 17: * @author dolphin.wan@gmail.com (Chi-Ngai Wan)
Shindig code doesnt track authorship

http://codereview.appspot.com/27104/diff/1/3#newcode22
Line 22: private static final String EOL = System.getProperty( "line.separator"
);
no need for space around param

http://codereview.appspot.com/27104/diff/1/4
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/parse/nekohtml/NekoCompactSerializerTest.java
(right):

http://codereview.appspot.com/27104/diff/1/4#newcode1
Line 1: // Copyright 2009 Google Inc. All Rights Reserved.
apache license

http://codereview.appspot.com/27104/diff/1/4#newcode11
Line 11: * @author dolphin.wan@gmail.com (Chi-Ngai Wan)
no authorship

[cnwan, 2009-04-10 06:18:57]

CL 27104 + 27100 + 28078

[cnwan, 2009-04-10 06:22:03]

Sorry. I've been diverted to other things recently.

Here is my latest CL with fixes suggested by Louis Ryan.
I also merged all my unsubmitted CLs(27100, 28078) into this one as they are
interrelated.

http://codereview.appspot.com/27104/diff/1/10
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/nekohtml/NekoCompactSerializer.java
(right):

http://codereview.appspot.com/27104/diff/1/10#newcode1
Line 1: // Copyright 2009 Google Inc. All Rights Reserved.
On 2009/03/31 17:02:15, louiscryan wrote:
> Make sure to use the apache license header

Oops. Done.

http://codereview.appspot.com/27104/diff/1/10#newcode32
Line 32: Matcher matcher = WHITESPACES.matcher(n.getTextContent());
On 2009/03/31 17:02:15, louiscryan wrote:
> regexs are not particularly efficient in Java and youre really only stripping
> out a very limited set of characters, better to implement as a function.
> 
> See org.apache.commons.lang.util.StringUtils and the methods
> stripStart, stripEnd, isWhitespace
> 
> If string is entirely whitepace you should probably completely suppress it
with
> the possible exception of a single newline for readability if the original
> string contained one.

Done.

I avoid to use Character.isWhitespace and StringUtil.isWhitespace, as it is
different from HTML 4.1 white space definition (See my new javadoc). String
entirely composed of whitespace are now collapsed completely. I don't make the
exception for single newline since the html is compacted for the reason of
smaller size. If I shoot for readability, I may not enable compaction at all.

http://codereview.appspot.com/27104/diff/1/10#newcode52
Line 52: return comment.startsWith("[if ") && comment.endsWith("<![endif]");
On 2009/03/31 17:02:15, louiscryan wrote:
> does it strictly start with [if or can there be leading space? might be
simpler
> to just do a containment check.

Done. replace with String.contains().

http://codereview.appspot.com/27104/diff/1/10#newcode56
Line 56: return SPECIAL_TAGS.contains(node.getNodeName());
On 2009/03/31 17:02:15, louiscryan wrote:
>  node names are not guaranteed to be lowercase, use
> node.getNodeName().toLowerCase()
> or
> Sets.newTreeSet(String.CASE_INSENSITIVE_ORDER)

Good suggestions! changed and added corresponding tests

http://codereview.appspot.com/27104/diff/1/3
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/parse/nekohtml/AbstractParserAndSerializerTest.java
(right):

http://codereview.appspot.com/27104/diff/1/3#newcode1
Line 1: // Copyright 2009 Google Inc. All Rights Reserved.
On 2009/03/31 17:02:15, louiscryan wrote:
> Apache license

Done.

http://codereview.appspot.com/27104/diff/1/3#newcode17
Line 17: * @author dolphin.wan@gmail.com (Chi-Ngai Wan)
On 2009/03/31 17:02:15, louiscryan wrote:
> Shindig code doesnt track authorship

Done.

http://codereview.appspot.com/27104/diff/1/3#newcode22
Line 22: private static final String EOL = System.getProperty( "line.separator"
);
On 2009/03/31 17:02:15, louiscryan wrote:
> no need for space around param

Done.

http://codereview.appspot.com/27104/diff/1/4
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/parse/nekohtml/NekoCompactSerializerTest.java
(right):

http://codereview.appspot.com/27104/diff/1/4#newcode1
Line 1: // Copyright 2009 Google Inc. All Rights Reserved.
On 2009/03/31 17:02:15, louiscryan wrote:
> apache license

Done.

http://codereview.appspot.com/27104/diff/1/4#newcode11
Line 11: * @author dolphin.wan@gmail.com (Chi-Ngai Wan)
On 2009/03/31 17:02:15, louiscryan wrote:
> no authorship

Done.

[louiscryan, 2009-04-17 22:52:19]

Sorry to take so long to review. I was OOO for 2 weeks. This looks good. I'm
happy to commit as is

http://codereview.appspot.com/27104/diff/4001/4015
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/nekohtml/NekoCompactSerializer.java
(right):

http://codereview.appspot.com/27104/diff/4001/4015#newcode90
Line 90: str = StringUtils.stripStart(str, HTML_WHITESPACE);
you can probably just drop these two calls given how your loop behaves

[cnwan, 2009-04-22 08:11:54]

http://codereview.appspot.com/27104/diff/4001/4015
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/nekohtml/NekoCompactSerializer.java
(right):

http://codereview.appspot.com/27104/diff/4001/4015#newcode90
Line 90: str = StringUtils.stripStart(str, HTML_WHITESPACE);
On 2009/04/17 22:52:20, louiscryan wrote:
> you can probably just drop these two calls given how your loop behaves

stripEnd can be dropped while stripStart cannot, caught by my tests.

[cnwan, 2009-04-22 08:29:04]

Resynced to head