1514666 - Remove use of nsISAXXMLReader in CalDAV

Status: RESOLVED FIXED

(Calendar :: Provider: ICS/WebDAV, task, P2)

Description

[Henri Sivonen (:hsivonen)]

Bug 1447707 will remove nsISAXXMLReader from m-c.

https://searchfox.org/comm-central/source/calendar/providers/caldav/calDavRequestHandlers.js uses it.

Comment 1

[Philipp Kewisch [:Fallen][📱📆]]

Fun times, thanks for the heads up. I have a patch that refactors a lot of caldav, but still uses the SAX reader. Maybe whatever we do in this bug could be on top of that.

Comment 2

[Henri Sivonen (:hsivonen)]

As noted in bug 1514669, moving the code into c-c would be one option at least for now.

Comment 3

[Henri Sivonen (:hsivonen)]

I expect it to be easier to migrate CalDAV to DOMParser than to migrate XMPP away from SAX, though.

Comment 4

[Henri Sivonen (:hsivonen)]

From the m-c perspective, the removal is now ready to land, but I could wait until the second week of January or so before landing it, since it seems potentially impolite to break c-c at this time of year. Are you planning to import the code to c-c, and if so, should this bug and bug 1514669 be collapsed into one?

Comment 5

[Philipp Kewisch [:Fallen][📱📆]]

I would prefer to remove use of the SAX reader in CalDAV, to do so I'd definitely need time until the second week of January. If we can't get to using DOMParser by then, importing into c-c is definitely an option. If the bugs should be merged depends on Florian's plans.

If you don't mind waiting until the second week of January that would be great!

Comment 6

[Henri Sivonen (:hsivonen)]

OK. I'll wait.

Comment 7

[Jorg K (CEST = GMT+2)]

The Sax parser has now been removed as announced for the second week of January. So should we fork it?
https://hg.mozilla.org/mozilla-central/rev/cc4350821ea2

Comment 8

[Philipp Kewisch [:Fallen][📱📆]]

Yes, forking is best for now.

Comment 9

[Magnus Melin [:mkmelin]]

Forking was done in bug 1518552. Did you ever get a start of this?

Comment 10

[Magnus Melin [:mkmelin]]

Did you ever get started on this? If so, can you attach the WIP?

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

Unfortunately I did not.

Comment 12

[Magnus Melin [:mkmelin]]

See also bug 1647252.

Comment 13

[Ping Chen [:rnons] (away)]

Attachment: Bug 1514666 - Replace nsISAXXMLReader with DOMParser in CalDavRequestHandlers. r=mkmelin,darktrojan

Add class XMLResponsHandler to imitate some behaviors of nsISAXXMLReader.

Comment 14

[Philipp Kewisch [:Fallen][📱📆]]

I took a quick look at the patch and Geoff's comments seem right to me. I would still prefer that instead of trying to imitate nsISAXXMLReader we instead remove that processing and have simple request/response classes such as in https://searchfox.org/comm-central/source/calendar/providers/caldav/modules/CalDavRequest.jsm

I know that may be a larger undertaking, but it would likely simplify the code quite a bit and remove a bunch of xpcom. If that isn't something being done in this bug please file a follow up.

Thank you for working this out!

Comment 15

[Ping Chen [:rnons] (away)]

(In reply to Philipp Kewisch [:Fallen] [:📆][:🧩] from comment #14)

I took a quick look at the patch and Geoff's comments seem right to me. I would still prefer that instead of trying to imitate nsISAXXMLReader we instead remove that processing and have simple request/response classes such as in https://searchfox.org/comm-central/source/calendar/providers/caldav/modules/CalDavRequest.jsm

I know that may be a larger undertaking, but it would likely simplify the code quite a bit and remove a bunch of xpcom. If that isn't something being done in this bug please file a follow up.

Seems it is tracked by bug 1648598.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d2dcf467b8f9
Replace nsISAXXMLReader with DOMParser in CalDavRequestHandlers. r=mkmelin,darktrojan