Closed Bug 1445396 Opened 6 years ago Closed 6 years ago

Port bug 1433566 - nsIDOMText is going away

Categories

(Thunderbird :: General, defect)

Product:
Thunderbird
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 61.0

People

(Reporter: bzbarsky, Assigned: jorgk-bmo)

References

Details

Attachments

(4 files, 2 obsolete files)

1445396-remove-unneeded-includes.patch (v3)
6.58 KB, patch
aceman
: review+
Details | Diff | Splinter Review
1445396-add-nsIDOMElement.patch
1.68 KB, patch
aceman
: review+
Details | Diff | Splinter Review
1445396-cal.patch
4.17 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
1445396-chat.patch
1.88 KB, patch
florian
: review+
Details | Diff | Splinter Review 
See bug 1445354.  There are various places in calendar and chat that test for instanceof Ci.nsIDOMText.  They should switch to testing .nodeType for (TEXT_NODE or CDATA_SECTION_NODE).
Thanks, ETA?
I plan to post the patches today.  Review might take a day or two, maybe.  I can hold off on landing a few days if needed.
Thanks, no need. There are only a handful of uses.
Let's start with this.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8958601 - Flags: review?(acelists)
Sorry, I needed something to land, make that a PLR ;-)
Keywords: leave-open
Found another one.
Attachment #8958601 - Attachment is obsolete: true
Attachment #8958601 - Flags: review?(acelists)
Attachment #8958626 - Flags: review?(acelists)
Sorry about the noise, found some more.
Attachment #8958626 - Attachment is obsolete: true
Attachment #8958626 - Flags: review?(acelists)
Attachment #8958629 - Flags: review?(acelists)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b8667006d07d
remove unneeded includes of nsIDOMText.h, nsIDOMElement.h and nsIDOMNodeList.h. r=aceman
Comment on attachment 8958629 [details] [diff] [review]
1445396-remove-unneeded-includes.patch (v3)

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

Thanks for the cleanup.
Can you add the include to mailnews/compose/src/nsMsgCompose.cpp ? nsIDOMElement is used there.
Also why do you remove it from nsNntpIncomingServer.cpp? That file does use nsIDOMElement. It does compile now, probabbly the file is included silently from some m-c file. But that may break any time, when they do cleanup.
After this patch no mailnews/ file includes this, but the uses are there.
Attachment #8958629 - Flags: review?(acelists)
For the JS part, take a look at the patch in bug 1433566, attachment 8945950 [details] [diff] [review]. JS tests both.
(In reply to :aceman from comment #9)
> Can you add the include to mailnews/compose/src/nsMsgCompose.cpp ?
> nsIDOMElement is used there.
OK.

> Also why do you remove it from nsNntpIncomingServer.cpp? That file does use
> nsIDOMElement.
Oops ;-(
Comment on attachment 8958629 [details] [diff] [review]
1445396-remove-unneeded-includes.patch (v3)

You can still approve this together with the next one.
Attachment #8958629 - Flags: review?(acelists)
Re-instate over-zealously removed include and also add it elsewhere.
Attachment #8958644 - Flags: review?(acelists)
Comment on attachment 8958644 [details] [diff] [review]
1445396-add-nsIDOMElement.patch

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

OK, thanks.
Attachment #8958644 - Flags: review?(acelists) → review+
Comment on attachment 8958629 [details] [diff] [review]
1445396-remove-unneeded-includes.patch (v3)

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

Thanks.
Attachment #8958629 - Flags: review?(acelists) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/397773b5b6d7
Follow-up: reinstate include of nsIDOMElement.h where needed. r=aceman
Conversion per comment 0. You may know better whether the place really needs to check CDATA too.
Attachment #8958660 - Flags: review?(philipp)
Conversion per comment 0. You may know better whether the place really needs to check CDATA too.
Attachment #8958661 - Flags: review?(clokep)
Comment on attachment 8958660 [details] [diff] [review]
1445396-cal.patch

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

Both text and cdata sounds right to me, thanks for the patch.
Attachment #8958660 - Flags: review?(philipp) → review+
I'll have to steal Patrick's review here soon since bug 1445354 just landed on inbound.
Summary: nsIDOMText is going away → Port bug 1445354 - nsIDOMText is going away
Sorry, bug 1433566.
Summary: Port bug 1445354 - nsIDOMText is going away → Port bug 1433566 - nsIDOMText is going away
Attachment #8958661 - Flags: review?(clokep) → review+
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d769ecd016c7
remove nsIDOMText use in calendar. r=philipp
https://hg.mozilla.org/comm-central/rev/5391a6864d53
remove nsIDOMText use in chat. r=florian
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 61.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: