RFR:8170868: DefaultProxySelector should use system defaults on Windows, MacOS and Gnome

[Chris Hegarty]

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.