1433193 - Port |Bug 1432944 - Remove all members from nsIDOMElement| to mailnews

Status: RESOLVED FIXED

(MailNews Core :: Backend, enhancement)

Description

[Jorg K (CEST = GMT+2)]

As per bug 1432944 comment #0 {Get|Set|Has}Attribute() need some changes:

https://searchfox.org/comm-central/search?q=-%3ESetAttribute%28&case=true&path=mailnews
https://searchfox.org/comm-central/search?q=-%3EHasAttribute%28&case=true&path=mailnews
https://searchfox.org/comm-central/search?q=-%3EGetAttribute%28&case=true&path=mailnews

Boris will tell us what those changes are since he said in bug 1432944 comment #2 (quote):
  Note that you will NOT be able to land sane GetAttribute changes
  before the actual patches for bug 1432944 land.

So Boris, can you please give some details.

Note that many/most/all nsIDOMElement::GetAttribute calls will already be switched to mozilla::dom::Element::GetAttribute in bug 1432603 (current patch in attachment 8945258 [details] [diff] [review]).

Comment 1

[Boris Zbarsky [:bzbarsky]]

So I think I might have been wrong.  The main issue with GetAttribute is that right now the two-arg form will work on Element only if the second arg is a subclass of nsString.  It will not work with nsAString as the second arg.

So you can do it if you don't have any situations like that.  If you _do_ have them, your compiler will tell you, and those will need to either get into a stack string that then copies into the nsAString, or wait for bug 1432944 to land and change the second arg of Element::GetAttribute to nsAString.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1433193-get-set-attribute.patch (v1)

This should do it. I removed nsMsgDBView::CycleThreadedColumn() since I couldn't find any reference to it.

I was in the mood to remove nsNntpIncomingServer::SetTree() and nsNntpIncomingServer::CycleHeader() since I can't see this used either. But there are other classes providing those methods, so perhaps they are used, I didn't have much time to research it.

I'll land this once my local compile succeeds.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d4624bdffc31
Port bug 1432944 to mailnews: replace use of nsIDOMElement::{Get|Set}Attribute(). rs=bustage-fix

Comment 4

[Jorg K (CEST = GMT+2)]

Boris, any issues you might find in the patch will be addressed in a follow-up patch. I think all the changes are straight forward, maybe there is a better way to get the "document element" here
https://hg.mozilla.org/comm-central/rev/d4624bdffc31#l3.26
to avoid the QI.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8946597 [details] [diff] [review]
1433193-get-set-attribute.patch (v1)

r=me with a few suggestions for improving the code.

>@@ -299,18 +300,18 @@ nsresult nsMsgMailSession::GetTopmostMsg
>+      nsCOMPtr<Element> domElement2 = do_QueryInterface(domElement);
>+      domElement2->GetAttribute(NS_LITERAL_STRING("windowtype"), windowType);

This method is a little silly.  It starts with 

       domDocument = do_QueryInterface(topMostWindow->GetDoc());

where topMostWindow is nsPIDOMWindowOuter.  Then it works with that.  How about:

    nsIDocument* domDocument = topMostWindow->GetDoc();
    NS_ENSURE_TRUE(domDocument, NS_ERROR_FAILURE);

    Element* domElement = domDocument->GetDocumentElement();
    NS_ENSURE_TRUE(domElement, NS_ERROR_FAILURE);

    domElement->GetAttribute(NS_LITERAL_STRING("windowtype"), windowType);

Way less QI, way less addref (though you can make domDocument and domElement strong refs if you wnt to be safe), won't break again when I remove nsIDOMDocument bits in bug 1434318.

>@@ -489,19 +489,24 @@ nsresult nsMsgCompose::TagEmbeddedObject

Here, you could QI directly to Element coming out of aNodeList.  Change IsEmbeddedObjectSafe to take Element*, remove the QI in there.  If QI fails, IsEmbeddedObjectSafe will already return false on null input, so you will just go on to the next node.  Would be clearer, and more forward-looking for when we remove nsIDOMNode.

You may want to call the three-arg version of SetAttribute instead of passing an explicit nullptr for the principal.  Would be clearer, I think.  Same at the other SetAttribute callsites.

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1433193-follow-up.patch (v1)

Hi Boris, thanks for the quick review and the suggestions. Sorry for landing something sub-optimal (but it got the show back on the road and others working again).

Here's the patch to implement all suggestions.

I've stared at this code for an hour. All the objects we process come from HTMLEditor::GetEmbeddedObjects() which returns four things: img, a, embed and body with background.

However, code in nsMsgCompose::IsEmbeddedObjectSafe() deals with img, a and link, which is not returned at all. embed is not treated. Equally, nsMsgComposeAndSend::GetEmbeddedObjectInfo() looks at img, a, link and body. Again, it will never see a link.

Am I missing something?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8946800 [details] [diff] [review]
1433193-follow-up.patch (v1)

r=me

I don't think you're missing anything.  Just more editor/mailnews nonsense code.  :(

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 1433193-follow-up2.patch (v1)

While looking at "send", the counter part of "compose" I came up with some more simplifications. Can I trouble you with another review while we're here? This also removes a bunch of nsIDOMNode uses.

Note that those "embedded objects" can be passed into nsMsgComposeAndSend::CreateRFC822Message() and the Outlook import uses this to pass in a list of nsIMsgEmbeddedImageData objects.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1433193-follow-up3.patch (v1)

Last patch, I promise ;-) - This one is easy.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8946813 [details] [diff] [review]
1433193-follow-up2.patch (v1)

>+++ b/mailnews/compose/src/nsMsgSend.cpp
>+    domSaveArray[j].element = domElement;

This can end up null, right?

>@@ -1810,23 +1800,21 @@ nsMsgComposeAndSend::ProcessMultipartRel
>+      RefPtr<HTMLImageElement>  image  = HTMLImageElement::FromContent(domSaveArray[j].element);
>+      RefPtr<HTMLLinkElement>   link   = HTMLLinkElement::FromContent(domSaveArray[j].element);
>+      RefPtr<HTMLAnchorElement> anchor = HTMLAnchorElement::FromContent(domSaveArray[j].element);
>+      RefPtr<HTMLBodyElement>   body   = HTMLBodyElement::FromContent(domSaveArray[j].element);

So all these should be FromContentOrNull.  Please make sure to fix that.

>+++ b/mailnews/compose/src/nsMsgSend.h

Probably better to forward-declare in the header and include in the .cpp.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8946833 [details] [diff] [review]
1433193-follow-up3.patch (v1)

r=me

Comment 12

[Jorg K (CEST = GMT+2)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #10)
> This can end up null, right?
Yes, on the Outlook import branch. Good catch, thanks.

> >+      RefPtr<HTMLBodyElement>   body   = HTMLBodyElement::FromContent(domSaveArray[j].element);
> So all these should be FromContentOrNull.  Please make sure to fix that.
Maybe better to put this into the preceding if.

> >+++ b/mailnews/compose/src/nsMsgSend.h
> Probably better to forward-declare in the header and include in the .cpp.
I guess you're suggesting got move
+#include "mozilla/dom/Element.h"
to the .cpp file (where it wouldn't be necessary since we include mozilla/dom/HTML* stuff).
Hmm, there was no complaint about this one:
https://hg.mozilla.org/comm-central/rev/cde38eb25eba56a89cbd4afd4d757db567065f92#l2.12
where I added the same thing to nsMsgCompose.h.

Comment 13

[Boris Zbarsky [:bzbarsky]]

That's what I'm suggesting, yes.  May save trouble with include hell in the long run.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: 1433193-follow-up2.patch (v1b)

OK, I removed the include and added a forward-declaration.

Speaking of hell, have you ever seen something like this:
for (i = mPreloadedAttachmentCount; i < (mPreloadedAttachmentCount + multipartCount);)

Yes, no 'i++'. So that's essentially a while loop, and yes, there is a 'continue' which relies on the index not being incremented. Nice, right? I took that out and made it a while loop.

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: 1433193-follow-up2.patch (v1c)

Grrr, forgot to refresh. Now with the forward-declaration.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4007d551bf7a
Follow-up: misc. code improvements. r=bz
https://hg.mozilla.org/comm-central/rev/22d78d1bbe82
Follow-up: misc. code improvements in nsMsgSend.cpp. r=bz
https://hg.mozilla.org/comm-central/rev/702ee2134198
Follow-up: misc. code improvements in nsMsgCompose.cpp. r=bz

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4b3ae1a74a6b
Follow-up: remove forward declaration. rs=bustage-fix