1312132 - Update references to nsISupportsArray usage from nsIRDFDataSource.idl in comm-central

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Eric Rahm [:erahm]]

+++ This bug was initially created as a clone of Bug #1309409 +++

|nsISupportsArray| is deprecated. In bug 1309409 |nsIRDFDataSource.{IsCommandEnabled,DoCommand}| are being converted to just take |nsISupports| for their |aSources| and |aArguments| args.

Any RDFDataSource implementation in comm-central will need to be updated to handle this change. The only one I know of is nsMsgFolderDataSource [1].

[1] https://dxr.mozilla.org/comm-central/rev/29f13170b60d132a6ccd868e31192b361f7b89be/mailnews/base/src/nsMsgFolderDataSource.cpp#671

Comment 1

[Eric Rahm [:erahm]]

Attachment: Update |nsMsgFolderDataSource| to match nsIRDFDataSource interface. r=?

This updates the nsMsgFolderDataSource implementation to match update to the
nsIRDFDataSource interface. The internal usage of nsISupportsArray is
maintained.

I've neither built nor tested the patch, but it should be a good starting
point. I'm not sure who should review it either.

Comment 2

[Jorg K (CEST = GMT+2)]

Comment on attachment 8803560 [details] [diff] [review]
Update |nsMsgFolderDataSource| to match nsIRDFDataSource interface. r=?

Aceman is our nsISupportsArray killer ;-)
Thanks for the heads-up.

Comment 3

[:aceman]

Comment on attachment 8803560 [details] [diff] [review]
Update |nsMsgFolderDataSource| to match nsIRDFDataSource interface. r=?

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

Looks good to me, you just need to add #include <nsISupportsArray.h> into nsMsgFolderDataSource.h as you removed it from the m-c file being included here.
And also this needs updates in nsMsg*DataSource files.

I'll update the patch.

::: mailnews/base/src/nsMsgFolderDataSource.h
@@ +85,2 @@
>                                nsIRDFResource*   aCommand,
> +                              nsISupportsArray/*nsISupportsArray<nsIRDFResource>*/* aArguments,

nsISupports too? Otherwise it does not compile.

Comment 4

[:aceman]

Attachment: Update |nsMsgFolderDataSource| to match nsIRDFDataSource interface v1.1

This does compile now (you need the m-c patch too).

This is probably used in Seamonkey so I'd like Ian to check it.
Or are we reasonably sure that just queryInterfacing from nsISupports to nsISupportsArray internally must work (when the passed in object is really nsISupportsArray)?

We should look if we really need nsISupportsArray for internal purposes in a followup bug.

Comment 5

[Kent James (:rkent)]

Comment on attachment 8803621 [details] [diff] [review]
Update |nsMsgFolderDataSource| to match nsIRDFDataSource interface v1.1

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

As a general comment, adding a QI gives us one more way to fail. As a result, I think that we should add an extra degree of anti-crash conservatism by adding an NS_ENSURE_STATE(supportsArray) after each QI, in cases where previously the code just assumed that the incoming parameter was valid.

I don't think I need to see this again if Ian is also going to review, as I think it mostly potentially affects SeaMonkey. Let's take my comments as feedback+ and let Ian's review be the decider here.

::: mailnews/base/src/nsMsgFolderDataSource.cpp
@@ +634,5 @@
>  {
>    nsresult rv;
>    nsCOMPtr<nsIMsgFolder> folder;
>  
> +  nsCOMPtr<nsISupportsArray> sources = do_QueryInterface(aSources);

add NS_ENSURE_STATE

@@ +677,5 @@
>    nsresult rv = NS_OK;
>    nsCOMPtr<nsISupports> supports;
>    nsCOMPtr<nsIMsgWindow> window;
>  
> +  nsCOMPtr<nsISupportsArray> sources = do_QueryInterface(aSources);

add NS_ENSURE_STATE for sources. Not needed for arguments since the code assumes later it might be null.

Comment 6

[:aceman]

Attachment: Update |nsMsgFolderDataSource| to match nsIRDFDataSource interface v1.2

Thanks.

Comment 7

[Ian Neal]

Comment on attachment 8804444 [details] [diff] [review]
Update |nsMsgFolderDataSource| to match nsIRDFDataSource interface v1.2

LGTM r/a=me

Comment 8

[:aceman]

Thanks.

Eric, you can now land the m-c patch in bug 1309409 and we will follow with c-c. Thanks for the cooperation.

Comment 9

[Axel Hecht]

I wish we could just remove the commands usage in nsMsgFolderDataSource, too.

Comment 10

[Jorg K (CEST = GMT+2)]

The M-C part has landed, so here we go with the C-C part:
https://hg.mozilla.org/comm-central/rev/2fc6e2544407602e43c6c35e4d9b62cda4ecca6d