On Thu, 13 Mar 2025 19:01:03 GMT, Joe Wang <[email protected]> wrote:
> Add public identifiers to the JDK built-in Catalog; Replace the incorrect
> Schema 1.1 DTD files (note the Public Identifier at line 2) with the correct
> Shema 1.0 DTDs.
Looks good to me, just a few very minor questions. Thank you
test/jaxp/javax/xml/jaxp/unittest/common/jdkcatalog/JDKCatalogTest.java line 51:
> 49: * @test
> 50: * @bug 8344800 8345353 8351969
> 51: * @modules java.xml/jdk.xml.internal
I know that this is not a part of your change, but this (line 56) doesn't seem
to be used at all. Could you please remove it if there will be another
revision? If not it's ok as is 😄
`static String CLS_DIR = System.getProperty("test.classes");`
test/jaxp/javax/xml/jaxp/unittest/common/jdkcatalog/JDKCatalogTest.java line 71:
> 69: static final String ROOT_ELEMENT = "{{rootElement}}";
> 70:
> 71: Catalog jdkCatalog;
Nitpick: if this will stay as a global var I'd personally make it private and
move to the top of the file. What do you think?
test/jaxp/javax/xml/jaxp/unittest/common/jdkcatalog/JDKCatalogTest.java line 78:
> 76: */
> 77: @DataProvider(name = "DTDsInJDKCatalog")
> 78: public Object[][] getDTDsInJDKCatalog() throws Exception {
Nit: Don't think `throw exception` is doing anything here.
test/jaxp/javax/xml/jaxp/unittest/common/jdkcatalog/JDKCatalogTest.java line
143:
> 141: // initialize JDKCatalog
> 142: JdkCatalog.init("continue");
> 143: jdkCatalog = JdkCatalog.catalog;
This seems to be a set up just for 1 test. Do you think it might be better to
have this logic in the beginning of the `testDTDsInJDKCatalog`?
test/jaxp/javax/xml/jaxp/unittest/common/jdkcatalog/JDKCatalogTest.java line
155:
> 153: @Test(dataProvider = "DTDsInJDKCatalog")
> 154: public void testDTDsInJDKCatalog(String publicId, String systemId)
> 155: throws Exception {
Nit: Don't think `throw exception` is doing anything here.
-------------
PR Review: https://git.openjdk.org/jdk/pull/24039#pullrequestreview-2685934886
PR Review Comment: https://git.openjdk.org/jdk/pull/24039#discussion_r1995807656
PR Review Comment: https://git.openjdk.org/jdk/pull/24039#discussion_r1995788572
PR Review Comment: https://git.openjdk.org/jdk/pull/24039#discussion_r1995810396
PR Review Comment: https://git.openjdk.org/jdk/pull/24039#discussion_r1995785004
PR Review Comment: https://git.openjdk.org/jdk/pull/24039#discussion_r1995810329
