Hi Arno, this looks good. I had already reviewed your change.
Just as for one thing that Volker mentioned: >- DefaultProxySelector.java > >322 return proxyList == null ? Arrays.asList(Proxy.NO_PROXY) : >proxyList; > >Not sure if it would make sense to preallocate a static List with a single >Proxy.NO_PROXY element and always return that if proxyList equals null? I also have the concern that if you return one static instance of the List, then someone could modify it. Alternatively one could preallocate a static list and return Collections.unmodifiableList() from it. But I don't know if that makes things faster or is even more overhead compared to always generating a new List. Best regards Christoph > -----Original Message----- > From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of Zeller, > Arno > Sent: Mittwoch, 21. Dezember 2016 13:53 > To: Volker Simonis <volker.simo...@gmail.com> > Cc: net-dev@openjdk.java.net > Subject: RE: RFR:8170868: DefaultProxySelector should use system defaults on > Windows, MacOS and Gnome > > Hi Volker, > > thanks for your valuable comments! I have a new patch ready that should > address your issues and contains also a forgotten change to the map file... > > New webrev: http://cr.openjdk.java.net/~clanger/webrevs/8170868.1/ > > > >- make/lib/NetworkingLibraries.gmk > ... > >Have you tried to use > >LIBNET_EXCLUDE_FILES := > >$(JDK_TOPDIR)/src/java.base/unix/native/libnet/DefaultProxySelector.c > > > >I think this should work and it would mke it possible to rename: > >src/java.base/macosx/native/libnet/DefaultProxySelector_mac.c > >to: > >src/java.base/macosx/native/libnet/DefaultProxySelector.c > >which is much nicer IMHO :) > > Great idea - it works and is of course the much nicer solution! > > >- DefaultProxySelector.java > > > >322 return proxyList == null ? Arrays.asList(Proxy.NO_PROXY) : > >proxyList; > > > >Not sure if it would make sense to preallocate a static List with a single > >Proxy.NO_PROXY element and always return that if proxyList equals null? > > I return a new List object each time, because the select(URI uri) method does > not state anything about > not modifying the returned list. In case I return a static list containing > only the > NO_PROXY element > a caller could remove the object from the list and other caller would use the > same modified list. > To avoid this I always create a new List object. > > >- java.base/unix/native/libnet/DefaultProxySelector.c > > > >You've removed "#include <strings.h>". Have you built on all Unix platforms > >(AIX, Solaris) to make sure you don't break anything? > > It compiled on Linux, AIX, Solaris and Mac without problems for me. > > >- java.base/windows/native/libnet/DefaultProxySelector.c > > > >Not sure if I understand this right, but in the gconf case above you insert > >all > >proxies returned by "g_proxy_resolver_lookup" into the prox-list returned by > >DefaultProxySelector_getSystemProxies. In the Windows case you write: > > > > 247 * From MSDN: The proxy server list contains one or more of > >the following strings separated by semicolons or whitespace. > > 248 * ([<scheme>=][<scheme>"://"]<server>[":"<port>]) > > 249 * We will only take the first entry here because the > >java.net.Proxy class has only one entry. > > > >Why can't you build up a proxy list here in the same way you do it for the > >gconf case on Unix? > > Sorry - I just forgot to implement it. Good that you found it. The new webrev > contains the missing functionality. > > >- src/java.base/macosx/native/libnet/DefaultProxySelector_mac.c > > > > 76 #define kResolveProxyRunLoopMode > >CFSTR("com.sap.jvm.DefaultProxySelector") > > > > > >I'm not familiar with the Mac programming model, but I don't think > >"com.sap.jvm.DefaultProxySelector" is a good name for > >kResolveProxyRunLoopMode. It should be something like > >"java.net.DefaultProxySelector" but I'm open for better proposals :) > > You are right - I changed it to "sun.net.spi.DefaultProxySelector". > > >PS: for your next RFR, can you please also add the estimated sisze and the > >bug > >id to the subject line (e.g. "RFR(M): > >8170868:DefaultProxySelector should..."). This makes it much easier to find a > >review thread :) > > I'll do my best next time... > > Best regards, > Arno > > >On Wed, Dec 14, 2016 at 5:06 PM, Zeller, Arno <arno.zel...@sap.com> wrote: > >> Hi, > >> > >> can you please review my proposal for bug 8170868 - DefaultProxySelector > >should use system defaults on Windows, MacOS and Gnome. > >> > >> Bug: > >> https://bugs.openjdk.java.net/browse/JDK-8170868 > >> > >> Webrev: > >> http://cr.openjdk.java.net/~clanger/webrevs/8170868.0/ > >> > >> Thanks a lot, > >> Arno Zeller > >>
