8004371: (props) Properties.loadFromXML needs small footprint XML parser as fallback when JAXP is not present

[Joe Wang]

Thanks for reviewing the changes.  Please see comments inline.

On 12/7/2012 11:38 AM, Mandy Chung wrote:
> On 12/5/12 10:58 AM, Alan Bateman wrote:
>> http://cr.openjdk.java.net/~alanb/8004371/webrev/
>>
>
> Yay!  Properties no longer requires JAXP to be present in order to 
> load/store properties in XML format.  This looks okay to me.  Some 
> minor comments:
>
> XMLStreamWriterImpl.isEncodingSupported - it returns true if any 
> string != "UTF-32" that assumes the given encoding is one of the few 
> known values.  Would it be better to check the explicit list of 
> supported encoding?
>     private boolean isEncodingSupported(String encoding) {
>         if (encoding.equalsIgnoreCase("UTF-32")) {
>             return false;
>         }
>         return true;
>     }

The spec only requires UTF-8 and 16. But the writer can actually handle 
most of the encodings. An explicit list would be quite long and require 
comprehensive tests. Since the ukit parser explicitly rejects UTF-32, I 
chose to do the same.  Other than that, the XMLWriter leaves it to 
java.nio.charset.Charset to determine if a specified encoding is 
supported through this call:
        try {
             cs = Charset.forName(encoding);
         } catch (IllegalCharsetNameException | 
UnsupportedCharsetException ex) {
             throw new XMLStreamException(new 
UnsupportedEncodingException(encoding));
         }
>
> PropertiesDefaultHandler.java L115-116: can be replaced with 
> Properties.stringPropertyNames() and foreach.

Done. Will send update to Alan.

>
> XMLStreamWriterImpl.java L156 - since the caller needs to unwrap 
> XMLStreamException to determine if the encoding is supported or not, 
> maybe better to throw this constructor to throw 
> UnsupportedEncodingException.

The writer implements partially the original StAX API.  So I tried not 
to change the writer API. But java.util.Properties expect 
UnsupportedEncodingException in case of invalid encoding, thus the 
compromise.

>
> Perhaps s/SAXParserImp/SAXParserImpl be consistent with the other 
> classname e.g. XMLStreamWriterImpl.

SAXParserImp was from UKit, but I followed StAX's convention for 
XMLStreamWriterImpl :)  I'll rename  SAXParserImp to SAXParserImpl.

>
> As you mentioned, there is still a good bit of clean-up required and I 
> skip the style and coding convention comments.  One question though - 
> jdk.internal.org.xml.sax.** are copied from JAXP source.  What should 
> the copyright years be?  It currently retains the same value from the 
> original copy.

I thought the rule was that the copyright years be retained if there's 
no material change.  But I saw Alan's comment that it'd be done by a 
script. So I'll leave it as is for now.

Thanks,
Joe

>
>> One issue that I'm still mulling over, as least for the profiles 
>> effort, is whether to only include the fallback provider in the 
>> smallest profile as it won't be used otherwise. If the eventual size 
>> isn't too significant then it might not be worth it of course.
>
> Do you plan to include 
> jdk.internal.util.xml.BasicXmlPropertiesProvider in 
> META-INF/services/sun.util.spi.XmlPropertiesProvider?
>
> I guess you are referring to what makefile change should be depending 
> on the decision?
>
> Mandy