Issue 4321043: Changing version function definition in ConcatUri & ProxyUri Versioner interface (Closed)
|
Created:
14 years, 11 months ago by pulkitgoyal2000 Modified:
14 years, 9 months ago Reviewers:
gagan.goku, dev-remailer Base URL:
http://svn.apache.org/repos/asf/shindig/trunk/ Visibility:
Public. |
Descriptionversion function definition is changed in ConcatUriVersion & ProxyUriversioner interface. Now, version function has information about type of resources (js, css etc.) in addition to their uris. html tag context in proxied uri and type in concat uri are used to populate this information.
Patch Set 1 #
Total comments: 20
Patch Set 2 : Address Nikhil's Comment #Patch Set 3 : Change the Flag Type from Boolean to String #
Total comments: 16
Patch Set 4 : Addressing Comment #
Total comments: 2
Patch Set 5 : Addressing Comment #
Total comments: 4
Patch Set 6 : Addressing Comments #
Total comments: 2
Patch Set 7 : Addressing Comments #
Total comments: 2
Patch Set 8 : Moving code to Versioner #Patch Set 9 : Remove error mismatch #
Total comments: 10
Patch Set 10 : Addressing Comments #
Total comments: 2
Patch Set 11 : Addressing Comments #Patch Set 12 : Sync and Upload #MessagesTotal messages: 25
Sign in to reply to this message.
Sign in to reply to this message.
http://codereview.appspot.com/4321043/diff/1/java/common/conf/shindig.properties File java/common/conf/shindig.properties (right): http://codereview.appspot.com/4321043/diff/1/java/common/conf/shindig.propert... java/common/conf/shindig.properties:173: # Do only image & jS urls versioning if it is true Change to "Version only image and javascript urls" http://codereview.appspot.com/4321043/diff/1/java/common/conf/shindig.propert... java/common/conf/shindig.properties:174: org.apache.shindig.uri.versionImageAndJsOnly=false could you rename versionImageAndJsOnly to versionOnlyImagesAndJS http://codereview.appspot.com/4321043/diff/1/java/gadgets/src/main/java/org/a... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java (right): http://codereview.appspot.com/4321043/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:60: private boolean versionImageAndJsOnly = false; could you rename this similarly http://codereview.appspot.com/4321043/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:62: dd http://codereview.appspot.com/4321043/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:82: public void setCssVersioning( shouldn't this be called setVersionImageAndJsOnly? http://codereview.appspot.com/4321043/diff/1/java/gadgets/src/main/java/org/a... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java (right): http://codereview.appspot.com/4321043/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:38: remove http://codereview.appspot.com/4321043/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:76: private boolean versionImageAndJsOnly = false; same comments regarding name of variable and next function http://codereview.appspot.com/4321043/diff/1/java/gadgets/src/test/java/org/a... File java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java (right): http://codereview.appspot.com/4321043/diff/1/java/gadgets/src/test/java/org/a... java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:34: import com.google.inject.AbstractModule; order http://codereview.appspot.com/4321043/diff/1/java/gadgets/src/test/java/org/a... java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:420: private Injector injectVersionImageAndJsOnly(final Boolean val, final String host, could you change val to something more descriptive http://codereview.appspot.com/4321043/diff/1/java/gadgets/src/test/java/org/a... File java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManagerTest.java (right): http://codereview.appspot.com/4321043/diff/1/java/gadgets/src/test/java/org/a... java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManagerTest.java:24: import com.google.inject.Guice; order
Sign in to reply to this message.
http://codereview.appspot.com/4321043/diff/1/java/common/conf/shindig.properties File java/common/conf/shindig.properties (right): http://codereview.appspot.com/4321043/diff/1/java/common/conf/shindig.propert... java/common/conf/shindig.properties:173: # Do only image & jS urls versioning if it is true On 2011/03/29 13:43:09, nikhilmadan23 wrote: > Change to "Version only image and javascript urls" Done. http://codereview.appspot.com/4321043/diff/1/java/common/conf/shindig.propert... java/common/conf/shindig.properties:174: org.apache.shindig.uri.versionImageAndJsOnly=false On 2011/03/29 13:43:09, nikhilmadan23 wrote: > could you rename versionImageAndJsOnly to versionOnlyImagesAndJS Done. http://codereview.appspot.com/4321043/diff/1/java/gadgets/src/main/java/org/a... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java (right): http://codereview.appspot.com/4321043/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:60: private boolean versionImageAndJsOnly = false; On 2011/03/29 13:43:09, nikhilmadan23 wrote: > could you rename this similarly Done. http://codereview.appspot.com/4321043/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:62: On 2011/03/29 13:43:09, nikhilmadan23 wrote: > dd Done. http://codereview.appspot.com/4321043/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:82: public void setCssVersioning( On 2011/03/29 13:43:09, nikhilmadan23 wrote: > shouldn't this be called setVersionImageAndJsOnly? Done. http://codereview.appspot.com/4321043/diff/1/java/gadgets/src/main/java/org/a... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java (right): http://codereview.appspot.com/4321043/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:38: On 2011/03/29 13:43:09, nikhilmadan23 wrote: > remove Done. http://codereview.appspot.com/4321043/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:76: private boolean versionImageAndJsOnly = false; On 2011/03/29 13:43:09, nikhilmadan23 wrote: > same comments regarding name of variable and next function Done. http://codereview.appspot.com/4321043/diff/1/java/gadgets/src/test/java/org/a... File java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java (right): http://codereview.appspot.com/4321043/diff/1/java/gadgets/src/test/java/org/a... java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:34: import com.google.inject.AbstractModule; On 2011/03/29 13:43:09, nikhilmadan23 wrote: > order Done. http://codereview.appspot.com/4321043/diff/1/java/gadgets/src/test/java/org/a... java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManagerTest.java:420: private Injector injectVersionImageAndJsOnly(final Boolean val, final String host, On 2011/03/29 13:43:09, nikhilmadan23 wrote: > could you change val to something more descriptive Done. http://codereview.appspot.com/4321043/diff/1/java/gadgets/src/test/java/org/a... File java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManagerTest.java (right): http://codereview.appspot.com/4321043/diff/1/java/gadgets/src/test/java/org/a... java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManagerTest.java:24: import com.google.inject.Guice; On 2011/03/29 13:43:09, nikhilmadan23 wrote: > order Done.
Sign in to reply to this message.
Sign in to reply to this message.
http://codereview.appspot.com/4321043/diff/16001/java/common/conf/shindig.pro... File java/common/conf/shindig.properties (right): http://codereview.appspot.com/4321043/diff/16001/java/common/conf/shindig.pro... java/common/conf/shindig.properties:176: org.apache.shindig.uri.tagsForVersioning=images,scripts shouldn't we set the default value to all tags so that current behavior stays same. also what about body and embed? http://codereview.appspot.com/4321043/diff/16001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java (right): http://codereview.appspot.com/4321043/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:24: import com.google.common.base.Splitter; nit: order http://codereview.appspot.com/4321043/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:63: private String tagsForVersioning = "link,script,img"; this was images,scripts in shindig.properties could you keep it consistent? http://codereview.appspot.com/4321043/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:189: for (String tag : Splitter.on(',').trimResults().omitEmptyStrings().split(this.tagsForVersioning)) { > 100 also, the this is redundant http://codereview.appspot.com/4321043/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:191: } can we do all of this just once in the constructor instead? http://codereview.appspot.com/4321043/diff/16001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java (right): http://codereview.appspot.com/4321043/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:41: import java.util.*; could you restore this to the original http://codereview.appspot.com/4321043/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:44: * Generates URIs for use by the Shindig proxy service. same comments as previous file.
Sign in to reply to this message.
http://codereview.appspot.com/4321043/diff/16001/java/common/conf/shindig.pro... File java/common/conf/shindig.properties (right): http://codereview.appspot.com/4321043/diff/16001/java/common/conf/shindig.pro... java/common/conf/shindig.properties:176: org.apache.shindig.uri.tagsForVersioning=images,scripts On 2011/05/09 07:01:59, nikhilmadan23 wrote: > shouldn't we set the default value to all tags so that current behavior stays > same. > also what about body and embed? Done. http://codereview.appspot.com/4321043/diff/16001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java (right): http://codereview.appspot.com/4321043/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:24: import com.google.common.base.Splitter; On 2011/05/09 07:01:59, nikhilmadan23 wrote: > nit: order Done. http://codereview.appspot.com/4321043/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:63: private String tagsForVersioning = "link,script,img"; On 2011/05/09 07:01:59, nikhilmadan23 wrote: > this was images,scripts in shindig.properties > could you keep it consistent? Injecting thru constructor. http://codereview.appspot.com/4321043/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:189: for (String tag : Splitter.on(',').trimResults().omitEmptyStrings().split(this.tagsForVersioning)) { On 2011/05/09 07:01:59, nikhilmadan23 wrote: > > 100 > also, the this is redundant in what sense it is redundant.? http://codereview.appspot.com/4321043/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:191: } On 2011/05/09 07:01:59, nikhilmadan23 wrote: > can we do all of this just once in the constructor instead? Done. http://codereview.appspot.com/4321043/diff/16001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java (right): http://codereview.appspot.com/4321043/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:41: import java.util.*; On 2011/05/09 07:01:59, nikhilmadan23 wrote: > could you restore this to the original Done. http://codereview.appspot.com/4321043/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:44: * Generates URIs for use by the Shindig proxy service. On 2011/05/09 07:01:59, nikhilmadan23 wrote: > same comments as previous file. What do you mean by this?
Sign in to reply to this message.
http://codereview.appspot.com/4321043/diff/16001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java (right): http://codereview.appspot.com/4321043/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:189: for (String tag : Splitter.on(',').trimResults().omitEmptyStrings().split(this.tagsForVersioning)) { On 2011/05/12 12:34:07, pulkitgoyal2000 wrote: > On 2011/05/09 07:01:59, nikhilmadan23 wrote: > > > 100 > > also, the this is redundant > in what sense it is redundant.? I meant the "this" in this.tagsForVersioning http://codereview.appspot.com/4321043/diff/16001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java (right): http://codereview.appspot.com/4321043/diff/16001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:44: * Generates URIs for use by the Shindig proxy service. On 2011/05/12 12:34:07, pulkitgoyal2000 wrote: > On 2011/05/09 07:01:59, nikhilmadan23 wrote: > > same comments as previous file. > What do you mean by this? I meant the initialization of the set of tags and the other comments in DefaultConcatUriManager which you have taken care of in the new change http://codereview.appspot.com/4321043/diff/22002/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java (right): http://codereview.appspot.com/4321043/diff/22002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:63: private Set<String> tagsForVersioning; make final?
Sign in to reply to this message.
http://codereview.appspot.com/4321043/diff/22002/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java (right): http://codereview.appspot.com/4321043/diff/22002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:63: private Set<String> tagsForVersioning; On 2011/05/12 13:31:40, nikhilmadan23 wrote: > make final? Done.
Sign in to reply to this message.
http://codereview.appspot.com/4321043/diff/16003/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java (right): http://codereview.appspot.com/4321043/diff/16003/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:91: this.tagsForVersioning = includeTagsBuilder.build(); This logic seems to be repeating in the DefaultConcatUriManager and in DefaultProxyUriManager. Is there any way we can keep this in one place? http://codereview.appspot.com/4321043/diff/16003/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:113: resourceUris could be empty at this point. How about adding an if (resourceUris.isEmpty()) check over here, similar to line 100?
Sign in to reply to this message.
http://codereview.appspot.com/4321043/diff/16003/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java (right): http://codereview.appspot.com/4321043/diff/16003/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:91: this.tagsForVersioning = includeTagsBuilder.build(); On 2011/05/13 05:51:49, pradnya wrote: > This logic seems to be repeating in the DefaultConcatUriManager and in > DefaultProxyUriManager. Is there any way we can keep this in one place? The only way we can think of is to put this in rewriter config & inject that rewriter config. Is it good to have rewriter config in non-rewriter class? http://codereview.appspot.com/4321043/diff/16003/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:113: On 2011/05/13 05:51:49, pradnya wrote: > resourceUris could be empty at this point. How about adding an if > (resourceUris.isEmpty()) check over here, similar to line 100? Done.
Sign in to reply to this message.
lgtm http://codereview.appspot.com/4321043/diff/33001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManagerTest.java (right): http://codereview.appspot.com/4321043/diff/33001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManagerTest.java:593: @SuppressWarnings("unchecked") don't remove the line
Sign in to reply to this message.
http://codereview.appspot.com/4321043/diff/33001/java/gadgets/src/test/java/o... File java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManagerTest.java (right): http://codereview.appspot.com/4321043/diff/33001/java/gadgets/src/test/java/o... java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManagerTest.java:593: @SuppressWarnings("unchecked") On 2011/05/13 07:25:27, nikhilmadan23 wrote: > don't remove the line Done.
Sign in to reply to this message.
lgtm Please send to dev@
Sign in to reply to this message.
Description: tagsForVersioning flag is introduced to version only specified tags' url. We want to omit css urls from versioning & the reason is - CSSResponseRewriter rewrites embedded resource URLs to versioned proxied URIs, if content of any of these embedded resources changes, we should ideally update the css resource's proxy URI's version to indicate that css resource has changed. Since we only check the content hash of original response for the css resource (which would not really have changed) and hence retain the old version for the css response incorrectly. Please review this at http://codereview.appspot.com/4321043/ Thanks Pulkit On Fri, May 13, 2011 at 1:55 PM, <pradnya@gmail.com> wrote: > lgtm > > Please send to dev@ > > > http://codereview.appspot.com/4321043/ >
Sign in to reply to this message.
I don't think i understand this fully, but do you just want to disable css versioning? In that case, why not move the logic to inside the versioner? http://codereview.appspot.com/4321043/diff/35002/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java (right): http://codereview.appspot.com/4321043/diff/35002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:110: this.tagsForVersioning.contains(puc.getHtmlTagContext().toLowerCase())) { wouldn't this also stop proxying of css urls?
Sign in to reply to this message.
In versioner, we dont have information about whether it Css, image or anything else thats the reason I moved this functionality to urimanager class. http://codereview.appspot.com/4321043/diff/35002/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java (right): http://codereview.appspot.com/4321043/diff/35002/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:110: this.tagsForVersioning.contains(puc.getHtmlTagContext().toLowerCase())) { On 2011/05/13 17:22:16, gagan.goku wrote: > wouldn't this also stop proxying of css urls? No..Proxying happens in Line 133 which is not dependent on resourcesUris.
Sign in to reply to this message.
On 2011/05/14 04:55:02, pulkitgoyal2000 wrote: > In versioner, we dont have information about whether it Css, image or anything > else thats the reason I moved this functionality to urimanager class. If you do it in versioner, u won't have to put the deciding logic in 2 places. > > http://codereview.appspot.com/4321043/diff/35002/java/gadgets/src/main/java/o... > File > java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java > (right): > > http://codereview.appspot.com/4321043/diff/35002/java/gadgets/src/main/java/o... > java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:110: > this.tagsForVersioning.contains(puc.getHtmlTagContext().toLowerCase())) { > On 2011/05/13 17:22:16, gagan.goku wrote: > > wouldn't this also stop proxying of css urls? > No..Proxying happens in Line 133 which is not dependent on resourcesUris. got it.
Sign in to reply to this message.
On 2011/05/14 05:01:35, gagan.goku wrote: > On 2011/05/14 04:55:02, pulkitgoyal2000 wrote: > > In versioner, we dont have information about whether it Css, image or anything > > else thats the reason I moved this functionality to urimanager class. > If you do it in versioner, u won't have to put the deciding logic in 2 places. Still code has to be in 2 places because.. ConcatUriVersioner is different from ProxyUriVersioner. > > > > > > http://codereview.appspot.com/4321043/diff/35002/java/gadgets/src/main/java/o... > > File > > > java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java > > (right): > > > > > http://codereview.appspot.com/4321043/diff/35002/java/gadgets/src/main/java/o... > > > java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:110: > > this.tagsForVersioning.contains(puc.getHtmlTagContext().toLowerCase())) { > > On 2011/05/13 17:22:16, gagan.goku wrote: > > > wouldn't this also stop proxying of css urls? > > No..Proxying happens in Line 133 which is not dependent on resourcesUris. > got it.
Sign in to reply to this message.
Moved the code to versioner...
Sign in to reply to this message.
http://codereview.appspot.com/4321043/diff/26008/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java (right): http://codereview.appspot.com/4321043/diff/26008/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:199: * @param container Container making the request add the param description and mention that its optional, older implementations can just ignore resourceTags. http://codereview.appspot.com/4321043/diff/26008/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java (right): http://codereview.appspot.com/4321043/diff/26008/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:49: public static final String VERSION_ALL_TAGS = "all"; revert http://codereview.appspot.com/4321043/diff/26008/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java (right): http://codereview.appspot.com/4321043/diff/26008/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:73: public static final String VERSION_ALL_TAGS = "all"; revert http://codereview.appspot.com/4321043/diff/26008/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:113: versions = Maps.newHashMapWithExpectedSize(resourceUris.size()); why change this? i think you just need to populate resourceTags correctly and call versioner.version with it, everything else can stay same http://codereview.appspot.com/4321043/diff/26008/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriManager.java (right): http://codereview.appspot.com/4321043/diff/26008/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriManager.java:252: List<String> version(List<Uri> resources, String container, List<String> resourceTags); describe the param you are adding
Sign in to reply to this message.
Hi Gagan, Also changed title & description as it is not more adding a flag cl. http://codereview.appspot.com/4321043/diff/26008/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java (right): http://codereview.appspot.com/4321043/diff/26008/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:199: * @param container Container making the request On 2011/05/17 02:03:29, gagan.goku wrote: > add the param description and mention that its optional, older implementations > can just ignore resourceTags. Done. http://codereview.appspot.com/4321043/diff/26008/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java (right): http://codereview.appspot.com/4321043/diff/26008/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java:49: public static final String VERSION_ALL_TAGS = "all"; On 2011/05/17 02:03:29, gagan.goku wrote: > revert Done. http://codereview.appspot.com/4321043/diff/26008/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java (right): http://codereview.appspot.com/4321043/diff/26008/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:73: public static final String VERSION_ALL_TAGS = "all"; On 2011/05/17 02:03:29, gagan.goku wrote: > revert Done. http://codereview.appspot.com/4321043/diff/26008/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:113: versions = Maps.newHashMapWithExpectedSize(resourceUris.size()); On 2011/05/17 02:03:29, gagan.goku wrote: > why change this? i think you just need to populate resourceTags correctly and > call versioner.version with it, everything else can stay same Done.. My Bad.. Forget to revert this file :) http://codereview.appspot.com/4321043/diff/26008/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriManager.java (right): http://codereview.appspot.com/4321043/diff/26008/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriManager.java:252: List<String> version(List<Uri> resources, String container, List<String> resourceTags); On 2011/05/17 02:03:29, gagan.goku wrote: > describe the param you are adding Done.
Sign in to reply to this message.
Small nits, mostly looks good. Do update the title and description to point out that you are populating the html tag context while sending it to the versioner. Let me know when you want me to commit this change. http://codereview.appspot.com/4321043/diff/40001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java (right): http://codereview.appspot.com/4321043/diff/40001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:200: * @param resourceTags Index-correlated list of html tags, one per resouceUris. Any older resourceUri here is a list of list of uris. So how does resourceTags correspond to it? Should it be List<List<String>> resourceTags ? Or are you saying that only similar tags are merged and so the 2nd level list is redundant? A better comment is needed here.
Sign in to reply to this message.
http://codereview.appspot.com/4321043/diff/40001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java (right): http://codereview.appspot.com/4321043/diff/40001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ConcatUriManager.java:200: * @param resourceTags Index-correlated list of html tags, one per resouceUris. Any older On 2011/05/17 13:53:24, gagan.goku wrote: > resourceUri here is a list of list of uris. So how does resourceTags correspond > to it? > Should it be List<List<String>> resourceTags ? > Or are you saying that only similar tags are merged and so the 2nd level list is > redundant? > > A better comment is needed here. Done...only similar tags are merged and so the 2nd level list is redundant
Sign in to reply to this message.
|
