Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
3 months ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
There is some room for improvements.
(Assignee)

Comment 1

4 years ago
Created attachment 8514618 [details] [diff] [review]
Remove iteratorObject hook

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

Comment 2

4 years ago
Created attachment 8514628 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Attachment #8514628 - Flags: review?(kvijayan)
(Assignee)

Updated

4 years ago
Attachment #8514618 - Flags: review?(jorendorff)
(Assignee)

Updated

4 years ago
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+
(Assignee)

Updated

4 years ago
Keywords: leave-open
(Assignee)

Comment 9

4 years ago
Created attachment 8519408 [details] [diff] [review]
empty-iterator

This introduce a function to explicitly create an empty property iterator, as required (by our implementation) for (for .. in).
Attachment #8519408 - Flags: review?(jorendorff)
(Assignee)

Comment 10

4 years ago
Created attachment 8519409 [details] [diff] [review]
empty-iterator
Attachment #8519408 - Attachment is obsolete: true
Attachment #8519408 - Flags: review?(jorendorff)
Attachment #8519409 - Flags: review?(jorendorff)
(Assignee)

Comment 11

4 years ago
Created attachment 8519411 [details] [diff] [review]
Make VectorTo*Iterator local

I think the only use in CrossCompartmentWrapper could use EnumeratedIdVectorToIterator.
(Assignee)

Updated

4 years ago
Attachment #8519411 - Flags: review?(jorendorff)
(Assignee)

Updated

4 years ago
Depends on: 1097267
(Assignee)

Updated

4 years ago
Attachment #8519409 - Flags: review?(jorendorff) → review?(efaustbmo)
(Assignee)

Updated

4 years ago
Attachment #8519411 - Flags: review?(jorendorff) → review?(efaustbmo)
(Assignee)

Comment 13

4 years ago
Created attachment 8522466 [details] [diff] [review]
Change various outparams to MutableHandleObject
Attachment #8522466 - Flags: review?(efaustbmo)
(Assignee)

Updated

4 years ago
Blocks: 783829
(Assignee)

Updated

4 years ago
Depends on: 1098412
(Assignee)

Updated

4 years ago
Depends on: 1099062
(Assignee)

Comment 14

4 years ago
Created attachment 8523171 [details] [diff] [review]
Change various outparams to MutableHandleObject
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+
(Assignee)

Comment 18

4 years ago
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.
(Assignee)

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 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.