Closed
Bug 1317871
Opened 8 years ago
Closed 8 years ago
Stop using nsISupportsArray for params that handle nsIArray
Categories
(MailNews Core :: Backend, defect)
MailNews Core
Backend
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 53.0
People
(Reporter: erahm, Assigned: erahm)
References
Details
Attachments
(1 file)
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.
Assignee | ||
Comment 1•8 years ago
|
||
This is my initial draft of removing some more nsISupportsArray usage. I have
neither compiled nor tested it.
Attachment #8811112 -
Flags: review?(acelists)
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 :)
Attachment #8811112 -
Flags: review?(philipp)
Attachment #8811112 -
Flags: review?(jorgk)
Attachment #8811112 -
Flags: review?(iann_bugzilla)
Attachment #8811112 -
Flags: review?(aleth)
Attachment #8811112 -
Flags: review?(acelists)
Comment 3•8 years ago
|
||
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"».
Assignee | ||
Comment 4•8 years ago
|
||
(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•8 years ago
|
||
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.
Attachment #8811112 -
Flags: review?(jorgk) → review+
Updated•8 years ago
|
Attachment #8811112 -
Flags: review?(aleth) → review+
Comment on attachment 8811112 [details] [diff] [review]
Stop using nsISupportsArray for params that handle nsIArray
r/a=me for SM parts
Attachment #8811112 -
Flags: review?(iann_bugzilla) → review+
Comment 7•8 years ago
|
||
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.
Attachment #8811112 -
Flags: review?(makemyday)
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•8 years ago
|
||
(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?
Updated•8 years ago
|
Attachment #8811112 -
Flags: review?(philipp)
Attachment #8811112 -
Flags: review?(makemyday)
Attachment #8811112 -
Flags: review+
Comment 10•8 years ago
|
||
Sure, no problem.
Comment 11•8 years ago
|
||
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.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
Comment 12•8 years ago
|
||
You did not need to clean trailing spaces in the whole file.
Comment 13•8 years ago
|
||
I did. I have none left now.
Comment 14•8 years ago
|
||
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•8 years ago
|
||
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 ;)
You need to log in
before you can comment on or make changes to this bug.
Description
•