Port bug 1434318 (removing various stuff from nsIDOMDocument) to comm-central

RESOLVED FIXED in Thunderbird 60.0

Status

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: bzbarsky, Assigned: jorgk)

Tracking

unspecified
Thunderbird 60.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

At the very least, GetDocumentElement is going away.  But I'm hoping to basically remove all nsIDOMDocument members.
CreateEvent is another one that is definitely used in comm-central.
Assignee

Comment 2

Last year
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #0)
> At the very least, GetDocumentElement is going away.  But I'm hoping to
> basically remove all nsIDOMDocument members.
I hope there are some replacements ;-)

Some code snippets:
mailnews/compose/src/nsMsgComposeService.cpp
  nsCOMPtr<nsIDOMDocument> domDocument;
  rv = contentViewer->GetDOMDocument(getter_AddRefs(domDocument));
  NS_ENSURE_SUCCESS(rv, rv);
  nsCOMPtr<nsIDocumentEncoder> docEncoder(do_CreateInstance(NS_HTMLCOPY_ENCODER_CONTRACTID, &rv));
  NS_ENSURE_SUCCESS(rv, rv);
  rv = docEncoder->Init(domDocument, NS_LITERAL_STRING("text/html"), 0);
  NS_ENSURE_SUCCESS(rv, rv);
Or that's not affected since it doesn't all a method on the object?

mailnews/compose/src/nsMsgCompose.cpp
  nsCOMPtr<nsIDOMDocument> doc;
  rv = m_editor->GetDocument(getter_AddRefs(doc));
  NS_ENSURE_SUCCESS_VOID(rv);
[snip]
    nsCOMPtr<nsIDOMText> textNode;
    rv = doc->CreateTextNode(... some text ..., getter_AddRefs(textNode));

In chat we use CreateEvent() and in some obscure importer (mailnews/import/winlivemail) there is a bunch more.
> I hope there are some replacements ;-)

Sure, all the stuff on nsIDocument that webidl bindings use.  ;)

> I hope there are some replacements ;-)

It'll be affected, because I'm removing the GetDOMDocument you're using there.  But:

  nsCOMPtr<nsIDOMDocument> domDocument = do_QueryInterface(contentViewer->GetDocument());

will work.

>    rv = doc->CreateTextNode(... some text ..., getter_AddRefs(textNode));

You'll need to QI to nsIDocument for now and use the CreateTextNode on that.  Long-term, of course, nsIEditor will start returning nsIDocument instead of nsIDOMDocument.

> In chat we use CreateEvent()

Yep, nsIDocument.
Assignee

Comment 4

Last year
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #3)
>   nsCOMPtr<nsIDOMDocument> domDocument =
> do_QueryInterface(contentViewer->GetDocument());
> will work.
Will work when bug 1434318 lands or should work now? Currently I get a compile error.

Equally, looking at nsMsgCompose.cpp, I tried following
https://searchfox.org/mozilla-central/rev/c56f656febb1aa58c72b218699e24a76425bd284/dom/xslt/xslt/txMozillaXSLTProcessor.cpp#1144
so
    nsCOMPtr<nsIDocument> doc2 = do_QueryInterface(doc);
    RefPtr<nsTextNode> textNode =
      doc2->CreateTextNode(Substring(aText, start, delimiter - start));

    nsCOMPtr<nsINode> divElem2 = do_QueryInterface(divElem);
    IgnoredErrorResult rv2;
    divElem2->AppendChild(*textNode, rv2);
and that doesn't compile :-(
'nsINode *nsINode::AppendChild(nsINode &,mozilla::ErrorResult &)': cannot convert argument 1 from 'nsTextNode' to 'nsINode &'

Also, nsTextNode can't be QI'ed to nsINode.

There's also
https://searchfox.org/mozilla-central/rev/c56f656febb1aa58c72b218699e24a76425bd284/editor/libeditor/HTMLEditorObjectResizer.cpp#673
where we have |nsCOMPtr<nsIContent> textInfo| a few lines above.
Flags: needinfo?(bzbarsky)
> Will work when bug 1434318 lands or should work now? 

Should work now.  (Also bug 1434318 just landed on inbound, but that's undelated.)

> Currently I get a compile error.

I don't see why.  https://searchfox.org/mozilla-central/source/docshell/base/nsIContentViewer.idl#134 has existed for a while.

What compile error do you get?

> 'nsINode *nsINode::AppendChild(nsINode &,mozilla::ErrorResult &)': cannot convert argument 1 from 'nsTextNode' to 'nsINode &'

Did you #include "nsTextNode.h"?

> where we have |nsCOMPtr<nsIContent> textInfo| a few lines above.

That's already using the nsIDocument version, not the nsIDOMDocumen one, so is not affected by any of this stuff.
Flags: needinfo?(bzbarsky)
Assignee

Comment 6

Last year
Bah, include missing :-( - I'll get back to you shortly.
Assignee

Comment 7

Last year
This should do it. A few notes:
1) nsTextNode.h is not exported, so the "local includes" trick.
   We can fix this in a follow-up if desired.
2) im/components/mintrayr/trayToolkit.cpp which uses
   nsIDOMDocument::CreateEvent() is Instantbird code which I
   can't even compile. So I'm not fixing that.
   Thunderbird's chat code is in chat/
3) I haven't looked a the stuff in
   mailnews/import/winlivemail/nsWMSettings.cpp
   mailnews/import/winlivemail/nsWMUtils.cpp
   mailnews/import/winlivemail/nsWMUtils.h
   in detail, looks like the only methods called on it are:
   nsWMUtils.cpp:146, nsIDOMParser::ParseFromStream() and
   nsWMUtils.cpp:158, GetElementsByTagName() which I have replaced.

BTW, the hunk in nsWMUtils.cpp doesn't compile:
c:\mozilla-source\comm-central\obj-i686-pc-mingw32\dist\include\nsCOMPtr.h(525): error C2440: 'static_cast': cannot convert from 'nsContentList *' to 'nsINodeList *'
c:\mozilla-source\comm-central\obj-i686-pc-mingw32\dist\include\nsCOMPtr.h(525): note: Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast
c:/mozilla-source/comm-central/mailnews/import/winlivemail/nsWMUtils.cpp(159): note: see reference to function template instantiation 'nsCOMPtr<nsINodeList>::nsCOMPtr<nsContentList>(already_AddRefed<nsContentList> &&)' being compiled
c:/mozilla-source/comm-central/mailnews/import/winlivemail/nsWMUtils.cpp(159): note: see reference to function template instantiation 'nsCOMPtr<nsINodeList>::nsCOMPtr<nsContentList>(already_AddRefed<nsContentList> &&)' being compiled
c:\mozilla-source\comm-central\obj-i686-pc-mingw32\dist\include\mozilla/TypeTraits.h(673): error C2139: 'nsContentList': an undefined class is not allowed as an argument to compiler intrinsic type trait '__is_base_of'
c:\mozilla-source\comm-central\obj-i686-pc-mingw32\dist\include\nsIDocument.h(207): note: see declaration of 'nsContentList'
c:\mozilla-source\comm-central\obj-i686-pc-mingw32\dist\include\mozilla/TypeTraits.h(743): note: see reference to class template instantiation 'mozilla::detail::BaseOfTester<Base,Derived>' being compiled with ...

although I think I copied your part 7.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8947243 - Flags: feedback?(bzbarsky)
Comment on attachment 8947243 [details] [diff] [review]
1434515-IDOMDocument.patch (v1) - landed after adding missing include.

>+  nsCOMPtr<nsINode> domNode = list->Item(0);

Probably better to just keep this one named "node".

Looks good to me.
Attachment #8947243 - Flags: review+
Attachment #8947243 - Flags: feedback?(bzbarsky)
Attachment #8947243 - Flags: feedback+
Assignee

Comment 9

Last year
Thanks for the r+, it sure looks good and with the previous variable name 'node' even better ;-)
But it doesn't compile. What am I missing?
> But it doesn't compile.

What is the error?
Assignee

Comment 11

Last year
Comment #7:
nsWMUtils.cpp(159) is |nsCOMPtr<nsINodeList> list = domDocument->GetElementsByTagName(tagName);|.
You can see the number in the patch.
Oh, I thought you meant using "node" does not compile.  ;)

The comment 7 error is because you did not include nsContentList.h.

Comment 13

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/50b6d1240504
Port bug 1434318: Replace various uses of IDOMDocument. r=bz
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Assignee

Comment 14

Last year
Thanks again, Boris. I landed this now after adding the missing include. We'll see whether I covered everything when bug 1434318 lands on M-C. What's the next target? The Necko guys also keep me busy by making changes to URL handling.
Target Milestone: --- → Thunderbird 60.0
Assignee

Updated

Last year
Attachment #8947243 - Attachment description: 1434515-IDOMDocument.patch (v1) - WIP → 1434515-IDOMDocument.patch (v1) - landed after adding missing include.
> What's the next target?

I don't know yet.  I'll definitely keep you posted.  ;)
Assignee

Comment 16

Last year
Missed one :-(
Comment on attachment 8947300 [details] [diff] [review]
1434515-one-more.patch

You need to handle the case of GetDocumentElement() returning null, no?  The old code did.

Comment 18

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/4ec396880934
Port bug 1434318: Replace various uses of IDOMDocument (one more). rs=bustage-fix
Please fix the null-check problem in the bustage fix.
Flags: needinfo?(jorgk)
Assignee

Comment 20

Last year
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #17)
> You need to handle the case of GetDocumentElement() returning null, no?  The
> old code did.
Hmm, yes, looking at it again, I thought the same. That will happen for an empty document?
_NodeTreeConvertible() checks the input and returns an error, which slightly changes the behaviour.

So I'd just have to add: |if (!rootElement) return NS_OK;| if the case can usually occur. But in an e-mail, isn't there always a body element?
> That will happen for an empty document?

One where the root was removed, yes.

> But in an e-mail, isn't there always a body element?

Dunno.  Do scripts get to run?  If so, no.
Assignee

Comment 22

Last year
I'll look at it again tomorrow since my thinking is a little clouded at 1:43 AM :-(

In the old code, could the case ever arise that rv==NS_OK but no root element returned?
If so, that could would have returned OK without setting the _retval argument, which is not exactly brilliant.

If the old GetDocumentElement() only returned a null element together with an error status, than the behaviour hasn't changed since _NodeTreeConvertible() checks the input. But I could add |if (!rootElement) return someError;|

Overall, this is used once in JS in MsgComposeCommands.js#5353, and there it's protected by a try/catch, so even if before it returned OK and now throws on an empty document, that's handled. The convertibility doesn't matter for an empty document since there's nothing to convert.

Do you have a preference?
> In the old code, could the case ever arise that rv==NS_OK but no root element returned?

In general, yes, in exactly the cases when nsIDocument::GetDocumentElement returns null.

> returned OK without setting the _retval argument, which is not exactly brilliant.

Yeah, that looks broken...

> If the old GetDocumentElement() only returned a null element together with an error status

It never returned an error status, fwiw.

Throwing on no root element makes sense to me.
Assignee

Comment 24

Last year
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #23)
> Throwing on no root element makes sense to me.
OK. Sorry about spending so much time on this little detail. Do you think we should leave it as it is and rely on _NodeTreeConvertible() checking its input, or insert |if (!rootElement) return NS_ERROR_NULL_POINTER;| before the call? A matter of taste, no? But since you explicitly asked me to fix the null-check problem, I want to make sure we agree.
Flags: needinfo?(bzbarsky)
I have no particular opinion; I have no idea what the API contract for _NodeTreeConvertible is.
Flags: needinfo?(bzbarsky)
Assignee

Comment 26

Last year
API contract, hmm, that's a big ask in undocumented legacy code :-(

OK, I'll add a comment like this and we close the chapter.
// In the unlikely event that there is no root element, _NodeTreeConvertible() will return an
// error, so no need to check here.
Assignee

Comment 27

Last year
I've looked at the error handling in that particular function and it needs a clean-up, so I filed bug 1434972. What has been landed here works and doesn't cause any crashes. Adding a comment won't fix the error handling which is bad. So I'll continue in the follow-up bug.
Flags: needinfo?(jorgk)
You need to log in before you can comment on or make changes to this bug.