[8] Review Request for 8007292 : Add JavaFX internal packages to package.access

[Sean Mullan]

On 10/09/2013 05:14 AM, Erik Joelsson wrote:
> Hello Sean,
>
> On 2013-10-09 06:33, David Holmes wrote:
>> Hi Sean,
>>
>> Not a full review.
>>
>> On 9/10/2013 5:52 AM, Sean Mullan wrote:
>>> Please review the fix for the following bug:
>>>
>>>    https://bugs.openjdk.java.net/browse/JDK-8007292
>>>
>>> This bug requires build changes and a new build tool to add additional
>>> restricted packages to the java.security file which are not part of
>>> OpenJDK. These packages are only added when doing a build including the
>>> open and closed sources.
>>>
>>> The restricted packages and new test are in the closed sources and will
>>> be reviewed separately.
>>>
>>> webrev: http://cr.openjdk.java.net/~mullan/webrevs/8007292/webrev.00/
>>
>> Based on your description and the ifndef OPENJDK it sounds to me that
>> this doesn't belong in the OpenJDK makefiles.
>>
> I agree in principle, but the pattern on how to handle this isn't well
> established yet and something we need to improve on. In this case we
> would need to make changes on the open side anyway to provide hooks for
> overrides of the behavior. I'm willing to accept this for now.

Thanks. Also, keep in mind that this hook allows each vendor,etc to add 
additional proprietary or internal packages to the restricted packages 
properties for their own build. So I think it is useful in general in 
that respect.

>> That aside I would think the CP+RM could be changed to a MV.
> Agreed.

Right. Will do.

> I would prefer the TOOL_ADDTO... line to be moved to
> jdk/makefiles/Tools.gmk with the other similar definitions, even if it
> is only used here atm.

Ok.

>> In the tool this code doesn't show correct use of try-with-resources:
>>
>> 51         try (BufferedReader br = new BufferedReader(new
>> FileReader(args[0]));
>>   52              BufferedWriter bw = new BufferedWriter(new
>> FileWriter(args[1]))) {
>>
>> The FileReader and FileWriter should also be covered by TWR:
>>
>>   try (FileReader fr = new FileReader(args[0]);
>>        BufferedReader br = new BufferedReader(fr);
>>        FileWriter fw = new FileWriter(args[1]);
>>        BufferedWriter bw = new BufferedWriter(fw)) {
>>
> I'm not familiar with the try-with-resources, but calling close on a
> BufferedReader/writer will close the underlying reader/writer so nothing
> will be left open, will it not?

That's what I thought as well. David?

>> Finally do we still use make/tools/Makefile in the new build?
>>
> No, we don't. If you want to add old build support for this, you would
> also need to add usage of the tool to the correct old makefile.

I don't think it's necessary to add this to the old build at this point.

I'll post another webrev later in the day with these updates.

Thanks,
Sean