:aceman Assignee
	Description • 8 years ago

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

	Jorg K (CEST = GMT+2)
	Comment 1 • 8 years ago

Can you please refresh my memory on why we're doing that and what is the replacement.

	:aceman Assignee
	Comment 2 • 8 years ago

See bug 394167 and bug 792209. Supposedly it is "the bad thing" :)

	:aceman Assignee
	Comment 3 • 8 years ago

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

	:aceman Assignee
	Comment 4 • 8 years ago

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.

	:aceman Assignee
	Comment 5 • 8 years ago

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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 6 • 8 years ago

Comment on attachment 8752606 [details] [diff] [review]
patch for mozilla-central

I don't see any problems from this patch.

	:aceman Assignee
	Comment 7 • 8 years ago

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

	Jorg K (CEST = GMT+2)
	Comment 8 • 8 years ago

What are the test failures?

(Better to NI? Masayuki-san).

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 10 • 8 years ago

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

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 11 • 8 years ago

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

	:aceman Assignee
	Comment 12 • 8 years ago

Thanks.

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

	:aceman Assignee
	Comment 13 • 8 years ago

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

	Jorg K (CEST = GMT+2)
	Comment 14 • 8 years ago

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.

	Jorg K (CEST = GMT+2)
	Comment 15 • 8 years ago

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

	aleth [:aleth]
	Comment 16 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c5a17252bb320c82ef5665b1640c43e7a6c8653
Bug 1272893 - Remove nsISupportsArray from GetEmbeddedObjects() in editor. r=masayuki

	Pulsebot
	Comment 17 • 8 years ago

Pushed by aleth@instantbird.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c5a17252bb3
Remove nsISupportsArray from GetEmbeddedObjects() in editor. r=masayuki

	Jorg K (CEST = GMT+2)
	Comment 18 • 8 years ago

Thanks. Let's clear the NI ;-)

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 19 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/8c5a17252bb3

	Jorg K (CEST = GMT+2)
	Comment 20 • 8 years ago

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