LGTM
Thx!
On 2010/12/15 13:20:23, satya3656 wrote:
http://codereview.appspot.com/3480041/diff/33001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java
(right):
http://codereview.appspot.com/3480041/diff/33001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java#newcode37
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:37:
import java.util.*;
On 2010/12/15 02:51:11, johnfargo wrote:
> no wildcards pls
Done.
http://codereview.appspot.com/3480041/diff/33001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java#newcode66
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:66:
if ((!imageElement.getAttribute("height").isEmpty() &&
On 2010/12/15 02:51:11, johnfargo wrote:
> split getAttribute(key).isEmpty() into helper method for readability
Done.
http://codereview.appspot.com/3480041/diff/33001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java#newcode68
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:68:
imageElement.getAttribute("id").isEmpty() &&
On 2010/12/15 02:51:11, johnfargo wrote:
> why is the emptiness of ID important
styles from external css can used 'id' to specify height/width and
since they
have more precedence to HTML height/width attributes, check for no
'id'
attribute is necessary.
http://codereview.appspot.com/3480041/diff/33001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java#newcode70
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:70:
(!imageElement.getAttribute("style").isEmpty())) {
On 2010/12/15 02:51:11, johnfargo wrote:
> So the rewriter accepts tags w/ style *and* class nodes? No CSS
complications
> there?
inline style tags have higher precedence over styles specified using
class
attributes. so it is fine.
http://codereview.appspot.com/3480041/diff/33001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java#newcode78
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:78:
proxied = proxyUriManager.process(uri);
On 2010/12/15 02:51:11, johnfargo wrote:
> You could just choose to mutate the URL right here and return
> VisitStatus.MODIFY. revisit(...) is useful really only when you want
(or need)
> batched operations performed on multiple nodes at a time.
Done.
http://codereview.appspot.com/3480041/diff/33001/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java#newcode145
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:145:
builder.putQueryParameter(UriCommon.Param.RESIZE_WIDTH.getKey(),
width);
Done.
FYI, plugging it in the proxy visitor is not good, as pages might
break with
this change as image styles can be specified in lot of different ways,
though is
a very rare practice. Having is as a optional rewriter can give the
freedom to
turn this feature on/off more easily.
http://codereview.appspot.com/3480041/
