On 10/18/2013 12:44 PM, Sean Mullan wrote:
Mostly looks good, just a few comments:
- for completeness, please add @Override tags to all existing methods
that are overridden
- you need to also override the new forEach and getOrDefault methods to
first check if the provider is initialized. See, for example, the get
method.
- nit, say either "security manager" or "Security Manager" but not both.
I would probably use the former since it was used already throughout.
- line 429, replace should be synchronized
- For the methods that require both put and remove permissions, can you
tweak the wording to say:
"... to see if it's ok to set this provider's property values and
remove this provider's properties."
and
"... denies access to set property values or remove properties."
no problem with all the above
- lines 681-684. I think this should call checkLegacy() like the other
impl methods otherwise legacyStrings might be null and throw an NPE.
That was a mistake where I should have had checkLegacy()
- line 733,787 remove indentation and align
Not sure how they got indented, that strange
- line 749-750, shouldn't that be legacyStrings.compute and BiFunction?
Whoops.. That's got cut-n-paste written all over it
I've updated the webrev
http://cr.openjdk.java.net/~ascarpino/8025763/webrev.01/
--Sean
On 10/17/2013 06:14 PM, Anthony Scarpino wrote:
Hi,
I need a code review for changes regarding
8025763 Provider does not override new Hashtable methods
http://cr.openjdk.java.net/~ascarpino/8025763/
thanks
Tony
