Clean up JS_DefineProperty* APIs

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(5 attachments)

(Assignee)

Description

2 years ago
Currently these APIs take both a Value and optional getter/setter. This made some sense before bug 1393790 but it's pretty silly now: when defining a data property, the getter/setter arguments are not relevant, and when defining an accessor property, the Value argument is confusing.

We should have separate overloads that take either a getter + setter or a Value. This is simpler but also the first step to remove slotful getters/setters and JSPROP_SHARED.
(Assignee)

Comment 1

2 years ago
This just makes the signature changes (getter/setter vs HandleValue arguments) without changing the internals yet.

Overall it's pretty simple because most callers don't pass a getter/setter.

There are some XPConnect callers that are a little more complicated.
Attachment #8902295 - Flags: review?(evilpies)
(Assignee)

Comment 2

2 years ago
bz, I feel bad asking this of you since you're just back from PTO, but if you could glance at the non-JS changes here that would be great :)
Flags: needinfo?(bzbarsky)
Comment on attachment 8902295 [details] [diff] [review]
Part 1 - Clean up public APIs

>+++ b/js/xpconnect/wrappers/XrayWrapper.cpp

I think it would make sense to combine the new if/else with the existing if/else above it.

Then you wouldn't need to smuggle the property value and attrs via desc in the value case, at least...  In the accessor case, due to the self-hosted thing it might still be simpler to do the rooting via desc.

Followup is probably ok for this if desired.

r=me on the non-js/src bits
Flags: needinfo?(bzbarsky)
Attachment #8902295 - Flags: review+
Comment on attachment 8902295 [details] [diff] [review]
Part 1 - Clean up public APIs

Review of attachment 8902295 [details] [diff] [review]:
-----------------------------------------------------------------

I am a big fan of everything that helps us remove slotful getters/setters and JSPROP_SHARED.

::: js/xpconnect/src/Sandbox.cpp
@@ -539,2 @@
>      unsigned attrs = pd.attributes() & ~(JSPROP_GETTER | JSPROP_SETTER);
> -    if (!JS_DefinePropertyById(cx, obj, id, v,

I guess the |v| here didn't matter.
Attachment #8902295 - Flags: review?(evilpies) → review+
(Assignee)

Comment 5

2 years ago
We can now easily split the static DefinePropertyById etc helper functions in jsapi.cpp into separate data vs accessor functions. This is pretty nice because we no longer have overhead from AutoRooterGetterSetter etc  when we're just defining a data property.

I also renamed them, for instance DefinePropertyById -> DefineAccessorPropertyById and DefineDataPropertyById.

After this we could do the same thing for js::DefineProperty.
Attachment #8902597 - Flags: review?(andrebargull)
Comment on attachment 8902597 [details] [diff] [review]
Part 2 - Split static helpers in data vs accessor functions

Review of attachment 8902597 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!

::: js/src/jsapi.cpp
@@ +2155,5 @@
>  
>  static bool
> +DefineAccessorPropertyById(JSContext* cx, HandleObject obj, HandleId id,
> +                           const JSNativeWrapper& get, const JSNativeWrapper& set,
> +                           unsigned attrs, unsigned flags)

Will there be a follow-up patch to removed the unused |flags| argument?

@@ +2225,5 @@
> +}
> +
> +static bool
> +DefineDataPropertyById(JSContext* cx, HandleObject obj, HandleId id, HandleValue value,
> +                       unsigned attrs, unsigned flags)

Same with this |flags| parameter.

@@ +2433,5 @@
> +static bool
> +DefineUCDataProperty(JSContext* cx, HandleObject obj, const char16_t* name, size_t namelen,
> +                     const Value& value_, unsigned attrs, unsigned flags)
> +{
> +    RootedValue value(cx, value_);

This is a pre-existing issue, but why do we need to root |value_| again? It looks like all callers already pass a HandleValue to DefineUCDataProperty.
Attachment #8902597 - Flags: review?(andrebargull) → review+

Comment 7

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21bde39def61
part 1 - Spit JS_DefineProperty* APIs in separate data/accessor overloads. r=evilpie,bz
(Assignee)

Comment 8

2 years ago
Ugh, awkward commit message typo.

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #3)
> I think it would make sense to combine the new if/else with the existing
> if/else above it.
> 
> Then you wouldn't need to smuggle the property value and attrs via desc in
> the value case, at least...  In the accessor case, due to the self-hosted
> thing it might still be simpler to do the rooting via desc.

Done.
Keywords: leave-open
(Assignee)

Comment 9

2 years ago
(In reply to André Bargull from comment #6)
> This is a pre-existing issue, but why do we need to root |value_| again? It
> looks like all callers already pass a HandleValue to DefineUCDataProperty.

Good point. I'll post follow-up patches to address these nits.
Comment on attachment 8902693 [details] [diff] [review]
Part 3 - Remove unused flags argument, unnecessary root

Review of attachment 8902693 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8902693 - Flags: review?(andrebargull) → review+
(Assignee)

Comment 12

2 years ago
The same thing but for the internal DefineProperty/DefineElement.

Unfortunately we have a lot of overloads. Not all of them are used but I decided to keep them for now.
Attachment #8902740 - Flags: review?(andrebargull)
(Assignee)

Comment 13

2 years ago
And finally the same thing for NativeDefineProperty and NativeDefineElement.

Here I also think some of the overloads are not used but I decided to keep them for now.
Attachment #8902743 - Flags: review?(evilpies)
Comment on attachment 8902740 [details] [diff] [review]
Part 4 - js::DefineProperty/DefineElement

Review of attachment 8902740 [details] [diff] [review]:
-----------------------------------------------------------------

Good to see all those |nullptr| arguments going away, but I'm wondering why you decided to add the property descriptor type to the DefineProperty function names, instead of simply using overloaded DefineProperty functions?

::: js/src/builtin/Intl.cpp
@@ +825,5 @@
>          a = Atomize(cx, lang.get(), strlen(lang.get()));
>          if (!a)
>              return false;
> +        if (!DefineDataProperty(cx, locales, a->asPropertyName(), TrueHandleValue,
> +                                JSPROP_ENUMERATE))

Nit, pre-existing: JSPROP_ENUMERATE could be removed.
Attachment #8902740 - Flags: review?(andrebargull) → review+
Comment on attachment 8902743 [details] [diff] [review]
Part 5 - NativeDefineProperty/NativeDefineElement

Review of attachment 8902743 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/NativeObject.cpp
@@ +1854,3 @@
>  {
>      Rooted<PropertyDescriptor> desc(cx);
> +    desc.initFields(nullptr, UndefinedHandleValue, attrs, getter, setter);

Not sure what we can guarantee about the attrs here at the moment, maybe just !READONLY?

@@ +1860,5 @@
> +bool
> +js::NativeDefineDataProperty(JSContext* cx, HandleNativeObject obj, HandleId id,
> +                             HandleValue value, unsigned attrs, ObjectOpResult& result)
> +{
> +    Rooted<PropertyDescriptor> desc(cx);

We should assert that none of the GETTER | SETTER flags are set.
Attachment #8902743 - Flags: review?(evilpies) → review+
(Assignee)

Comment 17

2 years ago
(In reply to André Bargull from comment #14)
> Good to see all those |nullptr| arguments going away, but I'm wondering why
> you decided to add the property descriptor type to the DefineProperty
> function names, instead of simply using overloaded DefineProperty functions?

I thought it would be nice to know immediately we're defining a data property when seeing |DefineDataProperty(...)|. If people prefer DefineProperty overloads I can go with that too, I don't care too much myself.
Flags: needinfo?(evilpies)
Flags: needinfo?(andrebargull)
I think splitting them is ok.
Flags: needinfo?(evilpies)
(In reply to Jan de Mooij [:jandem] from comment #17)
> I thought it would be nice to know immediately we're defining a data
> property when seeing |DefineDataProperty(...)|. If people prefer
> DefineProperty overloads I can go with that too, I don't care too much
> myself.

To be honest, I don't care much either. I simply was curious why you decided to use different names and I wanted to make sure that the choice to use different function names is deliberate and documented.  :-)
Flags: needinfo?(andrebargull)

Comment 20

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/09f20c2547ed
part 2 - Split static DefineProperty helpers in jsapi.cpp in data vs accessor functions. r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/3722d60f20ec
part 3 - Remove unused flags argument and an unnecessary root. r=anba

Comment 22

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5eb5af7c30a9
part 4 - Split js::DefineProperty/DefineElement in separate accessor vs data functions. r=anba

Comment 24

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/449873903718
part 5 - Split NativeDefineProperty in separate accessor vs data functions. r=evilpie
(Assignee)

Comment 25

2 years ago
(In reply to Limited Availabilty till Oct | Tom Schuster [:evilpie] from comment #15)
> Not sure what we can guarantee about the attrs here at the moment, maybe
> just !READONLY?

> We should assert that none of the GETTER | SETTER flags are set.

I pushed this patch as is to avoid bustage and bitrot. After 57 I want to spend some time making PropertyDescriptor and these flags sane.

At least the signatures make sense now so let's close this bug :)
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/449873903718
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.