Committing this CL; cleanup soonish. On Tue, Mar 2, 2010 at 9:00 AM, <[email protected]> wrote:
> LGTM > I see common code in the manager and the concat manager. > I think you can create a base manager for that code. > > > > http://codereview.appspot.com/217110/diff/3009/3010 > > File > > java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java > (right): > > http://codereview.appspot.com/217110/diff/3009/3010#newcode50 > > > java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:50: > * > > http://www.example.com/gadgets/proxy/refresh=1&.../http://www.foo.com/img.gif > Add the start and end section markers in the example > Added. > > http://codereview.appspot.com/217110/diff/3009/3010#newcode120 > > java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:120: > } > Maybe put common code in basic manager: getBaseUriBuilder() > Excellent idea; will do in cleanup. > > http://codereview.appspot.com/217110/diff/3009/3010#newcode227 > > java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java:227: > private String getReqConfig(String container, String key) { > This seems like a repeated code, maybe create a base uri manager. > Same. > > http://codereview.appspot.com/217110/diff/3009/3012 > > File > > java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManagerTest.java > (right): > > http://codereview.appspot.com/217110/diff/3009/3012#newcode79 > > java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManagerTest.java:79: > String path = "/proxy/" + DefaultProxyUriManager.CHAINED_PARAMS_TOKEN + > "/path"; > Test also the case that CHAINED_PARAM_TOKEN is at the end of the path. Provided @line86 and following. > > > http://codereview.appspot.com/217110/show >
