ImagePrefetchRewriter that prefetches images

This rewriter prefetches images referenced by <img> tags on the page being proxied / accelerated.

Prefetching images can decrease the page load time of pages as there are lesser resources that get blocked on download while javascript gets downloaded / executed. Also, some of time taken to parse the html document is avoided.

[gagan.goku, 2010-08-22 11:40:51]

Please take a look.

[satya3656, 2010-08-27 07:29:32]

http://codereview.appspot.com/2003041/diff/5001/6002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImagePreloadVisitor.java
(right):

http://codereview.appspot.com/2003041/diff/5001/6002#newcode60
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImagePreloadVisitor.java:60:
StringBuilder content = new StringBuilder();
Add scoping to the snippet.

http://codereview.appspot.com/2003041/diff/5001/6002#newcode64
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImagePreloadVisitor.java:64:
content.append("var ");
Won't 
content.append("var " + objName + " = new Image();")
content.append(objName + ".src = '" + src + "';")

Be more readable?

http://codereview.appspot.com/2003041/diff/5001/6002#newcode71
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImagePreloadVisitor.java:71:
content.append("';\n");
\n is not needed any where in the snippet. It just adds extra bytes. 
Same holds for the white space you also, you can remove them.

http://codereview.appspot.com/2003041/diff/5001/6002#newcode76
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImagePreloadVisitor.java:76:
content.append("var scripts = document.getElementsByTagName(\"script\");\n");
This will break, what if there is a script in the body of the document, you will
be removing that script tag instead of this one.

http://codereview.appspot.com/2003041/diff/5001/6002#newcode76
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImagePreloadVisitor.java:76:
content.append("var scripts = document.getElementsByTagName(\"script\");\n");
Is there any specific reason for removing it, other than just cleanup dom as it
is not needed any more.

http://codereview.appspot.com/2003041/diff/5001/6001
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImagePreloadRewriterTest.java
(right):

http://codereview.appspot.com/2003041/diff/5001/6001#newcode55
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImagePreloadRewriterTest.java:55:
+ "<a href=\"hello\">Hello</a>"
Add "<img src=\"\"></img>"

[gagan.goku, 2010-08-27 12:04:06]

http://codereview.appspot.com/2003041/diff/5001/6002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImagePreloadVisitor.java
(right):

http://codereview.appspot.com/2003041/diff/5001/6002#newcode60
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImagePreloadVisitor.java:60:
StringBuilder content = new StringBuilder();
On 2010/08/27 07:29:32, satya3656 wrote:
> Add scoping to the snippet.

Done.

http://codereview.appspot.com/2003041/diff/5001/6002#newcode64
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImagePreloadVisitor.java:64:
content.append("var ");
On 2010/08/27 07:29:32, satya3656 wrote:
> Won't 
> content.append("var " + objName + " = new Image();")
> content.append(objName + ".src = '" + src + "';")
> 
> Be more readable?

Yep, its much more readable. Stupid Intellij keeps telling me that
StringBuffer.append is more efficient though, so i got carried away. This is
better though.

http://codereview.appspot.com/2003041/diff/5001/6002#newcode71
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImagePreloadVisitor.java:71:
content.append("';\n");
On 2010/08/27 07:29:32, satya3656 wrote:
> \n is not needed any where in the snippet. It just adds extra bytes. 
> Same holds for the white space you also, you can remove them. 

I think we should retain whitespace and newlines. They make the js snippet more
readable and easier to debug.
Plus, i dont think we should be manually removing newlines and extra spaces.
These things are better left out for automated tools like simple first pass of a
javascript compiler.

http://codereview.appspot.com/2003041/diff/5001/6002#newcode76
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImagePreloadVisitor.java:76:
content.append("var scripts = document.getElementsByTagName(\"script\");\n");
On 2010/08/27 07:29:32, satya3656 wrote:
> This will break, what if there is a script in the body of the document, you
will
> be removing that script tag instead of this one.

What do you mean ? I am putting all of this javascript inside a script element.
When that script executes, it will be the last script element the browser has
parsed. So it will remove itself.

http://codereview.appspot.com/2003041/diff/5001/6002#newcode76
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImagePreloadVisitor.java:76:
content.append("var scripts = document.getElementsByTagName(\"script\");\n");
On 2010/08/27 07:29:32, satya3656 wrote:
> Is there any specific reason for removing it, other than just cleanup dom as
it
> is not needed any more.

Yes, adding more scripts skews up the indices of script elements compared to the
original page.
So if a javascript after this script relies on the 2nd javascript, it will not
refer to the correct script anymore.

[satya3656, 2010-08-27 12:36:35]

LGTM

small thing, you missed one minor comment in the test

http://codereview.appspot.com/2003041/diff/5001/6002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImagePreloadVisitor.java
(right):

http://codereview.appspot.com/2003041/diff/5001/6002#newcode76
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImagePreloadVisitor.java:76:
content.append("var scripts = document.getElementsByTagName(\"script\");\n");
On 2010/08/27 12:04:06, gagan.goku wrote:
> On 2010/08/27 07:29:32, satya3656 wrote:
> > This will break, what if there is a script in the body of the document, you
> will
> > be removing that script tag instead of this one.
> 
> What do you mean ? I am putting all of this javascript inside a script
element.
> When that script executes, it will be the last script element the browser has
> parsed. So it will remove itself.
> 

Sry, got confused, you are right.

[gagan.goku, 2010-08-27 12:49:55]

http://codereview.appspot.com/2003041/diff/5001/6002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImagePreloadVisitor.java
(right):

http://codereview.appspot.com/2003041/diff/5001/6002#newcode76
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImagePreloadVisitor.java:76:
content.append("var scripts = document.getElementsByTagName(\"script\");\n");
Added another test for empty src img's as well. PTAL

[satya3656, 2010-08-27 13:41:27]

LGTM

[gagan.goku, 2010-08-27 15:39:33]

Thanks Satya

Gagan

-- 
The only thing missing in life is background music.
-- Gagandeep Singh


On Fri, Aug 27, 2010 at 7:11 PM, <satya3656@gmail.com> wrote:

> LGTM
>
>
> http://codereview.appspot.com/2003041/
>

[Kuntal Loya, 2010-08-30 12:42:49]

Could you mention in the rewriter description the type of gain we are expecting
from it.

[gagan.goku, 2010-08-30 15:25:18]

On 2010/08/30 12:42:49, Kuntal Loya wrote:
> Could you mention in the rewriter description the type of gain we are
expecting
> from it.

Changed the description a bit. Please take a look now. If you had something else
in mind, please also post a sample snippet.

[satya3656, 2010-09-08 10:53:29]

http://codereview.appspot.com/2003041/diff/32001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java
(right):

http://codereview.appspot.com/2003041/diff/32001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:28:
import org.apache.shindig.gadgets.render.*;
Just import what you want, * is bad.