Closed Bug 1433193 Opened 2 years ago Closed 2 years ago

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


(MailNews Core :: Backend, enhancement)

Not set


(Not tracked)

Thunderbird 60.0


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



(4 files, 2 obsolete files)

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

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)
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
Attachment #8946597 - Flags: review?(bzbarsky)
Pushed by
Port bug 1432944 to mailnews: replace use of nsIDOMElement::{Get|Set}Attribute(). rs=bustage-fix
Closed: 2 years ago
Resolution: --- → FIXED
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
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();

    Element* domElement = domDocument->GetDocumentElement();

    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+
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)


I don't think you're missing anything.  Just more editor/mailnews nonsense code.  :(
Attachment #8946800 - Flags: review?(bzbarsky) → review+
Attached 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)
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)

Attachment #8946833 - Flags: review?(bzbarsky) → review+
(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:
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.
Attached patch 1433193-follow-up2.patch (v1b) (obsolete) — Splinter Review
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+
Grrr, forgot to refresh. Now with the forward-declaration.
Attachment #8946946 - Attachment is obsolete: true
Attachment #8946948 - Flags: review+
Pushed by
Follow-up: misc. code improvements. r=bz
Follow-up: misc. code improvements in nsMsgSend.cpp. r=bz
Follow-up: misc. code improvements in nsMsgCompose.cpp. r=bz
Pushed by
Follow-up: remove forward declaration. rs=bustage-fix
You need to log in before you can comment on or make changes to this bug.