Closed Bug 1002737 Opened 6 years ago Closed 5 years ago

General Cleanups to PropDesc

Categories

(Core :: JavaScript: Standard Library, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: efaust, Assigned: efaust)

References

Details

Attachments

(8 files)

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.
This also adds setters for a few internal fields, since this is no longer a function that can reach private fields.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8414072 - Flags: review?(jimb)
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+
> 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)
When you clear it, clear it. It's much easier to start from scratch every time.
Attachment #8414932 - Flags: review?(jorendorff)
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)
Attachment #8414934 - Flags: review?(jorendorff)
Attachment #8414936 - Flags: review?(jorendorff)
Feel free to pass this to Jimb, if it's more fitting.
Attachment #8414937 - Flags: review?(jorendorff)
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+
Attachment #8414932 - Flags: review?(jorendorff) → review+
Attachment #8414933 - Flags: review?(jorendorff) → review+
Attachment #8414934 - Flags: review?(jorendorff) → review+
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 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 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+
(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.
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 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+
You need to log in before you can comment on or make changes to this bug.