812302 - nsSAXXMLReader::HandleXMLDeclaration should report its results to somebody

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Alex Vincent [:WeirdAl]]

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);
};

Comment 1

[Alex Vincent [:WeirdAl]]

Attachment: patch

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

So SAX API doesn't have anything for XML declaration handling?
How is this case handled elsewhere where SAX is used?

Comment 3

[Alex Vincent [:WeirdAl]]

(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.

Comment 4

[Alex Vincent [:WeirdAl]]

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?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Using same file is ok, but use separate interfaces.

Comment 6

[Henri Sivonen (:hsivonen)]

(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.

Comment 7

[Alex Vincent [:WeirdAl]]

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

Comment 8

[Alex Vincent [:WeirdAl]]

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.

Comment 9

[Henri Sivonen (:hsivonen)]

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.)

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

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 11

[Alex Vincent [:WeirdAl]]

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.

Comment 12

[Alex Vincent [:WeirdAl]]

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.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 14

[Alex Vincent [:WeirdAl]]

Attachment: revised patch

Olli, does this need super-review from someone, for the API change?

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 691702 [details] [diff] [review]
revised patch

There you have it :)

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b627cef1d7d2

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b627cef1d7d2