Closed
Bug 812302
Opened 12 years ago
Closed 12 years ago
nsSAXXMLReader::HandleXMLDeclaration should report its results to somebody
Categories
(Core :: XML, defect)
Core
XML
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: WeirdAl, Assigned: WeirdAl)
References
(Depends on 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
10.71 KB,
patch
|
WeirdAl
:
review+
smaug
:
superreview+
|
Details | Diff | Splinter Review |
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); };
Assignee | ||
Updated•12 years ago
|
Version: 17 Branch → Trunk
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #682718 -
Flags: review?(bugs)
Comment 2•12 years ago
|
||
So SAX API doesn't have anything for XML declaration handling? How is this case handled elsewhere where SAX is used?
Assignee | ||
Comment 3•12 years ago
|
||
(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.
Assignee | ||
Comment 4•12 years ago
|
||
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•12 years ago
|
||
Using same file is ok, but use separate interfaces.
Assignee | ||
Updated•12 years ago
|
Attachment #682718 -
Flags: review?(bugs)
Comment 6•12 years ago
|
||
(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.
Assignee | ||
Comment 7•12 years ago
|
||
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
Assignee | ||
Comment 8•12 years ago
|
||
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
Comment 9•12 years ago
|
||
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•12 years ago
|
||
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.
Assignee | ||
Comment 11•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #682718 -
Flags: feedback?(hsivonen) → feedback+
Assignee | ||
Comment 12•12 years ago
|
||
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 13•12 years ago
|
||
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+
Assignee | ||
Comment 14•12 years ago
|
||
Olli, does this need super-review from someone, for the API change?
Attachment #691702 -
Flags: review+
Comment 15•12 years ago
|
||
Comment on attachment 691702 [details] [diff] [review] revised patch There you have it :)
Attachment #691702 -
Flags: superreview+
Assignee | ||
Updated•12 years ago
|
Attachment #691702 -
Flags: checkin?
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Updated•12 years ago
|
Attachment #682718 -
Attachment is obsolete: true
Updated•12 years ago
|
Attachment #691702 -
Flags: checkin?
Comment 16•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b627cef1d7d2
Flags: in-testsuite+
Keywords: checkin-needed
Comment 17•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b627cef1d7d2
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in
before you can comment on or make changes to this bug.
Description
•