> On Oct. 10, 2012, 6:34 a.m., Henry Saputra wrote:
> > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java,
> >  line 171
> > <https://reviews.apache.org/r/7309/diff/3/?file=175008#file175008line171>
> >
> >     Do we need else if over here? I think the if checks will guarantee that 
> > the Content-Disposition header is valid?

Yes, you are right, we don't need the if here. I will fix it. 


> On Oct. 10, 2012, 6:34 a.m., Henry Saputra wrote:
> > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java,
> >  line 168
> > <https://reviews.apache.org/r/7309/diff/3/?file=175008#file175008line168>
> >
> >     Can we change this check with StringUtils.isBlank instead of "== null" 
> > just in case the response origin server return empty or blank 
> > Content-Disposition header?

In theory, we don't need to check the empty because it will be checked by the 
following "contentDispositionValue.indexOf("attachment;") == -1". But I guess 
use it does no harm.  Will update the patch with your suggestion. Thanks, Henry.


- Erik


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7309/#review12306
-----------------------------------------------------------


On Oct. 10, 2012, 7:03 a.m., Erik Bi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7309/
> -----------------------------------------------------------
> 
> (Updated Oct. 10, 2012, 7:03 a.m.)
> 
> 
> Review request for shindig, Ryan Baxter, Dan Dumont, Stanton Sievers, and 
> Rich Thompson.
> 
> 
> Description
> -------
> 
> Problem 1: Shindig proxy doesn't respect the "content-Disposition" header 
> correctly, so when user click a button to "download" the file via shindig 
> proxy use, the file name in the pop-up dialog is always "p.txt". Here code is 
> updated to copy the right Content-Disposition header. 
> 
> Problem 2: Even with the right Content-Disposition header, IE8 handles 
> Content-Disposition header differently.
> 
> IE8 doesn't handle the Content-Disposition header like "Content-Disposition = 
> attachment; filename*= UTF-8''%e2%82%ac%20rates" correctly (should be a 
> limitation of IE8). 
> 
> Even with the right Content-Disposition header,  when you open click a button 
> to "download" the file via shindig proxy, you will get a message box asking 
> if you want to open or save this file. At that time file name displayed will 
> not be the one in the header, but the path name of the current urlstart, in 
> Shindig, it will probably be "proxy."
> 
> In order to fix, append a file name on proxy url and ignore that at the proxy 
> server side. 
> 
> For example, when you call 
> gadgets.io.getProxyUrl("http://target.example.com/image.gif";);
> it will get result like 
> http://host/proxy/image.gif?url=http%3a%2f%2ftarget.example.com%2fimage.gif"; +
>           "&refresh=3600" +
>           "&g=http%3a%2f%2fwww.gadget.com%2fgadget.xml" +
>           "&c=foo"
> 
> It will not effect any existing code
> 
> 
> This addresses bug shindig-1875.
>     https://issues.apache.org/jira/browse/shindig-1875
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/shindig/trunk/config/container.js 1369611 
>   
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/core.io/feature.xml
>  1383008 
>   
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/core.io/io.js
>  1395276 
>   
> http://svn.apache.org/repos/asf/shindig/trunk/features/src/test/javascript/features/core.io/iotest.js
>  1366998 
>   
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
>  1366998 
> 
> Diff: https://reviews.apache.org/r/7309/diff/
> 
> 
> Testing
> -------
> 
> Add Test case in iotest.js
> 
> 
> Thanks,
> 
> Erik Bi
> 
>

Reply via email to