310265 - nsArray does not provide class info

Status: RESOLVED WONTFIX

(Core :: XPCOM, enhancement)

Description

[Mark Hammond [:markh] [:mhammond]]

nsIArray is scriptable, but the nsArray implementation does not provide class
info.  Attaching a patch.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: Patch so nsArray provides class info for nsIArray and nsIMutableArray

Comment 2

[Benjamin Smedberg]

Comment on attachment 197656 [details] [diff] [review]
Patch so nsArray provides class info for nsIArray and nsIMutableArray

Timeless, you know a lot about CI, can you check this?

Comment 3

[timeless]

Comment on attachment 197656 [details] [diff] [review]
Patch so nsArray provides class info for nsIArray and nsIMutableArray

as i told mark, i'm hesitant due to perf backlash i got for some of my CI changes.

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 197656 [details] [diff] [review]
Patch so nsArray provides class info for nsIArray and nsIMutableArray

i am okay with this change if we don't regress as timeless indicates we might. 

Mark, do you have any data on this?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Did this ever get checked in?

Comment 6

[Darin Fisher]

nope

Comment 7

[Mark Hammond [:markh] [:mhammond]]

To be honest I just let it slide.  Timeless raise the spectre of getting hit with the performance stick (although I can't see how), and I could not identify existing code that needed this class info (although some code I was adding could have taken advantage of it).  For that reason I also could not justify taking time out to actually measure the perf.

If there is consensus we do actually want this I'm happy to help move forward.

Comment 8

[Boris Zbarsky [:bzbarsky]]

I can't think of obvious reasons why adding classinfo would regress perf, offhand...

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Mark, is this worth checking in at some point when the tree is quiet to measure perf impact?

Comment 10

[timeless]

Attachment: patch

two ways to check for CI in xpcshell:
js> Components.classes['@mozilla.org/array;1'].createInstance()
[xpconnect wrapped (nsISupports, nsIArray, nsIMutableArray) @ 0x100e2bc70 (native @ 0x100e2ba40)]

if you haven't used an @mozilla.org/array;1 array before then w/o CI you'd just get an nsISupports object

js> Components.classes['@mozilla.org/array;1'].createInstance().QueryInterface(Components.interfaces.nsIClassInfo)

this would throw an exception if there's no CI.

Comment 11

[(no longer active)]

Seems like we were worried that there's a perf impact here, which makes me not want to take this on cedar.

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 13

[Andrew McCreight [:mccr8]]

We've gone this long without class info on nsArray, so it feels like we can leave this alone unless there's some specific need for it.