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

RESOLVED FIXED in Thunderbird 52.0

Status

MailNews Core
Backend
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: erahm, Assigned: erahm)

Tracking

Trunk
Thunderbird 52.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

+++ 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
Created attachment 8803560 [details] [diff] [review]
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

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

Aceman is our nsISupportsArray killer ;-)
Thanks for the heads-up.
Attachment #8803560 - Flags: review?(acelists)

Comment 3

a year ago
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.

Updated

a year ago
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Component: General → Backend
OS: Unspecified → All
Product: Thunderbird → MailNews Core
Hardware: Unspecified → All
Version: unspecified → Trunk

Comment 4

a year ago
Created attachment 8803621 [details] [diff] [review]
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.
Attachment #8803560 - Attachment is obsolete: true
Attachment #8803560 - Flags: review?(acelists)
Attachment #8803621 - Flags: review?(rkent)
Attachment #8803621 - Flags: review?(iann_bugzilla)
Attachment #8803621 - Flags: feedback+

Updated

a year ago
Attachment #8803621 - Attachment is patch: true
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.
Attachment #8803621 - Flags: review?(rkent) → feedback+

Comment 6

a year ago
Created attachment 8804444 [details] [diff] [review]
Update |nsMsgFolderDataSource| to match nsIRDFDataSource interface v1.2

Thanks.
Attachment #8803621 - Attachment is obsolete: true
Attachment #8803621 - Flags: review?(iann_bugzilla)
Attachment #8804444 - Flags: review?(iann_bugzilla)

Comment 7

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

LGTM r/a=me
Attachment #8804444 - Flags: review?(iann_bugzilla) → review+

Comment 8

a year ago
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

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

Comment 10

a year ago
The M-C part has landed, so here we go with the C-C part:
https://hg.mozilla.org/comm-central/rev/2fc6e2544407602e43c6c35e4d9b62cda4ecca6d
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 52.0
You need to log in before you can comment on or make changes to this bug.