I've generated a fresh webrev with your changes.
http://cr.openjdk.java.net/~dholmes/8004265.v2/webrev.jdk/
Looking to get the Thumbs up on the src and test changes.
Thanks,
David
On 4/01/2013 5:54 PM, Alan Bateman wrote:
On 02/01/2013 18:53, Mandy Chung wrote:
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?
Thanks, this is more correct and will be in the next webrev.
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
Joe Darcy and I chatted about this recently and he suggested it would be
better if the default method be a no-op (with no side effects). It's
updated in the jdk8/profiles forest so should be in the next webrev.
sun.misc.URLClassPath
L808: typo "attribtue"
Fixed.
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.
We could although I'm not sure how this could arise (as you can't set an
empty profile name with the "p" option, and the "m" option to add/update
a manifest also disallows empty values).
L1000: looks like the convention is to use brackets even there is a
single statement in the if-statement body.
Okay.
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.
I considered this when updating the jar tool but decided at the time
that it shouldn't know about the profile names. It would be easy to do
of course.
UnsupportedProfileException
L29: probably better to link to the javadoc
{@link Attributes.Name#Profile Profile}
L38 and 44: {@code UnsupportedProfileException}
I've added {@code ...}.
make/tools/src/build/tools/RemoveMethods.java
L100: maybe print the method signature rather than just 'name'
L106: typo "no longed" -> "no longer"
Done.
The tests are hardcoded with the profile name and uses
Version.profileName().
Will there be a system property for the profile name?
A supported property or API to indicate the profile has the potential to
be problematic when we move to modules so it's not there. So far we
haven't had any real need for it as applications can easily check for
types to determine the profile. There are a few tests that do need to
know the names but most of the new tests aren't dependent on the name.
Otherwise, looks okay.
Thanks for looking at it. I think David plans to send out an updated
webrev in the next few days.
-Alan.
