The default bug view has changed. See this FAQ.

Convert |inIDOMUtils.getCSSStyleRules| to |nsIArray|

RESOLVED FIXED in Firefox 52

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: erahm, Assigned: erahm)

Tracking

({addon-compat})

unspecified
Firefox 52
addon-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

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

Updated

5 months ago
Assignee: nobody → erahm
(Assignee)

Comment 1

5 months ago
Created attachment 8802350 [details] [diff] [review]
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
Attachment #8802350 - Flags: review?(nfroyd)
(Assignee)

Comment 2

5 months ago
Created attachment 8802352 [details] [diff] [review]
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
(Assignee)

Comment 3

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

Comment 4

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

Comment 5

5 months ago
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.
Attachment #8802352 - Attachment is obsolete: true
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?
Attachment #8802350 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 7

5 months ago
Created attachment 8803119 [details] [diff] [review]
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
Attachment #8803119 - Flags: review?(cam)
(Assignee)

Comment 8

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

Comment 9

5 months ago
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.
Attachment #8803119 - Flags: feedback?(jorge)
(Assignee)

Comment 10

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

Comment 11

5 months ago
Created attachment 8803566 [details] [diff] [review]
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
Attachment #8803566 - Flags: review?(nfroyd)
(Assignee)

Updated

5 months ago
Attachment #8802350 - Attachment is obsolete: true
Attachment #8803119 - Flags: review?(cam) → review+
Blocks: 1312331
Comment on attachment 8803119 [details] [diff] [review]
Part 2: Convert |inIDOMUtils.getCSSStyleRules| to |nsIArrayExtensions|

Looks okay to me.
Attachment #8803119 - Flags: feedback?(jorge) → feedback+

Updated

5 months ago
Attachment #8803566 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 13

5 months ago
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

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2431c088b4c1
https://hg.mozilla.org/mozilla-central/rev/ad457eb85c94
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
You need to log in before you can comment on or make changes to this bug.