1416038 - Strip the SAX XML implemention to its minimum

Status: RESOLVED FIXED

(Core :: XML, enhancement, P3)

Description

[Nicholas Nethercote [inactive]]

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.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove nsISAXEntityResolver forward declaration

This interface no longer exists.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove nsISAXXMLFilter

It's unused.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Remove nsIMozSAXXMLDeclarationHandler, nsISAXDTDHandler, and nsISAXLexicalHandler

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.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Remove nsISAXXMLReader.[sg]etProperty

These are unused and also unimplemented.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Remove nsISAXXMLReader.[sg]etFeature

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.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Slim down nsISAXContentHandler

This patch removes three methods that are no-ops (or missing) in all our
implementations.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 7) - Remove nsISAXMutableAttributes

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.

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 8) - Remove nsISAXLocator

None of our nsISAXErrorHandler implementations use the locator arguments.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 9) - Add a comment about nsISAXXMLReader's lack of functionality

Comment 10

[Eric Rahm [:erahm]]

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`

Comment 11

[Eric Rahm [:erahm]]

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.

Comment 12

[Eric Rahm [:erahm]]

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

Comment 13

[Eric Rahm [:erahm]]

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.

Comment 14

[Eric Rahm [:erahm]]

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

Comment 15

[Nicholas Nethercote [inactive]]

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

Comment 16

[Nicholas Nethercote [inactive]]

Attachment: (part 10) - nsSAXXMLReader.cpp doesn't need to be an nsIExtendedExpatSink

Good suggestion!

Comment 17

[Nicholas Nethercote [inactive]]

Attachment: Update comm-central for SAX XML parser changes in mozilla-central

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

Comment 18

[Eric Rahm [:erahm]]

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

Comment 19

[:aceman]

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.

Comment 20

[Nicholas Nethercote [inactive]]

Attachment: Update comm-central for SAX XML parser changes in mozilla-central

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?

Comment 21

[:aceman]

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.

Comment 22

[:aceman]

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

Comment 23

[Nicholas Nethercote [inactive]]

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

[Dorel Luca [:dluca]]

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

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/323b721c2abf
Update comm-central for SAX XML parser changes in mozilla-central. r=aceman,philipp