Closed Bug 1514666 Opened 6 years ago Closed 4 years ago

Remove use of nsISAXXMLReader in CalDAV

Categories

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

Product:
Calendar
Component:
Provider: ICS/WebDAV
Type:
task

Tracking

(thunderbird_esr78 wontfix, thunderbird84 wontfix)

Status:
RESOLVED FIXED
Milestone:
85 Branch
Tracking Flags:
Tracking Status
thunderbird_esr78 --- wontfix
thunderbird84 --- wontfix

People

(Reporter: hsivonen, Assigned: rnons)

References

Details

Attachments

(1 file)

Bug 1514666 - Replace nsISAXXMLReader with DOMParser in CalDavRequestHandlers. r=mkmelin,darktrojan
(deleted), text/x-phabricator-request
Details
Summary: Prepare for m-c removal of nsISAXXMLReader → Prepare for m-c removal of nsISAXXMLReader in CalDAV
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.
As noted in bug 1514669, moving the code into c-c would be one option at least for now.
I expect it to be easier to migrate CalDAV to DOMParser than to migrate XMPP away from SAX, though.
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?
Flags: needinfo?(philipp)
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!
Flags: needinfo?(philipp)

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

Flags: needinfo?(philipp)

Yes, forking is best for now.

Flags: needinfo?(philipp)

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

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

Flags: needinfo?(philipp)

Unfortunately I did not.

Flags: needinfo?(philipp)
Type: enhancement → task
Priority: -- → P2
Summary: Prepare for m-c removal of nsISAXXMLReader in CalDAV → Remove use of nsISAXXMLReader in CalDAV
Assignee: nobody → remotenonsense

Add class XMLResponsHandler to imitate some behaviors of nsISAXXMLReader.

Status: NEW → ASSIGNED

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!

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

Attachment #9184720 - Attachment description: Bug 1514666 - Replace nsISAXXMLReader with DOMParser in CalDavRequestHandlers. r=mkmelin,Fallen → Bug 1514666 - Replace nsISAXXMLReader with DOMParser in CalDavRequestHandlers. r=mkmelin,darktrojan
Keywords: checkin-needed-tb
Target Milestone: --- → 85 Branch

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/d2dcf467b8f9
Replace nsISAXXMLReader with DOMParser in CalDavRequestHandlers. r=mkmelin,darktrojan

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed-tb
Resolution: --- → FIXED
Regressions: 1679776
Regressions: 1666156
No longer regressions: 1666156
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: