Closed Bug 1416038 Opened 8 years ago Closed 8 years ago

Strip the SAX XML implemention to its minimum

Categories

(Core :: XML, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

Attachments

(11 files, 1 obsolete file)

973 bytes, patch
erahm
: review+
Details | Diff | Splinter Review
2.08 KB, patch
erahm
: review+
Details | Diff | Splinter Review
26.29 KB, patch
erahm
: review+
Details | Diff | Splinter Review
4.13 KB, patch
erahm
: review+
Details | Diff | Splinter Review
8.28 KB, patch
erahm
: review+
Details | Diff | Splinter Review
9.29 KB, patch
erahm
: review+
Details | Diff | Splinter Review
10.94 KB, patch
erahm
: review+
Details | Diff | Splinter Review
15.25 KB, patch
erahm
: review+
Details | Diff | Splinter Review
1.46 KB, patch
erahm
: review+
Details | Diff | Splinter Review
5.70 KB, patch
erahm
: review+
Details | Diff | Splinter Review
3.46 KB, patch
aceman
: review+
Fallen
: review+
Details | Diff | Splinter Review
parser/xml/ implements a full SAX XML parser. We have a single use of this in mozilla-central (toolkit/components/feeds/FeedProcessor.js) and two uses in comm-central (chat/protocols/xmpp/xmpp-xml.jsm and calendar/providers/caldav/calDavRequestHandlers.js). All three of these files only use a fraction of the full SAX functionality. If we remove all the unused features we can cut ~1000 lines of code.
This interface no longer exists.
Attachment #8927153 - Flags: review?(erahm)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
It's unused.
Attachment #8927154 - Flags: review?(erahm)
Because none of our nsISAXXMLReader implementations set `dtdHandler`, `declarationHandler`, or `lexicalHandler`. The patch also removes test_xml_declaration.js, because that test is all about testing nsIMozSAXXMLDeclarationHandler.
Attachment #8927155 - Flags: review?(erahm)
These are unused and also unimplemented.
Attachment #8927156 - Flags: review?(erahm)
It's unused by any of our nsISAXXMLReader implementations. The patch also removes nsSAXXMLReader::mEnableNamespacePrefixes, which is now unused. And it removes test_namespace_support.js, which is all about testing features.
Attachment #8927157 - Flags: review?(erahm)
This patch removes three methods that are no-ops (or missing) in all our implementations.
Attachment #8927158 - Flags: review?(erahm)
There is a single implementation, nsSAXAttributes, and all the methods are unused... except for AddAttribute(), but that doesn't need to be declared in XPIDL because it's only used within nsSAXAttributes.cpp.
Attachment #8927159 - Flags: review?(erahm)
None of our nsISAXErrorHandler implementations use the locator arguments.
Attachment #8927160 - Flags: review?(erahm)
Priority: -- → P3
Attachment #8927153 - Flags: review?(erahm) → review+
Attachment #8927154 - Flags: review?(erahm) → review+
Attachment #8927155 - Flags: review?(erahm) → review+
Comment on attachment 8927155 [details] [diff] [review] (part 3) - Remove nsIMozSAXXMLDeclarationHandler, nsISAXDTDHandler, and nsISAXLexicalHandler Review of attachment 8927155 [details] [diff] [review]: ----------------------------------------------------------------- I wonder if it's worth removing the no-op `Handle` functions as well: - `nsSAXXMLReader::HandleComment` - `nsSAXXMLReader::HandleNotationDecl` - `nsSAXXMLReader::HandleUnparsedEntityDecl` - `nsSAXXMLReader::HandleXMLDeclaration`
Attachment #8927156 - Flags: review?(erahm) → review+
Attachment #8927157 - Flags: review?(erahm) → review+
Comment on attachment 8927158 [details] [diff] [review] (part 6) - Slim down nsISAXContentHandler Review of attachment 8927158 [details] [diff] [review]: ----------------------------------------------------------------- Seems like we could remove more no-ops: - nsSAXXMLReader::HandleStartNamespaceDecl - nsSAXXMLReader::HandleEndNamespaceDecl I realize now these (and most of the ones I mentioned before) are part of nsIExtendedExpatSink, it seems like we could just make `nsSAXXMLReader` not inherit from that and remove the implementations.
Attachment #8927158 - Flags: review?(erahm) → review+
Comment on attachment 8927159 [details] [diff] [review] (part 7) - Remove nsISAXMutableAttributes Review of attachment 8927159 [details] [diff] [review]: ----------------------------------------------------------------- ::: parser/xml/nsSAXAttributes.h @@ +26,5 @@ > public: > NS_DECL_ISUPPORTS > NS_DECL_NSISAXATTRIBUTES > + > + nsresult AddAttribute(const nsAString &aURI, It's odd this isn't an IDL method...
Attachment #8927159 - Flags: review?(erahm) → review+
Attachment #8927160 - Flags: review?(erahm) → review+
Comment on attachment 8927161 [details] [diff] [review] (part 9) - Add a comment about nsISAXXMLReader's lack of functionality Review of attachment 8927161 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for adding the note.
Attachment #8927161 - Flags: review?(erahm) → review+
Looks good, thanks for splitting everything up. I think it might be useful to follow up with one more patch to remove the usage of `nsIExtendedExpatSink` in `nsSAXXMLReader`.
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #12) > Comment on attachment 8927159 [details] [diff] [review] > (part 7) - Remove nsISAXMutableAttributes > > Review of attachment 8927159 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: parser/xml/nsSAXAttributes.h > @@ +26,5 @@ > > public: > > NS_DECL_ISUPPORTS > > NS_DECL_NSISAXATTRIBUTES > > + > > + nsresult AddAttribute(const nsAString &aURI, > > It's odd this isn't an IDL method... The commit message explains this: "There is a single implementation, nsSAXAttributes, and all the methods are unused... except for AddAttribute(), but that doesn't need to be declared in XPIDL because it's only used within nsSAXAttributes.cpp."
aceman, I stripped the SAX parser down to its minimum, because there was lots of functionality unused by either mozilla-central or comm-central. This means that the comm-central implementations (a) can remove a couple of no-op methods, and (b) need to account for the change to signature of the error-handling methods. As usual, this is untested, and because it's all JS code the fact that it compiles doesn't say much. But I think it should work :)
Attachment #8927974 - Flags: review?(acelists)
Comment on attachment 8927969 [details] [diff] [review] (part 10) - nsSAXXMLReader.cpp doesn't need to be an nsIExtendedExpatSink Review of attachment 8927969 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the follow up. ::: parser/xml/nsSAXXMLReader.cpp @@ -72,5 @@ > { > return NS_OK; > } > > -// nsIExtendedExpatSink nit, could just update the comment. > // nsIExpatSink
Attachment #8927969 - Flags: review?(erahm) → review+
Comment on attachment 8927974 [details] [diff] [review] Update comm-central for SAX XML parser changes in mozilla-central Review of attachment 8927974 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch and for the notification. This looks good, but I see more occurences of these functions in calendar, see https://dxr.mozilla.org/comm-central/search?q=ignorableWhitespace . Please also remove those if they are the same SAX object you are changing.
Attachment #8927974 - Flags: review?(acelists) → feedback+
Good catch! Leaving these in place wouldn't have affected correctness, but might as well remove them since they're unused. aceman, do you want to do a c-c try push before I land the m-c patches?
Attachment #8928000 - Flags: review?(acelists)
Attachment #8927974 - Attachment is obsolete: true
Comment on attachment 8928000 [details] [diff] [review] Update comm-central for SAX XML parser changes in mozilla-central Review of attachment 8928000 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, r+ for the chat part.
Attachment #8928000 - Flags: review?(philipp)
Attachment #8928000 - Flags: review?(acelists)
Attachment #8928000 - Flags: review+
(In reply to Nicholas Nethercote [:njn] from comment #20) > aceman, do you want to do a c-c try push before I land the m-c patches? Thanks, but I think this doesn't need to wait for us, you can land it freely. I'll land the c-c patch after review.
Attachment #8928000 - Flags: review?(philipp) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/193db89dfb3408096033113f223abd30809cbd83 Bug 1416038 (part 1) - Remove nsISAXEntityResolver forward declaration. r=erahm https://hg.mozilla.org/integration/mozilla-inbound/rev/55bab6da69e6fcda3be9577415ad0d54d1313f75 Bug 1416038 (part 2) - Remove nsISAXXMLFilter. r=erahm https://hg.mozilla.org/integration/mozilla-inbound/rev/53b1e4c7ae6f1ac5384f7f94d0f52063caea7106 Bug 1416038 (part 3) - Remove nsIMozSAXXMLDeclarationHandler, nsISAXDTDHandler, and nsISAXLexicalHandler. r=erahm https://hg.mozilla.org/integration/mozilla-inbound/rev/ca43c734e855ada63358c2016097a7fff46ba00e Bug 1416038 (part 4) - Remove nsISAXXMLReader.[sg]etProperty. r=erahm https://hg.mozilla.org/integration/mozilla-inbound/rev/aa7169d94e83c0977a1217340d89315c3c708d0e Bug 1416038 (part 5) - Remove nsISAXXMLReader.[sg]etFeature. r=erahm https://hg.mozilla.org/integration/mozilla-inbound/rev/b2fa81170a5efeed00d71504bf03c9159ffe3aa8 Bug 1416038 (part 6) - Slim down nsISAXContentHandler. r=erahm https://hg.mozilla.org/integration/mozilla-inbound/rev/a71af18b39d0115e57e9f2be8a58e2018873f7d4 Bug 1416038 (part 7) - Remove nsISAXMutableAttributes. r=erahm https://hg.mozilla.org/integration/mozilla-inbound/rev/ca72611799368845c56b95b0de47a37f2d14047b Bug 1416038 (part 8) - Remove nsISAXLocator. r=erahm https://hg.mozilla.org/integration/mozilla-inbound/rev/a6d60c62fa91a2e8b4429e8850067778a73cb581 Bug 1416038 (part 9) - Add a comment about nsISAXXMLReader's lack of functionality. r=erahm https://hg.mozilla.org/integration/mozilla-inbound/rev/ce6e3bf3c2dcb923a2f5ed88326a3a00c3358965 Bug 1416038 (part 10) - nsSAXXMLReader.cpp doesn't need to be an nsIExtendedExpatSink. r=erahm
Blocks: 1416980
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/323b721c2abf Update comm-central for SAX XML parser changes in mozilla-central. r=aceman,philipp
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: