1317871 - Stop using nsISupportsArray for params that handle nsIArray

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Eric Rahm [:erahm]]

This updates calls to interfaces that now take nsIArray to use an nsIArray
instead of an nsISupportsArray. Underneath the hood the concrete
implementation of nsISupportsArray also implements nsIArray so these
"just work," but nsISupportsArray is deprecated and will be removed so the
calls should be updated.

Comment 1

[Eric Rahm [:erahm]]

Attachment: Stop using nsISupportsArray for params that handle nsIArray

This is my initial draft of removing some more nsISupportsArray usage. I have
neither compiled nor tested it.

Comment 2

[:aceman]

Comment on attachment 8811112 [details] [diff] [review]
Stop using nsISupportsArray for params that handle nsIArray

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

Thanks, I forward to the respective owners of the various modules involved here.
Jorg, can you check the drag and drop part in TB on Windows? I'm not a friend of drag and drop especially on Linux :)

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 8811112 [details] [diff] [review]
Stop using nsISupportsArray for params that handle nsIArray

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

Eric, thanks for helping us out with the transition away from nsISupportsArray.

::: mail/base/content/nsDragAndDrop.js
@@ +84,5 @@
>      {
>        if (!aRetrievalFunc) 
>          throw "No data retrieval handler provided!";
>        
> +      var array = aRetrievalFunc(aFlavourSet);

In this file, you're just changing variable names:
s/supportsArray/array/.

Surely that's OK and makes no difference. So something is going on behind the scenes:

What is the aRetrievalFunc and how has that changed and why doesn't it matter?

Maybe you could elaborate a bit on (quote): «Underneath the hood the concrete implementation of nsISupportsArray also implements nsIArray so these "just work"».

Comment 4

[Eric Rahm [:erahm]]

(In reply to Jorg K (GMT+1) from comment #3)
> Comment on attachment 8811112 [details] [diff] [review]
> Stop using nsISupportsArray for params that handle nsIArray
> 
> Review of attachment 8811112 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Eric, thanks for helping us out with the transition away from
> nsISupportsArray.
> 
> ::: mail/base/content/nsDragAndDrop.js
> @@ +84,5 @@
> >      {
> >        if (!aRetrievalFunc) 
> >          throw "No data retrieval handler provided!";
> >        
> > +      var array = aRetrievalFunc(aFlavourSet);
> 
> In this file, you're just changing variable names:
> s/supportsArray/array/.
> 
> Surely that's OK and makes no difference. So something is going on behind
> the scenes:
> 
> What is the aRetrievalFunc and how has that changed and why doesn't it
> matter?

The param doc says it best, it's "a function that returns a nsIArray of nsITransferables". The only usage I've found is in chatzilla [1] (I have a patch queued up for that). The change doesn't matter because we updated the xpcom implementation of nsIArray to also provide and extended interface [2] that provides nsISupportsArray-like iteration.

> Maybe you could elaborate a bit on (quote): «Underneath the hood the
> concrete implementation of nsISupportsArray also implements nsIArray so
> these "just work"».

See above.

[1] https://dxr.mozilla.org/comm-central/rev/b071e6b0b3aec336818aedd0878c50f0df93a3cb/mozilla/extensions/irc/xul/content/nsClipboard.js#39,51-62
[2] http://searchfox.org/mozilla-central/rev/8562d3859b89ac89f46690b9ed2c473e0728d6c0/xpcom/ds/nsIArrayExtensions.idl#28-51

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 8811112 [details] [diff] [review]
Stop using nsISupportsArray for params that handle nsIArray

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

I looked at the two nsDragAndDrop.js files and they are fine. But the other files need to be checked by the other reviewers.

Comment 6

[Ian Neal]

Comment on attachment 8811112 [details] [diff] [review]
Stop using nsISupportsArray for params that handle nsIArray

r/a=me for SM parts

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 8811112 [details] [diff] [review]
Stop using nsISupportsArray for params that handle nsIArray

I'd like to land this soon. If Philipp is too busy, perhaps MakeMyDay can review the small change in the calendar file.

Comment 8

[:aceman]

Comment on attachment 8811112 [details] [diff] [review]
Stop using nsISupportsArray for params that handle nsIArray

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

::: mail/base/content/nsDragAndDrop.js
@@ +84,5 @@
>      {
>        if (!aRetrievalFunc) 
>          throw "No data retrieval handler provided!";
>        
> +      var array = aRetrievalFunc(aFlavourSet);

I think he knows that the thing passed into the function can also be used as a nsIArray. And the variable names are only changed to not convey the idea that the thing must be a nsISupportsArray.

@@ +91,1 @@
>        

Please remove the trailing spaces on the empty lines neighbouring to the ones you are touching.

@@ +98,2 @@
>            if (!trans) continue;
>            trans = trans.QueryInterface(Components.interfaces.nsITransferable);

I think this could be merged into array.queryElementAt(i, Components.interfaces.nsITransferable) and then trans checked for null.

The same in the /suite part.

Comment 9

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #8)
> Please remove the trailing spaces on the empty lines neighbouring to the
> ones you are touching.
Who are you addressing? I think Eric left this for us to finish off, which is fair enough. As soon as we get r+ from the calendar guys we can land this with the changes you suggested, OK?

Comment 10

[:aceman]

Sure, no problem.

Comment 11

[Jorg K (CEST = GMT+2)]

I landed this as it was, but with all the trailing spaces in nsDragAndDrop.js removed (it will look terrible in the changeset, but note that nsDragAndDrop.js had *no* real changes).

https://hg.mozilla.org/comm-central/rev/2cb52566a6b706280da63bff8a36e45ca5c22eae

Aceman, can you please move the array.queryElementAt() business to bug 1318185 if you don't mind.

Comment 12

[:aceman]

You did not need to clean trailing spaces in the whole file.

Comment 13

[Jorg K (CEST = GMT+2)]

I did. I have none left now.

Comment 14

[Jorg K (CEST = GMT+2)]

Sorry, I misread. I did not need to, right, but there were so many. You wanted to remove the ones in the vicinity of other changes, but more would have crept into the vicinity, so in the end it would have been a spiral going further and further. So it was quicker to hit them all as Richard would have done ;-)

Plus the suite/ file had none.

Comment 15

[:aceman]

You do not need to go recursive in the whitespace hunt.
Now the patch hides the real changes in the sea of whitespace.
I also complain to Richard if it is overdone :)

But yes, I'm also irritated by the whitespace and would like to kill it. Just in an organized manner ;)