Closed Bug 1484496 Opened Last year Closed Last year

Support JS iterator protocol for nsISimpleEnumerator

Categories

(Core :: XPConnect, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug)

Details

(Whiteboard: [overhead:12k])

Attachments

(11 files, 5 obsolete files)

46 bytes, text/x-phabricator-request
Nika
: review+
Details | Review
46 bytes, text/x-phabricator-request
froydnj
: review+
Details | Review
46 bytes, text/x-phabricator-request
froydnj
: review+
Details | Review
46 bytes, text/x-phabricator-request
mccr8
: review+
Details | Review
46 bytes, text/x-phabricator-request
froydnj
: review+
Details | Review
46 bytes, text/x-phabricator-request
Gijs
: review+
Details | Review
46 bytes, text/x-phabricator-request
Gijs
: review+
Details | Review
46 bytes, text/x-phabricator-request
snorp
: review+
Details | Review
46 bytes, text/x-phabricator-request
bgrins
: review+
Details | Review
46 bytes, text/x-phabricator-request
mccr8
: review+
Details | Review
46 bytes, text/x-phabricator-request
mccr8
: review+
Details | Review
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.
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.
(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.
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.
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.
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.
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.
This allows JS callers to automatically get the correct types during
interation, without having to explicitly specify them.
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.
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.
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.
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.
This allows JS callers to automatically get the correct types during
interation, without having to explicitly specify them.
Attachment #9002333 - Attachment is obsolete: true
Attachment #9002334 - Attachment is obsolete: true
Attachment #9002335 - Attachment is obsolete: true
Attachment #9002336 - Attachment is obsolete: true
Attachment #9002337 - Attachment is obsolete: true
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.
Attachment #9002299 - Flags: review+
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.
Attachment #9002300 - Flags: review+
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.
Attachment #9002295 - Flags: review+
Comment on attachment 9002296 [details]
Bug 1484496: Part 3 - Fix nsISimpleEnumerator implementations with broken contracts. r=froydnj

Nathan Froyd [:froydnj] has approved the revision.
Attachment #9002296 - Flags: review+
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.
Attachment #9002298 - Flags: review+
Comment on attachment 9002301 [details]
Bug 1484496: Part 5c - Convert mobile/ nsISimpleEnumerator users to use JS iteration. r=snorp

James Willcox (:snorp) (jwillcox@mozilla.com) has approved the revision.
Attachment #9002301 - Flags: review+
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.
Attachment #9002302 - Flags: review+
(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 on attachment 9002297 [details]
Bug 1484496: Part 4a - Add JS iterator support to nsISimpleEnumerator. r=mccr8

Andrew McCreight [:mccr8] has approved the revision.
Attachment #9002297 - Flags: review+
Attachment #9002294 - Attachment description: Bug 1484496: Part 1 - Add support for symbol properties to XPIDL. r=mccr8 → Bug 1484496: Part 1 - Add support for symbol properties to XPIDL. r=nika
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.
Attachment #9002303 - Flags: review+
Comment on attachment 9002304 [details]
Bug 1484496: Part 6 - Remove unused XPCOMUtils.IterSimpleEnumerator method. r=mccr8

Andrew McCreight [:mccr8] has approved the revision.
Attachment #9002304 - Flags: review+
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.
Attachment #9002294 - Flags: review+
(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.
Flags: needinfo?(jorgk)
Thanks for the heads-up, filed bug 1485820. Replacement examples in the toolkit and "remaining" patches.

ETA for landing?
Flags: needinfo?(jorgk)
(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.
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
https://hg.mozilla.org/integration/mozilla-inbound/rev/40ae82f3f8b57aa80f3891d09bc30fd8149af42f
Bug 1484496: Follow-up: Fix NoQueryNeeded assertion on Windows debug. r=bustage CLOSED TREE
Whiteboard: [overhead:5k] → [overhead:12k]
https://hg.mozilla.org/integration/mozilla-inbound/rev/701ceb33e3d751be2faa8bf1b99bd14d51f3eadb
Bug 1484496: Follow-up: Fix busted JS enumerator in Android directory service. r=bustage
See Also: → 1486147
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2e9f54416db3366258a526d9c2e439fc2ee0393
Bug 1484496: Follow-up: Fix more busted JS enumerators. r=bustage CLOSED TREE
Blocks: 1486182
See Also: → 1486249
Depends on: 1486311
Duplicate of this bug: 1483260
Depends on: 1488908
Depends on: 1513434
You need to log in before you can comment on or make changes to this bug.