Closed
Bug 1416038
Opened 8 years ago
Closed 8 years ago
Strip the SAX XML implemention to its minimum
Categories
(Core :: XML, enhancement, P3)
Core
XML
Tracking
()
RESOLVED
FIXED
mozilla59
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.
![]() |
Assignee | |
Comment 1•8 years ago
|
||
This interface no longer exists.
Attachment #8927153 -
Flags: review?(erahm)
![]() |
Assignee | |
Updated•8 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
![]() |
Assignee | |
Comment 3•8 years ago
|
||
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)
![]() |
Assignee | |
Comment 4•8 years ago
|
||
These are unused and also unimplemented.
Attachment #8927156 -
Flags: review?(erahm)
![]() |
Assignee | |
Comment 5•8 years ago
|
||
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)
![]() |
Assignee | |
Comment 6•8 years ago
|
||
This patch removes three methods that are no-ops (or missing) in all our
implementations.
Attachment #8927158 -
Flags: review?(erahm)
![]() |
Assignee | |
Comment 7•8 years ago
|
||
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)
![]() |
Assignee | |
Comment 8•8 years ago
|
||
None of our nsISAXErrorHandler implementations use the locator arguments.
Attachment #8927160 -
Flags: review?(erahm)
![]() |
Assignee | |
Comment 9•8 years ago
|
||
Attachment #8927161 -
Flags: review?(erahm)
Updated•8 years ago
|
Priority: -- → P3
Updated•8 years ago
|
Attachment #8927153 -
Flags: review?(erahm) → review+
Updated•8 years ago
|
Attachment #8927154 -
Flags: review?(erahm) → review+
Updated•8 years ago
|
Attachment #8927155 -
Flags: review?(erahm) → review+
Comment 10•8 years ago
|
||
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`
Updated•8 years ago
|
Attachment #8927156 -
Flags: review?(erahm) → review+
Updated•8 years ago
|
Attachment #8927157 -
Flags: review?(erahm) → review+
Comment 11•8 years ago
|
||
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 12•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8927160 -
Flags: review?(erahm) → review+
Comment 13•8 years ago
|
||
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+
Comment 14•8 years ago
|
||
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`.
![]() |
Assignee | |
Comment 15•8 years ago
|
||
(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."
![]() |
Assignee | |
Comment 17•8 years ago
|
||
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 18•8 years ago
|
||
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 19•8 years ago
|
||
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+
![]() |
Assignee | |
Comment 20•8 years ago
|
||
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)
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8927974 -
Attachment is obsolete: true
![]() |
||
Comment 21•8 years ago
|
||
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+
![]() |
||
Comment 22•8 years ago
|
||
(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.
Updated•8 years ago
|
Attachment #8928000 -
Flags: review?(philipp) → review+
![]() |
Assignee | |
Comment 23•8 years ago
|
||
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
Comment 24•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/193db89dfb34
https://hg.mozilla.org/mozilla-central/rev/55bab6da69e6
https://hg.mozilla.org/mozilla-central/rev/53b1e4c7ae6f
https://hg.mozilla.org/mozilla-central/rev/ca43c734e855
https://hg.mozilla.org/mozilla-central/rev/aa7169d94e83
https://hg.mozilla.org/mozilla-central/rev/b2fa81170a5e
https://hg.mozilla.org/mozilla-central/rev/a71af18b39d0
https://hg.mozilla.org/mozilla-central/rev/ca7261179936
https://hg.mozilla.org/mozilla-central/rev/a6d60c62fa91
https://hg.mozilla.org/mozilla-central/rev/ce6e3bf3c2dc
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment 25•8 years ago
|
||
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
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•