On Fri, 28 Apr 2023 05:47:24 GMT, Joe Wang <jo...@openjdk.org> wrote:
>> Add a system property, jdk.xml.config.file, to return the path to a custom >> JAXP configuration file. The current configuration file, jaxp.properties, >> that the JDK supports will become the default configuration file. >> >> CSR: https://bugs.openjdk.org/browse/JDK-8303531 >> >> Tests: XML SQE and JCK tests passed. > > Joe Wang has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains ten additional commits since > the last revision: > > - Merge branch 'master' into JDK-8303530 > - Update based on the review meeting on 4/5. Added (duplicated) definitions > in jaxp.properties; Rewrote Property Precedence with samples; Other updates. > - update javadoc as discussed in the review meeting; add a sample > configuration file jaxp.properties; update impl and tests accordingly. > - continue support stax.properties at the impl level, though dropped from > the spec > - change prefix from jdk to java > - change prefix from jdk to java; rm zip file that accidentally checked in > - update config file description; add a general scope and order section; > move value definition for external properties to class description > - clean up tests; fix copy&paste error. > - 8303530: Add system property for custom JAXP configuration file src/java.xml/share/classes/module-info.java line 33: > 31: * JAXP supports the use of a configuration file for the > 32: * <a href="#LookupMechanism">Factory Lookup Mechanism</a> and > 33: * setting properties that have defined corresponding system properties. The first sentence of the module description lists the 4 XML APIs and their acronyms. It is immediately followed by a section with title "JAXP Configuration File" which suggests that this is a configuration file for the first API that is listed (JAXP). As I understand it, properties for the streaming API can also be defined in this file. So it might be that a bit more setup is needed in the opening text to make it clearer that this is the configuration file for all of the APIs. src/java.xml/share/classes/module-info.java line 61: > 59: * By default, the <a href="#Factories">JAXP Factories</a> will look for a > 60: * configuration file called {@code jaxp.properties} in the {@code conf} > directory > 61: * of the Java installation and use the entries if any to customize the > behavior "of the Java installation" should probably say the run-time image, or maybe just change this to use ${java.home}/conf as it is used in other API docs. src/java.xml/share/classes/module-info.java line 74: > 72: * <h3 id="CF_SP">User-defined Configuration File</h3> > 73: * A user-defined configuration file can be set outside of the JDK by > using the > 74: * system property {@code java.xml.config.file}. "can be set out of the JDK" needs to be re-phrased. Maybe this sentence can be restructured to say that a system property can be set on the command line to specify the location of a configuration file on the file system. Adding "command line" would be helpful because "system property" is too overloaded in these docs. src/java.xml/share/classes/module-info.java line 257: > 255: * the {@link javax.xml.XMLConstants#FEATURE_SECURE_PROCESSING > FEATURE_SECURE_PROCESSING} > 256: * (hereafter referred to FSP), and the default values. The order of > precedence > 257: * for the configuration sources is defined as follows, with earlier > ones overriding the later: "using the API properties" is bit confusing. I think you mean that properties can be set via the APIs. ".. and the default values". I think it would be clearer to say that properties have default values when not set by previous four ways. src/java.xml/share/classes/module-info.java line 264: > 262: * </li> > 263: * <li><p> > 264: * Properties set using the corresponding System properties; Can you confirm that this means system properties set on the command line with -Dkey=value or System.setProperty(key, value)? (I understand that the catalog feature "RESOLVE" property helps to explain it but I'm concerned that "system property" is ambiguous throughout). src/java.xml/share/classes/module-info.java line 332: > 330: * @implNote > 331: * > 332: * <h2 id="ConfigurationFile">JAXP Configuration File</h2> I thin this means there are two anchors with the same name and two sections in the module description with the same title. I understand this is SE vs. JDK specific properties but this could easily go over the headers of someone reading this. I think it would be better if "Implementation Specific Features and Properties" were the first section of the implNote. It can start by saying that the JAXP Configuration file (link to the description at the top) can also be used for JDK/implementation specific properties. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/12985#discussion_r1181179391 PR Review Comment: https://git.openjdk.org/jdk/pull/12985#discussion_r1181179908 PR Review Comment: https://git.openjdk.org/jdk/pull/12985#discussion_r1181180282 PR Review Comment: https://git.openjdk.org/jdk/pull/12985#discussion_r1181181311 PR Review Comment: https://git.openjdk.org/jdk/pull/12985#discussion_r1181181811 PR Review Comment: https://git.openjdk.org/jdk/pull/12985#discussion_r1181183010
