Closed
Bug 1002737
Opened 7 years ago
Closed 7 years ago
General Cleanups to PropDesc
Categories
(Core :: JavaScript: Standard Library, defect)
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: efaust, Assigned: efaust)
References
Details
Attachments
(8 files)
10.54 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
9.60 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
2.35 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
11.52 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
1.77 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
6.47 KB,
patch
|
Details | Diff | Splinter Review | |
9.27 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
2.23 KB,
patch
|
jorendorff
:
review+
jimb
:
feedback+
|
Details | Diff | Splinter Review |
PropDesc has come about very organically. People have just added things as they were needed. Now that we want to expose it, we should probably add a bit more polish to the idea.
Assignee | ||
Comment 1•7 years ago
|
||
This also adds setters for a few internal fields, since this is no longer a function that can reach private fields.
Comment 2•7 years ago
|
||
Comment on attachment 8414072 [details] [diff] [review] Cleanup: Find a new home for PropDesc::unwrapDebuggerObjectsInto Review of attachment 8414072 [details] [diff] [review]: ----------------------------------------------------------------- Stealing review. Looks great. ::: js/src/vm/Debugger.cpp @@ +5357,5 @@ > return false; > + Handle<PropDesc> wrapped = Handle<PropDesc>::fromMarkedLocation(&descs[i]); > + MutableHandle<PropDesc> unwrapped = > + MutableHandle<PropDesc>::fromMarkedLocation(&unwrappedDescs[i]); > + if (!dbg->unwrapPropDescInto(cx, obj, wrapped, unwrapped)) Can we make AutoPropDescArrayRooter::operator[] return a MutableHandle? Then the fromMarkedLocation call is (a) hidden (b) located in a place where it's obviously-correct. And this code ends up looking even nicer than it was before: if (!dbg->unwrapPropDescInto(cx, obj, descs[i], unwrappedDescs[i]))
Attachment #8414072 -
Flags: review?(jimb) → review+
Assignee | ||
Comment 3•7 years ago
|
||
> Can we make AutoPropDescArrayRooter::operator[] return a MutableHandle? Then
> the fromMarkedLocation call is (a) hidden (b) located in a place where it's
> obviously-correct. And this code ends up looking even nicer than it was
> before:
Ask and you shall receive ;)
Attachment #8414930 -
Flags: review?(jorendorff)
Assignee | ||
Comment 4•7 years ago
|
||
When you clear it, clear it. It's much easier to start from scratch every time.
Attachment #8414932 -
Flags: review?(jorendorff)
Assignee | ||
Comment 5•7 years ago
|
||
It's only ever either UndefinedValue() or ObjectValue(). We can do that better. And it simplifies projecting the object back out for later use.
Attachment #8414933 -
Flags: review?(jorendorff)
Assignee | ||
Comment 6•7 years ago
|
||
Attachment #8414934 -
Flags: review?(jorendorff)
Assignee | ||
Comment 7•7 years ago
|
||
Attachment #8414935 -
Flags: review?(jorendorff)
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8414936 -
Flags: review?(jorendorff)
Assignee | ||
Comment 9•7 years ago
|
||
Feel free to pass this to Jimb, if it's more fitting.
Attachment #8414937 -
Flags: review?(jorendorff)
Comment 10•7 years ago
|
||
Comment on attachment 8414930 [details] [diff] [review] Make AutoPropDescArrayRooter into AutoPropDescVector Review of attachment 8414930 [details] [diff] [review]: ----------------------------------------------------------------- Nice. ::: js/src/jsobjinlines.h @@ +791,4 @@ > { > public: > + explicit AutoPropDescVector(JSContext *cx > + MOZ_GUARD_OBJECT_NOTIFIER_PARAM) This doesn't seem to be indented properly. "MOZ_" should line up wtih "JSContext".
Attachment #8414930 -
Flags: review?(jorendorff) → review+
Updated•7 years ago
|
Attachment #8414932 -
Flags: review?(jorendorff) → review+
Updated•7 years ago
|
Attachment #8414933 -
Flags: review?(jorendorff) → review+
Updated•7 years ago
|
Attachment #8414934 -
Flags: review?(jorendorff) → review+
Comment 11•7 years ago
|
||
Comment on attachment 8414935 [details] [diff] [review] Try to better honor the concept of [[Origin]] by converting makeObject() to getOrCreateDescriptorObject() Review of attachment 8414935 [details] [diff] [review]: ----------------------------------------------------------------- Is this still a good idea if [[Origin]] gets backed out of ES6? I think this is a behavior change and we should probably skip it.
Attachment #8414935 -
Flags: review?(jorendorff)
Comment 12•7 years ago
|
||
Comment on attachment 8414936 [details] [diff] [review] move PropDesc::wrapInto to JSCompartment::wrap() Review of attachment 8414936 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/PropDesc.h @@ -280,5 @@ > JSStrictPropertyOp setter() const { return desc()->setter(); } > - > - // We choose not to expose the debugger-specific parts of PropDesc, both > - // because they are not really general use, but also because they are a > - // pain to expose. Glad to see this go!
Attachment #8414936 -
Flags: review?(jorendorff) → review+
Comment 13•7 years ago
|
||
Comment on attachment 8414937 [details] [diff] [review] Random Cleanup: Fix DebugObject_defineProperties use of AutoPropDescVector(s) Review of attachment 8414937 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Debugger.cpp @@ +5372,4 @@ > return false; > } > > ErrorCopier ec(ac, dbg->toJSObject()); Side note: ErrorCopier's constructor has a weird signature because when it was written, entering a compartment could fail, and ~ErrorCopier needed an infallible way to switch compartments. ac.destroy() was the solution. Now entering a compartment is infallible and I think ErrorCopier could be revised to be less crazy. @@ +5375,5 @@ > ErrorCopier ec(ac, dbg->toJSObject()); > for (size_t i = 0; i < n; i++) { > bool dummy; > + if (!DefineProperty(cx, obj, ids.handleAt(i), > + descs[i], true, &dummy)) See if it'll fit on one line now.
Attachment #8414937 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 14•7 years ago
|
||
(In reply to Jason Orendorff [:jorendorff] from comment #13) > Comment on attachment 8414937 [details] [diff] [review] > Side note: ErrorCopier's constructor has a weird signature because when it > was written, entering a compartment could fail, and ~ErrorCopier needed an > infallible way to switch compartments. ac.destroy() was the solution. > > Now entering a compartment is infallible and I think ErrorCopier could be > revised to be less crazy. Filed bug 1004088.
Assignee | ||
Comment 15•7 years ago
|
||
Comment on attachment 8414937 [details] [diff] [review] Random Cleanup: Fix DebugObject_defineProperties use of AutoPropDescVector(s) Hey Jim, I keep missing you on IRC, but I just want to confirm that the three rooted arrays were just out of a habit of defensive programming, and not a major issue to unify.
Attachment #8414937 -
Flags: feedback?(jimb)
Comment 16•7 years ago
|
||
Comment on attachment 8414937 [details] [diff] [review] Random Cleanup: Fix DebugObject_defineProperties use of AutoPropDescVector(s) Review of attachment 8414937 [details] [diff] [review]: ----------------------------------------------------------------- As long as unwrapPropDescInto behaves properly when both its arguments point to the same memory, I don't see any problem with this. There shouldn't be any subtle magic going on in that code.
Attachment #8414937 -
Flags: feedback?(jimb) → feedback+
Assignee | ||
Comment 17•7 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b82adb960c54 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c0dd6b9cc07a remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ad09630ae9a3 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/6d4043272a0a remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5afce70dad1f remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/8a63bad8faed remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ec411f0ce167
Comment 18•7 years ago
|
||
Backed out for SM rootanalysis test failures. https://hg.mozilla.org/integration/mozilla-inbound/rev/b922ed24938f https://tbpl.mozilla.org/php/getParsedLog.php?id=40980241&tree=Mozilla-Inbound
Comment 19•7 years ago
|
||
And Windows GC crashes a la: https://tbpl.mozilla.org/php/getParsedLog.php?id=40983675&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=40983326&tree=Mozilla-Inbound
Assignee | ||
Comment 20•7 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/98b5ebd7a3d2 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/00481a443cf0 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d1a3b8ee7c7e remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c14b461b3c24 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a98a39b58a54 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1374cc1b03c0 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4db799c3cee9
Comment 21•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/98b5ebd7a3d2 https://hg.mozilla.org/mozilla-central/rev/00481a443cf0 https://hg.mozilla.org/mozilla-central/rev/d1a3b8ee7c7e https://hg.mozilla.org/mozilla-central/rev/c14b461b3c24 https://hg.mozilla.org/mozilla-central/rev/a98a39b58a54 https://hg.mozilla.org/mozilla-central/rev/1374cc1b03c0 https://hg.mozilla.org/mozilla-central/rev/4db799c3cee9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•