Hi Joe, thanks, looks fine now.
In test/javax/xml/jaxp/unittest/stream/XMLStreamReaderTest/StreamReaderTest.java, the brace in line 63 could still move one line up, but this really is nit-picking ;-) Best regards Christoph > -----Original Message----- > From: Joe Wang [mailto:huizhe.w...@oracle.com] > Sent: Donnerstag, 1. Dezember 2016 18:25 > To: Daniel Fuchs <daniel.fu...@oracle.com> > Cc: Langer, Christoph <christoph.lan...@sap.com>; core-libs- > d...@openjdk.java.net > Subject: Re: RFR (JAXP) 8167340: XMLStreamReader.getElementText return > corrupt content when size of element is > 8192 > > Thanks Christoph, Daniel! I've updated the test, removing the comments. > > As for 80 chars line limit, or slightly longer than that is fine with > me. Given our codebase, it would be an enormous undertaking. I'm > therefore fine with taking care of only the extremely long ones or any > that impedes the reviews, that is, side-by-side view. > > Best, > Joe > > On 12/1/16, 2:25 AM, Daniel Fuchs wrote: > > Hi Joe, > > > > I agree with Christoph's comments below. > > > > +1 > > > > best regards, > > > > -- daniel > > > > On 01/12/16 07:40, Langer, Christoph wrote: > >> Hi Joe, > >> > >> to me this looks good. > >> > >> Did you already remove the cleanups from > >> http://cr.openjdk.java.net/~joehw/jdk9/8167340/webrev/ ? I can't see > >> a lot of them any more... > >> > >> A few minor points: > >> > >> It seems you still have left some debugging code in > >> > test/javax/xml/jaxp/unittest/stream/XMLStreamReaderTest/StreamReaderTest > .java: > >> > >> 59 while (xmlStreamReader.hasNext()) { > >> 60 int event = xmlStreamReader.next(); > >> 61 if (event == XMLStreamConstants.START_ELEMENT) { > >> 62 if > >> (xmlStreamReader.getLocalName().equals("body"))// && bMessage) > >> > >> -> remove the && bMessage) > >> > >> 63 { > >> 64 String elementText = > >> xmlStreamReader.getElementText(); > >> 65 //System.out.println("elementText=" + > >> elementText + "EndOfElementText"); > >> > >> -> the commented System.out.println statement should be removed at > >> all, I suggest > >> > >> 66 // fail if elementText contains "</body>" > >> 67 > >> Assert.assertTrue(!elementText.contains("</body>"), "Fail: > >> elementText contains </body>"); > >> 68 } > >> 69 } > >> 70 } > >> > >> Other than that I'm wondering if the 80 chars line limit shall be > >> held which is not completely true in StreamReaderTest.java. But I > >> know how difficult that is, while keeping the code still readable. > >> Especially with the long speaking Class and method names ;-) > >> > >> Best regards > >> Christoph > >> > >> > >>> -----Original Message----- > >>> From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] > >>> On Behalf > >>> Of Joe Wang > >>> Sent: Mittwoch, 30. November 2016 22:21 > >>> To: core-libs-dev@openjdk.java.net > >>> Subject: RFR (JAXP) 8167340: XMLStreamReader.getElementText return > >>> corrupt > >>> content when size of element is > 8192 > >>> > >>> Hi, > >>> > >>> Please review an one-line fix and a bunch of cleanups. > >>> > >>> The reported issue was caused by a missed setting when the > >>> XMLStreamReader initializes the XML 1.1 scanner, so while the changeset > >>> involved 350 lines, the fix is really just the following: > >>> > >>> XMLStreamReaderImpl.java: > >>> @@ -605,11 +604,12 @@ > >>> ... > >>> + fEntityScanner.registerListener(fScanner); > >>> > >>> > >>> All other changes are cleanups, warnings. And BTW, warnings in the jaxp > >>> repo have come down from 5230 in 2015 to 3265, a result of a bit of > >>> cleanups here and there when we touch those classes. Still a long > >>> way to > >>> go, and it shows we may need to have a few dedicated patches. > >>> > >>> JBS: https://bugs.openjdk.java.net/browse/JDK-8167340 > >>> webrevs: http://cr.openjdk.java.net/~joehw/jdk9/8167340/webrev/ > >>> > >>> Thanks, > >>> Joe > >
