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

[Joe Wang]

Thanks Naoto for the fast review!

Best regards,
Joe

On 12/4/18, 1:53 PM, naoto.sato at oracle.com wrote:
> 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
>>>>>>
>>>>