Closed Bug 1091900 Opened 5 years ago Closed 5 years ago

Cleanup jsiter

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: evilpie, Assigned: evilpie)

References

Details

Attachments

(5 files, 2 obsolete files)

There is some room for improvements.
We should not be adding new iteratorObject uses anyway. @@iterator all the way.
It's almost disturbing how hard it is figure out the right includes. And how much is leaked from random headers.
Attachment #8514628 - Flags: review?(kvijayan)
Attachment #8514618 - Flags: review?(jorendorff)
Attachment #8514628 - Flags: review?(kvijayan) → review?(jdemooij)
Comment on attachment 8514628 [details] [diff] [review]
Move ForOfIterator to its own file

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

Nice :)
Attachment #8514628 - Flags: review?(jdemooij) → review+
Comment on attachment 8514618 [details] [diff] [review]
Remove iteratorObject hook

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

Very nice work. I love it.
Attachment #8514618 - Flags: review?(jorendorff) → review+
Keywords: leave-open
Attached patch empty-iterator (obsolete) — Splinter Review
This introduce a function to explicitly create an empty property iterator, as required (by our implementation) for (for .. in).
Attachment #8519408 - Flags: review?(jorendorff)
Attached patch empty-iteratorSplinter Review
Attachment #8519408 - Attachment is obsolete: true
Attachment #8519408 - Flags: review?(jorendorff)
Attachment #8519409 - Flags: review?(jorendorff)
I think the only use in CrossCompartmentWrapper could use EnumeratedIdVectorToIterator.
Attachment #8519411 - Flags: review?(jorendorff)
Depends on: 1097267
Attachment #8519409 - Flags: review?(jorendorff) → review?(efaustbmo)
Attachment #8519411 - Flags: review?(jorendorff) → review?(efaustbmo)
Attachment #8522466 - Flags: review?(efaustbmo)
Blocks: 783829
Depends on: 1098412
Depends on: 1099062
Attachment #8522466 - Attachment is obsolete: true
Attachment #8522466 - Flags: review?(efaustbmo)
Attachment #8523171 - Flags: review?(efaustbmo)
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)|?
Attachment #8523171 - Flags: review?(efaustbmo) → review+
Comment on attachment 8519411 [details] [diff] [review]
Make VectorTo*Iterator local

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

APPROVED.
Attachment #8519411 - Flags: review?(efaustbmo) → review+
Comment on attachment 8519409 [details] [diff] [review]
empty-iterator

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

Looks good. A nice refactor.
Attachment #8519409 - Flags: review?(efaustbmo) → review+
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.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.