Hi Chris, thanks for your comments! I adjusted the change to contain your suggestions and slightly modified src/java.base/share/classes/java/net/doc-files/net-properties.html to refer to macOS . Also found some missing adjustments of the copyright year myself...
>Some smaller comments and observations ( I'm still reviewing and testing ): ... >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. Removed reference to g_strv_length - I simply forgot to delete it after changing my code to iterate over the list. > Nit: L 167 / 358 parameter indentation Fixed. >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. I would like to keep the current approach because it avoids the need of locking to ensure that the initialization is done only by one thread. > The indentation of the main body of XXX_getSystemProxies > could be reduced a little if: > if (WinHttpGetIEProxyConfigForCurrentUser(&ie_proxy_config) == FALSE) > return NULL; Done. >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; Done. >5) I personally prefer the code of the old createProxy. Was there > specific reason why you changed it? Sorry for this - another leftover from a former change. I reverted back to the original. >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. Done. >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 > */ Done. The new webrev can be found at: http://cr.openjdk.java.net/~clanger/webrevs/8170868.4/ Best regards, Arno
