RFR (JAXP): 8144966: Catalog API: Null handling and reference to Reader

[Mandy Chung]

> On Dec 23, 2015, at 2:55 PM, huizhe wang <huizhe.wang at oracle.com> wrote:
> 
> Hi,
> 
> This is an improvement to the new Catalog API on null handling. At the package level, a null argument shall result in NPE. However, an exception was made to the methods in CatalogManager so that they can used conveniently. With this request, we're in favor of consistency over the minor convenience, which means a resolver can no longer be created by:
>         CatalogManager.catalogResolver(null);
> 
> Instead, the following may be used to achieve the same result:
>        CatalogManager.catalogResolver(CatalogFeatures.defaults());
> 
> While path can be empty, it can no longer be null. The following then would also get a NPE:
>        CatalogManager.catalogResolver(CatalogFeatures.defaults(), null);
> 

This change is reasonable and it’s good to pass CatalogFeatures.defaults() explicitly rather than special casing null to represent that.

> In consistence with ignoring invalid catalogs as required by the Catalog standard, the statements that required CatalogException when no catalog is found are removed.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8144966
> webrev: http://cr.openjdk.java.net/~joehw/jdk9/8144966/webrev/
> 

 41      * Creates a {@code Catalog} object using the specified feature settings and path to
 42      * a catalog file. If the path is empty, 

Nit: should it say “path to one or more catalog files”

What if “paths” is non-empty but all entries are invalid and no catalog file is read - will it read the system property?

line 46: s/path argument/paths argument/

Same comment applied to other factory methods.


CatalogImpl.java
 117         if (file.length > 0) {
 - Do you expect file to be null?  If so, NPE will be thrown.  If not, file == null in line 125 is not needed.

Otherwise the change looks okay.
Mandy