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

[Joe Wang]

On 12/7/2012 3:37 PM, Mandy Chung wrote:
> On 12/7/12 3:24 PM, Joe Wang wrote:
>> 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));
>>         }
>
> OK - you leave it for XMLWriter to check if it's a valid encoding.  It 
> may be better to include Charset.forName check in the 
> isEncodingSupporting method that would avoid confusion.

Sure. Will move this code into isEncodingSupported and change the method 
name to getCharset so that it'd return the charset if the encoding is 
not UTF32 and supported by the jdk.

>
>>>
>>> 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.
>>
> OK - PropertiesDefaultHandler uses the XMLStreamWriter API.  That's 
> fine with me.

Thanks,
Joe

>
> Thanks
> Mandy