Closed Bug 1272893 Opened 4 years ago Closed 3 years ago

remove some remaining uses of nsISupportsArray in send/import code of mailnews

Categories

(MailNews Core :: Composition, defect, minor)

defect
Not set
minor

Tracking

(firefox51 fixed)

RESOLVED FIXED
Thunderbird 51.0
Tracking Status
firefox51 --- fixed

People

(Reporter: aceman, Assigned: aceman)

References

Details

Attachments

(2 files, 3 obsolete files)

/mailnews/compose/public/nsIMsgSend.idl
    line 36 -- interface nsISupportsArray;
    line 253 -- in nsISupportsArray aEmbeddedObjects,

/mailnews/compose/src/nsMsgCompose.cpp
    line 327 -- nsCOMPtr<nsISupportsArray> aNodeList;
    line 449 -- nsCOMPtr<nsISupportsArray> aNodeList;

/mailnews/compose/src/nsMsgSend.cpp
    line 4043 -- nsISupportsArray *aEmbeddedObjects,

/mailnews/compose/src/nsMsgSend.h
    line 133 -- #include "nsISupportsArray.h"
    line 258 -- nsCOMPtr<nsISupportsArray> mEmbeddedObjectList;

/mailnews/import/src/nsImportService.cpp
    line 36 -- #include "nsISupportsArray.h"
    line 332 -- nsCOMPtr<nsISupportsArray> supportsArray;
    line 333 -- NS_NewISupportsArray(getter_AddRefs(supportsArray));
    line 343 -- supportsArray->AppendElement(item);
    line 350 -- supportsArray,

/mailnews/base/src/nsMsgAccountManager.h
    line 56 -- nsCOMPtr <nsISupportsArray> m_searchTerms;
Can you please refresh my memory on why we're doing that and what is the replacement.
See bug 394167 and bug 792209. Supposedly it is "the bad thing" :)
Attached patch patch for mailnews (obsolete) — Splinter Review
This works for me provided the other m-c patch is accepted.
Attachment #8752605 - Flags: review?(Pidgeot18)
Attached patch patch for mozilla-central (obsolete) — Splinter Review
This is a prerequisite for the mailnews patch. I hope m-c will welcome removal of nsISupportsArray. I was not sure if nodes->AppendElement() should use weak or strong reference to the added objects. But weak reference caused a test failure in c-c.

I have not found any uses GetEmbeddedObjects in m-c.
Attachment #8752606 - Flags: feedback?(ehsan)
Attachment #8752606 - Flags: feedback?(Pidgeot18)
Blocks: 1273001
Attachment #8752606 - Flags: feedback?(ehsan) → feedback?(masayuki)
Comment on attachment 8752606 [details] [diff] [review]
patch for mozilla-central

I don't see any problems from this patch.
Attachment #8752606 - Flags: feedback?(masayuki) → feedback+
Blocks: 1294369
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) from comment #6)
> Comment on attachment 8752606 [details] [diff] [review]
> patch for mozilla-central
> 
> I don't see any problems from this patch.

Thanks. Do you also understand why nodes->AppendElement(domNode, false); needs to have a strong reference (the 'false' argument) ? I just know with a weak reference our tests failed.
What are the test failures?

(Better to NI? Masayuki-san).
Flags: needinfo?(masayuki)
Attached patch patch for mozilla-central v1.1 (obsolete) — Splinter Review
Attachment #8752606 - Attachment is obsolete: true
Attachment #8752606 - Flags: feedback?(Pidgeot18)
Attachment #8791431 - Flags: review?(masayuki)
Although, I'm not sure which tests fail with weak reference. It seems that appending element with strong reference is correct since DOM nodes which are stored in the result of the method may be removed from the document after that.  However, the array may be accessible even after that.  Additionally, dom::Element is not a weak referable.  Then, AppendElement() must do nothing.
https://dxr.mozilla.org/mozilla-central/rev/8a494adbc5cced90a4edf0c98cffde906bf7f3ae/xpcom/ds/nsArray.cpp#113,116-117,120-121
Flags: needinfo?(masayuki)
Comment on attachment 8791431 [details] [diff] [review]
patch for mozilla-central v1.1

>+TextEditor::GetEmbeddedObjects(nsIArray** aNodeList)
>+ {
>+  NS_ENSURE_ARG_POINTER(aNodeList);

nit: Could you this style when you add/rewrite NS_ENSURE_*()?

  if (NS_WARN_IF(!aNodeList)) {
    return NS_ERROR_INVALID_ARG;
  }
Attachment #8791431 - Flags: review?(masayuki) → review+
Thanks.

The failing tests were in c-c. We will be sure when we check in both patches.
Attachment #8791431 - Attachment is obsolete: true
Attachment #8791550 - Flags: review+
I tested this by adding and attachment and an image in HTML composition. Both objects were preserved in the message created in Outbox.
Attachment #8752605 - Attachment is obsolete: true
Attachment #8752605 - Flags: review?(Pidgeot18)
Attachment #8791552 - Flags: review?(jorgk)
Comment on attachment 8791552 [details] [diff] [review]
patch for mailnews v1.1

Review of attachment 8791552 [details] [diff] [review]:
-----------------------------------------------------------------

Looks OK to me. Please coordinate the M-C/C-C landings. I suggest Aleth can push the M-C part to inbound and we land the C-C part when it gets merged to M-C.
Attachment #8791552 - Flags: review?(jorgk) → review+
Aleth, can you please push the M-C patch to inbound. Thanks.
Flags: needinfo?(aleth)
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c5a17252bb320c82ef5665b1640c43e7a6c8653
Bug 1272893 - Remove nsISupportsArray from GetEmbeddedObjects() in editor. r=masayuki
Pushed by aleth@instantbird.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c5a17252bb3
Remove nsISupportsArray from GetEmbeddedObjects() in editor. r=masayuki
Thanks. Let's clear the NI ;-)
Flags: needinfo?(aleth)
https://hg.mozilla.org/mozilla-central/rev/8c5a17252bb3
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 51.0
You need to log in before you can comment on or make changes to this bug.