Closed Bug 792209 (nuke-nsSupportsArray) Opened 12 years ago Closed 7 years ago

remove nsISupportsArray

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: froydnj, Assigned: erahm)

References

Details

(Keywords: arch, meta)

Attachments

(1 file)

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
Depends on: 792566
Depends on: 819936
Depends on: 819939
Depends on: 819940
Depends on: 394167, 407956, 399487
Depends on: 820377
Depends on: 820403
Depends on: 825278
Depends on: 825281
Depends on: 825291
Keywords: arch
Depends on: 851834
Depends on: 856238
Depends on: 860027
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
Depends on: 1308611
Depends on: 1308615
Depends on: 1309409
Depends on: 1309698
Depends on: 1310017
Depends on: 1310023
Depends on: 1310040
Depends on: 1311191
Depends on: 1311223
Depends on: 1311568
Depends on: 1311759
Depends on: 1312132
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.
Depends on: 1312143
Depends on: 1312144
Depends on: 1312856
Depends on: 1312901
Depends on: 1313507
Blocks: 7795
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.
Depends on: 1315812
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
Assignee: nobody → erahm
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.
Attachment #8811876 - Flags: review?(nfroyd)
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.
Attachment #8811876 - Flags: review?(nfroyd) → review+
(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.
No longer depends on: 437711
Depends on: 1324904
Depends on: 1324905
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).
(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.
https://hg.mozilla.org/mozilla-central/rev/70dec4a3877d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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.
You need to log in before you can comment on or make changes to this bug.