/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" :)

This works for me provided the other m-c patch is accepted.

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.

Successful try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ec896cdee6739854c7cc753e268e9998f8f68b4d

Comment on attachment 8752606 [details] [diff] [review] patch for mozilla-central I don't see any problems from this patch.

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

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

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; }

Thanks. The failing tests were in c-c. We will be sure when we check in both patches.

I tested this by adding and attachment and an image in HTML composition. Both objects were preserved in the message created in Outbox.

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.

Aleth, can you please push the M-C patch to inbound. Thanks.

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

C-C part: https://hg.mozilla.org/comm-central/rev/2b4a9b2ad034