1484496 - Support JS iterator protocol for nsISimpleEnumerator

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement)

Description

[Kris Maglione [:kmag]]

nsISimpleEnumerator doesn't map well to JS. The fact that we have dozens of JS star functions to wrap them testifies to this fact.

The places where we don't use the wrappers, though, are even worse.

I'm mainly doing this for the sake of code quality, but the change gets rid of hundreds of lines of code, and dozens of star functions, so it will almost certainly also save memory.

Comment 1

[Florian Quèze [:florian]]

Sometimes we also made the C++ code return a JS array instead of an enumerator to make it easy to use from JS, forcing the C++ code to compute all the elements eagerly. These cases may be difficult to find after the fact, but changing some arrays back to enumerators may have some performance benefits.

Comment 2

[Kris Maglione [:kmag]]

(In reply to Florian Quèze [:florian] (PTO until August 27th) from comment #1)
> changing some arrays back to enumerators may have some performance benefits.

I'd be surprised if it did. Most of our enumerators build arrays anyway, and building a JS array to begin with will probably generally be much cheaper than building a native array and then going through XPConnect to access each element.

There may be some cases where it makes sense, though.

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1484496: Part 1 - Add support for symbol properties to XPIDL. r=nika

This patch allows us to define methods or getters/setters for any of the
current set of well-known symbols. Those are defined by adding the [symbol]
attribute to a method:

  [symbol]
  Iterator iterator();

which causes the method to define a property with the well-known symbol which
matches its method name (Symbol.iterator, in this case).

Due to the implementation details of the XPIDL parser, this currently does not
support defining a non-symbol function with the same name as a symbol
function:

  [symbol]
  Iterator iterator();

  [binaryname(OtherIterator)]
  Thing iterator(in nsIDRef aIID);

throws for a duplicate method name, even though there is no actual conflict.

Comment 4

[Kris Maglione [:kmag]]

Attachment: Bug 1484496: Part 2 - Add common base class for all nsISimpleEnumerator implementations. r=froydnj

In order to allow JS callers to use nsISimpleEnumerator instances with the JS
iteration protocol, we'll need to additional methods to every instance. Since
we currently have a large number of unrelated implementations, it would be
best if they could share the same implementation for the JS portion of the
protocol.

This patch adds a stub nsSimpleEnumerator base class, and updates all existing
implementations to inherit from it. A follow-up will add a new base interface
to this class, and implement the additional functionality required for JS
iteration.

Comment 5

[Kris Maglione [:kmag]]

Attachment: Bug 1484496: Part 3 - Fix nsISimpleEnumerator implementations with broken contracts. r=froydnj

The nsISimpleEnumerator contract specifies that GetNext() returns
NS_ERROR_FAILURE when iteration is complete. Several implementations, however,
either return NS_OK and a null result, or return some other error code, when
iteration is complete.

Since my initial implementation of the JS iteration stubs rely on the
contract-defined behavior of GetNext(), these need to be fixed before it can
land.

Comment 6

[Kris Maglione [:kmag]]

Attachment: Bug 1484496: Part 4a - Add JS iterator support to nsISimpleEnumerator. r=mccr8

This patch adds simple stubs to convert between the nsISimpleEnumerator
iteration protocol and the JS iteration protocol.

Each iterable object is required to have an @@iterator method which returns an
object implementing the iterator protocol. The later objects, by convention,
also have such a method which returns the object itself.

This patch adds both a @@iterator() and entries() methods to
nsISimpleEnumerator. The former returns an iterator which returns plain
nsISupports objects. The latter accepts an IID and queries each element to
that IID before returning it. If any element fails to query, the error is
propagated to the caller.

Comment 7

[Kris Maglione [:kmag]]

Attachment: Bug 1484496: Part 4b - Add intrinsic type information to most nsSimpleEnumerators. r=froydnj

This allows JS callers to automatically get the correct types during
interation, without having to explicitly specify them.

Comment 8

[Kris Maglione [:kmag]]

Attachment: Bug 1484496: Part 5a - Convert browser/ nsISimpleEnumerator users to use JS iteration. r=Gijs

Comment 9

[Kris Maglione [:kmag]]

Attachment: Bug 1484496: Part 5b - Convert toolkit/ nsISimpleEnumerator users to use JS iteration. r=Gijs

Comment 10

[Kris Maglione [:kmag]]

Attachment: Bug 1484496: Part 5c - Convert mobile/ nsISimpleEnumerator users to use JS iteration. r=snorp

Comment 11

[Kris Maglione [:kmag]]

Attachment: Bug 1484496: Part 5d - Convert devtools/ nsISimpleEnumerator users to use JS iteration. r=bgrins

Comment 12

[Kris Maglione [:kmag]]

Attachment: Bug 1484496: Part 5e - Convert remaining nsISimpleEnumerator users to use JS iteration. r=mccr8

Comment 13

[Kris Maglione [:kmag]]

Attachment: Bug 1484496: Part 6 - Remove unused XPCOMUtils.IterSimpleEnumerator method. r=mccr8

Comment 14

[Kris Maglione [:kmag]]

Attachment: Bug 1484496: Part 1 - Add support for symbol properties to XPIDL. r=mccr8

This patch allows us to define methods or getters/setters for any of the
current set of well-known symbols. Those are defined by adding the [symbol]
attribute to a method:

  [symbol]
  Iterator iterator();

which causes the method to define a property with the well-known symbol which
matches its method name (Symbol.iterator, in this case).

Due to the implementation details of the XPIDL parser, this currently does not
support defining a non-symbol function with the same name as a symbol
function:

  [symbol]
  Iterator iterator();

  [binaryname(OtherIterator)]
  Thing iterator(in nsIDRef aIID);

throws for a duplicate method name, even though there is no actual conflict.

Comment 15

[Kris Maglione [:kmag]]

Attachment: Bug 1484496: Part 2 - Add common base class for all nsISimpleEnumerator implementations. r=froydnj

In order to allow JS callers to use nsISimpleEnumerator instances with the JS
iteration protocol, we'll need to additional methods to every instance. Since
we currently have a large number of unrelated implementations, it would be
best if they could share the same implementation for the JS portion of the
protocol.

This patch adds a stub nsSimpleEnumerator base class, and updates all existing
implementations to inherit from it. A follow-up will add a new base interface
to this class, and implement the additional functionality required for JS
iteration.

Comment 16

[Kris Maglione [:kmag]]

Attachment: Bug 1484496: Part 3 - Fix nsISimpleEnumerator implementations with broken contracts. r=froydnj

The nsISimpleEnumerator contract specifies that GetNext() returns
NS_ERROR_FAILURE when iteration is complete. Several implementations, however,
either return NS_OK and a null result, or return some other error code, when
iteration is complete.

Since my initial implementation of the JS iteration stubs rely on the
contract-defined behavior of GetNext(), these need to be fixed before it can
land.

Comment 17

[Kris Maglione [:kmag]]

Attachment: Bug 1484496: Part 4a - Add JS iterator support to nsISimpleEnumerator. r=mccr8

This patch adds simple stubs to convert between the nsISimpleEnumerator
iteration protocol and the JS iteration protocol.

Each iterable object is required to have an @@iterator method which returns an
object implementing the iterator protocol. The later objects, by convention,
also have such a method which returns the object itself.

This patch adds both a @@iterator() and entries() methods to
nsISimpleEnumerator. The former returns an iterator which returns plain
nsISupports objects. The latter accepts an IID and queries each element to
that IID before returning it. If any element fails to query, the error is
propagated to the caller.

Comment 18

[Kris Maglione [:kmag]]

Attachment: Bug 1484496: Part 4b - Add intrinsic type information to most nsSimpleEnumerators. r=mccr8

This allows JS callers to automatically get the correct types during
interation, without having to explicitly specify them.

Comment 19

[:Gijs (he/him)]

Comment on attachment 9002299 [details]
Bug 1484496: Part 5a - Convert browser/ nsISimpleEnumerator users to use JS iteration. r=Gijs

:Gijs (he/him) has approved the revision.

Comment 20

[:Gijs (he/him)]

Comment on attachment 9002300 [details]
Bug 1484496: Part 5b - Convert toolkit/ nsISimpleEnumerator users to use JS iteration. r=Gijs

:Gijs (he/him) has approved the revision.

Comment 21

[Nathan Froyd [:froydnj]]

Comment on attachment 9002295 [details]
Bug 1484496: Part 2 - Add common base class for all nsISimpleEnumerator implementations. r=froydnj

Nathan Froyd [:froydnj] has approved the revision.

Comment 22

[Nathan Froyd [:froydnj]]

Comment on attachment 9002296 [details]
Bug 1484496: Part 3 - Fix nsISimpleEnumerator implementations with broken contracts. r=froydnj

Nathan Froyd [:froydnj] has approved the revision.

Comment 23

[Nathan Froyd [:froydnj]]

Comment on attachment 9002298 [details]
Bug 1484496: Part 4b - Add intrinsic type information to most nsSimpleEnumerators. r=froydnj

Nathan Froyd [:froydnj] has approved the revision.

Comment 24

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 9002301 [details]
Bug 1484496: Part 5c - Convert mobile/ nsISimpleEnumerator users to use JS iteration. r=snorp

James Willcox (:snorp) ([e-mail]) has approved the revision.

Comment 25

[Brian Grinstead [:bgrins]]

Comment on attachment 9002302 [details]
Bug 1484496: Part 5d - Convert devtools/ nsISimpleEnumerator users to use JS iteration. r=bgrins

Brian Grinstead [:bgrins] has approved the revision.

Comment 26

[Andrew McCreight [:mccr8]]

Please be sure to document [symbol] on https://developer.mozilla.org/en-US/docs/Mozilla/XPIDL

Comment 27

[Kris Maglione [:kmag]]

(In reply to Andrew McCreight [:mccr8] from comment #26)
> Please be sure to document [symbol] on
> https://developer.mozilla.org/en-US/docs/Mozilla/XPIDL

Hm... that should really be moved to in-tree docs...

Comment 28

[Andrew McCreight [:mccr8]]

Comment on attachment 9002297 [details]
Bug 1484496: Part 4a - Add JS iterator support to nsISimpleEnumerator. r=mccr8

Andrew McCreight [:mccr8] has approved the revision.

Comment 29

[Andrew McCreight [:mccr8]]

Comment on attachment 9002303 [details]
Bug 1484496: Part 5e - Convert remaining nsISimpleEnumerator users to use JS iteration. r=mccr8

Andrew McCreight [:mccr8] has approved the revision.

Comment 30

[Andrew McCreight [:mccr8]]

Comment on attachment 9002304 [details]
Bug 1484496: Part 6 - Remove unused XPCOMUtils.IterSimpleEnumerator method. r=mccr8

Andrew McCreight [:mccr8] has approved the revision.

Comment 31

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 9002294 [details]
Bug 1484496: Part 1 - Add support for symbol properties to XPIDL. r=nika

:Nika Layzell (digging out of backlog) has approved the revision.

Comment 32

[Kris Maglione [:kmag]]

(In reply to Andrew McCreight [:mccr8] from comment #30)
> Comment on attachment 9002304 [details]
> Bug 1484496: Part 6 - Remove unused XPCOMUtils.IterSimpleEnumerator method.
> r=mccr8
> 
> Andrew McCreight [:mccr8] has approved the revision.
>
> This is used in one place in Thunderbird (
> mailnews/db/gloda/modules/index_msg.js ) so you should file a bug on them.

Comment 33

[Jorg K (CEST = GMT+2)]

Thanks for the heads-up, filed bug 1485820. Replacement examples in the toolkit and "remaining" patches.

ETA for landing?

Comment 34

[Kris Maglione [:kmag]]

(In reply to Jorg K (GMT+2) from comment #33)
> Thanks for the heads-up, filed bug 1485820. Replacement examples in the
> toolkit and "remaining" patches.
> 
> ETA for landing?

As soon as my build finishes.

Comment 35

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/10d2e81f3c8a157151bf5cca7133e65458a859eb
Bug 1484496: Part 1 - Add support for symbol properties to XPIDL. r=nika

https://hg.mozilla.org/integration/mozilla-inbound/rev/062e4138bfde6fb0f010d3fabb82b052b2a1b301
Bug 1484496: Part 2 - Add common base class for all nsISimpleEnumerator implementations. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/15a738855cb06dd495c14fde3c8b54dde513c098
Bug 1484496: Part 3 - Fix nsISimpleEnumerator implementations with broken contracts. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ca4845423179d1f0a2e929a827014b393564b76
Bug 1484496: Part 4a - Add JS iterator support to nsISimpleEnumerator. r=mccr8

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8c65dc566057853a19c477e0b30cd9e81d6326b
Bug 1484496: Part 4b - Add intrinsic type information to most nsSimpleEnumerators. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d6c4d5d3097c603e01e366129efccdb667c6254
Bug 1484496: Part 5a - Convert browser/ nsISimpleEnumerator users to use JS iteration. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b53fb2a71a4086d5951b24e4ea0e7f76d9e3669
Bug 1484496: Part 5b - Convert toolkit/ nsISimpleEnumerator users to use JS iteration. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f1617759de56e327eb0b4edf1c4689a95ad8023
Bug 1484496: Part 5c - Convert mobile/ nsISimpleEnumerator users to use JS iteration. r=snorp

https://hg.mozilla.org/integration/mozilla-inbound/rev/719523b165252de73b82dda809015d3582fca6af
Bug 1484496: Part 5d - Convert devtools/ nsISimpleEnumerator users to use JS iteration. r=bgrins

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b496ebb5bbf0c42c3dd64a90022fbadd4005239
Bug 1484496: Part 5e - Convert remaining nsISimpleEnumerator users to use JS iteration. r=mccr8

https://hg.mozilla.org/integration/mozilla-inbound/rev/2bd127de1f05cd8379c279caea727690b847e5b9
Bug 1484496: Part 6 - Remove unused XPCOMUtils.IterSimpleEnumerator method. r=mccr8

Comment 36

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/40ae82f3f8b57aa80f3891d09bc30fd8149af42f
Bug 1484496: Follow-up: Fix NoQueryNeeded assertion on Windows debug. r=bustage CLOSED TREE

Comment 37

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1049dde16ddb13fbb7b05d362a028a9aa24839f
Bug 1484496: Follow-up: Fix more NoQueryNeeded assertions. r=bustage

Comment 38

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/701ceb33e3d751be2faa8bf1b99bd14d51f3eadb
Bug 1484496: Follow-up: Fix busted JS enumerator in Android directory service. r=bustage

Comment 39

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/10d2e81f3c8a
https://hg.mozilla.org/mozilla-central/rev/062e4138bfde
https://hg.mozilla.org/mozilla-central/rev/15a738855cb0
https://hg.mozilla.org/mozilla-central/rev/8ca484542317
https://hg.mozilla.org/mozilla-central/rev/e8c65dc56605
https://hg.mozilla.org/mozilla-central/rev/6d6c4d5d3097
https://hg.mozilla.org/mozilla-central/rev/1b53fb2a71a4
https://hg.mozilla.org/mozilla-central/rev/3f1617759de5
https://hg.mozilla.org/mozilla-central/rev/719523b16525
https://hg.mozilla.org/mozilla-central/rev/7b496ebb5bbf
https://hg.mozilla.org/mozilla-central/rev/2bd127de1f05
https://hg.mozilla.org/mozilla-central/rev/40ae82f3f8b5
https://hg.mozilla.org/mozilla-central/rev/d1049dde16dd
https://hg.mozilla.org/mozilla-central/rev/701ceb33e3d7

Comment 40

[[github robot]]

Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/df335e87576c786e59839e429727fdb10d399d36
Backport Bug 1484496 - Support JS iterator protocol for nsISimpleEnumerator

Comment 41

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2e9f54416db3366258a526d9c2e439fc2ee0393
Bug 1484496: Follow-up: Fix more busted JS enumerators. r=bustage CLOSED TREE

Comment 42

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/e2e9f54416db

Comment 44

[[e-mail]]

There still seems to be one reference to IterSimpleEnumerator in DXR:
https://dxr.mozilla.org/mozilla-central/rev/87a95e1b7ec691bef7b938e722fe1b01cce68664/toolkit/mozapps/update/nsUpdateService.js#3056