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

2020-03-30 Thread Lance Andersen 
+1  

Good catch Naoto

> On Mar 30, 2020, at 6:44 PM, Joe Wang  wrote:
> 
> 
> On 3/30/20 3:12 PM, naoto.s...@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.s...@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
> 
>>> 
> 

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com

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

2020-03-30 Thread naoto . sato 

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.s...@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.s...@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

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

2020-03-30 Thread Joe Wang 



On 3/30/20 3:12 PM, naoto.s...@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.s...@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

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

2020-03-30 Thread naoto . sato 

Hi Joe,

Much better and cleaner! One nit is that you could remove "docLocator != 
null &&" as instanceof checks null.


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.s...@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

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

2020-03-30 Thread Lance Andersen 
HI Joe,

This version looks OK

Best
Lance

> On Mar 30, 2020, at 5: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.s...@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
>>> 
> 

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com

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

2020-03-30 Thread Joe Wang 

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.s...@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

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

2020-03-30 Thread naoto . sato 

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