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

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Boris Zbarsky [:bzbarsky]]

At the very least, GetDocumentElement is going away.  But I'm hoping to basically remove all nsIDOMDocument members.

Comment 1

[Boris Zbarsky [:bzbarsky]]

CreateEvent is another one that is definitely used in comm-central.

Comment 2

[Jorg K (CEST = GMT+2)]

(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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 4

[Jorg K (CEST = GMT+2)]

(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.

Comment 5

[Boris Zbarsky [: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.

Comment 6

[Jorg K (CEST = GMT+2)]

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

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1434515-IDOMDocument.patch (v1) - landed after adding missing include.

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.

Comment 8

[Boris Zbarsky [: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.

Comment 9

[Jorg K (CEST = GMT+2)]

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?

Comment 10

[Boris Zbarsky [:bzbarsky]]

> But it doesn't compile.

What is the error?

Comment 11

[Jorg K (CEST = GMT+2)]

Comment #7:
nsWMUtils.cpp(159) is |nsCOMPtr<nsINodeList> list = domDocument->GetElementsByTagName(tagName);|.
You can see the number in the patch.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Oh, I thought you meant using "node" does not compile.  ;)

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/50b6d1240504
Port bug 1434318: Replace various uses of IDOMDocument. r=bz

Comment 14

[Jorg K (CEST = GMT+2)]

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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> What's the next target?

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

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1434515-one-more.patch

Missed one :-(

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4ec396880934
Port bug 1434318: Replace various uses of IDOMDocument (one more). rs=bustage-fix

Comment 19

[Boris Zbarsky [:bzbarsky]]

Please fix the null-check problem in the bustage fix.

Comment 20

[Jorg K (CEST = GMT+2)]

(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?

Comment 21

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 22

[Jorg K (CEST = GMT+2)]

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?

Comment 23

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 24

[Jorg K (CEST = GMT+2)]

(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.

Comment 25

[Boris Zbarsky [:bzbarsky]]

I have no particular opinion; I have no idea what the API contract for _NodeTreeConvertible is.

Comment 26

[Jorg K (CEST = GMT+2)]

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.

Comment 27

[Jorg K (CEST = GMT+2)]

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.