Remove nsISupportsArray usage from nsIRDFDataSource.idl

RESOLVED FIXED in Firefox 52

Status

()

Core
RDF
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: erahm, Assigned: erahm)

Tracking

48 Branch
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

|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.
Created attachment 8800109 [details] [diff] [review]
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
Attachment #8800109 - Flags: review?(l10n)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Created attachment 8800110 [details] [diff] [review]
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
Attachment #8800110 - Flags: review?(l10n)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=83a31794e438

Comment 4

a year ago
Switching to my non-l10n identity.

Updated

a year ago
Attachment #8800109 - Flags: review?(l10n) → review?(axel)

Updated

a year ago
Attachment #8800110 - Flags: review?(l10n) → review?(axel)

Updated

a year ago
Attachment #8800110 - Flags: review?(axel) → review+

Comment 5

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

Updated

a year ago
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(benjamin)

Comment 6

a year ago
Looks like it's used, but with a bit of magic. Kent, do you have a better idea about this?
Flags: needinfo?(mkmelin+mozilla) → needinfo?(rkent)
(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.
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++.
Flags: needinfo?(rkent)
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?
Flags: needinfo?(axel)

Comment 10

a year ago
I'd like to hear what Benjamin says.
Flags: needinfo?(axel)
(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

a year ago
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.
Flags: needinfo?(benjamin)
(In reply to Axel Hecht from comment #10)
> I'd like to hear what Benjamin says.

Okay, any thoughts now?
Flags: needinfo?(axel)

Comment 14

a year ago
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" ?
Attachment #8800109 - Flags: review?(axel) → review-

Updated

a year ago
Flags: needinfo?(axel)
Created attachment 8803543 [details] [diff] [review]
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
Attachment #8803543 - Flags: review?(axel)
Attachment #8800109 - Attachment is obsolete: true
Blocks: 1312132
(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

a year ago
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.
Attachment #8803543 - Flags: review?(axel) → review+
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2cca74f8c4c1
https://hg.mozilla.org/mozilla-central/rev/40f84048af16
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.