1311191 - Convert |inIDOMUtils.getCSSStyleRules| to |nsIArray|

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Eric Rahm [:erahm]]

nsISupportsArray is deprecated. We'd like to switch |inIDOMUtils.getCSSStyleRules| [1] over to nsIArray instead. This is easy enough internally, but it is used a fair amount in addons (mainly firebug).

I propose two steps:

1) Temporarily add an extended interface to nsArray that includes the methods used to iterate over an nsISupportsArray, mainly 'Count' and 'GetElementAt' (QueryElementAt is also used, but is already provided by nsIArray).

2) Convert the interface to nsIArray and update internal references.

This way addons can continue to work during a period of deprecation (probably one release cycle).

[1] http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/layout/inspector/inIDOMUtils.idl#27

Comment 1

[Eric Rahm [:erahm]]

Attachment: Part 1: Add nsISupportsArray-like iteration to nsArray

This adds an intermediate interface, nsIArrayExtensions, that inherits from
nsIArray. This is necessary as nsISupportsArray implements nsIArray as well
so the methods could not just be addded to nsIArray. nsIMutableArray inherits
from nsIArrayExtensions and so any interface that works with an nsIMutableArray
can be updated to return an nsIArrayExtensions.

This will allow interfaces that currently return an nsISupportsArray to instead
return an nsIArrayExtensions and internally work with an nsIMutableArray.
Consumers of these functions will continue to be able to use
nsISupportsArray-like iteration even though they're now working with an
nsIArray.

MozReview-Commit-ID: 9uRjsJbg9Jp

Comment 2

[Eric Rahm [:erahm]]

Attachment: Part 2: Convert |inIDOMUtils.getCSSStyleRules| to |nsIArray|. r?

This shows how we're going to use the nsIArrayExtensions interface. I'll hold
off on review until part 1 is settled.

MozReview-Commit-ID: ZdrFrbclX8

Comment 3

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc171b8c8f9e

Comment 4

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0bea31fd6e9

Comment 5

[Eric Rahm [:erahm]]

Comment on attachment 8802352 [details] [diff] [review]
Part 2: Convert |inIDOMUtils.getCSSStyleRules| to |nsIArray|. r?

Nathan pointed out that adding a |GetElementAt()| is useful (at least on the JS-side), so we'll probably keep that regardless of deprecating nsISupportsArray. That means changing the various JS files is unnecessary. I'll update with a smaller patch.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8802350 [details] [diff] [review]
Part 1: Add nsISupportsArray-like iteration to nsArray

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

Documentation nits, but r=me.

::: xpcom/ds/nsIArrayExtensions.idl
@@ +9,5 @@
> + * Helper interface for allowing scripts to treat nsIArray instances as if
> + * they were nsISupportsArray instances while iterating.
> + *
> + * This is merely intended as a stop-gap solution until add-ons have enough
> + * time to transition to proper nsIArray methods.

How about:

nsISupportsArray is convenient to iterate over in JavaScript:

for (let i = 0; i < array.Count(); ++i) {
  let elem = array.GetElementAt(i);
  ...
}

but doing the same with nsIArray is somewhat less convenient, since queryElementAt is not nearly so nice to use from JavaScript.  So we provide this extension interface so interfaces that currently return nsISupportsArray can start returning nsIArrayExtensions and all JavaScript should Just Work.  Eventually we'll roll this interface into nsIArray itself, possibly getting rid of the Count() method, as it duplicates nsIArray functionality.

@@ +16,5 @@
> +interface nsIArrayExtensions : nsIArray
> +{
> +  /**
> +   * Method for determining the length of the array in the same format as
> +   * nsISupportsArray (via nsICollection).

See below.

@@ +22,5 @@
> +  uint32_t Count();
> +
> +  /**
> +   * Method for retrieving an element in the same format as nsISupportsArray
> +   * (via nsICollection).

Please transfer over relevant documentation from nsISupportsArray/nsICollection, particularly the null-on-invalid-index behavior, so that we don't have this documentation hanging around when we remove nsISupportsArray and then some poor soul wonders what is going on.

I'm not sure how to modify the Count() documentation in the same way.  Maybe don't mention nsISupportsArray there at all, since we've already described the purpose of this interface in the header comment above?

Comment 7

[Eric Rahm [:erahm]]

Attachment: Part 2: Convert |inIDOMUtils.getCSSStyleRules| to |nsIArrayExtensions|

This converts |getCSSStyleRules| to return an |nsIArrayExtensions| instead of
an |nsISupportsArray|. |nIArrayExtensions| still provides the iteration
mechanisms -- |Count| and |GetElementAt| -- that add-ons expect so they should
not be affected.

Cameron it looks like you've done the most reviews around this code, but feel
free to redirect (other options look like bz or dbaron).

MozReview-Commit-ID: ZdrFrbclX8

Comment 8

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6f3f285303b

Comment 9

[Eric Rahm [:erahm]]

Comment on attachment 8803119 [details] [diff] [review]
Part 2: Convert |inIDOMUtils.getCSSStyleRules| to |nsIArrayExtensions|

Jorge, another idl change that affects add-ons. AFIACT this should be a transparent change (see comment 1), but just wanted to double check that it looks okay.

Comment 10

[Eric Rahm [:erahm]]

(In reply to Nathan Froyd [:froydnj] from comment #6)
> Comment on attachment 8802350 [details] [diff] [review]
> Part 1: Add nsISupportsArray-like iteration to nsArray
> 
> Review of attachment 8802350 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Documentation nits, but r=me.
> 
> ::: xpcom/ds/nsIArrayExtensions.idl
> @@ +9,5 @@
> > + * Helper interface for allowing scripts to treat nsIArray instances as if
> > + * they were nsISupportsArray instances while iterating.
> > + *
> > + * This is merely intended as a stop-gap solution until add-ons have enough
> > + * time to transition to proper nsIArray methods.
> 
> How about:
> 
> .. snip ..

Looks good, updated.

> 
> @@ +16,5 @@
> > +interface nsIArrayExtensions : nsIArray
> > +{
> > +  /**
> > +   * Method for determining the length of the array in the same format as
> > +   * nsISupportsArray (via nsICollection).
> 
> See below.
> 
> @@ +22,5 @@
> > +  uint32_t Count();
> > +
> > +  /**
> > +   * Method for retrieving an element in the same format as nsISupportsArray
> > +   * (via nsICollection).
> 
> Please transfer over relevant documentation from
> nsISupportsArray/nsICollection, particularly the null-on-invalid-index
> behavior, so that we don't have this documentation hanging around when we
> remove nsISupportsArray and then some poor soul wonders what is going on.

nsISupportsArray/nsICollection have no documentation. I can adapt the docs from nsIArray though.

> I'm not sure how to modify the Count() documentation in the same way.  Maybe
> don't mention nsISupportsArray there at all, since we've already described
> the purpose of this interface in the header comment above?

I'll add a note that this is an alias for |length|.

Comment 11

[Eric Rahm [:erahm]]

Attachment: Part 1: Add nsISupportsArray-like iteration to nsArray

Nathan, this just changes the comments in nsIArrayExtensions.idl. Can you give
that portion another look?

MozReview-Commit-ID: 9uRjsJbg9Jp

Comment 12

[Jorge Villalobos [:jorgev] (he/him)]

Comment on attachment 8803119 [details] [diff] [review]
Part 2: Convert |inIDOMUtils.getCSSStyleRules| to |nsIArrayExtensions|

Looks okay to me.

Comment 13

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2431c088b4c11110274e97a5e8d3acb935db2119
Bug 1311191 - Part 1: Add nsISupportsArray-like iteration to nsArray. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad457eb85c9460c04fafcc32949e212daf0851ee
Bug 1311191 - Part 2: Convert |inIDOMUtils.getCSSStyleRules| to |nsIArrayExtensions|. r=heycam

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2431c088b4c1
https://hg.mozilla.org/mozilla-central/rev/ad457eb85c94