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

[Sean Mullan]

Updated webrev: 
http://cr.openjdk.java.net/~mullan/webrevs/8007292/webrev.01/

Let me know if there are any more comments, otherwise I will plan to 
push tomorrow.

Thanks,
Sean

On 10/09/2013 09:20 AM, Sean Mullan wrote:
> 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
>