Closed Bug 812302 Opened 7 years ago Closed 7 years ago

nsSAXXMLReader::HandleXMLDeclaration should report its results to somebody

Categories

(Core :: XML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: WeirdAl, Assigned: WeirdAl)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

I need this for my XML parsing.  The code comment indicates there's a SAX interface for handling the XML declaration, but I can't find it.

Henri, what would you say to:

[scriptable, uuid(...)]
interface nsISAXXMLDeclarationHandler : nsISupports {
  void handleXMLDeclaration(in AString version, in AString encoding, in boolean standalone);
};
Version: 17 Branch → Trunk
Attached patch patch (obsolete) — Splinter Review
Attachment #682718 - Flags: review?(bugs)
So SAX API doesn't have anything for XML declaration handling?
How is this case handled elsewhere where SAX is used?
(In reply to Olli Pettay [:smaug] from comment #2)
> So SAX API doesn't have anything for XML declaration handling?
> How is this case handled elsewhere where SAX is used?

I do not know.
I found something on the SAX pages, but it's buried a bit deeply.
http://www.saxproject.org/apidoc/org/xml/sax/ext/Locator2.html

This is an extension on top of the SAXLocator interface.  This only covers the version and encoding, though.  For the standalone flag, we're apparently supposed to use SAXXMLReader::GetFeature("http://xml.org/sax/features/is-standalone"); .  See http://www.saxproject.org/apidoc/org/xml/sax/package-summary.html for that detail.

Personally, I think this interface stinks, especially since it splits the values from the XML declaration among two different interfaces.  But if that's what the standard says, then I have some rewriting to do...

Olli, instead of adding a nsISAXLocator2 interface file, we can just stick both of them in the same file.  Or we could combine the two interfaces into one.  Which do you prefer?
Using same file is ok, but use separate interfaces.
Attachment #682718 - Flags: review?(bugs)
(In reply to Alex Vincent [:WeirdAl] from comment #0)
> I need this for my XML parsing.

What's the use case, more specifically?

(In reply to Alex Vincent [:WeirdAl] from comment #4)
> Personally, I think this interface stinks

The reason why it is buried is that if you want to know what exactly the XML declaration said, you are doing something you are not supposed to be doing. If your application isn't an XML editor that tries to maintain the original source formatting (in which case you'd need to retain the whitespace between attributes, etc.), relying on information that isn't exposed via the SAX ContentHandler interface is always a bug in the sense that only the information exposed by the SAX ContentHandler information is the logical contents of the XML document. Everything else is just incidental syntax noise that isn't supposed to carry any meaning.
Preserving source formatting is the precise use case I have.  Yes, my application is (or eventually will be) a XML editor.  :)

I'm well aware of the problems with whitespace in attributes, and I intend to try building a solution for that as well.  http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2012-November/037962.html
This could be much harder to properly fix than I anticipated.  First, we have to have a SAXLocator always available to the content handler.  That's bug 526588.  Second, the locator has to have the correct source line and column numbers in the document at all times - something which nsExpatDriver doesn't to my knowledge support.

It's a lot of work for very little gain.
Depends on: 526588
I think we shouldn’t feel constrained by the design of the Java SAX API. That is, I think we don’t need to put this on Locator. We could introduce a new handler interface with a callback instead.

(I’m worried about new stuff—stuff other than feeds—growing a dependency on this API, though.)
If we add non-SAX stuff to this API, at least the naming needs to make it clear that it is our
addition. Perhaps nsIMozSomething... if an interface is needed.
Comment on attachment 682718 [details] [diff] [review]
patch

(In reply to Henri Sivonen (:hsivonen) from comment #9)
> I think we shouldn’t feel constrained by the design of the Java SAX API.
> That is, I think we don’t need to put this on Locator. We could introduce a
> new handler interface with a callback instead.

That's what my current patch does (see comment 0), but Olli is right:  I should rename the interface.

I've also been considering skipping this entirely and manually trying to find the XML declaration.  In theory that would be extremely easy.  In practice, if it wasn't there I'd have to reset the stream I'm reading from back to the beginning.  nsJARInputStream doesn't implement nsISeekableStream, though, so I'd have to create two streams...

> (I’m worried about new stuff—stuff other than feeds—growing a dependency on
> this API, though.)

This is a fair concern.  I'm only seeking this because I need some kind of handler for my XML editing, and that's a dependency right there.

We could have this off by default, requiring the user call setFeature() or setProperty() with some custom URL to enable it.
Attachment #682718 - Flags: feedback?(hsivonen)
Attachment #682718 - Flags: feedback?(hsivonen) → feedback+
Comment on attachment 682718 [details] [diff] [review]
patch

Huh.  I was expecting some feedback, not a simple rubber-stamp.  :)

Olli, keep in mind this patch predated your comment to rename the interface.  Please see comment 11 for my suggested changes, which at this moment I don't know if we want.
Attachment #682718 - Flags: review?(bugs)
Comment on attachment 682718 [details] [diff] [review]
patch

Uh, SAX stuff uses odd coding style.
But at least make 
nsISAXXMLDeclarationHandler * *aDeclarationHandler to use the same coding style.

When you create patches, -p -U 8 please.

r+ if you change the interface name.
Attachment #682718 - Flags: review?(bugs) → review+
Attached patch revised patchSplinter Review
Olli, does this need super-review from someone, for the API change?
Attachment #691702 - Flags: review+
Comment on attachment 691702 [details] [diff] [review]
revised patch

There you have it :)
Attachment #691702 - Flags: superreview+
Attachment #691702 - Flags: checkin?
Keywords: checkin-needed
Attachment #682718 - Attachment is obsolete: true
Attachment #691702 - Flags: checkin?
https://hg.mozilla.org/mozilla-central/rev/b627cef1d7d2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.