Request for Review: Java SE 8 Compact Profiles

[David Holmes]

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.