Thanks for the comments Alan… On 30 Jan 2015, at 14:32, Alan Bateman <alan.bate...@oracle.com> wrote:
> On 30/01/2015 13:36, Chris Hegarty wrote: >> This is phase 1, of getting java.net.URL work with modules. >> >> Being able to effectively specify URL protocol handler factories as fully >> qualified class names, through the 'java.protocol.handler.pkgs' system >> property is problematic. It requires the protocol handler factory >> implementation class to be public and accessible, as the current >> implementation tries to instantiate it through core reflection. Since the >> protocol handler factory must be an implementation of a >> URLStreamHandlerFactory, it lends itself to being retrofitted with a >> ServiceLoader lookup. Note, the 'java.protocol.handler.pkgs' system property >> mechanism predates ServiceLoader. URL protocol handlers would most likely >> have used service loader if it were available at the time. >> >> Some URL protocol handlers are fundamental to the platform itself, like >> 'file' and 'jar', it is not appropriate to attempt a service loader lookup >> for these, as they may lead to recursive initialization issues. However, >> Java Plugin, Webstart, and possibly other containers, do override the 'jar' >> handler. A new API will be provided for this purpose. Providing an API >> solution should not interfere with system initialization as it will only be >> called after the system comes up and user code is executing. >> >> The 'file' protocol handler factory will no longer be overridable, and the >> system property will no longer be consulted. >> >> Webrev and spec diffs: >> http://cr.openjdk.java.net/~chegar/8064924/00/ >> >> I want to agree the approach and spec first, so that the spec changes can be >> submitted for approval. A comprehensive test will be added before the code >> changes are finalised. >> >> > I've had a number of off-list discussions with Chris on this and I think the > proposal and the ordering is good. It preserves behavior for those setting > the system-wide URLStreamHandlerFactory with setURLStreamHandlerFactory and > only really creates a migration issue for those setting the JDK 1.0/1.1 era > pkgs property property. > > One thing that I wasn't aware of is the hairy in SAAJ. That is some thing > that will need to coordinated with upstream and hopefully Miroslav can help > on that. As upstream might still have a constraint that the code compile with > JDK 7 and JDK 8 then some adjustments might be required. Right. My changes are mainly a placeholder to remind me to revisit this. I suspect this code will continue to set the system property and call the new method reflectively, if available. I’ll agree this approach with Miran before finalising the changes. > In any, the focus now is agreeing the API changes before digging into the > code changes. > > I've read through the javadoc and it looks good. A couple of suggestions: > > - step 2: alternative wording: "The createURLStreamHandler method of each > factory is invoked, in registration order, with the protocol string, until a > factory returns non-null”. Thanks, updated. > - step 3: "ServiceLoader mechanism is used" might be better than > "ServiceLoader mechanism tries”. Updated. > - step 3: "ServiceConfigurationErrors", maybe this should be singular as the > first will cause the search to bail. Related to this is > createURLStreamHandler terminating with an error or runtime exception, you > may want to mention that too. I added a note about this. > In the @apiNote in addURLStreamHandlerFactory then "containers" might not be > understood by the reader. I don't have an alternative but maybe make it clear > that this is for system-wide configuration and not intended to be used by > transient applications. I made a change to indicate that this is system-wide. Update webrev and spec diffs: http://cr.openjdk.java.net/~chegar/8064924/01/ -Chris. > -Alan
