RFR [15/java.xml] 8238183: SAX2StAXStreamWriter cannot deal with comments prior to the root element

[naoto.sato at oracle.com]

Looks good. Thanks for the update.

Naoto

On 3/30/20 3:44 PM, Joe Wang wrote:
> 
> On 3/30/20 3:12 PM, naoto.sato at oracle.com wrote:
>> Hi Joe,
>>
>> Much better and cleaner! One nit is that you could remove "docLocator 
>> != null &&" as instanceof checks null.
> 
> Indeed, null check is removed:
> http://cr.openjdk.java.net/~joehw/jdk15/8238183/webrev_03/
> 
> Thanks,
> Joe
> 
>>
>> Naoto
>>
>> On 3/30/20 2:56 PM, Joe Wang wrote:
>>> Hi Naoto,
>>>
>>> Thanks for the review!
>>>
>>> I refactored the code around getting the XML version and encoding 
>>> from the locator to get rid of the CCE. The impls with EventWriter 
>>> and StreamWriter share a base class. All three classes are therefore 
>>> refactored. No material change to the EventWriter impl. Two fields, 
>>> xmlVersion and encoding, are added since I expect they will be needed 
>>> later when we work on fixing the declaration. Note that, as of the 
>>> current impl, encoding is not referenced in the StreamWriter impl, 
>>> which is part of the issue in transforming the declaration 
>>> (JDK-8241711).
>>>
>>> Here's webrev version 2:
>>> http://cr.openjdk.java.net/~joehw/jdk15/8238183/webrev_02/
>>>
>>> -Joe
>>>
>>> On 3/30/20 11:23 AM, naoto.sato at oracle.com wrote:
>>>> Hi Joe,
>>>>
>>>> Can writeStartDocument() be simplified by checking "docLocator 
>>>> instanceof Locator2"? This way it won't need to catch CCE and issue 
>>>> no-arg version.
>>>>
>>>> Otherwise looks good to me.
>>>>
>>>> Naoto
>>>>
>>>> On 3/30/20 11:02 AM, Joe Wang wrote:
>>>>> Hi,
>>>>>
>>>>> Please review a fix for the StAXResult impl. The issue was that it 
>>>>> output comment prior to the declaration, resulting in an invalid 
>>>>> XML document. The patch focuses on fixing this issue, but it does 
>>>>> not cover other issues the StAXResult impl may have (e.g. 
>>>>> JDK-8241711).
>>>>>
>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8238183
>>>>> webrev: http://cr.openjdk.java.net/~joehw/jdk15/8238183/webrev/
>>>>>
>>>>> Thanks,
>>>>> Joe
>>>>>
>>>
>