Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(59)

Issue 823042: Consolidate all Proxy url params to ProxyUri

Can't Edit
Can't Publish+Mail
Start Review
Created:
16 years ago by zhoresh
Modified:
16 years ago
Reviewers:
johnfargo, shindig.remailer
Base URL:
http://svn.apache.org/repos/asf/shindig/trunk/
Visibility:
Public.

Description

The latest rewriters refactoring introduce new ProyUri class to handle proxy paramaters. Some parameters like image resiaze, fallback url, mime type etc, are not captured by it, and specifically handled by the proxy servlet. The change here add all the params to ProxyUri, and eliminate the usage of the original url by the proxy servlet/handler. It also make the proxy Uri responsible for the conversion to url parameter (it already did the parsing in the constructor) The change is not complete, I send it out for preliminary/design review. Update is coming soon. TODO: - Complete tests - Apply same rules for concat

Patch Set 1 #

Patch Set 2 : Attahced include added tes, clean up, and concat (which didn't really needed much) #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+373 lines, -137 lines) Patch
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java View 2 chunks +5 lines, -4 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/image/BasicImageRewriter.java View 2 chunks +5 lines, -4 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java View 2 chunks +3 lines, -2 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java View 1 6 chunks +10 lines, -46 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultConcatUriManager.java View 2 chunks +13 lines, -26 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManager.java View 1 chunk +2 lines, -14 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriBase.java View 1 7 chunks +105 lines, -28 lines 0 comments Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriManager.java View 1 3 chunks +94 lines, -4 lines 1 comment Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriCommon.java View 1 1 chunk +10 lines, -1 line 0 comments Download
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java View 2 chunks +2 lines, -1 line 0 comments Download
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultProxyUriManagerTest.java View 1 4 chunks +124 lines, -7 lines 0 comments Download

Messages

Total messages: 5
zhoresh
16 years ago (2010-03-29 16:58:01 UTC) #1
zhoresh
Attahced include added tes, clean up, and concat (which didn't really needed much)
16 years ago (2010-03-29 23:48:42 UTC) #2
johnfargo
The strategy generally looks good, though I admit the preponderance of query params starts calling ...
16 years ago (2010-03-30 06:19:35 UTC) #3
zhoresh
I started implementing using a map based. but it didn't look right to me with ...
16 years ago (2010-03-30 16:04:56 UTC) #4
johnfargo
16 years ago (2010-03-30 19:09:38 UTC) #5
On Tue, Mar 30, 2010 at 9:04 AM, Ziv Horesh <zhoresh@gmail.com> wrote:

> I started implementing using a map based. but it didn't look right to me
> with some parameters saved directly and some as map.
> I had to do more code to keep parameters in sync. Also the current code try
> to validate each param, so keeping direct value seems appropriate.
>

Seems like a reasonable argument. I'll remove the setter for now and commit.

Cheers,
John


>
> But now that you mention it, I might still want to add a map, for what we
> discussed as a service that support experimental params.
>
>
> On Mon, Mar 29, 2010 at 11:19 PM, <johnfargo@gmail.com> wrote:
>
>> The strategy generally looks good, though I admit the preponderance of
>> query params starts calling into question a more general Map-based
>> strategy. For the moment though, looks good. Thoughts?
>>
>>
>> http://codereview.appspot.com/823042/diff/9001/10005
>> File
>>
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriManager.java
>> (right):
>>
>> http://codereview.appspot.com/823042/diff/9001/10005#newcode90
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/ProxyUriManager.java:90:
>> public ProxyUri setFallbackUrl(String url) {
>> unused?
>
>
> All the set function are not really used at this point. They will be needed
> for a proxy service that has more global API then just url.
>
>
>>
>>
>> http://codereview.appspot.com/823042/show
>>
>
>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b