792209 - (nuke-nsSupportsArray) remove nsISupportsArray

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nathan Froyd [:froydnj]]

nsISupportsArray is ugly and it's deprecated (for almost 10 years!); we should just remove it.  Let's use this bug to coordinate the various pieces of work that need to be done

Comment 2

[Eric Rahm [:erahm]]

This is still lingering, we can see it in 6 interfaces:

  - docshell/shistory/nsISHEntry.idl
  - dom/media/nsIMediaManager.idl
  - layout/inspector/inIDOMUtils.idl
  - rdf/base/nsIRDFDataSource.idl
  - widget/nsIDragService.idl
  - widget/nsIFormatConverter.idl

We can see an example in embedding/components/windowwatcher/nsIWindowWatcher.idl where a param is declared as |nsISupports| [1] to allow both |nsIArray| and |nsISupportsArray| to be passed in, internally it is converted to an |nsIArray| [2]. This might be an interesting solution allowing us to remove usage internally but not totally break addons.

[1] http://searchfox.org/mozilla-central/rev/fd672b97f13aa83af5f04caa7b61bd443fb623e9/embedding/components/windowwatcher/nsIWindowWatcher.idl#53-56
[2] http://searchfox.org/mozilla-central/rev/fd672b97f13aa83af5f04caa7b61bd443fb623e9/embedding/components/windowwatcher/nsWindowWatcher.cpp#307

Comment 3

[Eric Rahm [:erahm]]

All of the remaining IDL interfaces in m-c have been updated, two are pending review:
  - layout/inspector/inIDOMUtils.idl (bug 1311191)
  - rdf/base/nsIRDFDataSource.idl (bug 1309409)

There's still a bit of usage in the tree (ignoring xpcom where it's defined):

- toolkit/obsolete/content/nsClipboard.js

Add-ons folks indicated it might be okay to nuke the obsolete dir, I'll file a bug to track that.

- xpfe/components/directory/nsDirectoryViewer.cpp
  xpfe/components/directory/nsDirectoryViewer.h

We can probably convert these to nsIMutableArray (or even nsCOMArray).

- browser/base/content/browser.js
  browser/components/nsBrowserContentHandler.js
  browser/components/extensions/ext-windows.js

I haven't totally worked this out, but we seem to have some functionality where we set window.arguments[0] to a nsISupportsArray of URLs. I haven't been able to find a consumer of that yet.

- browser/base/content/sanitizeDialog.js

This looks like an invokeDragSession user I may have missed.

- embedding/components/windowwatcher/nsWindowWatcher.cpp

This is our handling of window args being either nsISupportsArray or nsIArray, since nsISupportsArray *is* an nsIArray now I think we can get rid of this logic and make the args nsIArray.

Comment 4

[Eric Rahm [:erahm]]

Just landed bug 1309409 (rdf and friends), if that sticks we'll have removed all usage in m-c aside from the definition in xpcom.

Comment 5

[Eric Rahm [:erahm]]

The IDL files have been marked as deprecated in bug 1315812 as of Firefox 52. The plan is to let that make it to release and then completely remove the files. I posted a notice to dev-platform as well [1].

[1] https://groups.google.com/d/msg/mozilla.dev.platform/_JghW4BJgls/V1qHcvq3BgAJ

Comment 6

[Eric Rahm [:erahm]]

Attachment: Remove nsISupportsArray

This is the final removal patch. I wanted to get it done now while all of this is fresh, but we'll hold off on landing until 52 is on release.

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8811876 [details] [diff] [review]
Remove nsISupportsArray

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

So we're going to go through a complete 3-month release "cycle" rather than a six-week one?  That works for me, but I had interpreted "waiting a release cycle" to be the six-week one.  This probably works out a little better for addon compat and for Thunderbird, I suppose.

Comment 8

[Eric Rahm [:erahm]]

(In reply to Nathan Froyd [:froydnj] from comment #7)
> Comment on attachment 8811876 [details] [diff] [review]
> Remove nsISupportsArray
> 
> Review of attachment 8811876 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So we're going to go through a complete 3-month release "cycle" rather than
> a six-week one?  That works for me, but I had interpreted "waiting a release
> cycle" to be the six-week one.  This probably works out a little better for
> addon compat and for Thunderbird, I suppose.

Yes it's primarily for add-ons, I'm less concerned about TB because a) I'm giving them patches to work with and b) they are aware of what's going on.

Comment 9

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bb2262a01b2

Comment 10

[:aceman]

Hi, I see you are pushing this to try server. What is the plan to merge this in m-c?
We still have one remaining usage in mailnews in bug 1318806 (which you have filed, thanks).

Comment 11

[Eric Rahm [:erahm]]

(In reply to :aceman from comment #10)
> Hi, I see you are pushing this to try server. What is the plan to merge this
> in m-c?
> We still have one remaining usage in mailnews in bug 1318806 (which you have
> filed, thanks).

I'm testing out a rebased version. My plan is to land when 52 hits release next week.

Comment 12

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=428350d6a401

Comment 13

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/70dec4a3877dd1dac1f819baa3557ed21f8b0249
Bug 792209 - Remove nsISupportsArray. r=froydnj

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/70dec4a3877d

Comment 15

[Mark Smith [:mcs]]

Can this change be added to https://developer.mozilla.org/en-US/Firefox/Releases/55? It broke one of the old legacy extensions that I work on.