Closed Bug 1310040 Opened 3 years ago Closed 3 years ago

Make nsISupportsArray implement nsIArray

Categories

(Core :: XPCOM, defect)

48 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: erahm, Assigned: erahm)

References

Details

(Keywords: addon-compat)

Attachments

(1 file)

As a stop-gap on the way to full deprecation (and breaking of addons) we'd like to add a path for addons to transition to using nsIArray.

The plan is to make nsISupportsArray implement the nsIArray interface. Then we can happily convert our few remaining interfaces that rely on nsISupportsArray to nsIArray. After a full release cycle we can then remove nsISupportsArray.
This makes nsSupportsArray implement the |nsIArray| interface. This will allow
us to replace references to nsISupportsArray as a param in our gecko interfaces
with nsIArray instead.

The goal is to remove this adapter (and nsISupportsArray) after a full release
cycle.

MozReview-Commit-ID: If7RiO5muIk
Attachment #8801931 - Flags: review?(nfroyd)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Comment on attachment 8801931 [details] [diff] [review]
Make nsSupportsArray implement nsIArray

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

::: xpcom/ds/nsICollection.idl
@@ +18,5 @@
>    void SetElementAt(in uint32_t index, in nsISupports item);
>    void AppendElement(in nsISupports item);
>    void RemoveElement(in nsISupports item);
>  
> +  [binaryname(DeprecatedEnumerate)]

This clashes with |nsISumpleEnumerator nsIArray.enumerate()| (only on the binary side), so it is renamed.

::: xpcom/ds/nsSupportsArray.cpp
@@ +218,5 @@
> +  return Count(aLength);
> +}
> +
> +NS_IMETHODIMP
> +nsSupportsArray::QueryElementAt(uint32_t aIndex, const nsIID& aIID, void** aResult)

Both nsIArray and nsISupportsArray (via nsICollection) declare this method with identical signatures. Having them share seems to work but the implementation had to be moved out of the header to avoid redefinition.

::: xpcom/tests/unit/test_nsIMutableArray.js
@@ +106,5 @@
>    }
>    do_check_eq(arr.length, i);
>  }
>  
> +function test_nssupportsarray_interop() {

We didn't have xpcshell tests for nsISupportsArray, so this seemed like a reasonable place to add a test for QI'ing an |nsIArray| from an |nsISupportsArray|.
Blocks: 1309698
Note: this can't land until bug 1308317 lands as it depends on simplifications made there.
Depends on: 1308317
Comment on attachment 8801931 [details] [diff] [review]
Make nsSupportsArray implement nsIArray

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

Progress!

::: xpcom/ds/nsICollection.idl
@@ +18,5 @@
>    void SetElementAt(in uint32_t index, in nsISupports item);
>    void AppendElement(in nsISupports item);
>    void RemoveElement(in nsISupports item);
>  
> +  [binaryname(DeprecatedEnumerate)]

I think we should add a comment here that is more-or-less a cut-and-paste of your bug comment.
Attachment #8801931 - Flags: review?(nfroyd) → review+
It might also be a good idea to add a console message about the deprecation when nsSupportsArray is instantiated from JS. But I guess we probably have too much internal code using it at this point...
(In reply to Kris Maglione [:kmag] from comment #5)
> It might also be a good idea to add a console message about the deprecation
> when nsSupportsArray is instantiated from JS. But I guess we probably have
> too much internal code using it at this point...

I agree, I'd like to mark the whole interface as deprecated (which I think will make it spit out console warnings) but there are still too many internal uses. We're definitely getting close though!
https://hg.mozilla.org/mozilla-central/rev/ad002143ba47
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.