Tom S [:evilpie] Reporter
	Description • 10 years ago

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

	Tom S [:evilpie] Reporter
	Comment 1 • 8 years ago

After the removal of Reflect.enumerate this isn't really that important anymore for ES6.

	Shu-yu Guo [:shu]
	Comment 6 • 7 years ago

Let's see how badly removing __iterator__ and Iterator breaks things: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2dd4314da721d1ba5819230190613b003fbb17f9

	Masatoshi Kimura [:emk] Assignee
	Comment 7 • 7 years ago

Is there any progress?

	Tom S [:evilpie] Reporter
	Comment 8 • 7 years ago

Shu actually left, maybe you can pick this up?

	Masatoshi Kimura [:emk] Assignee
	Comment 9 • 7 years ago

OK, taking.

	Jan de Mooij [:jandem]
	Comment 14 • 7 years ago
mozreview-review

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 :)

	Masatoshi Kimura [:emk] Assignee
	Comment 19 • 7 years ago
mozreview-review-reply

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.

	Luke Wagner [:luke]
	Comment 20 • 7 years ago
mozreview-review

Comment on attachment 8901426 [details]
Bug 1098412 - Remove and update tests that use __iterator__.

https://reviewboard.mozilla.org/r/172914/#review178646

	Luke Wagner [:luke]
	Comment 21 • 7 years ago
mozreview-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

	Luke Wagner [:luke]
	Comment 22 • 7 years ago
mozreview-review

Comment on attachment 8901428 [details]
Bug 1098412 - Remove __iterator__ implementation.

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

	Luke Wagner [:luke]
	Comment 23 • 7 years ago
mozreview-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!

	Pulsebot
	Comment 28 • 7 years ago

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

	Phil Ringnalda (:philor)
	Comment 29 • 7 years ago

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.

	Pulsebot
	Comment 34 • 7 years ago

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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 35 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/929a062222d8
https://hg.mozilla.org/mozilla-central/rev/8f1c6864d359
https://hg.mozilla.org/mozilla-central/rev/24297940e626
https://hg.mozilla.org/mozilla-central/rev/fce9e9b0be2b

	Kohei Yoshino [:kohei]
	Comment 37 • 7 years ago

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2017/legacy-iterator-protocol-has-been-removed/

	Florian Scholz (Open Web Docs)
	Comment 38 • 7 years ago

https://developer.mozilla.org/en-US/Firefox/Releases/57#JavaScript
https://developer.mozilla.org/en-US/docs/Archive/Web/Iterator
https://developer.mozilla.org/en-US/docs/Archive/Web/StopIteration