1091900 - Cleanup jsiter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S. (please needinfo tschuster)]

There is some room for improvements.

Comment 1

[Tom S. (please needinfo tschuster)]

Attachment: Remove iteratorObject hook

We should not be adding new iteratorObject uses anyway. @@iterator all the way.

Comment 2

[Tom S. (please needinfo tschuster)]

Attachment: Move ForOfIterator to its own file

It's almost disturbing how hard it is figure out the right includes. And how much is leaked from random headers.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8514628 [details] [diff] [review]
Move ForOfIterator to its own file

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

Nice :)

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 8514618 [details] [diff] [review]
Remove iteratorObject hook

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

Very nice work. I love it.

Comment 5

[Tom S. (please needinfo tschuster)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc265cc31095

Comment 6

[Tom S. (please needinfo tschuster)]

Fixed build bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/50b67052e480

Comment 7

[Carsten Book [:Tomcat]]

sorry had to back this out in https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound&revision=dc265cc31095 since one of this changes caused i guess https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3630077&repo=mozilla-inbound

Comment 8

[Tom S. (please needinfo tschuster)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92d4cb1178bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/e368632d6840

Comment 9

[Tom S. (please needinfo tschuster)]

Attachment: empty-iterator

This introduce a function to explicitly create an empty property iterator, as required (by our implementation) for (for .. in).

Comment 10

[Tom S. (please needinfo tschuster)]

Attachment: empty-iterator

Comment 11

[Tom S. (please needinfo tschuster)]

Attachment: Make VectorTo*Iterator local

I think the only use in CrossCompartmentWrapper could use EnumeratedIdVectorToIterator.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/92d4cb1178bd
https://hg.mozilla.org/mozilla-central/rev/e368632d6840

Comment 13

[Tom S. (please needinfo tschuster)]

Attachment: Change various outparams to MutableHandleObject

Comment 14

[Tom S. (please needinfo tschuster)]

Attachment: Change various outparams to MutableHandleObject

Comment 15

[Eric Faust [:efaust]]

Comment on attachment 8523171 [details] [diff] [review]
Change various outparams to MutableHandleObject

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

r=me with comment addressed.

::: dom/base/nsGlobalWindow.cpp
@@ +940,5 @@
>      return false;
>    }
>    return js::AppendUnique(cx, props, innerProps);
>  }
>  

nit: new second empty line

::: js/src/proxy/Proxy.cpp
@@ -389,4 @@
>  {
>      JS_CHECK_RECURSION(cx, return false);
>      const BaseProxyHandler *handler = proxy->as<ProxyObject>().handler();
> -    vp.setUndefined(); // default result if we refuse to perform this action

I think this is still needed, no? What if policy.returnValue() is |false|, then what is projected out in objp? Can't we replace this with |objp.set(nullptr)|?

Comment 16

[Eric Faust [:efaust]]

Comment on attachment 8519411 [details] [diff] [review]
Make VectorTo*Iterator local

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

APPROVED.

Comment 17

[Eric Faust [:efaust]]

Comment on attachment 8519409 [details] [diff] [review]
empty-iterator

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

Looks good. A nice refactor.

Comment 18

[Tom S. (please needinfo tschuster)]

Comment on attachment 8523171 [details] [diff] [review]
Change various outparams to MutableHandleObject

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

::: js/src/proxy/Proxy.cpp
@@ -389,4 @@
>  {
>      JS_CHECK_RECURSION(cx, return false);
>      const BaseProxyHandler *handler = proxy->as<ProxyObject>().handler();
> -    vp.setUndefined(); // default result if we refuse to perform this action

In theory we shouldn't touch the out value in that error case, but it can't hurt to do either.

Comment 19

[Tom S. (please needinfo tschuster)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=633052c58bac
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a1582c375c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/22179ebb537b
https://hg.mozilla.org/integration/mozilla-inbound/rev/14a34e921e1b

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8a1582c375c2
https://hg.mozilla.org/mozilla-central/rev/22179ebb537b
https://hg.mozilla.org/mozilla-central/rev/14a34e921e1b

Comment 21

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.