334302 - Adjust SAXLocator reports

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Robert Sayre]

Need to get input source and sax locator systemid/publicid reports correct when there's an error.

Comment 1

[Alex Vincent [:WeirdAl]]

Currently, the SAXXMLReader passes in null for the locator to nsISAXErrorHandler. The question is, what's the Right Way to get the locator's fields from the expat sink to the error handler?

I tried an approach for bug 342164 that was apparently wrong.  peterv believes I don't need to pass the information through nsIExpatSink::ReportError (or in that case, ReportScriptError).

Comment 2

[Robert Sayre]

*** Bug 343289 has been marked as a duplicate of this bug. ***

Comment 3

[Alex Vincent [:WeirdAl]]

Attachment: patch for locator using parseFromStream

Per discussion with sayrer, this is not intended to fix the whole bug; I'm not doing InputSource.

Yes, I have tested this.

Comment 4

[Robert Sayre]

Comment on attachment 236028 [details] [diff] [review]
patch for locator using parseFromStream

r=sayrer. sorry this so long.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 236028 [details] [diff] [review]
patch for locator using parseFromStream

>Index: parser/xml/src/nsSAXLocator.cpp
>Index: parser/xml/src/nsSAXLocator.h
> class nsSAXLocator : public nsISAXLocator
>   nsSAXLocator();

Why not make the line/column and system/public IDs args to the constructor?  If you also need a no-arg ctor, you could have both, but I'm not sure why this object even has a contract and CID....  Craeting it via XPCOM seems pointless, so why is it allowed?

>-  nsString mPublicId;
>-  nsString mSystemId;
>+  nsAutoString mPublicId;
>+  nsAutoString mSystemId;

Er, why?  Unless there's a _really_ good reason, please undo that change.  If such a reason exists, I'd like to hear it.

>Index: parser/xml/src/nsSAXXMLReader.cpp
>+    nsSAXLocator* locator = new nsSAXLocator;

() before the ; please.

>+    if (!locator)
>+      return NS_ERROR_OUT_OF_MEMORY;

Curlies around if body, please.

>@@ -459,16 +480,20 @@ nsSAXXMLReader::ParseFromStream(nsIInput

>+  mSystemId.Truncate();
>+  mPublicId.Truncate();

Please document why this needs to be done.

Comment 6

[Alex Vincent [:WeirdAl]]

(In reply to comment #5)
> >+    if (!locator)
> >+      return NS_ERROR_OUT_OF_MEMORY;
> 
> Curlies around if body, please.

Local file style says not to do so...

Comment 7

[Robert Sayre]

>
> Why not make the line/column and system/public IDs args to the constructor?  If
> you also need a no-arg ctor, you could have both, but I'm not sure why this
> object even has a contract and CID....  Craeting it via XPCOM seems pointless,
> so why is it allowed?

Filed Bug 352273 on deCOMtamination (feel free to take, ajvincent). It's a separate bug from this one, which concerns getting the correct information into the locator.

Comment 8

[Alex Vincent [:WeirdAl]]

Attachment: patch for locator using parseFromStream, v2

Answering review comments.  Please re-review.

Comment 9

[Alex Vincent [:WeirdAl]]

Comment on attachment 237935 [details] [diff] [review]
patch for locator using parseFromStream, v2

>@@ -293,20 +297,33 @@ nsSAXXMLReader::ReportError(const PRUnic
>+    nsSAXLocator* locator = new nsSAXLocator(mPublicId,
>+                                             mSystemId,
>+                                             lineNumber,
>+                                             columnNumber);
>+    if (!locator)
>+      return NS_ERROR_OUT_OF_MEMORY;
>+
>+    rv = mErrorHandler->FatalError(locator, nsDependentString(aErrorText));

Hm, I actually just realized I should probably delete the locator now, but I am not entirely sure...

Just "delete locator" here?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 237935 [details] [diff] [review]
patch for locator using parseFromStream, v2

>Index: parser/xml/src/nsSAXLocator.cpp
>+nsSAXLocator::nsSAXLocator(nsAString & aPublicId,
>+                           nsAString & aSystemId,

"const nsString&" for both of those, please.

>Index: parser/xml/src/nsSAXXMLReader.cpp
>@@ -293,20 +297,33 @@ nsSAXXMLReader::ReportError(const PRUnic

>+    nsSAXLocator* locator = new nsSAXLocator(mPublicId,

This is an XPCOM object, right?  So you must take a ref to it before passing it around.  Might I recommend:

  nsCOMPtr<nsISAXLocator> locator = new ....

and then the delete will happen at the right time too.

sr=bzbarsky with those two nits.

Comment 11

[Robert Sayre]

Comment on attachment 237935 [details] [diff] [review]
patch for locator using parseFromStream, v2

r=sayrer with bz's changes.

Comment 12

[Robert Sayre]

Comment on attachment 237935 [details] [diff] [review]
patch for locator using parseFromStream, v2

need peterv's r+ here. bz and I aren't XML peers.

Comment 13

[Robert Sayre]

*** Bug 352273 has been marked as a duplicate of this bug. ***

Comment 14

[Peter Van der Beken [:peterv]]

Comment on attachment 237935 [details] [diff] [review]
patch for locator using parseFromStream, v2

Both sayrer and bz are now peers (module owners page is still pending), so you guys don't need me anymore ;-).

Comment 15

[Alex Vincent [:WeirdAl]]

Attachment: patch for locator using parseFromStream (final)

updated to reflect r+/sr+ comments, need a checkin please.  :)

Comment 16

[Robert Sayre]

I filed bug 352539 for input source enhancements, so this is closed.

Checking in htmlparser/src/nsParserModule.cpp;
/cvsroot/mozilla/parser/htmlparser/src/nsParserModule.cpp,v  <--  nsParserModule.cpp
new revision: 1.62; previous revision: 1.61
done
Checking in xml/src/Makefile.in;
/cvsroot/mozilla/parser/xml/src/Makefile.in,v  <--  Makefile.in
new revision: 1.4; previous revision: 1.3
done
Checking in xml/src/nsSAXLocator.cpp;
/cvsroot/mozilla/parser/xml/src/nsSAXLocator.cpp,v  <--  nsSAXLocator.cpp
new revision: 1.3; previous revision: 1.2
done
Checking in xml/src/nsSAXLocator.h;
/cvsroot/mozilla/parser/xml/src/nsSAXLocator.h,v  <--  nsSAXLocator.h
new revision: 1.3; previous revision: 1.2
done
Checking in xml/src/nsSAXXMLReader.cpp;
/cvsroot/mozilla/parser/xml/src/nsSAXXMLReader.cpp,v  <--  nsSAXXMLReader.cpp
new revision: 1.12; previous revision: 1.11
done
Checking in xml/src/nsSAXXMLReader.h;
/cvsroot/mozilla/parser/xml/src/nsSAXXMLReader.h,v  <--  nsSAXXMLReader.h
new revision: 1.5; previous revision: 1.4
done