Re: RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)

2013-01-11 Thread Daniel Fuchs
On 1/9/13 9:30 PM, Mandy Chung wrote: L152: would it be better to replace the base service class name with the classname (i.e. javax.xml.XMLEventFactory) 152* If {@code factoryId} is the base service class name, 153* use the service-provider loading facilities, defined by the

Re: RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)

2013-01-11 Thread Mandy Chung
On 1/11/2013 5:59 AM, Daniel Fuchs wrote: Hi Mandy, This is done. I have updated the webrev: http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.stream/webrev.05/ typo in: 242* If {@code factoryId} is javax.xml.stream.XMLIntputFactory, Otherwise, looks good. Thanks for

Re: RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)

2013-01-11 Thread Daniel Fuchs
Thanks Mandy! Good catch! I've updated the webrev: http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.stream/webrev.06/ -- daniel On 1/11/13 6:23 PM, Mandy Chung wrote: On 1/11/2013 5:59 AM, Daniel Fuchs wrote: Hi Mandy, This is done. I have updated the webrev:

Re: RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)

2013-01-09 Thread Alan Bateman
On 08/01/2013 12:20, Daniel Fuchs wrote: : I've clarified the spec. I also documented an additional step, which was done by the implementation but not documented - which is that the stream factory will also look up for properties in jaxp.properties if stax.properties was not found. webrev:

Re: RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)

2013-01-09 Thread Daniel Fuchs
Thanks for the comments Alan! I have implemented them and refreshed the webrev for the record: http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.stream/webrev.04/ -- daniel On 1/9/13 4:43 PM, Alan Bateman wrote: On 08/01/2013 12:20, Daniel Fuchs wrote: : I've clarified the spec. I

Re: RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)

2013-01-09 Thread Alan Bateman
On 09/01/2013 17:26, Daniel Fuchs wrote: Thanks for the comments Alan! I have implemented them and refreshed the webrev for the record: http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.stream/webrev.04/ -- daniel You've addressed my comments so thumbs up from me on this chapter.

Re: RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)

2013-01-09 Thread Mandy Chung
On 1/9/2013 9:26 AM, Daniel Fuchs wrote: http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.stream/webrev.04/ Daniel - thanks for updating the spec for the newFactory(String, ClassLoader) method. In XMLEventFactory.java (and same comment apply to XMLInputFactory and

Re: RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)

2013-01-09 Thread Daniel Fuchs
Hi Mandy, Please find clarifications in line: On 1/9/13 9:30 PM, Mandy Chung wrote: On 1/9/2013 9:26 AM, Daniel Fuchs wrote: http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.stream/webrev.04/ Daniel - thanks for updating the spec for the newFactory(String, ClassLoader) method.

Re: RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)

2013-01-09 Thread Mandy Chung
On 1/9/2013 1:13 PM, Daniel Fuchs wrote: Hi Mandy, Please find clarifications in line: On 1/9/13 9:30 PM, Mandy Chung wrote: Since there is a behavioral change, the following statements are not true any more and I think they can be removed. 166* No changes in behavior are defined

Re: RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)

2013-01-08 Thread Daniel Fuchs
On 1/8/13 5:13 AM, Mandy Chung wrote: Hi Daniel, I also agree with option 3 be the best option among them. Joe's suggestion to throw an exception if factoryId is not the base service type is good so that any existing application depending on that behavior will catch this change. Are you going

Re: RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)

2013-01-07 Thread Daniel Fuchs
Thanks Joe. To make things clear I have pushed a revised webrev with solution 3. http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.stream/webrev.02.3/ The lines of interest are in FactoryFinder.java - right hand side: 267 if (type.getName().equals(factoryId)) { 268

Re: RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)

2013-01-07 Thread Mandy Chung
Hi Daniel, I also agree with option 3 be the best option among them. Joe's suggestion to throw an exception if factoryId is not the base service type is good so that any existing application depending on that behavior will catch this change. Are you going to revise the spec - there is

Re: RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)

2013-01-04 Thread Daniel Fuchs
Hi guys, Happy new year to you all! And apologies for this long email :-) I think we still haven't converged on this issue in javax.xml.stream - so let me make a recap. The issue is for the newInstance/newFactory methods that take a factoryId parameter, in the factories for the

Re: RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)

2013-01-04 Thread Joe Wang
Hi Daniel, Yes, I agree with 3. As I said before, we should return an error if factoryId != type.getName() since it indicates a configuration error. Scenario 4 does exist, but it's beyond the current spec. Such an impl should not use the default API. The StAX spec is not always clear. My

Re: RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)

2012-12-19 Thread Daniel Fuchs
On 12/19/12 12:10 AM, Joe Wang wrote: It's different. If 'foo.bar' is specified but not found, it indicates a configuration error. If the factory falls back to an impl by the default factory id, it would serve to hide the error. Yes - I fully agree with that. Note that newInstance/newFactory

Re: RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)

2012-12-18 Thread Paul Sandoz
On Dec 18, 2012, at 7:02 AM, Mandy Chung mandy.ch...@oracle.com wrote: Hi Daniel, There are currently several different FactoryFinder class in the JAXP implementation and there might be slight difference in each of them. Do you know if it has been looked into whether it's feasible to

Re: RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)

2012-12-18 Thread Daniel Fuchs
Hi, Thanks for the review. I updated the webrev to keep track of your suggested change. http://cr.openjdk.java.net/~dfuchs/JDK-7169894/javax.xml.stream/webrev.01/ -- daniel On 12/18/12 4:00 PM, Alan Bateman wrote: I looked through this installment and aside from an aside from an alignment

Re: RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)

2012-12-18 Thread Joe Wang
Hi Daniel, The call: T provider = findServiceProvider(type) in static T T find(ClassT type, String factoryId, ClassLoader cl, String fallbackClassName) ignored factoryId, and assumed it's the same as type.getName(). I looked back I had the same bug in my original patch. -Joe On 12/18/2012

Re: RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)

2012-12-18 Thread Daniel Fuchs
On 12/18/12 6:29 PM, Joe Wang wrote: Hi Daniel, The call: T provider = findServiceProvider(type) in static T T find(ClassT type, String factoryId, ClassLoader cl, String fallbackClassName) ignored factoryId, and assumed it's the same as type.getName(). I looked back I had the same bug in my

Re: RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)

2012-12-18 Thread Joe Wang
On 12/18/2012 10:00 AM, Daniel Fuchs wrote: On 12/18/12 6:29 PM, Joe Wang wrote: Hi Daniel, The call: T provider = findServiceProvider(type) in static T T find(ClassT type, String factoryId, ClassLoader cl, String fallbackClassName) ignored factoryId, and assumed it's the same as

Re: RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)

2012-12-18 Thread Mandy Chung
On 12/18/12 1:09 AM, Paul Sandoz wrote: On Dec 18, 2012, at 7:02 AM, Mandy Chungmandy.ch...@oracle.com wrote: Hi Daniel, There are currently several different FactoryFinder class in the JAXP implementation and there might be slight difference in each of them. Do you know if it has been

Re: RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)

2012-12-18 Thread Joe Wang
On 12/18/2012 2:23 PM, Daniel Fuchs wrote: On 12/18/12 8:00 PM, Joe Wang wrote: On 12/18/2012 10:00 AM, Daniel Fuchs wrote: On 12/18/12 6:29 PM, Joe Wang wrote: Hi Daniel, The call: T provider = findServiceProvider(type) in static T T find(ClassT type, String factoryId, ClassLoader cl,

RFR: javax.xml.stream: Using ServiceLoader to load JAXP stream factories (7169894: JAXP Plugability Layer: using service loader)

2012-12-17 Thread Daniel Fuchs
Hi, Here is a new webrev in the series that addresses using ServiceLoader in JAXP for JDK 8. 7169894: JAXP Plugability Layer: using service loader This changeset addresses modification in the javax.xml.stream package. It is similar to changes proposed for the javax.xml.parsers package [1],