Closed
Bug 978229
Opened 10 years ago
Closed 9 years ago
ES6 Proxies: IsSealed() et al delenda est
Categories
(Core :: JavaScript: Standard Library, defect)
Core
JavaScript: Standard Library
Tracking
()
RESOLVED
FIXED
People
(Reporter: efaust, Assigned: efaust)
References
Details
(Whiteboard: [js:p1])
Attachments
(4 files)
4.68 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
2.08 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
908 bytes,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
2.96 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
Firstly, there's no way that passive paraphrastic is right, because "al" is misdeclined. (Alii, maybe? "Ad takes the?" "A-accusative!" "Right, so dom*um*") These methods looks clever, but actually mask a few subtle mistakes in our implementations with respect to when we throw and how many times we call getOwnPropertyDescriptor on the target.
Updated•10 years ago
|
Component: JavaScript Engine → JavaScript: Standard Library
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [js:p1]
Assignee | ||
Comment 1•10 years ago
|
||
This isn't strictly necessary, but having a fallible overload of IdToValue which is only called when we are trying to expose that id as a string (property name) to a proxy trap is a poor name choice. Let's change it.
Assignee | ||
Comment 2•10 years ago
|
||
This also removes a call of HasOwn, which should die for similar reasons.
Attachment #8387220 -
Flags: review?(jorendorff)
Updated•10 years ago
|
Attachment #8387217 -
Flags: review?(jorendorff) → review+
Comment 3•10 years ago
|
||
Comment on attachment 8387220 [details] [diff] [review] Part 1: remove IsSealed from and cleanup [[HasProperty]] Review of attachment 8387220 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. HasOwn and IsSealed, good grief! ::: js/src/jit-test/tests/proxy/testDirectProxyHas6.js @@ +8,5 @@ > + 'foo' in new Proxy(target, { > + has: function (target, name) { > + return false; > + } > + }), false); One more thing to test: Don't throw a type error if the trap reports a nonexistent property as existing, regardless of extensibility. (!) ::: js/src/jsproxy.cpp @@ +1949,5 @@ > if (!Invoke(cx, ObjectValue(*handler), trap, ArrayLength(argv), argv, &trapResult)) > return false; > > // step 6 > bool success = ToBoolean(trapResult);; Kill the extra ; while you're passing through?
Attachment #8387220 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 4•10 years ago
|
||
This is trivial, but still needs doing.
Assignee | ||
Updated•10 years ago
|
Attachment #8407610 -
Flags: review?(jorendorff)
Updated•10 years ago
|
Attachment #8407610 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 5•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/8d3be1c62d8d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/21d545a5ac3f remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/159a32e886d0
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8d3be1c62d8d https://hg.mozilla.org/mozilla-central/rev/21d545a5ac3f https://hg.mozilla.org/mozilla-central/rev/159a32e886d0
Flags: in-testsuite+
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8437124 -
Flags: review?(jwalden+bmo)
Updated•10 years ago
|
Attachment #8437124 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 9•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/06bd71f490fa
Assignee | ||
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ed25ba82a9d
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/06bd71f490fa https://hg.mozilla.org/mozilla-central/rev/8ed25ba82a9d
Updated•9 years ago
|
Updated•9 years ago
|
Flags: needinfo?(efaustbmo)
You need to log in
before you can comment on or make changes to this bug.
Description
•