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
