Code Review Request: 8025763

[Anthony Scarpino]

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
>