Request for Review: Java SE 8 Compact Profiles (original) (raw)

Mandy Chung mandy.chung at oracle.com
Wed Jan 2 18:53:45 UTC 2013

Hi David,

On 12/20/2012 10:18 PM, David Holmes wrote:

jdk repo: http://cr.openjdk.java.net/~dholmes/8004265/webrev.jdk/

I reviewed the src/test changes in the jdk repo and have a few comments:

Attributes.java: 568 * {@code Name} object for {@code Profile} manifest attribute used 569 * by applications packaged as JAR files to indicate the minimum 570 * profile required to execute the application.

The Profile attribute can be specified in both applications and libraries. Shoud L569 be changed with s/applications/applications or libraries?

Pack200.java I think the default implementation for addPropertyChangeListener and removePropertyChangeListener requiring a non-null listener is a right choice. On the other hand, I found that the current pack200 implementation allows the given listener be null that seems to be a bug and the Pack200 class spec also specifies to throw NPE if null argument is passed to a method and looks like what you have

sun.misc.URLClassPath L808: typo "attribtue"

L820: An empty "Profile" attribute is invalidand Version.supportsProfile returns false if requiredProfile parameter is empty even if the runtime is a full JRE. This is fine but I was wondering if the exception message can indicate if the attribute value is invalid to help diagnosis.

L1000: looks like the convention is to use brackets even there is a single statement in the if-statement body.

sun.tools.jar.Main It would be helpful if the jar tool checks if the input profile name to the -p option is valid and outputs error.

UnsupportedProfileException L29: probably better to link to the javadoc {@link Attributes.Name#Profile Profile} L38 and 44: {@code UnsupportedProfileException}

make/tools/src/build/tools/RemoveMethods.java L100: maybe print the method signature rather than just 'name' L106: typo "no longed" -> "no longer"

The tests are hardcoded with the profile name and uses Version.profileName(). Will there be a system property for the profile name?

Otherwise, looks okay.

Mandy

More information about the core-libs-dev mailing list