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

RESOLVED FIXED in Thunderbird 60.0

Status

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: jorgk, Assigned: jorgk)

Tracking

Trunk
Thunderbird 60.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

Assignee

Description

Last year
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]).
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
Assignee

Comment 2

Last year
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.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8946597 - Flags: review?(bzbarsky)

Comment 3

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d4624bdffc31
Port bug 1432944 to mailnews: replace use of nsIDOMElement::{Get|Set}Attribute(). rs=bustage-fix
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Assignee

Comment 4

Last year
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.
Target Milestone: --- → Thunderbird 60.0
Version: 52 → Trunk
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.
Attachment #8946597 - Flags: review?(bzbarsky) → review+
Assignee

Comment 6

Last year
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?
Attachment #8946800 - Flags: review?(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.  :(
Attachment #8946800 - Flags: review?(bzbarsky) → review+
Assignee

Comment 8

Last year
Posted patch 1433193-follow-up2.patch (v1) (obsolete) — Splinter Review
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.
Attachment #8946813 - Flags: review?(bzbarsky)
Assignee

Comment 9

Last year
Last patch, I promise ;-) - This one is easy.
Attachment #8946833 - Flags: review?(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.
Attachment #8946813 - Flags: review?(bzbarsky) → review+
Comment on attachment 8946833 [details] [diff] [review]
1433193-follow-up3.patch (v1)

r=me
Attachment #8946833 - Flags: review?(bzbarsky) → review+
Assignee

Comment 12

Last year
(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.
That's what I'm suggesting, yes.  May save trouble with include hell in the long run.
Assignee

Comment 14

Last year
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.
Attachment #8946813 - Attachment is obsolete: true
Attachment #8946946 - Flags: review+
Assignee

Comment 15

Last year
Grrr, forgot to refresh. Now with the forward-declaration.
Attachment #8946946 - Attachment is obsolete: true
Attachment #8946948 - Flags: review+

Comment 16

Last year
Pushed by mozilla@jorgk.com:
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

Last year
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/4b3ae1a74a6b
Follow-up: remove forward declaration. rs=bustage-fix
You need to log in before you can comment on or make changes to this bug.