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

[Joe Wang]

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
>>>>
>>