Closed
Bug 1155700
Opened 10 years ago
Closed 8 years ago
Make built-in Map/Set visible with XRay vision
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: azasypkin, Assigned: evilpie)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
Currently we should use Components.utils.waiveXrays() to make Map/Set visible in privileged code, but it would be great to make it visible through XRay vision as other well known built-in types.
Comment 1•10 years ago
|
||
Yes, this would be nice. Just needs somebody to do it. CCing the two people aside from me who know how to implement XrayToJS. :-)
Blocks: XrayToJS
Comment 2•10 years ago
|
||
(In reply to Oleg Zasypkin [:azasypkin] from comment #0)
> Currently we should use Components.utils.waiveXrays() to make Map/Set
> visible in privileged code, but it would be great to make it visible through
> XRay vision as other well known built-in types.
"Should"? That sounds scary to me, how is that safe?
(In reply to Bobby Holley (:bholley) from comment #1)
> Yes, this would be nice. Just needs somebody to do it. CCing the two people
> aside from me who know how to implement XrayToJS. :-)
Unfortunately, my Q2 is already accounted for :(
Reporter | ||
Comment 3•10 years ago
|
||
(In reply to Nick Fitzgerald [:fitzgen] from comment #2)
> (In reply to Oleg Zasypkin [:azasypkin] from comment #0)
> > Currently we should use Components.utils.waiveXrays() to make Map/Set
> > visible in privileged code, but it would be great to make it visible through
> > XRay vision as other well known built-in types.
>
> "Should"? That sounds scary to me, how is that safe?
>
Well, "should" was probably too strong :) We pass Map (test generated data) from the test(web content context) to API mock (privileged context) in Marionette JS integration tests(gaia/FxOS). So in tests we can sacrifice safety for the simpler code.
Comment 4•9 years ago
|
||
Taking, aiming for Q3, possibly along with bug 1023984 but we'll see how this one goes first.
Updated•9 years ago
|
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → evilpies
Assignee | ||
Comment 5•8 years ago
|
||
Assignee | ||
Comment 6•8 years ago
|
||
My plan is to extend plain-object-prototypes.js in the future and fix some other prototypes. I am not sure exactly how useful the error message test is, and if it might actually make sense if it said something like "incompatible MapPrototype".
Attachment #8807987 -
Attachment is obsolete: true
Attachment #8809750 -
Flags: review?(arai.unmht)
Comment 7•8 years ago
|
||
Comment on attachment 8809750 [details] [diff] [review]
Use ClassSpec for Map/Set
Review of attachment 8809750 [details] [diff] [review]:
-----------------------------------------------------------------
r+ with some fix in tests.
::: js/src/builtin/MapObject.cpp
@@ +302,5 @@
> + nullptr,
> + MapObject::staticProperties,
> + MapObject::methods,
> + MapObject::properties,
> + nullptr,
trailing `nullptr` can be omitted.
@@ +1032,5 @@
> + nullptr,
> + SetObject::staticProperties,
> + SetObject::methods,
> + SetObject::properties,
> + nullptr,
here too, `nullptr` can be omitted.
::: js/src/jit-test/tests/basic/plain-object-prototypes.js
@@ +6,5 @@
> + Date.prototype,
> +];
> +
> +// if ('Promise' in this)
> +// prototypes.push(Promise.prototype)
This needs some comment or bug number.
also this can be simply moved into `prototypes` array, since Promise always exists.
@@ +13,5 @@
> + delete prototype[Symbol.toStringTag];
> + assertEq(Object.prototype.toString.call(prototype), "[object Object]");
> +}
> +
> +// Make sure the real class name is not exposed by Errors.
Please move this test *before* removing @@toStringTag, or simply new file.
@@ +28,5 @@
> +assertThrowsObjectError(() => Map.prototype.has(0));
> +assertThrowsObjectError(() => Set.prototype.has(0));
> +assertThrowsObjectError(() => WeakMap.prototype.has({}));
> +assertThrowsObjectError(() => WeakSet.prototype.has({}));
> +assertThrowsObjectError(() => WeakSet.prototype.has({}));
\ No newline at end of file
Please remove duplication and add Date.prototype.getSeconds() or something instead.
Attachment #8809750 -
Flags: review?(arai.unmht) → review+
Assignee | ||
Comment 8•8 years ago
|
||
Sadly because we still can't xray iterators I had to bring back that hack in test_XrayToJS. I will try to look into writing xray support iterators soon.
Assignee | ||
Comment 9•8 years ago
|
||
Bz, sorry for always making you review those, but Bobby is busy as well.
Flags: needinfo?(bzbarsky)
Comment 10•8 years ago
|
||
Comment on attachment 8810125 [details] [diff] [review]
Enable Map/Set Xrays
Please test all the things exposed over Xrays. I assume testXray() tests "constructor" and perhaps toString(), right? And you're testing size/get on Map and size/has on Set. But still need to test has on Map, set/delete on Map, add/delete on Set, clear/forEach on both. And if the iterator stuff actually works, test that too, please (entries, values, keys, for-of loop).
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 11•8 years ago
|
||
Sorry, that was sloppy. Values, keys and @@iterator don't work, because we don't have Xrays to Iterators.
Attachment #8810125 -
Attachment is obsolete: true
Comment 13•8 years ago
|
||
Comment on attachment 8811891 [details] [diff] [review]
v2 Enable Map/Set Xrays
r=me
Flags: needinfo?(bzbarsky)
Attachment #8811891 -
Flags: review+
Comment 14•8 years ago
|
||
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e93d0eef9f72
Change Map/Set to use ClassSpec. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/547235a8b10e
Enable Map/Set Xrays. r=bz
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e93d0eef9f72
https://hg.mozilla.org/mozilla-central/rev/547235a8b10e
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•