Request for Review: Java SE 8 Compact Profiles

[Alan Bateman]

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.