Issue 2044045: Adding ImageAttributeRewriter that adds the height and width attributes if they are not specified. (Closed)
|
Created:
15 years, 4 months ago by satya3656 Modified:
15 years, 2 months ago Base URL:
http://svn.apache.org/repos/asf/shindig/trunk/ Visibility:
Public. |
DescriptionAdding ImageAttributeRewriter that adds the height and width attributes if they are not specified in the <img> tag.
Pros:
This helps in reducing the reflow's of the page when rendering it.
Cons:
Add extra burden of parsing the meta data of the images which may add to the latency.
Patch Set 1 #
Total comments: 24
Patch Set 2 : Addressing the comments, #Patch Set 3 : Upadte the code to use future tasks. #
Total comments: 2
Patch Set 4 : Addressing the gagans comments. #
Total comments: 22
Patch Set 5 : Addressing the comments. #
Total comments: 6
Patch Set 6 : Added tests #Patch Set 7 : Addressing the comments. #
Total comments: 1
Patch Set 8 : sync to head(r1006167) #
Total comments: 29
Patch Set 9 : Addressing the comments. #
Total comments: 4
Patch Set 10 : Addressing the comments. #
Total comments: 4
Patch Set 11 : Addressing the comments. #Patch Set 12 : Addressing Mannion Comments. #Patch Set 13 : Sync to head.(r1023057) #Patch Set 14 : Fixing some unused imports #
MessagesTotal messages: 31
Need to add the tests, but someone to have a look at it before adding them.
Sign in to reply to this message.
Would be nice if you run validation with this first. That will give you a good idea of the type of html/js ppl write and how much impact this rewriter is going to have. http://codereview.appspot.com/2044045/diff/1/3 File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java (right): http://codereview.appspot.com/2044045/diff/1/3#newcode61 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:61: // we process the <img> tag when it don't have 'class' and 'id' attributes does not have 'class' and ... http://codereview.appspot.com/2044045/diff/1/3#newcode62 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:62: // inorder to avoid conflicts from css styles. in order http://codereview.appspot.com/2044045/diff/1/3#newcode63 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:63: if (!"".equals(src) && "".equals(height) && "".equals(width)) { shouldnt you be doing StringUtils.isEmpty(height) && StringUtils.isEmpty(width) so that the null case is also covered. http://codereview.appspot.com/2044045/diff/1/3#newcode67 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:67: Uri tgt = new UriBuilder().parse(src).toUri(); please rename to target / imgResource http://codereview.appspot.com/2044045/diff/1/3#newcode69 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:69: HttpResponse response = requestPipeline.execute(req); you might want to consider making these future tasks so you can fetch all image resources parallelly and then modify their attributes in revisit. http://codereview.appspot.com/2044045/diff/1/3#newcode89 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:89: LOG.info("Unable to read reponnse for the image resource " + src); LOG.warning / LOG.severe maybe ? http://codereview.appspot.com/2044045/diff/1/3#newcode91 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:91: LOG.info("Unable to fetch the image resource " + src); same here http://codereview.appspot.com/2044045/diff/1/3#newcode93 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:93: LOG.info("Unable to parse the image resource " + src); same here http://codereview.appspot.com/2044045/diff/1/3#newcode100 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:100: // TODO(satya): Need to pass the request parameters as well ? Ideally you should. If the original request was for user agent X, we should really pass it on. Though this is kind of tricky because the browser might not send same headers for current html and this image resource. I think the todo is okay for now. http://codereview.appspot.com/2044045/diff/1/3#newcode104 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:104: HttpRequest req = new HttpRequest(tgt); please rename to imgResource / target. http://codereview.appspot.com/2044045/diff/1/3#newcode109 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:109: req.setFollowRedirects(false); Shouldnt you be setting it to true because you want to fetch the image to compute its height / width. But if you set it to true, it will get cached and then we might serve it out, which will cause other problems. So maybe you need to add this resource to a separate cache / add follow redirects logic / do not cache this resource. http://codereview.appspot.com/2044045/diff/1/3#newcode115 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:115: return true; return false as you are not modifying any node in this method.
Sign in to reply to this message.
Upadte the code to use future tasks.
Sign in to reply to this message.
http://codereview.appspot.com/2044045/diff/1/java/gadgets/src/main/java/org/a... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java (right): http://codereview.appspot.com/2044045/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:61: // we process the <img> tag when it don't have 'class' and 'id' attributes On 2010/09/01 02:54:03, gagan.goku wrote: > does not have 'class' and ... Done. http://codereview.appspot.com/2044045/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:62: // inorder to avoid conflicts from css styles. On 2010/09/01 02:54:03, gagan.goku wrote: > in order Done. http://codereview.appspot.com/2044045/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:63: if (!"".equals(src) && "".equals(height) && "".equals(width)) { On 2010/09/01 02:54:03, gagan.goku wrote: > shouldnt you be doing StringUtils.isEmpty(height) && StringUtils.isEmpty(width) > so that the null case is also covered. Null case won't occur here, getAttribute() will empty string if the attribute is not specified http://codereview.appspot.com/2044045/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:67: Uri tgt = new UriBuilder().parse(src).toUri(); On 2010/09/01 02:54:03, gagan.goku wrote: > please rename to target / imgResource Done. http://codereview.appspot.com/2044045/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:69: HttpResponse response = requestPipeline.execute(req); On 2010/09/01 02:54:03, gagan.goku wrote: > you might want to consider making these future tasks so you can fetch all image > resources parallelly and then modify their attributes in revisit Is it a good idea to have save state locally between the visit and revisit calls? http://codereview.appspot.com/2044045/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:89: LOG.info("Unable to read reponnse for the image resource " + src); On 2010/09/01 02:54:03, gagan.goku wrote: > LOG.warning / LOG.severe maybe ? Changed to warning, we don't want it to fail because we are not able to read the image. http://codereview.appspot.com/2044045/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:91: LOG.info("Unable to fetch the image resource " + src); On 2010/09/01 02:54:03, gagan.goku wrote: > same here Done. http://codereview.appspot.com/2044045/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:93: LOG.info("Unable to parse the image resource " + src); On 2010/09/01 02:54:03, gagan.goku wrote: > same here Done. http://codereview.appspot.com/2044045/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:100: // TODO(satya): Need to pass the request parameters as well ? On 2010/09/01 02:54:03, gagan.goku wrote: > Ideally you should. If the original request was for user agent X, we should > really pass it on. > Though this is kind of tricky because the browser might not send same headers > for current html and this image resource. > > I think the todo is okay for now. k http://codereview.appspot.com/2044045/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:104: HttpRequest req = new HttpRequest(tgt); On 2010/09/01 02:54:03, gagan.goku wrote: > please rename to imgResource / target. Done. http://codereview.appspot.com/2044045/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:109: req.setFollowRedirects(false); On 2010/09/01 02:54:03, gagan.goku wrote: > Shouldnt you be setting it to true because you want to fetch the image to > compute its height / width. > But if you set it to true, it will get cached and then we might serve it out, > which will cause other problems. > > So maybe you need to add this resource to a separate cache / add follow > redirects logic / do not cache this resource. I do want to use the information from cache if possible, to make the pass faster. http://codereview.appspot.com/2044045/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:115: return true; On 2010/09/01 02:54:03, gagan.goku wrote: > return false as you are not modifying any node in this method. Done.
Sign in to reply to this message.
http://codereview.appspot.com/2044045/diff/9001/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java (right): http://codereview.appspot.com/2044045/diff/9001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:138: private String processAllImgResources(List<Node> nodes, can you move this function out of this class into a separate class ? i think concat servlet also uses similar functionality.
Sign in to reply to this message.
Addressing the gagans comments.
Sign in to reply to this message.
http://codereview.appspot.com/2044045/diff/9001/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java (right): http://codereview.appspot.com/2044045/diff/9001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:138: private String processAllImgResources(List<Node> nodes, On 2010/09/28 09:42:40, gagan.goku wrote: > can you move this function out of this class into a separate class ? > i think concat servlet also uses similar functionality. Refactored most of the common code. Have a look now.
Sign in to reply to this message.
Change looking good. Please reupload as there was some chunk mismatch. Also, small style issues: http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcher.java (right): http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcher.java:45: * Issue parallel requests to all the image resources that are needed for the Issue parallel requests to for all resources that are needed. http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcher.java:53: List<Pair<Uri, FutureTask<RequestContext>>> futureTasks) { is it possible to return List<Pair<Uri, FutureTask<RequestContext>>> instead of taking it as an input parameter ? http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcher.java:66: * Issue parallel requests to all the image resources that are needed for the remove image http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcher.java:74: futureTasks.clear(); if possible, return Map<Uri, FutureTask<RequestContext>> rather than taking it as an input param http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcher.java:76: Uri imgUri = request.getUri(); rename imgUri to uri http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java (right): http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java:33: } Since this class is pretty small, you can move ImageAttribuiteVisitor as an inner class of ImageAttributeRewriter to save an extra file. http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java (right): http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:75: if (!"".equals(src) && "".equals(height) && "".equals(width)) { you can push these to the outer if using something like: if StingUtils.isEmpty(node.getAttributes().getNamedItem("height")) http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:84: LOG.info("Adding the STYLE tag with " + nodes.size() + " in revisit"); LOG.fine ? http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:120: if (head.getChildNodes().getLength() > 0) { you can always say: head.insertBefore(style, head.getFirstChild()) and it will do the right thing even when head.getFirstChild() is null. http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:211: throw new GadgetException(GadgetException.Code.INVALID_PARAMETER, you can ignore this check as its redundant. http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:214: req.setFollowRedirects(false); should not follow redirects ? The image could redirect to another image.
Sign in to reply to this message.
Addressed the comments. http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcher.java (right): http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcher.java:45: * Issue parallel requests to all the image resources that are needed for the On 2010/10/08 03:14:17, gagan.goku wrote: > Issue parallel requests to for all resources that are needed. Done. http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcher.java:53: List<Pair<Uri, FutureTask<RequestContext>>> futureTasks) { On 2010/10/08 03:14:17, gagan.goku wrote: > is it possible to return List<Pair<Uri, FutureTask<RequestContext>>> instead of > taking it as an input parameter ? Done. http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcher.java:66: * Issue parallel requests to all the image resources that are needed for the On 2010/10/08 03:14:17, gagan.goku wrote: > remove image Done. http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcher.java:74: futureTasks.clear(); On 2010/10/08 03:14:17, gagan.goku wrote: > if possible, return Map<Uri, FutureTask<RequestContext>> rather than taking it > as an input param Done. http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcher.java:76: Uri imgUri = request.getUri(); On 2010/10/08 03:14:17, gagan.goku wrote: > rename imgUri to uri Done. http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java (right): http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java:33: } On 2010/10/08 03:14:17, gagan.goku wrote: > Since this class is pretty small, you can move ImageAttribuiteVisitor as an > inner class of ImageAttributeRewriter to save an extra file. Done. http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java (right): http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:75: if (!"".equals(src) && "".equals(height) && "".equals(width)) { On 2010/10/08 03:14:17, gagan.goku wrote: > you can push these to the outer if using something like: > if StingUtils.isEmpty(node.getAttributes().getNamedItem("height")) Hope it is ok now. http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:84: LOG.info("Adding the STYLE tag with " + nodes.size() + " in revisit"); On 2010/10/08 03:14:17, gagan.goku wrote: > LOG.fine ? Removed, Added for some debugging purpose http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:120: if (head.getChildNodes().getLength() > 0) { On 2010/10/08 03:14:17, gagan.goku wrote: > you can always say: > head.insertBefore(style, head.getFirstChild()) and it will do the right thing > even when head.getFirstChild() is null. Done. http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:211: throw new GadgetException(GadgetException.Code.INVALID_PARAMETER, On 2010/10/08 03:14:17, gagan.goku wrote: > you can ignore this check as its redundant. Done. http://codereview.appspot.com/2044045/diff/21001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeVisitor.java:214: req.setFollowRedirects(false); On 2010/10/08 03:14:17, gagan.goku wrote: > should not follow redirects ? The image could redirect to another image. Hmm changed, somehow missed this from your previous comments.
Sign in to reply to this message.
Addressing the comments.
Sign in to reply to this message.
Lgtm. Please send out to dev@ for review. Btw, there is still a chunk mismatch in ConcatServlet.java. Make sure the side by side diff is correct before sending out to dev. I reviewd the combined file view. http://codereview.appspot.com/2044045/diff/34001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java (right): http://codereview.appspot.com/2044045/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java:116: Map<Uri, FutureTask<MultipleResourceHttpFetcher.RequestContext>> futureTasks = fetcher.fetchUnique(resourceRequests); line length http://codereview.appspot.com/2044045/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java:128: /** fix formatting. http://codereview.appspot.com/2044045/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java:139: Map<Uri, FutureTask<MultipleResourceHttpFetcher.RequestContext>> futureTasks) { try import static MultipleResourceHttpFetcher.RequestContext ?
Sign in to reply to this message.
Added tests
Sign in to reply to this message.
Addressing the comments.
Sign in to reply to this message.
http://codereview.appspot.com/2044045/diff/34001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java (right): http://codereview.appspot.com/2044045/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java:116: Map<Uri, FutureTask<MultipleResourceHttpFetcher.RequestContext>> futureTasks = fetcher.fetchUnique(resourceRequests); On 2010/10/08 16:50:31, gagan.goku wrote: > line length Done. http://codereview.appspot.com/2044045/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java:128: /** On 2010/10/08 16:50:31, gagan.goku wrote: > fix formatting. Done. http://codereview.appspot.com/2044045/diff/34001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java:139: Map<Uri, FutureTask<MultipleResourceHttpFetcher.RequestContext>> futureTasks) { On 2010/10/08 16:50:31, gagan.goku wrote: > try import static MultipleResourceHttpFetcher.RequestContext ? Done.
Sign in to reply to this message.
sync to head(r1006167)
Sign in to reply to this message.
sync to head(r1006167)
Sign in to reply to this message.
http://codereview.appspot.com/2044045/diff/46001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcher.java (right): http://codereview.appspot.com/2044045/diff/46001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcher.java:116: private HttpRequest httpReq; you can make these final http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcher.java:52: List<Pair<Uri, FutureTask<RequestContext>>> futureTasks = you can use Maps.newHashMap() http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcher.java:74: Map<Uri, FutureTask<RequestContext>> futureTasks = Would this be shorter: Map<Uri, HttpRequest> uriToReq = Maps.newHashMap(); for (HttpRequest request : requests) { uriToReq.put(request.getUri(), request); } return fetchAll(uriToReq.values()); http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcher.java:94: private HttpRequest httpReq; you can make these final http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java (right): http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java:66: private transient ExecutorService executor = Executors.newSingleThreadExecutor(); out of curiosity, why are you using a single threaded executor ? why not use the default executor by injecting it ? http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcherTest.java (right): http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcherTest.java:111: List<HttpRequest> requests = new ArrayList<HttpRequest>(); you can return ImmutableList.of(reqCxt1.getHttpReq(), reqCxt2.getHttpReq(), reqCxt3.getHttpReq()); http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewritterTest.java (right): http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewritterTest.java:64: Node img = elem("img", "class", "classname"); for more robust testing, add a dummy src to dontVisitImgTagWithClass, dontVisitImgTagWithId, dontVisitImgTagWithHeight and dontVisitImgTagWithWidth http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewritterTest.java:93: public void visitImgTagWithOnlySrc() throws Exception { visitImgTagWithSrc http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewritterTest.java:94: Node img = elem("img", "src", "http://test.jpg"); add some dummy attributes which you dont care about. http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewritterTest.java:101: Node html = htmlDoc(new Node[] {}); redundant line ? http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewritterTest.java:109: List<Node> nodes = new ArrayList<Node>(); you can say ImmutableList.of(img1, img2) http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewritterTest.java:116: expect(requestPipeline.execute(eq(reqCxtImg1.getHttpReq()))).andReturn(reqCxtImg1.getHttpResp()); line length http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewritterTest.java:131: Node head = doc.getFirstChild().getFirstChild(); doc.getElementsByTagName("head").item(0) is clearer ? http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewritterTest.java:132: assertEquals(1, head.getChildNodes().getLength()); also test that its a style node http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewritterTest.java:151: // Should not reach here. dont catch the gadget exception if you think this condition should never be hit.
Sign in to reply to this message.
Addressing the comments.
Sign in to reply to this message.
http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcher.java (right): http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcher.java:52: List<Pair<Uri, FutureTask<RequestContext>>> futureTasks = On 2010/10/10 04:19:26, gagan.goku wrote: > you can use Maps.newHashMap() Using Lists.newArrayList(); http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcher.java:74: Map<Uri, FutureTask<RequestContext>> futureTasks = Done refactoring as per the discussion. http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcher.java:94: private HttpRequest httpReq; On 2010/10/10 04:19:26, gagan.goku wrote: > you can make these final Done. http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java (right): http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java:66: private transient ExecutorService executor = Executors.newSingleThreadExecutor(); On 2010/10/10 04:19:26, gagan.goku wrote: > out of curiosity, why are you using a single threaded executor ? > why not use the default executor by injecting it ? Done. http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcherTest.java (right): http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcherTest.java:111: List<HttpRequest> requests = new ArrayList<HttpRequest>(); On 2010/10/10 04:19:26, gagan.goku wrote: > you can return ImmutableList.of(reqCxt1.getHttpReq(), reqCxt2.getHttpReq(), > reqCxt3.getHttpReq()); Done. http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewritterTest.java (right): http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewritterTest.java:64: Node img = elem("img", "class", "classname"); On 2010/10/10 04:19:26, gagan.goku wrote: > for more robust testing, add a dummy src to dontVisitImgTagWithClass, > dontVisitImgTagWithId, dontVisitImgTagWithHeight and dontVisitImgTagWithWidth Done. http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewritterTest.java:93: public void visitImgTagWithOnlySrc() throws Exception { On 2010/10/10 04:19:26, gagan.goku wrote: > visitImgTagWithSrc Done. http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewritterTest.java:94: Node img = elem("img", "src", "http://test.jpg"); On 2010/10/10 04:19:26, gagan.goku wrote: > add some dummy attributes which you dont care about. Done. http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewritterTest.java:101: Node html = htmlDoc(new Node[] {}); On 2010/10/10 04:19:26, gagan.goku wrote: > redundant line ? Done. http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewritterTest.java:109: List<Node> nodes = new ArrayList<Node>(); On 2010/10/10 04:19:26, gagan.goku wrote: > you can say ImmutableList.of(img1, img2) Done. http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewritterTest.java:116: expect(requestPipeline.execute(eq(reqCxtImg1.getHttpReq()))).andReturn(reqCxtImg1.getHttpResp()); On 2010/10/10 04:19:26, gagan.goku wrote: > line length Done. http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewritterTest.java:131: Node head = doc.getFirstChild().getFirstChild(); On 2010/10/10 04:19:26, gagan.goku wrote: > doc.getElementsByTagName("head").item(0) is clearer ? Done. http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewritterTest.java:132: assertEquals(1, head.getChildNodes().getLength()); On 2010/10/10 04:19:26, gagan.goku wrote: > also test that its a style node Done. http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewritterTest.java:151: // Should not reach here. On 2010/10/10 04:19:26, gagan.goku wrote: > dont catch the gadget exception if you think this condition should never be hit. Done.
Sign in to reply to this message.
LGTM. Please send out to dev@ http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcher.java (right): http://codereview.appspot.com/2044045/diff/57001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcher.java:52: List<Pair<Uri, FutureTask<RequestContext>>> futureTasks = On 2010/10/10 04:19:26, gagan.goku wrote: > you can use Maps.newHashMap() Sorry, i meant Lists.newArrayList() http://codereview.appspot.com/2044045/diff/67001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcher.java:86: futureTasks.put(uri, httpFetcher); Good that you made this the common function. probably add // Fetch the content of the requested uri as the function level comment. Also, private FutureTask<RequestContext> createHttpFetcher(HttpRequest request) { might fit on 1 line. http://codereview.appspot.com/2044045/diff/67001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java (right): http://codereview.appspot.com/2044045/diff/67001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java:66: private transient ExecutorService executor = Executors.newSingleThreadExecutor(); setting it initially as Executors.newSingleThreadExecutor(); is probably useless as you are anyways setting it in the constructor.
Sign in to reply to this message.
http://codereview.appspot.com/2044045/diff/67001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcher.java (right): http://codereview.appspot.com/2044045/diff/67001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/http/MultipleResourceHttpFetcher.java:86: private FutureTask<RequestContext> createHttpFetcher( On 2010/10/12 09:50:39, gagan.goku wrote: > Good that you made this the common function. > > probably add > // Fetch the content of the requested uri > as the function level comment. > Also, private FutureTask<RequestContext> createHttpFetcher(HttpRequest request) > { might fit on 1 line. Done. http://codereview.appspot.com/2044045/diff/67001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java (right): http://codereview.appspot.com/2044045/diff/67001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java:66: private transient ExecutorService executor = Executors.newSingleThreadExecutor(); On 2010/10/12 09:50:39, gagan.goku wrote: > setting it initially as Executors.newSingleThreadExecutor(); is probably useless > as you are anyways setting it in the constructor. Done.
Sign in to reply to this message.
Addressing the comments.
Sign in to reply to this message.
LGTM, other than a couple of nitpicky things http://codereview.appspot.com/2044045/diff/54002/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java (right): http://codereview.appspot.com/2044045/diff/54002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java:65: private transient ExecutorService executor; Not sure I understand why this is transient - if it is a required field and set in the constructor, why isn't it final? http://codereview.appspot.com/2044045/diff/54002/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java (right): http://codereview.appspot.com/2044045/diff/54002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:171: List<HttpRequest> requests = new ArrayList<HttpRequest>(); Maybe use Lists.newArrayList() here
Sign in to reply to this message.
Addressing the comments.
Sign in to reply to this message.
http://codereview.appspot.com/2044045/diff/54002/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java (right): http://codereview.appspot.com/2044045/diff/54002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java:65: private transient ExecutorService executor; On 2010/10/12 14:35:53, mat.mannion wrote: > Not sure I understand why this is transient - if it is a required field and set > in the constructor, why isn't it final? Changed to final. http://codereview.appspot.com/2044045/diff/54002/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java (right): http://codereview.appspot.com/2044045/diff/54002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ConcatProxyServlet.java:171: List<HttpRequest> requests = new ArrayList<HttpRequest>(); On 2010/10/12 14:35:53, mat.mannion wrote: > Maybe use Lists.newArrayList() here Done.
Sign in to reply to this message.
On 2010/10/12 14:44:36, satya3656 wrote: > Addressing the comments. There is a typo in the name of one of the test classes - ImageAttributeRewritter instead of ImageAttributeRewriter.
Sign in to reply to this message.
Addressing Mannion Comments.
Sign in to reply to this message.
Fixing some unused imports
Sign in to reply to this message.
Hi Mannion, Addressed your comments, can you check if the change looks good now.
Sign in to reply to this message.
Build looks awesome. Minor change to patch: Converted ImageAttributeRewriter.java:110 to use Lists.newArrayList(). Committed as r1024077.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
