I feel lighter just reading it....
http://codereview.appspot.com/11250/diff/1/29 File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/AbstractHttpCache.java (right): http://codereview.appspot.com/11250/diff/1/29#newcode28 Line 28: public static final char KEY_SEPARATOR = ':'; Use a non-URI safe separator to prevent collisions with opensocial id types which are all URL safe by definition. viewer id can easily be 0:0. How about | ? http://codereview.appspot.com/11250/diff/1/29#newcode64 Line 64: private static boolean isCacheable(HttpResponse response) { should be protected and non-static to facilitate overrides. http://codereview.appspot.com/11250/diff/1/29#newcode68 Line 68: private static boolean isCacheable(HttpRequest request) { protected non-static http://codereview.appspot.com/11250/diff/1/29#newcode97 Line 97: protected static String createKey(HttpRequest request) { protected non-static. http://codereview.appspot.com/11250/diff/1/29#newcode120 Line 120: private static String getOwnerId(HttpRequest request) { these and all below should be protected static for re-use http://codereview.appspot.com/11250/diff/1/29#newcode182 Line 182: public HttpResponse removeResponse(HttpRequest request) { move above isCacheable http://codereview.appspot.com/11250/diff/1/29#newcode200 Line 200: return response.getCacheExpiration() > System.currentTimeMillis(); someday we'll use clocks :( http://codereview.appspot.com/11250/diff/1/25 File java/gadgets/src/main/java/org/apache/shindig/gadgets/http/BasicHttpFetcher.java (right): http://codereview.appspot.com/11250/diff/1/25#newcode42 Line 42: * Implementation of a {...@code RemoteObjectFetcher} using standard java.net Can you clean up this comment. Its complete junk. http://codereview.appspot.com/11250/diff/1/25#newcode151 Line 151: if (!"GET".equals(request.getMethod())) { I dont think we really ever want to use the HttpURLConnection cache. http://codereview.appspot.com/11250/diff/1/25#newcode162 Line 162: if (e instanceof java.net.SocketTimeoutException || log? http://codereview.appspot.com/11250/diff/1/37 File java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthModule.java (right): http://codereview.appspot.com/11250/diff/1/37#newcode33 Line 33: import org.apache.commons.io.IOUtils; I think these go above com.google http://codereview.appspot.com/11250/diff/1/43 File java/gadgets/src/main/java/org/apache/shindig/gadgets/preload/HttpPreloader.java (right): http://codereview.appspot.com/11250/diff/1/43#newcode52 Line 52: // TODO: This needs to be fixed. TODO is TODONE http://codereview.appspot.com/11250/diff/1/44 File java/gadgets/src/main/java/org/apache/shindig/gadgets/render/HtmlRenderer.java (right): http://codereview.appspot.com/11250/diff/1/44#newcode173 Line 173: // POST the preloaded content, with a method override of GET in this code path the original render request is the one used to cache and not the proxy one so while I think its good practice to set the override header its technically not necessary here. Can we make our custom headers constants on HttpRequest. http://codereview.appspot.com/11250/diff/1/44#newcode177 Line 177: .setPostBody(UTF8.encode(postContent).array()); set content-type and charset. http://codereview.appspot.com/11250/diff/1/23 File java/gadgets/src/test/java/org/apache/shindig/gadgets/JsFeatureLoaderTest.java (right): http://codereview.appspot.com/11250/diff/1/23#newcode20 Line 20: import static org.easymock.EasyMock.eq; import order http://codereview.appspot.com/11250/diff/1/5 File java/gadgets/src/test/java/org/apache/shindig/gadgets/http/DefaultHttpCacheTest.java (right): http://codereview.appspot.com/11250/diff/1/5#newcode75 Line 75: } Several of the removed tests exercise code that isnt exercised elsewhere as an individual unit. i.e expiry testing, error negative caching ... Should probably just be checked with mock request/response objects. http://codereview.appspot.com/11250
