Issue 3480041: Adding ImageResizeRewriter that makes server side scaling of images possible. (Closed)
|
Created:
15 years, 1 month ago by satya3656 Modified:
15 years ago Base URL:
http://svn.apache.org/repos/asf/shindig/trunk/ Visibility:
Public. |
DescriptionAdding ImageResizeRewriter that appends image size attributes (extracted from
inline styles, height, width) to the proxied resource urls, so that
server side resizing of these images can be done whenever applicable.
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressing the comments. #
Total comments: 8
Patch Set 3 : Addressing the comments. #
Total comments: 14
Patch Set 4 : Addressing the comments. #
Total comments: 6
Patch Set 5 : Addressing the comments. #
Total comments: 12
Patch Set 6 : Addressing the comments. #
MessagesTotal messages: 17
Some minor comments. http://codereview.appspot.com/3480041/diff/1/java/gadgets/src/main/java/org/a... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java (right): http://codereview.appspot.com/3480041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:51: node.getNodeName().toLowerCase().equals("img")) { You can use equalsIgnoreCase here http://codereview.appspot.com/3480041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:57: if ((!"".equals(imageElement.getAttribute("height")) && You can use IsEmpty() here and elsewhere. http://codereview.appspot.com/3480041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:67: if (nodes.size() == 0) { Use isEmpty()
Sign in to reply to this message.
http://codereview.appspot.com/3480041/diff/1/java/gadgets/src/main/java/org/a... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java (right): http://codereview.appspot.com/3480041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:82: if ("height".equalsIgnoreCase(splits[0].trim())) { what if its height: 100% / 100 em. In that case, its better to send the unit to the server as well so it can resize correctly. http://codereview.appspot.com/3480041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:86: if ("width".equalsIgnoreCase(splits[0].trim())) { mention that inline style trumps everything, and inline height/width attribute trumps height/width inherited from css. http://codereview.appspot.com/3480041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:97: builder.addQueryParameter(UriCommon.Param.RESIZE_HEIGHT.getKey(), height); shouldnt your clear out the existing RESIZE_HEIGHT query parameters if any ?
Sign in to reply to this message.
http://codereview.appspot.com/3480041/diff/1/java/gadgets/src/main/java/org/a... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java (right): http://codereview.appspot.com/3480041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:51: node.getNodeName().toLowerCase().equals("img")) { On 2010/12/09 15:14:15, nikhilmadan23 wrote: > You can use equalsIgnoreCase here Done. http://codereview.appspot.com/3480041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:57: if ((!"".equals(imageElement.getAttribute("height")) && On 2010/12/09 15:14:15, nikhilmadan23 wrote: > You can use IsEmpty() here and elsewhere. Done. http://codereview.appspot.com/3480041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:67: if (nodes.size() == 0) { On 2010/12/09 15:14:15, nikhilmadan23 wrote: > Use isEmpty() Done. http://codereview.appspot.com/3480041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:82: if ("height".equalsIgnoreCase(splits[0].trim())) { On 2010/12/10 05:32:30, gagan.goku wrote: > what if its height: 100% / 100 em. In that case, its better to send the unit to > the server as well so it can resize correctly. It is very difficult to interpret the actual value in case of %, em which depends on other styles/context where the img is placed, so are only considering images with absolute units. http://codereview.appspot.com/3480041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:86: if ("width".equalsIgnoreCase(splits[0].trim())) { On 2010/12/10 05:32:30, gagan.goku wrote: > mention that inline style trumps everything, and inline height/width attribute > trumps height/width inherited from css. Done. http://codereview.appspot.com/3480041/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:97: builder.addQueryParameter(UriCommon.Param.RESIZE_HEIGHT.getKey(), height); On 2010/12/10 05:32:30, gagan.goku wrote: > shouldnt your clear out the existing RESIZE_HEIGHT query parameters if any ? using put instead of add that will take care of this.
Sign in to reply to this message.
http://codereview.appspot.com/3480041/diff/7001/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java (right): http://codereview.appspot.com/3480041/diff/7001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:40: private static final Logger LOG = Logger.getLogger(ImageResizeRewriter.class.getName()); Remove logger. http://codereview.appspot.com/3480041/diff/7001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:62: (!imageElement.getAttribute("style").isEmpty())) { We should add a check to ensure only proxied url nodes get reserved here. http://codereview.appspot.com/3480041/diff/7001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:87: String[] splits = attr.split(":"); Should we check for len(splits) == 2 here? http://codereview.appspot.com/3480041/diff/7001/java/gadgets/src/test/java/or... File java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriterTest.java (right): http://codereview.appspot.com/3480041/diff/7001/java/gadgets/src/test/java/or... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriterTest.java:59: + " style=\"height:50px; width:110px\">" Could you also add an img node which has style="height:50px" and width="60px" (and another node with height and width interchanged in this example)?
Sign in to reply to this message.
Addressing the comments.
Sign in to reply to this message.
http://codereview.appspot.com/3480041/diff/7001/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java (right): http://codereview.appspot.com/3480041/diff/7001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:40: private static final Logger LOG = Logger.getLogger(ImageResizeRewriter.class.getName()); On 2010/12/13 07:23:29, anupama.dutta wrote: > Remove logger. Done. http://codereview.appspot.com/3480041/diff/7001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:62: (!imageElement.getAttribute("style").isEmpty())) { On 2010/12/13 07:23:29, anupama.dutta wrote: > We should add a check to ensure only proxied url nodes get reserved here. Done. http://codereview.appspot.com/3480041/diff/7001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:87: String[] splits = attr.split(":"); On 2010/12/13 07:23:29, anupama.dutta wrote: > Should we check for len(splits) == 2 here? Done. http://codereview.appspot.com/3480041/diff/7001/java/gadgets/src/test/java/or... File java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriterTest.java (right): http://codereview.appspot.com/3480041/diff/7001/java/gadgets/src/test/java/or... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriterTest.java:59: + " style=\"height:50px; width:110px\">" Added FYI My rewriter ignores rewriting for these cases
Sign in to reply to this message.
http://codereview.appspot.com/3480041/diff/16001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java (right): http://codereview.appspot.com/3480041/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:40: * This rewriter helps in appending the image size parameters (extracted from inline styles) (extracted from inline styles, height and width) http://codereview.appspot.com/3480041/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:41: * to the proxied resource urls so that server side resizing can be done when ever possible. Could you add the following here: Non-proxied resource URLs are ignored by this rewriter. http://codereview.appspot.com/3480041/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:63: // b) it has inline style attribute that has 'height' and 'width' properties. We are actually reserving all nodes that have "style" attribute without checking for presence of "height" and "width" attributes - assuming this is intentional, please fix the comment. http://codereview.appspot.com/3480041/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:64: // TODO(satya): please beware of max-height, etc feilds. feilds -> fields. http://codereview.appspot.com/3480041/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:70: // We want to resize urls only that are proxied through us. We want to append image resize params only to urls that are proxied through us. http://codereview.appspot.com/3480041/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:102: // Inline style tags trumps everything, including inline height/width attribute, trumps -> trump http://codereview.appspot.com/3480041/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:124: if (styleHeight != "" && styleWidth != "") { Currently, if we have style="height:50px" and height="100px" and width="60px" for an image, we will choose final height and width as 100 and 60 instead of 50 and 60. Please fix this logic and add a testcase.
Sign in to reply to this message.
Addressing the comments.
Sign in to reply to this message.
http://codereview.appspot.com/3480041/diff/16001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java (right): http://codereview.appspot.com/3480041/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:40: * This rewriter helps in appending the image size parameters (extracted from inline styles) On 2010/12/14 07:20:34, anupama.dutta wrote: > (extracted from inline styles, height and width) Done. http://codereview.appspot.com/3480041/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:41: * to the proxied resource urls so that server side resizing can be done when ever possible. On 2010/12/14 07:20:34, anupama.dutta wrote: > Could you add the following here: > Non-proxied resource URLs are ignored by this rewriter. Done. http://codereview.appspot.com/3480041/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:63: // b) it has inline style attribute that has 'height' and 'width' properties. On 2010/12/14 07:20:34, anupama.dutta wrote: > We are actually reserving all nodes that have "style" attribute without checking > for presence of "height" and "width" attributes - assuming this is intentional, > please fix the comment. Done. http://codereview.appspot.com/3480041/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:64: // TODO(satya): please beware of max-height, etc feilds. On 2010/12/14 07:20:34, anupama.dutta wrote: > feilds -> fields. Done. http://codereview.appspot.com/3480041/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:70: // We want to resize urls only that are proxied through us. On 2010/12/14 07:20:34, anupama.dutta wrote: > We want to append image resize params only to urls that are proxied through us. Done. http://codereview.appspot.com/3480041/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:102: // Inline style tags trumps everything, including inline height/width attribute, On 2010/12/14 07:20:34, anupama.dutta wrote: > trumps -> trump Done. http://codereview.appspot.com/3480041/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:124: if (styleHeight != "" && styleWidth != "") { On 2010/12/14 07:20:34, anupama.dutta wrote: > Currently, if we have style="height:50px" and height="100px" and width="60px" > for an image, we will choose final height and width as 100 and 60 instead of 50 > and 60. Please fix this logic and add a testcase. Changed the logic. For you example, as inline style override height/width attributes. The effective height=50 and width=60
Sign in to reply to this message.
LGTM. http://codereview.appspot.com/3480041/diff/16002/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java (right): http://codereview.appspot.com/3480041/diff/16002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:99: // units are relative to the parent, it is more difficult to infer those values. Add to this comment: Specifically, we consider only the following cases: i) style specifies both height and width ii) height and width are both specified and style does not specify these attributes. iii) height and width are both specified and style overrides one of these. All other cases are ignored. http://codereview.appspot.com/3480041/diff/16002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:134: if (height == "" || width == "") { height.isEmpty() || width.isEmpty() http://codereview.appspot.com/3480041/diff/16002/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriterTest.java (right): http://codereview.appspot.com/3480041/diff/16002/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriterTest.java:106: + "&resize_h=60&resize_w=110\" style=\"width:110px\" width=\"50px\">" Please add one more test with "em" instead of "px" and ensure that it does not get the image-resize params.
Sign in to reply to this message.
Addressing the comments.
Sign in to reply to this message.
http://codereview.appspot.com/3480041/diff/16002/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java (right): http://codereview.appspot.com/3480041/diff/16002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:99: // units are relative to the parent, it is more difficult to infer those values. On 2010/12/14 09:35:34, anupama.dutta wrote: > Add to this comment: > Specifically, we consider only the following cases: > i) style specifies both height and width > ii) height and width are both specified and style does not specify these > attributes. > iii) height and width are both specified and style overrides one of these. > All other cases are ignored. Done. http://codereview.appspot.com/3480041/diff/16002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:134: if (height == "" || width == "") { On 2010/12/14 09:35:34, anupama.dutta wrote: > height.isEmpty() || width.isEmpty() Done. http://codereview.appspot.com/3480041/diff/16002/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriterTest.java (right): http://codereview.appspot.com/3480041/diff/16002/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriterTest.java:106: + "&resize_h=60&resize_w=110\" style=\"width:110px\" width=\"50px\">" On 2010/12/14 09:35:34, anupama.dutta wrote: > Please add one more test with "em" instead of "px" and ensure that it does not > get the image-resize params. Done.
Sign in to reply to this message.
http://codereview.appspot.com/3480041/diff/33001/java/gadgets/src/main/java/o... 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/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:37: import java.util.*; no wildcards pls http://codereview.appspot.com/3480041/diff/33001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:66: if ((!imageElement.getAttribute("height").isEmpty() && split getAttribute(key).isEmpty() into helper method for readability http://codereview.appspot.com/3480041/diff/33001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:68: imageElement.getAttribute("id").isEmpty() && why is the emptiness of ID important? http://codereview.appspot.com/3480041/diff/33001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:70: (!imageElement.getAttribute("style").isEmpty())) { So the rewriter accepts tags w/ style *and* class nodes? No CSS complications there? http://codereview.appspot.com/3480041/diff/33001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:78: proxied = proxyUriManager.process(uri); 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. http://codereview.appspot.com/3480041/diff/33001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageResizeRewriter.java:145: builder.putQueryParameter(UriCommon.Param.RESIZE_WIDTH.getKey(), width); With chained proxy syntax, you can't always add these params directly. For tighter control over syntax, it would be ideal to simply take the processed ProxyUri you've pulled in visit(), call setResize(...) on it, then resend to proxyUriManager.make(...). That introduces some inefficiency (double-make of proxyUri), but you'd get chained syntax support. As noted above, you could do this directly in visit(...) rather than revisit(...) since you're not using batched operation here. Perhaps better still would be to improve ProxyingVisitor to extract the resizing "hints" from the DOM and pass them to the ProxyUri objects created there. You could do so by refactoring this class into a helper class ImageSizeExtractor with method getSizes(Element).
Sign in to reply to this message.
http://codereview.appspot.com/3480041/diff/33001/java/gadgets/src/main/java/o... 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/o... 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/o... 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/o... 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/o... 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/o... 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/o... 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.
Sign in to reply to this message.
LGTM Thx! On 2010/12/15 13:20:23, satya3656 wrote: > http://codereview.appspot.com/3480041/diff/33001/java/gadgets/src/main/java/o... > 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/o... > 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/o... > 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/o... > 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/o... > 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/o... > 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/o... > 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.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
