Adjust SAXLocator reports

RESOLVED FIXED

Status

()

Core
XML
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Robert Sayre, Assigned: WeirdAl)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

12 years ago
Need to get input source and sax locator systemid/publicid reports correct when there's an error.
(Assignee)

Comment 1

12 years ago
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).
(Reporter)

Comment 2

12 years ago
*** Bug 343289 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 3

12 years ago
Created attachment 236028 [details] [diff] [review]
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.
Attachment #236028 - Flags: review?(sayrer)
(Reporter)

Comment 4

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

r=sayrer. sorry this so long.
Attachment #236028 - Flags: review?(sayrer) → review+
(Assignee)

Updated

12 years ago
Attachment #236028 - Flags: superreview?(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.
Attachment #236028 - Flags: superreview?(bzbarsky) → superreview-
(Assignee)

Comment 6

12 years ago
(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...
(Reporter)

Comment 7

12 years ago
>
> 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.

(Assignee)

Comment 8

12 years ago
Created attachment 237935 [details] [diff] [review]
patch for locator using parseFromStream, v2

Answering review comments.  Please re-review.
Attachment #236028 - Attachment is obsolete: true
Attachment #237935 - Flags: superreview?(bzbarsky)
Attachment #237935 - Flags: review?(sayrer)
(Assignee)

Comment 9

12 years ago
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 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.
Attachment #237935 - Flags: superreview?(bzbarsky) → superreview+
(Reporter)

Comment 11

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

r=sayrer with bz's changes.
Attachment #237935 - Flags: review?(sayrer) → review+
(Reporter)

Comment 12

12 years ago
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.
Attachment #237935 - Flags: review?(peterv)
(Reporter)

Comment 13

12 years ago
*** Bug 352273 has been marked as a duplicate of this bug. ***
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 ;-).
Attachment #237935 - Flags: review?(peterv)
(Assignee)

Comment 15

12 years ago
Created attachment 238220 [details] [diff] [review]
patch for locator using parseFromStream (final)

updated to reflect r+/sr+ comments, need a checkin please.  :)
Attachment #237935 - Attachment is obsolete: true
(Reporter)

Updated

12 years ago
Assignee: sayrer → ajvincent
(Reporter)

Comment 16

12 years ago
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
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.