Bug 1142784, part 3 - Change js::DefinePropertyOp and a few property-defining functions to use PropertyDescriptor rather than separate (value, attrs, getter, setter) arguments. r=Waldo.
authorJason Orendorff <jorendorff@mozilla.com>
Sat, 28 Feb 2015 11:23:44 -0600
changeset 266115 c8cf2a03ddcf31ea2d4355e52a89037468cd15f9
parent 266114 7eca624923335b1ef0952ff3979f4a083d73bf78
child 266116 192e8bcb8b803d36a396ad72ae000abc06d5c132
push id830
push userraliiev@mozilla.com
push dateFri, 19 Jun 2015 19:24:37 +0000
treeherdermozilla-release@932614382a68 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersWaldo
bugs1142784
milestone39.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1142784, part 3 - Change js::DefinePropertyOp and a few property-defining functions to use PropertyDescriptor rather than separate (value, attrs, getter, setter) arguments. r=Waldo.
js/public/Class.h
js/src/builtin/TypedObject.cpp
js/src/builtin/TypedObject.h
js/src/jsapi.h
js/src/jsfriendapi.h
js/src/jsobj.cpp
js/src/jsobj.h
js/src/proxy/BaseProxyHandler.cpp
js/src/proxy/Proxy.cpp
js/src/vm/NativeObject.cpp
js/src/vm/NativeObject.h
js/src/vm/ScopeObject.cpp
js/src/vm/UnboxedObject.cpp
js/src/vm/UnboxedObject.h
--- a/js/public/Class.h
+++ b/js/public/Class.h
@@ -328,18 +328,18 @@ typedef void
 /* js::Class operation signatures. */
 
 namespace js {
 
 typedef bool
 (* LookupPropertyOp)(JSContext *cx, JS::HandleObject obj, JS::HandleId id,
                      JS::MutableHandleObject objp, JS::MutableHandle<Shape*> propp);
 typedef bool
-(* DefinePropertyOp)(JSContext *cx, JS::HandleObject obj, JS::HandleId id, JS::HandleValue value,
-                     JSGetterOp getter, JSSetterOp setter, unsigned attrs,
+(* DefinePropertyOp)(JSContext *cx, JS::HandleObject obj, JS::HandleId id,
+                     JS::Handle<JSPropertyDescriptor> desc,
                      JS::ObjectOpResult &result);
 typedef bool
 (* HasPropertyOp)(JSContext *cx, JS::HandleObject obj, JS::HandleId id, bool *foundp);
 typedef bool
 (* GetPropertyOp)(JSContext *cx, JS::HandleObject obj, JS::HandleObject receiver, JS::HandleId id,
                   JS::MutableHandleValue vp);
 typedef bool
 (* SetPropertyOp)(JSContext *cx, JS::HandleObject obj, JS::HandleObject receiver, JS::HandleId id,
--- a/js/src/builtin/TypedObject.cpp
+++ b/js/src/builtin/TypedObject.cpp
@@ -1746,18 +1746,18 @@ ReportPropertyError(JSContext *cx,
     JS_ReportErrorNumber(cx, GetErrorMessage, nullptr,
                          errorNumber, propName);
 
     JS_free(cx, propName);
     return false;
 }
 
 bool
-TypedObject::obj_defineProperty(JSContext *cx, HandleObject obj, HandleId id, HandleValue v,
-                                GetterOp getter, SetterOp setter, unsigned attrs,
+TypedObject::obj_defineProperty(JSContext *cx, HandleObject obj, HandleId id,
+                                Handle<JSPropertyDescriptor> desc,
                                 ObjectOpResult &result)
 {
     Rooted<TypedObject *> typedObj(cx, &obj->as<TypedObject>());
     return ReportTypedObjTypeError(cx, JSMSG_OBJECT_NOT_EXTENSIBLE, typedObj);
 }
 
 bool
 TypedObject::obj_hasProperty(JSContext *cx, HandleObject obj, HandleId id, bool *foundp)
--- a/js/src/builtin/TypedObject.h
+++ b/js/src/builtin/TypedObject.h
@@ -523,18 +523,18 @@ class TypedObject : public JSObject
 
     static bool obj_lookupProperty(JSContext *cx, HandleObject obj,
                                    HandleId id, MutableHandleObject objp,
                                    MutableHandleShape propp);
 
     static bool obj_lookupElement(JSContext *cx, HandleObject obj, uint32_t index,
                                   MutableHandleObject objp, MutableHandleShape propp);
 
-    static bool obj_defineProperty(JSContext *cx, HandleObject obj, HandleId id, HandleValue v,
-                                   GetterOp getter, SetterOp setter, unsigned attrs,
+    static bool obj_defineProperty(JSContext *cx, HandleObject obj, HandleId id,
+                                   Handle<JSPropertyDescriptor> desc,
                                    ObjectOpResult &result);
 
     static bool obj_hasProperty(JSContext *cx, HandleObject obj, HandleId id, bool *foundp);
 
     static bool obj_getProperty(JSContext *cx, HandleObject obj, HandleObject receiver,
                                 HandleId id, MutableHandleValue vp);
 
     static bool obj_getElement(JSContext *cx, HandleObject obj, HandleObject receiver,
--- a/js/src/jsapi.h
+++ b/js/src/jsapi.h
@@ -2592,16 +2592,28 @@ class MutablePropertyDescriptorOperation
     void clear() {
         object().set(nullptr);
         setAttributes(0);
         setGetter(nullptr);
         setSetter(nullptr);
         value().setUndefined();
     }
 
+    void initFields(HandleObject obj, HandleValue v, unsigned attrs,
+                    JSGetterOp getterOp, JSSetterOp setterOp) {
+        MOZ_ASSERT(getterOp != JS_PropertyStub);
+        MOZ_ASSERT(setterOp != JS_StrictPropertyStub);
+
+        object().set(obj);
+        value().set(v);
+        setAttributes(attrs);
+        setGetter(getterOp);
+        setSetter(setterOp);
+    }
+
     void assign(JSPropertyDescriptor &other) {
         object().set(other.obj);
         setAttributes(other.attrs);
         setGetter(other.getter);
         setSetter(other.setter);
         value().set(other.value);
     }
 
--- a/js/src/jsfriendapi.h
+++ b/js/src/jsfriendapi.h
@@ -354,18 +354,18 @@ namespace js {
  *
  * NB: Should not be called directly.
  */
 
 extern JS_FRIEND_API(bool)
 proxy_LookupProperty(JSContext *cx, JS::HandleObject obj, JS::HandleId id, JS::MutableHandleObject objp,
                     JS::MutableHandle<Shape*> propp);
 extern JS_FRIEND_API(bool)
-proxy_DefineProperty(JSContext *cx, JS::HandleObject obj, JS::HandleId id, JS::HandleValue value,
-                     JSGetterOp getter, JSSetterOp setter, unsigned attrs,
+proxy_DefineProperty(JSContext *cx, JS::HandleObject obj, JS::HandleId id,
+                     JS::Handle<JSPropertyDescriptor> desc,
                      JS::ObjectOpResult &result);
 extern JS_FRIEND_API(bool)
 proxy_HasProperty(JSContext *cx, JS::HandleObject obj, JS::HandleId id, bool *foundp);
 extern JS_FRIEND_API(bool)
 proxy_GetProperty(JSContext *cx, JS::HandleObject obj, JS::HandleObject receiver, JS::HandleId id,
                   JS::MutableHandleValue vp);
 extern JS_FRIEND_API(bool)
 proxy_SetProperty(JSContext *cx, JS::HandleObject obj, JS::HandleObject receiver, JS::HandleId id,
--- a/js/src/jsobj.cpp
+++ b/js/src/jsobj.cpp
@@ -3169,32 +3169,39 @@ js::GetOwnPropertyDescriptor(JSContext *
         return false;
 
     desc.value().set(value);
     desc.object().set(obj);
     return true;
 }
 
 bool
+js::DefineProperty(JSContext *cx, HandleObject obj, HandleId id, Handle<PropertyDescriptor> desc,
+                   ObjectOpResult &result)
+{
+    if (DefinePropertyOp op = obj->getOps()->defineProperty)
+        return op(cx, obj, id, desc, result);
+    return NativeDefineProperty(cx, obj.as<NativeObject>(), id, desc, result);
+}
+
+bool
 js::DefineProperty(ExclusiveContext *cx, HandleObject obj, HandleId id, HandleValue value,
                    JSGetterOp getter, JSSetterOp setter, unsigned attrs,
                    ObjectOpResult &result)
 {
-    MOZ_ASSERT(getter != JS_PropertyStub);
-    MOZ_ASSERT(setter != JS_StrictPropertyStub);
     MOZ_ASSERT(!(attrs & JSPROP_PROPOP_ACCESSORS));
 
-    DefinePropertyOp op = obj->getOps()->defineProperty;
-    if (op) {
+    Rooted<PropertyDescriptor> desc(cx);
+    desc.initFields(obj, value, attrs, getter, setter);
+    if (DefinePropertyOp op = obj->getOps()->defineProperty) {
         if (!cx->shouldBeJSContext())
             return false;
-        return op(cx->asJSContext(), obj, id, value, getter, setter, attrs, result);
+        return op(cx->asJSContext(), obj, id, desc, result);
     }
-    return NativeDefineProperty(cx, obj.as<NativeObject>(), id, value, getter, setter, attrs,
-                                result);
+    return NativeDefineProperty(cx, obj.as<NativeObject>(), id, desc, result);
 }
 
 bool
 js::DefineProperty(ExclusiveContext *cx, HandleObject obj, PropertyName *name, HandleValue value,
                    JSGetterOp getter, JSSetterOp setter, unsigned attrs,
                    ObjectOpResult &result)
 {
     RootedId id(cx, NameToId(name));
--- a/js/src/jsobj.h
+++ b/js/src/jsobj.h
@@ -760,16 +760,20 @@ StandardDefineProperty(JSContext *cx, Ha
  * Same as above except without the ObjectOpResult out-parameter. Throws a
  * TypeError on failure.
  */
 extern bool
 StandardDefineProperty(JSContext *cx, HandleObject obj, HandleId id,
                        Handle<PropertyDescriptor> desc);
 
 extern bool
+DefineProperty(JSContext *cx, HandleObject obj, HandleId id,
+               Handle<PropertyDescriptor> desc, ObjectOpResult &result);
+
+extern bool
 DefineProperty(ExclusiveContext *cx, HandleObject obj, HandleId id, HandleValue value,
                JSGetterOp getter, JSSetterOp setter, unsigned attrs, ObjectOpResult &result);
 
 extern bool
 DefineProperty(ExclusiveContext *cx, HandleObject obj, PropertyName *name, HandleValue value,
                JSGetterOp getter, JSSetterOp setter, unsigned attrs, ObjectOpResult &result);
 
 extern bool
--- a/js/src/proxy/BaseProxyHandler.cpp
+++ b/js/src/proxy/BaseProxyHandler.cpp
@@ -104,26 +104,23 @@ js::SetPropertyIgnoringNamedGetter(JSCon
         // The spec calls this variable "parent", but that word has weird
         // connotations in SpiderMonkey, so let's go with "proto".
         RootedObject proto(cx);
         if (!GetPrototype(cx, obj, &proto))
             return false;
         if (proto)
             return SetProperty(cx, proto, receiver, id, vp, result);
 
-        // Change ownDesc to be a complete descriptor for a configurable,
-        // writable, enumerable data property. Then fall through to step 5.
-        ownDesc.clear();
-        ownDesc.setAttributes(JSPROP_ENUMERATE);
+        // Step 4.d.
+        ownDesc.setDataDescriptor(UndefinedHandleValue, JSPROP_ENUMERATE);
     }
 
     // Step 5.
     if (ownDesc.isDataDescriptor()) {
-        // Steps 5.a-b, adapted to our nonstandard implementation of ES6
-        // [[Set]] return values.
+        // Steps 5.a-b.
         if (!ownDesc.writable())
             return result.fail(JSMSG_READ_ONLY);
 
         // Nonstandard SpiderMonkey special case: setter ops.
         SetterOp setter = ownDesc.setter();
         MOZ_ASSERT(setter != JS_StrictPropertyStub);
         if (setter && setter != JS_StrictPropertyStub)
             return CallJSSetterOp(cx, setter, receiver, id, vp, result);
--- a/js/src/proxy/Proxy.cpp
+++ b/js/src/proxy/Proxy.cpp
@@ -554,26 +554,20 @@ js::proxy_LookupProperty(JSContext *cx, 
     } else {
         objp.set(nullptr);
         propp.set(nullptr);
     }
     return true;
 }
 
 bool
-js::proxy_DefineProperty(JSContext *cx, HandleObject obj, HandleId id, HandleValue value,
-                         GetterOp getter, SetterOp setter, unsigned attrs,
+js::proxy_DefineProperty(JSContext *cx, HandleObject obj, HandleId id,
+                         Handle<JSPropertyDescriptor> desc,
                          ObjectOpResult &result)
 {
-    Rooted<PropertyDescriptor> desc(cx);
-    desc.object().set(obj);
-    desc.value().set(value);
-    desc.setAttributes(attrs);
-    desc.setGetter(getter);
-    desc.setSetter(setter);
     return Proxy::defineProperty(cx, obj, id, desc, result);
 }
 
 bool
 js::proxy_HasProperty(JSContext *cx, JS::HandleObject obj, JS::HandleId id, bool *foundp)
 {
     return Proxy::has(cx, obj, id, foundp);
 }
--- a/js/src/vm/NativeObject.cpp
+++ b/js/src/vm/NativeObject.cpp
@@ -1335,36 +1335,39 @@ CheckAccessorRedefinition(ExclusiveConte
 
     if (!cx->isJSContext())
         return false;
 
     return Throw(cx->asJSContext(), id, JSMSG_CANT_REDEFINE_PROP);
 }
 
 bool
-js::NativeDefineProperty(ExclusiveContext *cx, HandleNativeObject obj, HandleId id, HandleValue value,
-                         GetterOp getter, SetterOp setter, unsigned attrs,
+js::NativeDefineProperty(ExclusiveContext *cx, HandleNativeObject obj, HandleId id,
+                         Handle<JSPropertyDescriptor> desc,
                          ObjectOpResult &result)
 {
+    GetterOp getter = desc.getter();
+    SetterOp setter = desc.setter();
+    unsigned attrs = desc.attributes();
     MOZ_ASSERT(getter != JS_PropertyStub);
     MOZ_ASSERT(setter != JS_StrictPropertyStub);
     MOZ_ASSERT(!(attrs & JSPROP_PROPOP_ACCESSORS));
 
     AutoRooterGetterSetter gsRoot(cx, attrs, &getter, &setter);
 
     RootedShape shape(cx);
-    RootedValue updateValue(cx, value);
+    RootedValue updateValue(cx, desc.value());
     bool shouldDefine = true;
 
     /*
      * If defining a getter or setter, we must check for its counterpart and
      * update the attributes and property ops.  A getter or setter is really
      * only half of a property.
      */
-    if (attrs & (JSPROP_GETTER | JSPROP_SETTER)) {
+    if (desc.isAccessorDescriptor()) {
         if (!NativeLookupOwnProperty<CanGC>(cx, obj, id, &shape))
             return false;
         if (shape) {
             /*
              * If we are defining a getter whose setter was already defined, or
              * vice versa, finish the job via obj->changeProperty.
              */
             if (IsImplicitDenseOrTypedArrayElement(shape)) {
@@ -1388,17 +1391,17 @@ js::NativeDefineProperty(ExclusiveContex
                                                      (attrs & JSPROP_SETTER)
                                                      ? setter
                                                      : shape->setter());
                 if (!shape)
                     return false;
                 shouldDefine = false;
             }
         }
-    } else if (!(attrs & JSPROP_IGNORE_VALUE)) {
+    } else if (desc.hasValue()) {
         // If we did a normal lookup here, it would cause resolve hook recursion in
         // the following case. Suppose the first script we run in a lazy global is
         // |parseInt()|.
         //   - js::InitNumberClass is called to resolve parseInt.
         //   - js::InitNumberClass tries to define the Number constructor on the
         //     global.
         //   - We end up here.
         //   - This lookup for 'Number' triggers the global resolve hook.
@@ -1494,28 +1497,38 @@ js::NativeDefineProperty(ExclusiveContex
     JS_ALWAYS_TRUE(UpdateShapeTypeAndValue(cx, obj, shape, updateValue));
 
     if (!CallAddPropertyHook(cx, obj, shape, updateValue))
         return false;
     return result.succeed();
 }
 
 bool
+js::NativeDefineProperty(ExclusiveContext *cx, HandleNativeObject obj, HandleId id,
+                         HandleValue value, GetterOp getter, SetterOp setter, unsigned attrs,
+                         ObjectOpResult &result)
+{
+    Rooted<PropertyDescriptor> desc(cx);
+    desc.initFields(obj, value, attrs, getter, setter);
+    return NativeDefineProperty(cx, obj, id, desc, result);
+}
+
+bool
 js::NativeDefineProperty(ExclusiveContext *cx, HandleNativeObject obj, PropertyName *name,
-                         HandleValue value, GetterOp getter, SetterOp setter,
-                         unsigned attrs, ObjectOpResult &result)
+                         HandleValue value, GetterOp getter, SetterOp setter, unsigned attrs,
+                         ObjectOpResult &result)
 {
     RootedId id(cx, NameToId(name));
     return NativeDefineProperty(cx, obj, id, value, getter, setter, attrs, result);
 }
 
 bool
 js::NativeDefineElement(ExclusiveContext *cx, HandleNativeObject obj, uint32_t index,
-                        HandleValue value, GetterOp getter, SetterOp setter,
-                        unsigned attrs, ObjectOpResult &result)
+                        HandleValue value, GetterOp getter, SetterOp setter, unsigned attrs,
+                        ObjectOpResult &result)
 {
     RootedId id(cx);
     if (index <= JSID_INT_MAX) {
         id = INT_TO_JSID(index);
         return NativeDefineProperty(cx, obj, id, value, getter, setter, attrs, result);
     }
 
     AutoRooterGetterSetter gsRoot(cx, attrs, &getter, &setter);
--- a/js/src/vm/NativeObject.h
+++ b/js/src/vm/NativeObject.h
@@ -1251,16 +1251,21 @@ IsObjectValueInCompartment(Value v, JSCo
  * ("Ordinary Object Internal Methods"). It's an ongoing project.
  *
  * Many native objects are not "ordinary" in ES6, so these functions also have
  * to serve some of the special needs of Functions (9.2, 9.3, 9.4.1), Arrays
  * (9.4.2), Strings (9.4.3), and so on.
  */
 
 extern bool
+NativeDefineProperty(ExclusiveContext *cx, HandleNativeObject obj, HandleId id,
+                     Handle<PropertyDescriptor> desc,
+                     ObjectOpResult &result);
+
+extern bool
 NativeDefineProperty(ExclusiveContext *cx, HandleNativeObject obj, HandleId id, HandleValue value,
                      JSGetterOp getter, JSSetterOp setter, unsigned attrs,
                      ObjectOpResult &result);
 
 extern bool
 NativeDefineProperty(ExclusiveContext *cx, HandleNativeObject obj, PropertyName *name,
                      HandleValue value, GetterOp getter, SetterOp setter,
                      unsigned attrs, ObjectOpResult &result);
--- a/js/src/vm/ScopeObject.cpp
+++ b/js/src/vm/ScopeObject.cpp
@@ -464,22 +464,21 @@ static bool
 with_LookupProperty(JSContext *cx, HandleObject obj, HandleId id,
                     MutableHandleObject objp, MutableHandleShape propp)
 {
     RootedObject actual(cx, &obj->as<DynamicWithObject>().object());
     return LookupProperty(cx, actual, id, objp, propp);
 }
 
 static bool
-with_DefineProperty(JSContext *cx, HandleObject obj, HandleId id, HandleValue value,
-                    JSGetterOp getter, JSSetterOp setter, unsigned attrs,
+with_DefineProperty(JSContext *cx, HandleObject obj, HandleId id, Handle<PropertyDescriptor> desc,
                     ObjectOpResult &result)
 {
     RootedObject actual(cx, &obj->as<DynamicWithObject>().object());
-    return DefineProperty(cx, actual, id, value, getter, setter, attrs, result);
+    return DefineProperty(cx, actual, id, desc, result);
 }
 
 static bool
 with_HasProperty(JSContext *cx, HandleObject obj, HandleId id, bool *foundp)
 {
     RootedObject actual(cx, &obj->as<DynamicWithObject>().object());
     return HasProperty(cx, actual, id, foundp);
 }
--- a/js/src/vm/UnboxedObject.cpp
+++ b/js/src/vm/UnboxedObject.cpp
@@ -675,46 +675,47 @@ UnboxedPlainObject::obj_lookupProperty(J
         propp.set(nullptr);
         return true;
     }
 
     return LookupProperty(cx, proto, id, objp, propp);
 }
 
 /* static */ bool
-UnboxedPlainObject::obj_defineProperty(JSContext *cx, HandleObject obj, HandleId id, HandleValue v,
-                                       GetterOp getter, SetterOp setter, unsigned attrs,
+UnboxedPlainObject::obj_defineProperty(JSContext *cx, HandleObject obj, HandleId id,
+                                       Handle<JSPropertyDescriptor> desc,
                                        ObjectOpResult &result)
 {
     const UnboxedLayout &layout = obj->as<UnboxedPlainObject>().layout();
 
     if (const UnboxedLayout::Property *property = layout.lookup(id)) {
-        if (!getter && !setter && attrs == JSPROP_ENUMERATE) {
+        if (!desc.getter() && !desc.setter() && desc.attributes() == JSPROP_ENUMERATE) {
             // This define is equivalent to setting an existing property.
-            if (obj->as<UnboxedPlainObject>().setValue(cx, *property, v))
+            if (obj->as<UnboxedPlainObject>().setValue(cx, *property, desc.value()))
                 return true;
         }
 
         // Trying to incompatibly redefine an existing property requires the
         // object to be converted to a native object.
         if (!convertToNative(cx, obj))
             return false;
 
-        return DefineProperty(cx, obj, id, v, getter, setter, attrs);
+        return DefineProperty(cx, obj, id, desc, result) &&
+               result.checkStrict(cx, obj, id);
     }
 
     // Define the property on the expando object.
     Rooted<UnboxedExpandoObject *> expando(cx, ensureExpando(cx, obj.as<UnboxedPlainObject>()));
     if (!expando)
         return false;
 
     // Update property types on the unboxed object as well.
-    AddTypePropertyId(cx, obj, id, v);
+    AddTypePropertyId(cx, obj, id, desc.value());
 
-    return DefineProperty(cx, expando, id, v, getter, setter, attrs, result);
+    return DefineProperty(cx, expando, id, desc, result);
 }
 
 /* static */ bool
 UnboxedPlainObject::obj_hasProperty(JSContext *cx, HandleObject obj, HandleId id, bool *foundp)
 {
     if (obj->as<UnboxedPlainObject>().containsUnboxedOrExpandoProperty(cx, id)) {
         *foundp = true;
         return true;
--- a/js/src/vm/UnboxedObject.h
+++ b/js/src/vm/UnboxedObject.h
@@ -187,18 +187,18 @@ class UnboxedPlainObject : public JSObje
 
   public:
     static const Class class_;
 
     static bool obj_lookupProperty(JSContext *cx, HandleObject obj,
                                    HandleId id, MutableHandleObject objp,
                                    MutableHandleShape propp);
 
-    static bool obj_defineProperty(JSContext *cx, HandleObject obj, HandleId id, HandleValue v,
-                                   GetterOp getter, SetterOp setter, unsigned attrs,
+    static bool obj_defineProperty(JSContext *cx, HandleObject obj, HandleId id,
+                                   Handle<JSPropertyDescriptor> desc,
                                    ObjectOpResult &result);
 
     static bool obj_hasProperty(JSContext *cx, HandleObject obj, HandleId id, bool *foundp);
 
     static bool obj_getProperty(JSContext *cx, HandleObject obj, HandleObject receiver,
                                 HandleId id, MutableHandleValue vp);
 
     static bool obj_setProperty(JSContext *cx, HandleObject obj, HandleObject receiver,