RFR: 8351969: Add Public Identifiers to the JDK built-in Catalog

[Mikhail Yankelevich]

On Thu, 13 Mar 2025 19:01:03 GMT, Joe Wang <joehw at openjdk.org> 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