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

[Erik Joelsson]

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.
> That aside I would think the CP+RM could be changed to a MV.
Agreed.

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.

>
> 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?
>
> 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.

/Erik