[icedtea-web] PR1474: Can't get javaws to use SOCKS proxy

[Omair Majid]

Hi Jiri,

Thanks for the review!

On 09/24/2013 01:09 PM, Jiri Vanek wrote:
> On 09/23/2013 07:46 PM, Omair Majid wrote:
>> The attached patch improves how manual SOCKS proxy configuration is
>> handled. Specifically, if there is a socks proxy specified, it is used
>> even for http(s) and ftp, unless a more specific proxy for those is
>> specified. 'sameProxy' now affects the https/http/ftp protocols, but not
>> the socket protocol.

>> +++ b/netx/net/sourceforge/jnlp/config/DeploymentConfiguration.java

>>       /** the proxy type. possible values are {@code
>> JNLPProxySelector.PROXY_TYPE_*} */
>>       public static final String KEY_PROXY_TYPE =
>> "deployment.proxy.type";
> 
> I would add an empty line here , just for readibility

Fixed.

>> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPProxySelector.java

>> -        } else if (scheme.equals("http")) {
>> +        } else if (scheme.equals("http")&&  proxyHttpHost != null) {
> 
> There should be spaces before &&, but I do not inissts.

Fixed now.

> Is the null
> check really wonted? Is not NPE better ?

I would rather handle (well, ignore) a missing configuration rather than
crashing on it. If you look in the bug, the reporter is editing the
configuration files in ways that will pretty much break things if I
don't handle nulls here.

>> +        if (proxySocks4Host != null) {
>>               SocketAddress sa = new
>> InetSocketAddress(proxySocks4Host, proxySocks4Port);
>>               proxies.add(new Proxy(Type.SOCKS, sa));
>> -        } else {
> 
> Is this removed else intentional? " but not the socket protocol" ?

Yes. This code below acts like the 'else':

>> +        }
>> +
>> +        if (proxies.size() == 0) {
>>               proxies.add(Proxy.NO_PROXY);
>>           }

Basically, we add a socks proxy if there's one defined, no matter
whether a http(s)/ftp proxy is defined or not. And if there's no socks
or http(s) or ftp proxy, then return NO_PROXY.

>> +++
>> b/tests/netx/unit/net/sourceforge/jnlp/runtime/JNLPProxySelectorTest.java

>> +    // TODO implement this
>> +    @Ignore("Implement this")
> hmhm - how did test suite survived ignored test?

My guess is that 'make check' uses junit proper (no custom hacks) to run
the test suite and @Ignore works as expected.

> /me probably never
> tested. But I think if xmls/htmls/dailyreport will crash on ot, then it
> is best reason to finally fix them :)

I see that tests/report-styles/jreport.xsl already handles @Ignore. Is
there anything else I need to check?

> Known to fial is not option here?

I don't think it fails as much as it is not implemented at all.

> Otherwise good enough to go. Are you planing also 1,4 backport?

Thanks for the review! At this point, I am not planning to backport to
1.4. Maybe once I have the rest of proxy stuff under test, maybe.

Thanks,
Omair
-- 
PGP Key: 66484681 (http://pgp.mit.edu/)
Fingerprint = F072 555B 0A17 3957 4E95  0056 F286 F14F 6648 4681
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr1474-02.patch
Type: text/x-patch
Size: 5399 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130924/4b5d0c30/pr1474-02.patch