Arno,
On 11/01/17 14:21, Zeller, Arno wrote:
Hi Chris,
I have addressed your comments in a new webrev. Can you please have a look at
this one?
http://cr.openjdk.java.net/~clanger/webrevs/8170868.3/
This is looking much better, thank you.
Some smaller comments and observations ( I'm still reviewing
and testing ):
1) With this change there may be network connections made
to support WPAD and retrieve PAC files. This should be
fine as system proxy settings are coming from a trusted
source, i.e. the system.
2) unix/native/libnet/DefaultProxySelector.c
You have changes to make g_strv_length available, but then
count the proxies in the list manually. Maybe just use
g_strv_length.
Nit: L 167 / 358 parameter indentation
3) windows/native/libnet/DefaultProxySelector.c
The HINTERNET session could be set lazily, for the case
where there are no auto proxy settings. Not sure if it
worth it though.
The indentation of the main body of XXX_getSystemProxies
could be reduced a little if:
if (WinHttpGetIEProxyConfigForCurrentUser(&ie_proxy_config) == FALSE)
return NULL;
4) macosx/native/libnet/DefaultProxySelector.c
The indentation of the main body of XXX_getSystemProxies
could be reduced a little if:
proxyDicRef = CFNetworkCopySystemProxySettings();
if (proxyDicRef == NULL)
return NULL;
5) I personally prefer the code of the old createProxy. Was there
specific reason why you changed it?
6) Can you generally check the line lengths in all files, and try
where possible to keep it below 100, e.g. some comments could
easily be wrapped.
7) I would like to add jtreg tags to test/java/net/ProxySelector/
SystemProxies.java to have it run in any automated test systems.
There would be no failure mode of this test, as it requires
manually inspecting the output, but it would at least give some
coverage to this new code. Something like:
/*
* @test
* @bug 8170868
* @summary Basic test to provide some coverage of system proxy
code. Will
* always pass. Should be run manually for specific systems to
inspect output.
* @run/othervm -Djava.net.useSystemProxies=true SystemProxies
*/
-Chris.
