Closed
Bug 1457441
Opened 6 years ago
Closed 6 years ago
Port bug 1455674 |Get rid of nsIDOMElement| to mailnews
Categories
(MailNews Core :: General, enhancement)
MailNews Core
General
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 61.0
People
(Reporter: jorgk-bmo, Assigned: jorgk-bmo)
References
Details
Attachments
(2 files, 2 obsolete files)
13.88 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
5.51 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
mailnews/compose/src/nsMsgCompose.cpp:9:10: fatal error: 'nsIDOMElement.h' file not found We still use it in a few files: https://dxr.mozilla.org/comm-central/search?q=nsIDOMElement&redirect=false
Assignee | ||
Comment 1•6 years ago
|
||
Boris, that hit us without warning :-( What's the replacement for, for example: nsCOMPtr<nsIDOMElement> rootElement; nsresult rv = m_editor->GetRootElement(getter_AddRefs(rootElement)); GetRoot() didn't appear to compile. All use of Element is via RefPtr now, not longer nsCOMPtr?
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 2•6 years ago
|
||
This fixes some issues in nsMsgCompose.cpp. I have to leave now and won't be back for at least four hours. Very bad timing :-(
Assignee | ||
Comment 3•6 years ago
|
||
Looks like the next replacement is missing is this: rv = htmlEditor->CreateElementWithDefaults(NS_LITERAL_STRING("div"), getter_AddRefs(divElem)); Four occurrences. And then we have nsresult rv = mailEditor->GetEmbeddedObjects(getter_AddRefs(aNodeList)); with nsCOMPtr<nsIDOMElement> domElement; for (i = 0; i < numNodes; i ++) { domElement = do_QueryElementAt(aNodeList, i);
Assignee | ||
Comment 4•6 years ago
|
||
Aceman, could you take a look at the C++ part please.
Assignee | ||
Comment 5•6 years ago
|
||
nsMsgCompose.cpp compiles.
Assignee | ||
Comment 6•6 years ago
|
||
It all compiles. I'm really confused which pointer type should be used here, RefPtr or nsCOMPtr.
Attachment #8971556 -
Attachment is obsolete: true
Attachment #8971646 -
Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
Attachment #8971651 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 8•6 years ago
|
||
Let's see where this takes us ;-)
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/9570d59497ae Port bug 1455674: replace use of nsIDOMElement (C++ part). rs=bustage-fix https://hg.mozilla.org/comm-central/rev/811eb21fce9d Port bug 1455674: replace use of nsIDOMElement (JS part). rs=bustage-fix
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 10•6 years ago
|
||
I'll do a follow-up if required.
Target Milestone: --- → Thunderbird 61.0
Comment 11•6 years ago
|
||
> Boris, that hit us without warning Sorry, I thought I had filed a thing. In any case, this should be pretty straightforward/mechanical to update to. > What's the replacement for, for example: > nsCOMPtr<nsIDOMElement> rootElement; > nsresult rv = m_editor->GetRootElement(getter_AddRefs(rootElement)); Using RefPtr<Element> instead of nsCOMPtr<nsIDOMElement>, just like for all the other cases where we do "webidl Foo;" in xpidl. > All use of Element is via RefPtr now, not longer nsCOMPtr? RefPtr is preferred. You _can_ use nsCOMPtr<Element> if you have to (e.g. if you do_QueryInterface into it), but it's not the preferred idiom.
Comment 12•6 years ago
|
||
Comment on attachment 8971651 [details] [diff] [review] 1457441-nsIDOMElement2.patch (v3) >@@ -340,26 +339,25 @@ nsresult nsMsgCompose::ResetUrisForEmbed >+ mozilla::dom::HTMLImageElement::FromNodeOrNull(domElement); domElement can't be null here; the case when it is took the early-continue above. >+ rv = GetNodeLocation(divElem->AsDOMNode(), address_of(parent), &offset); You may want to start working on getting rid of nsIDOMNode in mailnews too. The fact that you can do this in the mailnews IDL before we kill nsIDOMNode from Gecko should make that simpler.... >+ rv = selection->CollapseNative(divElem, 0); Yeah... when nsIDOMNode dies, CollapseNative will just go away and be replaced with Collapse() taking nsINode. On the other hand, if bug 1387143 gets fixed you will need to use Selection here anyway.... I guess this is OK for now, as long as we're clear that this code will need to get touched again. >+++ b/mailnews/news/src/nsNntpIncomingServer.cpp >+ RefPtr<Element> element; Why is that OK? This file doesn't seem to import this type or the mozilla::dom namespace. I suspect unified builds are all that's saving you here.
Attachment #8971651 -
Flags: review?(bzbarsky) → review+
Comment 13•6 years ago
|
||
Comment on attachment 8971654 [details] [diff] [review] 1457441-js-part.patch r=me
Attachment #8971654 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 14•6 years ago
|
||
Thanks, Boris! (In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #12) > domElement can't be null here; the case when it is took the early-continue > above. I'll fix that. > You may want to start working on getting rid of nsIDOMNode in mailnews too. Filed bug 1458057. I considered doing it here, but time was running out. > The fact that you can do this in the mailnews IDL before we kill nsIDOMNode > from Gecko should make that simpler.... I don't understand this. "Mailnews IDL"? I'd just switch the C++ code to nsINode. > >+++ b/mailnews/news/src/nsNntpIncomingServer.cpp > >+ RefPtr<Element> element; > Why is that OK? It's not OK, I'll fix it. It compiled and as far as I know, we don't do unified compiles.
Comment 15•6 years ago
|
||
> I don't understand this. "Mailnews IDL"? See the mailnews/ hits under https://searchfox.org/comm-central/search?q=nsIDOMNode&path=.idl
Comment 16•6 years ago
|
||
Ah, I guess there's nothing really under mailnews there. Great.
Comment 17•6 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/3584cece3188 Follow-up. r=bz
You need to log in
before you can comment on or make changes to this bug.
Description
•