Closed Bug 1098412 Opened 5 years ago Closed 2 years ago

Remove __iterator__ and the legacy Iterator constructor

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: evilpie, Assigned: emk)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat, dev-doc-complete, site-compat, Whiteboard: [DocArea=JS])

Attachments

(4 files, 4 obsolete files)

The custom __iterator__ hook makes implementing [[Enumerate]] slightly more annoying. Let's try to remove it.

Telemetry indicates between 0-3 uses, but I am not sure what his measures. mxr mostly shows hits in the addon-sdk, which might be slightly annoying, when it breaks users.

http://telemetry.mozilla.org/#filter=nightly%2F36%2FSIMPLE_MEASURES_JS_CUSTOMITER&aggregates=multiselect-all!Submissions!Mean!5th%20percentile!25th%20percentile!median!75th%20percentile!95th%20percentile&evoOver=Builds&locked=true&sanitize=true&renderhistogram=Graph
Blocks: 1091900
Depends on: 1098617
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
Keywords: addon-compat
Depends on: 1099329
Depends on: 1099455
Blocks: 1103158
Depends on: 1127084
No longer depends on: 1127084
Blocks: 1112564
After the removal of Reflect.enumerate this isn't really that important anymore for ES6.
No longer blocks: es6
Let's see how badly removing __iterator__ and Iterator breaks things: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2dd4314da721d1ba5819230190613b003fbb17f9
Is there any progress?
Shu actually left, maybe you can pick this up?
OK, taking.
Assignee: nobody → VYV03354
Attachment #8879343 - Attachment is obsolete: true
Attachment #8879344 - Attachment is obsolete: true
Attachment #8879345 - Attachment is obsolete: true
Attachment #8879346 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Summary: Remove __iterator__ → Remove __iterator__ and the legacy Iterator constructor
Comment on attachment 8901428 [details]
Bug 1098412 - Remove __iterator__ implementation.

https://reviewboard.mozilla.org/r/172918/#review178546

::: js/src/jsiter.cpp:951
(Diff revision 1)
>      if (flags == 0 || flags == JSITER_ENUMERATE) {
>          if (MOZ_UNLIKELY(obj->is<ProxyObject>()))
>              return Proxy::enumerate(cx, obj);
>      }
>  
>      RootedObject res(cx);

This no longer needs to be a root. Please change this to `JSObject* res` and move it right before:

if (flags & JSITER_FOREACH) {

Also, please remove iteratorIntrinsic from vm/CommonPropertyNames.h That will catch a few more places where you can remove code, like CanStoreInIteratorCache :)
Comment on attachment 8901428 [details]
Bug 1098412 - Remove __iterator__ implementation.

https://reviewboard.mozilla.org/r/172918/#review178546

> This no longer needs to be a root. Please change this to `JSObject* res` and move it right before:
> 
> if (flags & JSITER_FOREACH) {
> 
> Also, please remove iteratorIntrinsic from vm/CommonPropertyNames.h That will catch a few more places where you can remove code, like CanStoreInIteratorCache :)

Thsnks for the comment, fixed.
Comment on attachment 8901426 [details]
Bug 1098412 - Remove and update tests that use __iterator__.

https://reviewboard.mozilla.org/r/172914/#review178646
Attachment #8901426 - Flags: review?(luke) → review+
Comment on attachment 8901427 [details]
Bug 1098412 - Remove and update tests that use the legacy Iterator constructor.

https://reviewboard.mozilla.org/r/172916/#review178648
Attachment #8901427 - Flags: review?(luke) → review+
Comment on attachment 8901428 [details]
Bug 1098412 - Remove __iterator__ implementation.

https://reviewboard.mozilla.org/r/172918/#review178650
Attachment #8901428 - Flags: review?(luke) → review+
Comment on attachment 8901429 [details]
Bug 1098412 - Remove the legacy Iterator constructor.

https://reviewboard.mozilla.org/r/172920/#review178652

\o/  Thanks for finishing this!
Attachment #8901429 - Flags: review?(luke) → review+
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/1643079d7661
Remove and update tests that use __iterator__. r=luke
https://hg.mozilla.org/integration/autoland/rev/8281805e7c3c
Remove and update tests that use the legacy Iterator constructor. r=luke
https://hg.mozilla.org/integration/autoland/rev/e9a0298824d6
Remove __iterator__ implementation. r=luke
https://hg.mozilla.org/integration/autoland/rev/f4da55f291cb
Remove the legacy Iterator constructor. r=luke
Backed out in https://hg.mozilla.org/integration/autoland/rev/eb4a7917f3e1 for failing the devtools test browser_webconsole_bug_632347_iterators_generators.js  like https://treeherder.mozilla.org/logviewer.html#?job_id=126571859&repo=autoland

Horrifyingly enough, that test is "skip-if = e10s" which now means "only run on Win7 debug" so other than running locally, on try you have to do Win7 debug to see it run.
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/929a062222d8
Remove and update tests that use __iterator__. r=luke
https://hg.mozilla.org/integration/autoland/rev/8f1c6864d359
Remove and update tests that use the legacy Iterator constructor. r=luke
https://hg.mozilla.org/integration/autoland/rev/24297940e626
Remove __iterator__ implementation. r=luke
https://hg.mozilla.org/integration/autoland/rev/fce9e9b0be2b
Remove the legacy Iterator constructor. r=luke
Depends on: 1395366
Duplicate of this bug: 881061
Depends on: 1395744
No longer depends on: 1395366
Duplicate of this bug: 1124835
Duplicate of this bug: 1148944
Depends on: 1461324
You need to log in before you can comment on or make changes to this bug.