Issue 4003047: CacheEnforcementVisitor is added to script & css concat rewriters (Closed)
|
Created:
15 years, 1 month ago by pulkitgoyal2000 Modified:
15 years, 1 month ago Base URL:
http://svn.apache.org/repos/asf/shindig/trunk/ Visibility:
Public. |
DescriptionCacheEnforcementRewriter is added to relevant rewriters so that only non-private & cached resources are proxied or modified.
Patch Set 1 #
Total comments: 7
Patch Set 2 : Addressing Comment #
Total comments: 4
Patch Set 3 : Addressing Comments #
Total comments: 4
Patch Set 4 : Addressing Comments #
Total comments: 2
Patch Set 5 : Adding Comments #Patch Set 6 : Remove unused variable #Patch Set 7 : Mischunk error #
MessagesTotal messages: 12
Sign in to reply to this message.
http://codereview.appspot.com/4003047/diff/1/java/gadgets/src/main/java/org/a... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java (right): http://codereview.appspot.com/4003047/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java:47: import java.util.concurrent.Executor; Order alphabetically. http://codereview.appspot.com/4003047/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java:51: import java.util.Arrays; Order alphabetically. http://codereview.appspot.com/4003047/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ImageAttributeRewriter.java:79: I'm not sure if this matters but which executor do we want to use here. Do we want the same one for both the visitor? Could you check with Satya? http://codereview.appspot.com/4003047/diff/1/java/gadgets/src/main/java/org/a... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ScriptConcatContentRewriter.java (right): http://codereview.appspot.com/4003047/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ScriptConcatContentRewriter.java:26: import org.apache.shindig.gadgets.http.RequestPipeline; Order alphabetically. http://codereview.appspot.com/4003047/diff/1/java/gadgets/src/main/java/org/a... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/StyleConcatContentRewriter.java (right): http://codereview.appspot.com/4003047/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/StyleConcatContentRewriter.java:27: import org.apache.shindig.gadgets.http.HttpCache; Order alphabetically.
Sign in to reply to this message.
Removed ImageAttibuteRewriter.java as it is not a stable rewriter. http://codereview.appspot.com/4003047/diff/1/java/gadgets/src/main/java/org/a... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ScriptConcatContentRewriter.java (right): http://codereview.appspot.com/4003047/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ScriptConcatContentRewriter.java:26: import org.apache.shindig.gadgets.http.RequestPipeline; On 2011/01/24 11:30:19, nikhilmadan23 wrote: > Order alphabetically. Done. http://codereview.appspot.com/4003047/diff/1/java/gadgets/src/main/java/org/a... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/StyleConcatContentRewriter.java (right): http://codereview.appspot.com/4003047/diff/1/java/gadgets/src/main/java/org/a... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/StyleConcatContentRewriter.java:27: import org.apache.shindig.gadgets.http.HttpCache; On 2011/01/24 11:30:19, nikhilmadan23 wrote: > Order alphabetically. Done.
Sign in to reply to this message.
lgtm Could you add a comment near the class definition saying that you are using a CacheEnforcementVisitor in these rewriters? http://codereview.appspot.com/4003047/diff/9001/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ScriptConcatContentRewriter.java (right): http://codereview.appspot.com/4003047/diff/9001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ScriptConcatContentRewriter.java:50: Extra blank line. http://codereview.appspot.com/4003047/diff/9001/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/StyleConcatContentRewriter.java (right): http://codereview.appspot.com/4003047/diff/9001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/StyleConcatContentRewriter.java:50: Extra line.
Sign in to reply to this message.
http://codereview.appspot.com/4003047/diff/9001/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ScriptConcatContentRewriter.java (right): http://codereview.appspot.com/4003047/diff/9001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ScriptConcatContentRewriter.java:50: On 2011/01/25 09:26:02, nikhilmadan23 wrote: > Extra blank line. Done. http://codereview.appspot.com/4003047/diff/9001/java/gadgets/src/main/java/or... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/StyleConcatContentRewriter.java (right): http://codereview.appspot.com/4003047/diff/9001/java/gadgets/src/main/java/or... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/StyleConcatContentRewriter.java:50: On 2011/01/25 09:26:02, nikhilmadan23 wrote: > Extra line. Done.
Sign in to reply to this message.
lgtm Could you add Anupama to the reviewers list.
Sign in to reply to this message.
http://codereview.appspot.com/4003047/diff/17001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ScriptConcatContentRewriter.java (right): http://codereview.appspot.com/4003047/diff/17001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ScriptConcatContentRewriter.java:56: @Named("shindig.concat.executor") Executor executor) { Shall we order these params as featureConfigFactory, executor, cache, requestPipeline, concatUriManager? http://codereview.appspot.com/4003047/diff/17001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ScriptConcatContentRewriter.java:68: new CacheEnforcementVisitor(config, executor, cache, requestPipeline, TAGS), Since TAGS are used only once in this entire class, could we not directly use CacheEnforcementVisitor.Tags.SCRIPT here?
Sign in to reply to this message.
Can you change the issue title to "CacheEnforcementVisitor is added to script & css concat rewriters "
Sign in to reply to this message.
http://codereview.appspot.com/4003047/diff/17001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ScriptConcatContentRewriter.java (right): http://codereview.appspot.com/4003047/diff/17001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ScriptConcatContentRewriter.java:56: @Named("shindig.concat.executor") Executor executor) { On 2011/01/25 10:28:26, anupama.dutta wrote: > Shall we order these params as featureConfigFactory, executor, cache, > requestPipeline, concatUriManager? Done. http://codereview.appspot.com/4003047/diff/17001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ScriptConcatContentRewriter.java:68: new CacheEnforcementVisitor(config, executor, cache, requestPipeline, TAGS), On 2011/01/25 10:28:26, anupama.dutta wrote: > Since TAGS are used only once in this entire class, could we not directly use > CacheEnforcementVisitor.Tags.SCRIPT here? Done.
Sign in to reply to this message.
LGTM. Please address the small nit mentioned below and send it to dev@ with Fargo, Gagan and Lindner as reviewers. http://codereview.appspot.com/4003047/diff/23001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ScriptConcatContentRewriter.java (right): http://codereview.appspot.com/4003047/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ScriptConcatContentRewriter.java:37: * REwrites scripts. REwrites scripts -> Concatenates non-private and cached script resources.
Sign in to reply to this message.
http://codereview.appspot.com/4003047/diff/23001/java/gadgets/src/main/java/o... File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ScriptConcatContentRewriter.java (right): http://codereview.appspot.com/4003047/diff/23001/java/gadgets/src/main/java/o... java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ScriptConcatContentRewriter.java:37: * REwrites scripts. On 2011/01/27 15:36:59, anupama.dutta wrote: > REwrites scripts -> Concatenates non-private and cached script resources. Done.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
