1309409 - Remove nsISupportsArray usage from nsIRDFDataSource.idl

Status: RESOLVED FIXED

(Core Graveyard :: RDF, defect)

Description

[Eric Rahm [:erahm]]

|nsISupportsArray| is deprecated. It looks like it should be straightforward to convert |nsIRDFDataSource.{IsCommandEnabled,DoCommand}| to just take |nsISupports| for their |aSources| and |aArguments| args as they are never used internally.

A textual search of the addons repo shows several hits for implementors of those functions, but they just pass through to a gecko instance.

Comment 1

[Eric Rahm [:erahm]]

Attachment: Part 1: Remove nsISupportsArray usage from nsIRDFDataSource

This converts the usage of nsISupportsArray in nsIRDFDataSource to just
nsISupports. Internally none of the params are used, all external usages in
the addons repo appear to just be passthroughs.

Regardless, any external implementors wanting to pass in an nsISupportsArray
can still do so as it is derived from nsISupports.

MozReview-Commit-ID: JJSHAQKiLSZ

Comment 2

[Eric Rahm [:erahm]]

Attachment: Part 2: Use nsCOMArray instead of nsISupportsArray for mHashArcs

This removes the final usages of nsISupportsArray from RDF. An unused
nsCOMPtr<nsISupportsArray> is removed and another is converted to an nsCOMArray
instead.

MozReview-Commit-ID: C5x7EIASuAt

Comment 3

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=83a31794e438

Comment 4

[Axel Hecht]

Switching to my non-l10n identity.

Comment 5

[Axel Hecht]

Comment on attachment 8800109 [details] [diff] [review]
Part 1: Remove nsISupportsArray usage from nsIRDFDataSource

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

This one I'm conflicted with.

We do have an implementation of DoCommand in mailnews, https://dxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgFolderDataSource.cpp#671.

I have to admit, I have no idea if that's actually called anywhere. The commands part of RDF never made it into my head.

I wonder if we win in practice here with binary compat if we fake backwards compatibility, or if we're better off with just right-out removing the commands part of the API, breaking binary compat explicitly.

:bs, do you have recollection of the original story here? Also, an opinion on binary compat vs not?

:mkmelin, any opinion on the nsMsgFolderDataSource code?

Comment 6

[Magnus Melin [:mkmelin]]

Looks like it's used, but with a bit of magic. Kent, do you have a better idea about this?

Comment 7

[Eric Rahm [:erahm]]

(In reply to Axel Hecht from comment #5)
> Comment on attachment 8800109 [details] [diff] [review]
> Part 1: Remove nsISupportsArray usage from nsIRDFDataSource
> 
> Review of attachment 8800109 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This one I'm conflicted with.
> 
> We do have an implementation of DoCommand in mailnews,
> https://dxr.mozilla.org/comm-central/source/mailnews/base/src/
> nsMsgFolderDataSource.cpp#671.

Good point, I checked out addons but neglected c-c. If we don't particularly feel like thinking about this we can just have nsMsgFolderDataSource qi the nsISupports args to nsISupportsArray, which leaves some usage of nsISupportsArray but at least not in m-c.

Comment 8

[Kent James (:rkent)]

I am not aware of any actual uses of the DoCommand functions within Thunderbird. I believe that SeaMonkey still has some old-style RDF XUL user interface code. Maybe that uses it? But probably not through C++.

Comment 9

[Eric Rahm [:erahm]]

Axel, it's not clear to me what we want to do here. Can we split out full removal to a separate bug, file a bug for converting c-c to nsISupports, and just land this as-is?

Comment 10

[Axel Hecht]

I'd like to hear what Benjamin says.

Comment 11

[Kent James (:rkent)]

(In reply to Eric Rahm [:erahm] from comment #9)
> Axel, it's not clear to me what we want to do here. Can we split out full
> removal to a separate bug, file a bug for converting c-c to nsISupports, and
> just land this as-is?

Just from a c-c perspective, it is clear to us that you are free to remove nsISupportsArray at any time, and we will deal with the breakage. Ideally you file a bug, even more ideally you add a patch to that bug, but those are above and beyond what is required of you.

Comment 12

[Benjamin Smedberg]

FF/gecko doesn't use the command machinery at all, and all of RDF is going to be removed from m-c in the not too far future. So I really don't care what we do in the meantime.

Comment 13

[Eric Rahm [:erahm]]

(In reply to Axel Hecht from comment #10)
> I'd like to hear what Benjamin says.

Okay, any thoughts now?

Comment 14

[Axel Hecht]

Comment on attachment 8800109 [details] [diff] [review]
Part 1: Remove nsISupportsArray usage from nsIRDFDataSource

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

OK, I've made up my mind.

Only in nsCompositeDataSource.cpp, fix the comments to keep it talking about plural sources and arguments? The impl changes are fine.

All mozilla-central impls should do what nsFileSystemDataSource is doing:

return(NS_ERROR_NOT_IMPLEMENTED);

Feel free to just strip the comments for the arguments there.

And then let's file a follow-up to make the comm-central DSes have the same implementation, and fix regressions in their UI if they show up.

::: rdf/base/nsCompositeDataSource.cpp
@@ +1103,2 @@
>                                            nsIRDFResource*   aCommand,
> +                                          nsISupports/*<nsIRDFResource>*/* aArguments,

Can you fix the comments here for something like "nsIRDFResource container" ?

Comment 15

[Eric Rahm [:erahm]]

Attachment: Part 1: Remove nsISupportsArray usage from nsIRDFDataSource

This converts the usage of nsISupportsArray in nsIRDFDataSource to just
nsISupports. Internally none of the params are used, all external usages in
the addons repo appear to just be passthroughs.

Regardless, any external implementors wanting to pass in an nsISupportsArray
can still do so as it is derived from nsISupports.

Additionally the |IsCommandEnabled| and |DoCommand| stubs are updated to just
return NS_ERROR_NOT_IMPLEMENTED as this functionallity is currently not
supported.

MozReview-Commit-ID: JJSHAQKiLSZ

Comment 16

[Eric Rahm [:erahm]]

(In reply to Axel Hecht from comment #14)
> Comment on attachment 8800109 [details] [diff] [review]
> Part 1: Remove nsISupportsArray usage from nsIRDFDataSource
> 
> Review of attachment 8800109 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> OK, I've made up my mind.
> 
> Only in nsCompositeDataSource.cpp, fix the comments to keep it talking about
> plural sources and arguments? The impl changes are fine.

Done.

> All mozilla-central impls should do what nsFileSystemDataSource is doing:
> 
> return(NS_ERROR_NOT_IMPLEMENTED);

Done.

> Feel free to just strip the comments for the arguments there.

Done.

> And then let's file a follow-up to make the comm-central DSes have the same
> implementation, and fix regressions in their UI if they show up.

Filed bug 1312132 with an untested patch.

> ::: rdf/base/nsCompositeDataSource.cpp
> @@ +1103,2 @@
> >                                            nsIRDFResource*   aCommand,
> > +                                          nsISupports/*<nsIRDFResource>*/* aArguments,
> 
> Can you fix the comments here for something like "nsIRDFResource container" ?

Done.

Comment 17

[Axel Hecht]

Comment on attachment 8803543 [details] [diff] [review]
Part 1: Remove nsISupportsArray usage from nsIRDFDataSource

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

Thanks for your help here and in comm-central. r=me.

Comment 18

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2cca74f8c4c155c3a7219f5a4f29b5e92dddc870
Bug 1309409 - Part 1: Remove nsISupportsArray usage from nsIRDFDataSource. r=Pike

https://hg.mozilla.org/integration/mozilla-inbound/rev/40f84048af16b07dae9fa10f7bfb50daffb7a38f
Bug 1309409 - Part 2: Use nsCOMArray instead of nsISupportsArray for mHashArcs. r=Pike

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2cca74f8c4c1
https://hg.mozilla.org/mozilla-central/rev/40f84048af16