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
