Closed Bug 1416038 Opened 3 years ago Closed 3 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: njn, Assigned: njn)

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.