Bug 792209 (nuke-nsSupportsArray)

remove nsISupportsArray

RESOLVED FIXED in Firefox 55

Status

()

Core
XPCOM
RESOLVED FIXED
5 years ago
3 months ago

People

(Reporter: froydnj, Assigned: erahm)

Tracking

({arch, meta})

unspecified
mozilla55
arch, meta
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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
(Reporter)

Updated

5 years ago
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
Depends on: 840913
Depends on: 437711

Updated

4 years ago
Keywords: arch
Depends on: 851834

Updated

4 years ago
Depends on: 856238
Depends on: 860027
(Assignee)

Updated

8 months ago
Duplicate of this bug: 1308592
(Assignee)

Comment 2

8 months ago
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
(Assignee)

Updated

8 months ago
Depends on: 1308611
(Assignee)

Updated

8 months ago
Depends on: 1308615
(Assignee)

Updated

8 months ago
Depends on: 1309409
(Assignee)

Updated

7 months ago
Depends on: 1309698
(Assignee)

Updated

7 months ago
Depends on: 1310017
(Assignee)

Updated

7 months ago
Depends on: 1310023
(Assignee)

Updated

7 months ago
Depends on: 1310040
(Assignee)

Updated

7 months ago
Depends on: 1311191
(Assignee)

Updated

7 months ago
Depends on: 1311223
(Assignee)

Updated

7 months ago
Depends on: 1311568
(Assignee)

Updated

7 months ago
Depends on: 1311759
(Assignee)

Updated

7 months ago
Depends on: 1312132
(Assignee)

Comment 3

7 months ago
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.
(Assignee)

Updated

7 months ago
Depends on: 1312143
(Assignee)

Updated

7 months ago
Depends on: 1312144
(Assignee)

Updated

7 months ago
Depends on: 1312856
(Assignee)

Updated

7 months ago
Depends on: 1312901
(Assignee)

Updated

7 months ago
Depends on: 1313507
Blocks: 7795
(Assignee)

Comment 4

7 months ago
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.
(Assignee)

Updated

7 months ago
Depends on: 1315812
(Assignee)

Comment 5

6 months ago
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)

Updated

6 months ago
Assignee: nobody → erahm
(Assignee)

Comment 6

6 months ago
Created attachment 8811876 [details] [diff] [review]
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.
Attachment #8811876 - Flags: review?(nfroyd)
(Reporter)

Comment 7

6 months ago
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+
(Assignee)

Comment 8

6 months ago
(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.
(Assignee)

Updated

6 months ago
No longer depends on: 437711

Updated

5 months ago
Depends on: 1324904

Updated

5 months ago
Depends on: 1324905
(Assignee)

Comment 9

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

Comment 10

3 months ago
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).
(Assignee)

Comment 11

3 months ago
(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.
(Assignee)

Comment 12

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=428350d6a401
(Assignee)

Comment 13

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

Comment 14

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/70dec4a3877d
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.