1394835 - Clean up JS_DefineProperty* APIs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jan de Mooij [:jandem]]

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.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Clean up public APIs

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.

Comment 2

[Jan de Mooij [:jandem]]

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 :)

Comment 3

[Boris Zbarsky [: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

Comment 4

[Tom S [:evilpie]]

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.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Split static helpers in data vs accessor functions

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.

Comment 6

[André Bargull [:anba]]

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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21bde39def61
part 1 - Spit JS_DefineProperty* APIs in separate data/accessor overloads. r=evilpie,bz

Comment 8

[Jan de Mooij [:jandem]]

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.

Comment 9

[Jan de Mooij [:jandem]]

(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 10

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Remove unused flags argument, unnecessary root

Comment 11

[André Bargull [:anba]]

Comment on attachment 8902693 [details] [diff] [review]
Part 3 - Remove unused flags argument, unnecessary root

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

Thanks!

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Part 4 - js::DefineProperty/DefineElement

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.

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Part 5 - NativeDefineProperty/NativeDefineElement

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.

Comment 14

[André Bargull [:anba]]

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.

Comment 15

[Tom S [:evilpie]]

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.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/21bde39def61

Comment 17

[Jan de Mooij [:jandem]]

(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.

Comment 18

[Tom S [:evilpie]]

I think splitting them is ok.

Comment 19

[André Bargull [:anba]]

(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.  :-)

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/09f20c2547ed
https://hg.mozilla.org/mozilla-central/rev/3722d60f20ec

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5eb5af7c30a9
part 4 - Split js::DefineProperty/DefineElement in separate accessor vs data functions. r=anba

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/5eb5af7c30a9

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/449873903718
part 5 - Split NativeDefineProperty in separate accessor vs data functions. r=evilpie

Comment 25

[Jan de Mooij [:jandem]]

(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 :)

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/449873903718