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)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 61.0

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

References

Details

Attachments

(2 files, 2 obsolete files)

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
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)
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 :-(
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);
Aceman, could you take a look at the C++ part please.
Flags: needinfo?(acelists)
Keywords: leave-open
Version: 52 → Trunk
nsMsgCompose.cpp compiles.
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)
Onto the JS part now.
Flags: needinfo?(acelists)
Let's see where this takes us ;-)
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8971654 - Flags: review?(bzbarsky)
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
I'll do a follow-up if required.
Target Milestone: --- → Thunderbird 61.0
> 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 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 on attachment 8971654 [details] [diff] [review]
1457441-js-part.patch

r=me
Attachment #8971654 - Flags: review?(bzbarsky) → review+
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.
> I don't understand this. "Mailnews IDL"?

See the mailnews/ hits under https://searchfox.org/comm-central/search?q=nsIDOMNode&path=.idl
Ah, I guess there's nothing really under mailnews there.  Great.
See Also: → 1458174
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: