Closed
Bug 1246318
Opened 7 years ago
Closed 7 years ago
Remove [[Enumerate]] and associated reflective capabilities
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: evilpie, Assigned: evilpie)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, site-compat, Whiteboard: [DocArea=JS])
Attachments
(4 files)
12.78 KB,
patch
|
efaust
:
review+
|
Details | Diff | Splinter Review |
4.17 KB,
patch
|
efaust
:
review+
|
Details | Diff | Splinter Review |
9.04 KB,
patch
|
efaust
:
review+
|
Details | Diff | Splinter Review |
2.29 KB,
patch
|
efaust
:
review+
|
Details | Diff | Splinter Review |
tl;dr This is going away and we are better of that way.
Assignee | ||
Updated•7 years ago
|
Assignee: evilpies → nobody
Comment 1•7 years ago
|
||
dev-doc-info |
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy/handler/enumerate https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect/enumerate
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → evilpies
Assignee | ||
Comment 4•7 years ago
|
||
The enumerate trap is not pure virtual anymore, so nobody is forced to implement this trap. In the first local iteration of this patch, I accidentally removed some calls to BaseProxyHandler::enumerate that were overwriting a call to the DirectProxyHandler version.
Attachment #8716902 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 5•7 years ago
|
||
Attachment #8716903 -
Flags: review?(jorendorff)
Assignee | ||
Comment 6•7 years ago
|
||
Attachment #8716980 -
Flags: review?(efaustbmo)
Updated•7 years ago
|
Attachment #8716902 -
Attachment is patch: true
Assignee | ||
Comment 7•7 years ago
|
||
This is basically a partial backout of Bug 783829. There is no good reason to support new-style iterators anymore in this context.
Attachment #8716994 -
Flags: review?(efaustbmo)
Comment 8•7 years ago
|
||
Comment on attachment 8716902 [details] [diff] [review] Make the enumerate trap non-standard Review of attachment 8716902 [details] [diff] [review]: ----------------------------------------------------------------- r=me with question below addressed. ::: js/public/Proxy.h @@ +58,5 @@ > * (CPOWs, implemented in js/ipc) > * > * ### Proxies and internal methods > * > + * ES 2015 specifies 13 internal methods. The runtime semantics of just nit: Should say ES2016. We are removing it in this year's edition. ::: js/src/proxy/DeadObjectProxy.h @@ +37,5 @@ > virtual bool construct(JSContext* cx, HandleObject proxy, const CallArgs& args) const override; > > /* SpiderMonkey extensions. */ > // BaseProxyHandler::getPropertyDescriptor will throw by calling getOwnPropertyDescriptor. > + // BaseProxyHandler::enumerate will throw by calling ownKeys. Didn't we just remove BPH::enumerate?
Attachment #8716902 -
Flags: review?(efaustbmo) → review+
Updated•7 years ago
|
Attachment #8716980 -
Flags: review?(efaustbmo) → review+
Comment 9•7 years ago
|
||
Comment on attachment 8716994 [details] [diff] [review] Remove support for new style iterators with for..in Review of attachment 8716994 [details] [diff] [review]: ----------------------------------------------------------------- I'm kinda surprised to see this. Do we not do new iterators internally?
Attachment #8716994 -
Flags: review?(efaustbmo) → review+
Assignee | ||
Comment 10•7 years ago
|
||
(In reply to Eric Faust [:efaust] from comment #8) > Comment on attachment 8716902 [details] [diff] [review] > Make the enumerate trap non-standard > > Review of attachment 8716902 [details] [diff] [review]: > ----------------------------------------------------------------- > ::: js/src/proxy/DeadObjectProxy.h > @@ +37,5 @@ > > virtual bool construct(JSContext* cx, HandleObject proxy, const CallArgs& args) const override; > > > > /* SpiderMonkey extensions. */ > > // BaseProxyHandler::getPropertyDescriptor will throw by calling getOwnPropertyDescriptor. > > + // BaseProxyHandler::enumerate will throw by calling ownKeys. > > Didn't we just remove BPH::enumerate? Actually not yet. I moved BPH::enumerate to the non-standard section of BPH. It is also not pure-virtual anymore, so nobody is required to implement enumerate. However this made me realize that we can probably completely remove BPH::enumerate, but this will break the enumerate (called iterate in this case) trap for ScriptedIndirectProxy. For that matter Wrappers are the only other special case, because they usually inherit the DirectProxy::enumerate trap, which calls GetIterator(target).
Assignee | ||
Comment 11•7 years ago
|
||
(In reply to Eric Faust [:efaust] from comment #9) > Comment on attachment 8716994 [details] [diff] [review] > Remove support for new style iterators with for..in > > Review of attachment 8716994 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm kinda surprised to see this. Do we not do new iterators internally? Okay. So internally, most of the time we end up with NativeIteratorNext, so we never actually take a "standard" path. With the changes in ES2016 you can't create any custom iterator for for..in. However we have our very old non-standard extensions Iterator and __iterator__. Iterator still uses the old iteration protocol. __iterator__ is of course user defined, but I don't see why we should support the new iteration protocol there.
Comment 12•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd209a5854c0 https://hg.mozilla.org/integration/mozilla-inbound/rev/da0d807517ca https://hg.mozilla.org/integration/mozilla-inbound/rev/874423b7e907
Assignee | ||
Updated•7 years ago
|
Whiteboard: [DocArea=JS] → [DocArea=JS], keep-open
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Whiteboard: [DocArea=JS], keep-open → [DocArea=JS]
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fd209a5854c0 https://hg.mozilla.org/mozilla-central/rev/da0d807517ca https://hg.mozilla.org/mozilla-central/rev/874423b7e907
Comment 14•7 years ago
|
||
Comment on attachment 8716903 [details] [diff] [review] Remove the still disabled Reflect.enumerate code Review of attachment 8716903 [details] [diff] [review]: ----------------------------------------------------------------- Stealing, since Jason is out of the office this week. APPROVED.
Attachment #8716903 -
Flags: review?(jorendorff) → review+
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/be82a484968d
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment 17•7 years ago
|
||
Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/proxy-s-enumerate-handler-has-been-removed/
Keywords: site-compat
Comment 18•7 years ago
|
||
https://developer.mozilla.org/en-US/Firefox/Releases/47 https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy/handler/enumerate https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect/enumerate
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•