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

[Jiri Vanek]

On 09/24/2013 07:19 PM, Omair Majid wrote:
> 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.

Well, maybe some debug information at least?

>
>>> +        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?

If you have seen xml+html and it was ok, then it is more then enough. Otherwise I will be looking forward to daily robot post processing :)
>
>> 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.
>

Fair enough. Ok to push then.


J.