Hi Arno,
you change looks good. Thanks for addressing this long standing issue!
Please find some minor comments below:
- make/lib/NetworkingLibraries.gmk
28 ifeq ($(OPENJDK_TARGET_OS), macosx)
29 LIBNET_EXCLUDE_FILES := DefaultProxySelector.c
30 endif
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 :)
- 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?
- 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?
- 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?
- 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 :)
Thank you and best regards,
Volker
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 :)
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
>
