Thanks Sean and Mandy for the feedback. I believe I incorporated all
feedback into the latest patch, with one exception (Sean see below).
http://cr.openjdk.java.net/~apetcher/8168075/webrev.02/
On 1/23/2017 12:15 PM, Sean Mullan wrote:
On 1/19/17 10:28 AM, Adam Petcher wrote:
My last attempt to solve this problem didn't work because some classes
needed for string formatting were not loaded by init level 3 in some
cases. So I had to backtrack and try a different approach.
This patch avoids localization and message formatting when the VM is not
booted. In this case, non-localized messages are printed, and simplified
message formatting code is used. Once the VM is loaded, messages are
localized and formatted in the usual way.
http://cr.openjdk.java.net/~apetcher/8168075/webrev.01/
Looks good, just a couple of comments:
- PolicyUtil.getLocalizedMessage
Don't think you need this method, since
LocalizedMessage.getLocalizedString is public.
Removed. I also renamed LocalizedMessage.getLocalizedString to
"getMessage" to remove some redundancy and make the expressions that
reference it shorter.
- LocalizedMessage.java
Not sure I see the need for the constructor or toLocalizedString
method, as I think you can just call getLocalizedString, ex:
LocalizedMessage localizedMsg = new LocalizedMessage
("alias.name.not.provided.pe.name.");
Object[] source = {pe.name};
throw new Exception(localizedMsg.toLocalizedString(source));
becomes:
throw new
Exception(LocalizedMessage.getLocalizedString("alias.name.not.provided.pe.name.",
source));
In PolicyParser.ParsingException, there is a comment stating that the
MessageFormat is stored in the exception, and it is only formatted when
getLocalizedMessage() is called on the exception. This arrangement
avoids unnecessary formatting and permission checks in cases where the
message is never localized and formatted. I kept this arrangement after
I replaced MessageFormat with LocalizedMessage. If there is no value to
delaying/avoiding the message formatting, then I can remove the
constructor and non-static method as you suggest.
Here are the changes I made based on this feedback:
1) I changed the name of LocalizedMessage.toLocalizedString to "format"
to make it more clear that I was trying to mirror the behavior of
MessageFormat (and also to shorten the name and remove redundancy).
2) I added a comment to the constructor describing how it can be used to
delay/avoid message formatting.
3) I changed all references to LocalizedMessage that don't involve
ParsingException to use only the static method of LocalizedMessage, as
you suggested.
(saves creating an extra object).
- MessageFormatting.java
Minor nit: please use "java.security.policy==error.policy" instead of
"policy=error.policy" The java.security.policy is a newer jtreg option
that matches the syntax of the java.security.policy option. I'd like
to discourage use of the policy option going forward.
Thanks,
Sean
On 1/11/2017 8:34 AM, Adam Petcher wrote:
Please review the following bug fix:
http://cr.openjdk.java.net/~apetcher/8168075/webrev.00/
This fixes a bug in which a permission check would try to load
resources while the system class loader is being initialized.
Resources cannot be loaded at this time, so this change ensures that
the resources are loaded earlier.
