Closed Bug 1452352 Opened 2 years ago Closed 2 years ago

Port bug 1452235 - Remove nsIDOMSerializer

Categories

(MailNews Core :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 61.0

People

(Reporter: jorgk-bmo, Assigned: Paenglab)

References

Details

Attachments

(1 file, 3 obsolete files)

+++ This bug was initially created as a clone of Bug #1452235 +++

Boris will be removing the "@mozilla.org/xmlextras/xmlserializer;1" and replacing it with ChromeUtils.importGlobalProperties(["XMLSerializer"]); followed by |new XMLSerializer()|.  There look like one use each in calendar and mailnews that will need updating.

Richard, could to take this?
Flags: needinfo?(richard.marti)
Attached patch nsIDOMSerializer.patch (obsolete) — Splinter Review
Jörg, is this what you thought? I haven't tested it.
Flags: needinfo?(richard.marti)
Attachment #8965938 - Flags: review?(jorgk)
Comment on attachment 8965938 [details] [diff] [review]
nsIDOMSerializer.patch

Review of attachment 8965938 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

::: calendar/base/modules/calXMLUtils.jsm
@@ +162,5 @@
>   * @param doc       The DOM document to serialize
>   * @return          The DOM document as a string.
>   */
>  cal.xml.serializeDOM = function(doc) {
> +    let serializer = new XMLSerializer());

You have two closing parenthesis here, at the other call site as well.
Attachment #8965938 - Flags: review?(philipp)
Attachment #8965938 - Flags: review?(jorgk)
Attachment #8965938 - Flags: review+
Attached patch nsIDOMSerializer.patch (obsolete) — Splinter Review
Fixed the parenthesis.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8965942 - Flags: review?(philipp)
Attachment #8965938 - Attachment is obsolete: true
Attachment #8965938 - Flags: review?(philipp)
Comment on attachment 8965942 [details] [diff] [review]
nsIDOMSerializer.patch

Review of attachment 8965942 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with these changes:

::: calendar/base/modules/calXMLUtils.jsm
@@ +10,1 @@
>  Components.utils.importGlobalProperties(["XMLHttpRequest"]);

You can just combine this to 

Cu.importGlobalProperties(["XMLSerializer", "XMLHttpRequest"]);

I believe importGlobalProperties is not available on ChromeUtils

::: mailnews/extensions/newsblog/content/feed-parser.js
@@ +2,5 @@
>   * This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +ChromeUtils.importGlobalProperties(["XMLSerializer"]);

Same here, you need to use Cu instead.
Attachment #8965942 - Flags: review?(philipp) → review+
Attached patch nsIDOMSerializer.patch (obsolete) — Splinter Review
Changed to Cu. I also changed the line 
Components.utils.importGlobalProperties(["XMLHttpRequest"]);
to
Cu.importGlobalProperties(["XMLHttpRequest"]);
in calXMLUtils.jsm.
Attachment #8965942 - Attachment is obsolete: true
Attachment #8965945 - Flags: review+
Keywords: checkin-needed
(In reply to Philipp Kewisch [:Fallen]  from comment #4)
> You can just combine this to 
> Cu.importGlobalProperties(["XMLSerializer", "XMLHttpRequest"]);
Philipp wanted one line here, sorry.
Ah, now I see how this was meant. Sorry, not the best on JS.
Attachment #8965945 - Attachment is obsolete: true
Attachment #8965961 - Flags: review+
I remove the c-n as it would break TB without bug 1452235 landed (not checked with bug 1452235 but without).
Keywords: checkin-needed
Hmm, I've asked Boris in bug 1452235 comment #7. We have this already:
mailnews/extensions/newsblog/content/feed-subscriptions.js
2357 let serializer = new XMLSerializer();
So I'm unsure, but I wasn't going to land it today.
Maybe it's because of the Cu.importGlobalProperties. With the patch applied I couldn't open the console.
You can find the list of supported properties for Cu.importGlobalProperties here: https://searchfox.org/comm-central/source/mozilla/js/xpconnect/src/Sandbox.cpp#801

it doesn't contain XMLSerializer yet, so it would break.

using new XMLSerializer() already works in DOM windows, just not in jsms etc, which is what Cu.importGlobalProperties is for.
Bug 1452235 is in m-i now.
Keywords: checkin-needed
Pushed by richard.marti@gmail.com:
https://hg.mozilla.org/comm-central/rev/78c17f98345c
Port bug 1452235: Replace use of nsIDOMSerializer. r=jorgk
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 61.0
The change in calendar/modules/utils/calXMLUtils.jsm got overwritten by recent Calendar refactoring, so doing it again :-(
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c5068dc7ac72
Port bug 1452235: Replace use of nsIDOMSerializer (follow-up). r=me
You need to log in before you can comment on or make changes to this bug.