Run VanillCajaHtmlSerializer with ASCII only mode off and don't remove comments

This change runs VanillCajaHtmlSerializer with ASCII only mode off. Previously all non-ASCII characters were being escaped, leading to a bloat up of the serialized page.

Also call parseUnsafe instead of parse in order to not remove comments while serializing

[nikhilmadan23, 2011-04-29 10:03:28]

Validation seems fine on the 15 blogs.

@Atul and Gagan: Could you please take a look at this?

- Nikhil

On Fri, Apr 29, 2011 at 1:14 PM,  <nikhilmadan23@gmail.com> wrote:
> Reviewers: anupama.dutta, gagan.goku,
>
> Description:
> This change runs VanillCajaHtmlSerializer with ASCII only mode off.
> Previously all non-ASCII characters were being escaped, leading to a
> bloat up of the serialized page.
>
> Also call parseUnsafe instead of parse in order to not remove comments
> while serializing
>
> Please review this at http://codereview.appspot.com/4423077/
>
> Affected files:
>
 java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/VanillaCajaHtmlSerializer.java
>
>
> Index:
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/VanillaCajaHtmlSerializer.java
> ===================================================================
> ---
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/VanillaCajaHtmlSerializer.java
>     (revision 1097308)
> +++
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/VanillaCajaHtmlSerializer.java
>     (working copy)
> @@ -38,8 +38,13 @@
>       if (doc.getDoctype() != null) {
>         HtmlSerialization.outputDocType(doc.getDoctype(), sw);
>       }
> -      sw.append(Nodes.render(doc,
> -              new RenderContext(new Concatenator(sw, null)).asXml() ?
> MarkupRenderMode.XML : MarkupRenderMode.HTML));
> +      RenderContext renderContext =
> +          new RenderContext(new Concatenator(sw, null))
> +              // More compact but needs charset set correctly
> +              .withAsciiOnly(false)
> +              .withMarkupRenderMode(MarkupRenderMode.HTML);
> +
> +      Nodes.renderUnsafe(doc, renderContext);
>       return sw.toString();
>     } catch (IOException e) {
>       return null;
>
>
>

[anupama.dutta, 2011-04-29 12:04:23]

LGTM. Let us add a test for this class in another patch.

[atulvasu, 2011-04-29 16:26:52]

Ship It! lgtm

[gagan.goku, 2011-04-30 02:08:18]

Please add a test case since this seems like a big change.

http://codereview.appspot.com/4423077/diff/2001/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/VanillaCajaHtmlSerializer.java
(right):

http://codereview.appspot.com/4423077/diff/2001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/VanillaCajaHtmlSerializer.java:45:
.withMarkupRenderMode(MarkupRenderMode.HTML);
u sure u dont need HTML4_BACKWARDS_COMPAT ?

http://codereview.appspot.com/4423077/diff/2001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/VanillaCajaHtmlSerializer.java:48:
Nodes.renderUnsafe(doc, renderContext);
u shud not be calling a deprecated method.
Call Nodes.render(doc, Namespaces.HTML_DEFAULT, renderContext, true)
if that is not public, make it public in caja

[nikhilmadan23, 2011-04-30 17:05:52]

Tests added

http://codereview.appspot.com/4423077/diff/2001/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/VanillaCajaHtmlSerializer.java
(right):

http://codereview.appspot.com/4423077/diff/2001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/VanillaCajaHtmlSerializer.java:45:
.withMarkupRenderMode(MarkupRenderMode.HTML);
On 2011/04/30 02:08:18, gagan.goku wrote:
> u sure u dont need HTML4_BACKWARDS_COMPAT ?

I think we should stick with the previous mode for now, which was
MarkupRenderMode.HTML, until we find any problems with it. Don't want to make
too many changes together.

http://codereview.appspot.com/4423077/diff/2001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/VanillaCajaHtmlSerializer.java:48:
Nodes.renderUnsafe(doc, renderContext);
On 2011/04/30 02:08:18, gagan.goku wrote:
> u shud not be calling a deprecated method.
> Call Nodes.render(doc, Namespaces.HTML_DEFAULT, renderContext, true)
> if that is not public, make it public in caja

As discussed, this function was added in caja and marked as deprecated because
its not totally safe, in the sense that IE comments are executable, and they
don't encourage its use for cajoled code.
I've added a todo to talk to move to a non-deprecated function in caja. If it
fine with you, we would be grateful if we could get this in for now, while we
carry on with the caja change in parallel.

[gagan.goku, 2011-05-01 06:41:07]

looks okay. Please address the small comment, will commit once ur done.

http://codereview.appspot.com/4423077/diff/8001/java/gadgets/src/test/java/or...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/parse/caja/VanillaCajaHtmlSerializerTest.java
(right):

http://codereview.appspot.com/4423077/diff/8001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/parse/caja/VanillaCajaHtmlSerializerTest.java:58:
+ "</div></body></html>";
can you add a comment here as to what happens if wantsComments = false ?

[nikhilmadan23, 2011-05-01 07:10:27]

Thanks!

http://codereview.appspot.com/4423077/diff/8001/java/gadgets/src/test/java/or...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/parse/caja/VanillaCajaHtmlSerializerTest.java
(right):

http://codereview.appspot.com/4423077/diff/8001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/parse/caja/VanillaCajaHtmlSerializerTest.java:58:
+ "</div></body></html>";
On 2011/05/01 06:41:07, gagan.goku wrote:
> can you add a comment here as to what happens if wantsComments = false ?

Done.

[gagan.goku, 2011-05-01 10:41:12]

Build looks good.
Committed as r1098248.