RFR (JDK 12/java.base) 8213325: (props) Properties.loadFromXML does not fully comply with the spec

[naoto.sato at oracle.com]

Hi Joe,

Those changes in the resource bundles look fine to me.

Naoto

On 12/4/18 1:45 PM, Joe Wang wrote:
> Hi Naoto and all,
> 
> Could you please take a look at the changes to the resource files for 
> tests of JDK-8172365 & JDK-8210408?
> 
> Current webrevs:
> http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v04/
> 
> Previous:
> http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v03/
> 
> Thanks,
> Joe
> 
> On 11/16/18, 2:27 PM, Joe Wang wrote:
>> Hi Daniel,
>>
>> On 11/16/18, 10:32 AM, Daniel Fuchs wrote:
>>> Hi Joe,
>>>
>>> 1. It looks as if the parser will not complain if the root element
>>>    is seen twice - and it looks as if it is a supported feature
>>>    since the previous implementation also had a counter.
>>>
>>>    Should this be tested?
>>>    If this is a feature and multiple roots are supported
>>>    then the new impl might have an issue with allowing only
>>>    one comment (maybe).
>>
>> Removed "properties" from the ALLOWED_ELEMENTS so that it will report 
>> an error on properties inside properties, and added a test 
>> (IllegalElement.xml).
>>>
>>> 2. The rootElm qName is now set at two locations. Is the overridden
>>>    method below still needed? Could it have been used instead of
>>>    introducing a new startDTD method?
>>>
>>>  212     @Override
>>>  213     public void notationDecl(String name, String publicId, 
>>> String systemId) throws SAXException {
>>>  214         rootElm = name;
>>>  215     }
>>
>> Removed the method override. startDTD is the correct place to 
>> accomplish this.
>>
>> Updated webrev:
>> http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v03/
>>
>> Previous:
>> http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v02/
>>
>> Best regards,
>> Joe
>>
>>>
>>> best regards,
>>>
>>> -- daniel
>>>
>>> On 15/11/2018 20:26, Joe Wang wrote:
>>>> Hi Daniel,
>>>>
>>>> I deleted the endDTD method. It could have been used to signal the 
>>>> end of DTD parsing and therefore serve as a validation point. 
>>>> However, the parser would have thrown Exceptions if a DTD parsing 
>>>> wasn't completed properly, that seems to make an endDTD check 
>>>> unnecessary, at least as the current parser impl goes.
>>>>
>>>> The small footprint parser has its problems, for example, not 
>>>> providing specific error messages when things go wrong. But since 
>>>> this is a Properties usage only, it serves the purpose well enough.
>>>>
>>>> Updated webrev:
>>>> http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v02/
>>>>
>>>> Previous:
>>>> http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev_v01/
>>>>
>>>> Best regards,
>>>> Joe
>>>>
>>>> On 11/14/18, 2:32 AM, Daniel Fuchs wrote:
>>>>> Hi Joe,
>>>>>
>>>>> I do not see where the new DTDHandler::endDTD methos is called.
>>>>> Is that an oversight or is there more magic?
>>>>>
>>>>> best regards,
>>>>>
>>>>> -- daniel
>>>>>
>>>>> On 14/11/2018 05:18, Joe Wang wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Please review a patch to bring the small footprint parser for 
>>>>>> java.util.Properties compliant with the specification.
>>>>>>
>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8213325
>>>>>> webrevs: http://cr.openjdk.java.net/~joehw/jdk12/8213325/webrev/
>>>>>>
>>>>>> Thanks,
>>>>>> Joe
>>>>>
>>>