489329 - implement new Object constructor methods from ecmascript 5 spec

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, defect)

Description

[rspeyer]

User-Agent:       Opera/9.64 (Macintosh; Intel Mac OS X; U; en) Presto/2.1.1
Build Identifier: Rhino 1.7 release 2 2009 03 22

Object.getPrototypeOf should return the prototype of a given object

Reproducible: Always

Actual Results:  
js> Object.getPrototypeOf
js> Object.getPrototypeOf() === undefined;
js: "<stdin>", line 3: uncaught JavaScript runtime exception: TypeError: Cannot find function getPrototypeOf in object function Object() { [native code for Object.Object, arity=1] }
.
	at <stdin>:3

js> [undefined, null, true, 1, 'hello'].every(function(value) {
  >   try {
  >     Object.getPrototypeOf(value);
  >     return false;
  >   } catch (e if e instanceof TypeError) {
  >     return true;
  >   } catch (e) {
  >     return false;
  >   }
  > });
true
js> [(function(){}), [], {}].every(function(obj) {
  >   return Object.getPrototypeOf(obj) === obj.__proto__;
  > });
js: "<stdin>", line 17: uncaught JavaScript runtime exception: TypeError: Cannot find function getPrototypeOf in object function Object() { [native code for Object.Object, arity=1] }
.
	at <stdin>:17
	at <stdin>:16

js> function UserDefined() {}
js> [Date, UserDefined].every(function(type) {
  >   var instance;
  >   eval('instance = new '+type.name);
  >   return Object.getPrototypeOf(instance) === type.prototype;
  > });
js: "<stdin>", line 25: uncaught JavaScript runtime exception: TypeError: Cannot find function getPrototypeOf in object function Object() { [native code for Object.Object, arity=1] }
.
	at <stdin>:25
	at <stdin>:22

js> 


Expected Results:  
js> Object.getPrototypeOf;
function getPrototypeOf() { [native code for Object.getPrototypeOf, arity=1] }
js> Object.getPrototypeOf() === undefined;
true
js> var nonObjects = [undefined, null, true, 1, 'hello'];
js> nonObjects.every(function(value) {
  >   try {
  >     Object.getPrototypeOf(value);
  >     return false;
  >   } catch (e if e instanceof TypeError) {
  >     return true;
  >   } catch (e) {
  >     return false;
  >   }
  > });
true
js> [(function(){}), [], {}].every(function(obj) {
  >   return Object.getPrototypeOf(obj) === obj.__proto__;
  > });
true
js> function UserDefined() {}
js> [Date, UserDefined].every(function(type) {
  >   var instance;
  >   eval('instance = new '+type.name);
  >   return Object.getPrototypeOf(instance) === type.prototype;
  > });
true

Comment 1

[rspeyer]

Attachment: initial patch for Object.getPrototypeOf functionality

Comment 2

[rspeyer]

Attachment: patch for getPrototypeOf, keys, getPropertyNames and getPropertyDescriptor methods on Object

Note, this attachment depends on the patch submitted for bug 492525

Comment 3

[Norris Boyd]

Comment on attachment 376910 [details] [diff] [review]
patch for getPrototypeOf, keys, getPropertyNames and getPropertyDescriptor methods on Object

>diff --git a/src/org/mozilla/javascript/NativeObject.java b/src/org/mozilla/javascript/NativeObject.java
>index 39f4dd3..49f00e5 100644
>--- a/src/org/mozilla/javascript/NativeObject.java
>+++ b/src/org/mozilla/javascript/NativeObject.java
>@@ -71,6 +71,20 @@ public class NativeObject extends IdScriptableObject
>     }
> 
>     @Override
>+    protected void fillConstructorProperties(IdFunctionObject ctor)
>+    {
>+        addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_getPrototypeOf,
>+                "getPrototypeOf", 1);
>+        addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_keys,
>+                "keys", 1);
>+        addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_getOwnPropertyNames,
>+                "getOwnPropertyNames", 1);
>+        addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_getOwnPropertyDescriptor,
>+                "getOwnPropertyDescriptor", 2);
>+        super.fillConstructorProperties(ctor);
>+    }
>+
>+    @Override
>     protected void initPrototypeId(int id)
>     {
>         String s;
>@@ -256,6 +270,60 @@ public class NativeObject extends IdScriptableObject
>               }
>               return Undefined.instance;
> 
>+          case ConstructorId_getPrototypeOf:
>+              {
>+                if (args.length < 1)
>+                    return Undefined.instance;

I think Object.getPrototypeOf() should throw the same error as Object.getPrototypeOf(undefined). Same for the other functions below.

>+
>+                Object arg = args[0];
>+
>+                if ( !(arg instanceof Scriptable) )
>+                    throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.toString(arg));

See comment below about error message. You can get the type of the argument by calling ScriptRuntime.typeof.

>+
>+                Scriptable obj = (Scriptable) arg;
>+                return obj.getPrototype();
>+              }
>+          case ConstructorId_keys:
>+              {
>+                if (args.length < 1)
>+                    return Undefined.instance;
>+
>+                Object arg = args[0];
>+
>+                if ( !(arg instanceof Scriptable) )
>+                    throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.toString(arg));
>+
>+                Scriptable obj = (Scriptable) arg;
>+                return cx.newArray(scope, obj.getIds());

Unfortunately getIds isn't quite right as far as the type of the elements:

js> o = { a:3, "1":2 }
[object Object]
js> a = Object.keys(o)
a,1
js> typeof a[1]
number

Should be string. Same for getOwnPropertyNames.

>+              }
>+          case ConstructorId_getOwnPropertyNames:
>+              {
>+                if (args.length < 1)
>+                    return Undefined.instance;
>+
>+                Object arg = args[0];
>+
>+                if ( !(arg instanceof ScriptableObject) )
>+                    throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.toString(arg));
>+
>+                ScriptableObject obj = (ScriptableObject) arg;
>+                return cx.newArray(scope, obj.getAllIds());
>+              }
>+          case ConstructorId_getOwnPropertyDescriptor:
>+              {
>+                if (args.length < 2)
>+                    return Undefined.instance;

No, Object.getOwnPropertyDescriptor() is equivalent to Object.getOwnPropertyDescriptor(undefined, undefined)

>+
>+                Object arg = args[0];
>+                if ( !(arg instanceof ScriptableObject) )
>+                    throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.toString(arg));
>+                ScriptableObject obj = (ScriptableObject) arg;
>+                String name = ScriptRuntime.toString(args[1]);
>+
>+                PropertyDescriptor desc = obj.getOwnPropertyDescriptor(name);
>+                return desc == null ? Undefined.instance : desc.fromPropertyDescriptor();
>+              }
>+
>           default:
>             throw new IllegalArgumentException(String.valueOf(id));
>         }
>@@ -303,6 +371,11 @@ public class NativeObject extends IdScriptableObject
>     }
> 
>     private static final int
>+        ConstructorId_getPrototypeOf = -1,
>+        ConstructorId_keys = -2,
>+        ConstructorId_getOwnPropertyNames = -3,
>+        ConstructorId_getOwnPropertyDescriptor = -4,
>+
>         Id_constructor           = 1,
>         Id_toString              = 2,
>         Id_toLocaleString        = 3,
>diff --git a/src/org/mozilla/javascript/PropertyDescriptor.java b/src/org/mozilla/javascript/PropertyDescriptor.java
>new file mode 100644
>index 0000000..3f5aaa9
>--- /dev/null
>+++ b/src/org/mozilla/javascript/PropertyDescriptor.java
>@@ -0,0 +1,106 @@
>+package org.mozilla.javascript;
>+
>+public class PropertyDescriptor implements Cloneable {
>+  protected Boolean enumerable = null;
>+  protected Boolean configurable = null;
>+  private Object value = null;
>+  private Boolean writable = null;
>+  // TODO these two values might be mutable, is it still okay to clone?
>+  private Callable getter = null; 
>+  private Callable setter = null;
>+
>+  public PropertyDescriptor enumerable(Boolean enumerable) {
>+    PropertyDescriptor copy = this.copy();
>+    copy.enumerable = enumerable;
>+    return copy;
>+  }
>+  public PropertyDescriptor configurable(Boolean configurable) {
>+    PropertyDescriptor copy = this.copy();
>+    copy.configurable = configurable;
>+    return copy;
>+  }
>+  public PropertyDescriptor value(Object value) {
>+    if (this.isAccessorDescriptor()) 
>+      throw new UnsupportedOperationException("Cannot add value to an accessor property descriptor");
>+
>+    PropertyDescriptor copy = this.copy();
>+    copy.value = value;
>+    return copy;
>+  }
>+  public PropertyDescriptor writable(Boolean writable) {
>+    if (this.isAccessorDescriptor()) 
>+      throw new UnsupportedOperationException("Cannot add writable to an accessor property descriptor");
>+
>+    PropertyDescriptor copy = this.copy();
>+    copy.writable = writable;
>+    return copy;
>+  }
>+  public PropertyDescriptor getter(Callable getter) {
>+    if (this.isDataDescriptor()) 
>+      throw new UnsupportedOperationException("Cannot add getter to a data property descriptor");
>+
>+    PropertyDescriptor copy = this.copy();
>+    copy.getter = getter;
>+    return copy;
>+  }
>+  public PropertyDescriptor setter(Callable setter) {
>+    if (this.isDataDescriptor()) 
>+      throw new UnsupportedOperationException("Cannot add setter to a data property descriptor");
>+
>+    PropertyDescriptor copy = this.copy();
>+    copy.setter = setter;
>+    return copy;
>+  }
>+
>+  public boolean isEnumerable() {
>+    return enumerable;
>+  }
>+  public boolean isConfigurable() {
>+    return configurable;
>+  }
>+  public Object getValue() {
>+    return this.value;
>+  }
>+  public boolean isWritable() {
>+    return writable;
>+  }
>+  public Callable getGetter() {
>+    return getter;
>+  }
>+  public Callable getSetter() {
>+    return setter;
>+  }
>+
>+  public boolean isDataDescriptor() {
>+    return value != null || writable != null;
>+  }
>+  public boolean isAccessorDescriptor() {
>+    return getter != null || setter != null;
>+  }
>+  public boolean isGenericDescriptor() {
>+    return !isDataDescriptor() && !isAccessorDescriptor();
>+  }
>+
>+  public NativeObject fromPropertyDescriptor() {
>+    NativeObject obj = new NativeObject();
>+    if (isDataDescriptor()) {
>+      if (value != null) obj.defineProperty("value", value, ScriptableObject.EMPTY);
>+      if (writable != null) obj.defineProperty("writable", writable, ScriptableObject.EMPTY);
>+    } else if (isAccessorDescriptor()) {
>+      if (getter != null) obj.defineProperty("get", getter, ScriptableObject.EMPTY);
>+      if (setter != null) obj.defineProperty("set", setter, ScriptableObject.EMPTY);
>+    }
>+    if (enumerable != null) obj.defineProperty("enumerable", enumerable, ScriptableObject.EMPTY);
>+    if (configurable != null) obj.defineProperty("configurable", configurable, ScriptableObject.EMPTY);
>+    return obj;
>+  }
>+
>+  private PropertyDescriptor copy() {
>+    try {
>+      return (PropertyDescriptor) this.clone();
>+    } catch (CloneNotSupportedException ex) {
>+      throw new RuntimeException("PropertyDescriptor does not support cloning", ex);
>+    }
>+  }
>+
>+}
>diff --git a/src/org/mozilla/javascript/ScriptableObject.java b/src/org/mozilla/javascript/ScriptableObject.java
>index 0c96d6c..5065076 100644
>--- a/src/org/mozilla/javascript/ScriptableObject.java
>+++ b/src/org/mozilla/javascript/ScriptableObject.java
>@@ -202,6 +202,14 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>             }
>         }
> 
>+        PropertyDescriptor getPropertyDescriptor() {
>+          return new PropertyDescriptor().
>+            value(value).
>+            writable((attributes & READONLY) == 0).
>+            enumerable((attributes & DONTENUM) == 0).
>+            configurable((attributes & PERMANENT) == 0);

I think this isn't the best approach, because the prototype and parent aren't set, for one:

js> x = Object.getOwnPropertyDescriptor(o, "a")
js: uncaught JavaScript runtime exception: TypeError: Cannot find default value for object.
js> typeof x
object
js> Object.getPrototypeOf(x)
null

I think you should use Context.newObject to create a new empty object and then set the appropriate properties on it for all places where you create a PropertyDescriptor. I don't think you need the PropertyDescriptor class.

>+        }
>+
>     }
> 
>     private static final class GetterSlot extends Slot
>@@ -215,6 +223,14 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>         {
>             super(name, indexOrHash, attributes);
>         }
>+
>+        PropertyDescriptor getPropertyDescriptor() {
>+          return super.getPropertyDescriptor().
>+            value(null).
>+            writable(null).
>+            getter((Callable) getter).
>+            setter((Callable) setter);
>+        }
>     }
> 
>     static void checkValidAttributes(int attributes)
>@@ -2523,6 +2539,11 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>         }
>     }
> 
>+    protected PropertyDescriptor getOwnPropertyDescriptor(String name) {
>+      Slot slot = getSlot(name, 0, SLOT_QUERY);
>+      return (slot == null) ? null : slot.getPropertyDescriptor();
>+    }
>+
>     // Methods and classes to implement java.util.Map interface
> 
>     public int size() {
>diff --git a/src/org/mozilla/javascript/resources/Messages.properties b/src/org/mozilla/javascript/resources/Messages.properties
>index a80e6a5..9f51f2c 100644
>--- a/src/org/mozilla/javascript/resources/Messages.properties
>+++ b/src/org/mozilla/javascript/resources/Messages.properties
>@@ -260,6 +260,9 @@ msg.bad.regexp.compile =\
>     Only one argument may be specified if the first argument to \
>     RegExp.prototype.compile is a RegExp object.
> 
>+msg.arg.not.object =\
>+    Argument is not an object {0}.
>+

I think a better error message would be "Object expected, had instead <type>", and similar for the other new functions you're defining.

> # Parser
> msg.got.syntax.errors = \
>     Compilation produced {0} syntax errors.
>diff --git a/testsrc/build.xml b/testsrc/build.xml
>index b4bde34..c907f00 100644
>--- a/testsrc/build.xml
>+++ b/testsrc/build.xml
>@@ -118,7 +118,7 @@
>         <pathelement location="${test.jstests.jar}"/>
>       </classpath>
>       <batchtest todir="build/test">
>-        <fileset dir="${test.classes}" includes="**/tests/*Test.class"/>
>+        <fileset dir="${test.classes}" includes="**/tests/**/*Test.class"/>
>         <fileset dir="${test.classes}" includes="**/StandardTests.class"/>
>       </batchtest>
>       <formatter type="xml"/>
>diff --git a/testsrc/doctests/480758.doctest b/testsrc/doctests/480758.doctest
>index 675483a..3893400 100755
>diff --git a/testsrc/doctests/object.getownpropertydescriptor.doctest b/testsrc/doctests/object.getownpropertydescriptor.doctest
>new file mode 100644
>index 0000000..8026267
>--- /dev/null
>+++ b/testsrc/doctests/object.getownpropertydescriptor.doctest
>@@ -0,0 +1,41 @@
>+js> Object.getOwnPropertyDescriptor;
>+function getOwnPropertyDescriptor() { [native code for Object.getOwnPropertyDescriptor, arity=2] }
>+js> Object.getOwnPropertyDescriptor() === undefined;
>+true
>+js> [undefined, null, true, 1, "hello"].forEach(function(val) {
>+  >   try {
>+  >     Object.getOwnPropertyDescriptor(val, 'p');
>+  >     print("Object.getOwnPropertyDescriptor("+val+", 'p') should have thrown a TypeError");
>+  >   } catch (e if e instanceof TypeError) {
>+  >     // expected this
>+  >   } catch (e) {
>+  >     print("Object.getOwnPropertyDescriptor("+val+", 'p') should have thrown a TypeError, but instead threw "+e)
>+  >   }
>+  > })
>+
>+js> Object.getOwnPropertyDescriptor({}, 'p') === undefined;
>+true
>+
>+js> var desc = Object.getOwnPropertyDescriptor({p:1}, 'p');
>+js> desc.value
>+1
>+js> desc.writable
>+true
>+js> desc.enumerable
>+true
>+js> desc.configurable
>+true
>+
>+js> var desc = Object.getOwnPropertyDescriptor({ get p() {}, set p() {} }, 'p');
>+js> desc.value === undefined;
>+true
>+js> desc.writable === undefined;
>+true
>+js> desc.get.toSource()
>+(function () {})
>+js> desc.set.toSource()
>+(function () {})
>+js> desc.enumerable
>+true
>+js> desc.configurable
>+true
>diff --git a/testsrc/doctests/object.getownpropertynames.doctest b/testsrc/doctests/object.getownpropertynames.doctest
>new file mode 100644
>index 0000000..4fa5a64
>--- /dev/null
>+++ b/testsrc/doctests/object.getownpropertynames.doctest
>@@ -0,0 +1,56 @@
>+js> Object.getOwnPropertyNames;
>+function getOwnPropertyNames() { [native code for Object.getOwnPropertyNames, arity=1] }
>+js> Object.getOwnPropertyNames() === undefined;
>+true
>+js> [undefined, null, true, 1, "hello"].forEach(function(val) {
>+  >   try {
>+  >     Object.getOwnPropertyNames(val);
>+  >     print("Object.getOwnPropertyNames("+val+") should have thrown a TypeError");
>+  >   } catch (e if e instanceof TypeError) {
>+  >     // this was expected
>+  >   } catch (e) {
>+  >     print("Object.getOwnPropertyNames("+val+") should have thrown a TypeError, but instead threw "+e)
>+  >   }
>+  > })
>+
>+js> Object.getOwnPropertyNames({}).toSource();
>+[]
>+js> Object.getOwnPropertyNames({a:2}).toSource();
>+["a"]
>+js> Object.getOwnPropertyNames({a:1, b:2}).toSource();
>+["a", "b"]
>+js> Object.getOwnPropertyNames({'a.b':1, 'c d':2}).toSource();
>+["a.b", "c d"]
>+
>+js> Object.getOwnPropertyNames([]).toSource();
>+["length"]
>+js> Object.getOwnPropertyNames(['a', 'b', 'c']).toSource();
>+[0, 1, 2, "length"]
>+
>+js> function UserDefined() { this.a = 1; this.b = 2 };
>+js> var obj = new UserDefined()
>+js> Object.getOwnPropertyNames(obj).toSource()
>+["a", "b"]
>+
>+js> UserDefined.prototype.c = 3;
>+3
>+js> Object.getOwnPropertyNames(obj).toSource()
>+["a", "b"]
>+
>+js> // test properties of result are enumerable
>+js> for (var p in Object.getOwnPropertyNames({a:2, b:3})) print(p)
>+0
>+1
>+
>+js> // test that properties of result are writable
>+js> var k = Object.getOwnPropertyNames({a:2, b:3});
>+js> k[1] = 'c'; k.toSource();
>+["a", "c"]
>+
>+js> // test that properties of result are configurable
>+js> var k = Object.getOwnPropertyNames({a:2, b:3})
>+js> delete k[1];
>+true
>+js> k
>+a,
>+js> // TODO test that the attributes of the properties can be changed
>diff --git a/testsrc/doctests/object.getprototypeof.doctest b/testsrc/doctests/object.getprototypeof.doctest
>new file mode 100644
>index 0000000..dcd797a
>--- /dev/null
>+++ b/testsrc/doctests/object.getprototypeof.doctest
>@@ -0,0 +1,33 @@
>+js> Object.getPrototypeOf;
>+function getPrototypeOf() { [native code for Object.getPrototypeOf, arity=1] }
>+
>+js> // FIXME: not sure if this is correct behaviour, should we be expecting a TypeError here instead?
>+js> Object.getPrototypeOf() === undefined;
>+true
>+
>+js> var nonObjects = [undefined, null, true, 1, 'hello'];
>+js> nonObjects.every(function(value) {
>+  >   try {
>+  >     Object.getPrototypeOf(value);
>+  >     return false;
>+  >   } catch (e if e instanceof TypeError) {
>+  >     return true;
>+  >   } catch (e) {
>+  >     return false;
>+  >   }
>+  > });
>+true
>+
>+js> [(function(){}), [], {}].every(function(obj) {
>+  >   return Object.getPrototypeOf(obj) === obj.__proto__;
>+  > });
>+true
>+
>+js> function UserDefined() {}
>+js> [Date, UserDefined].every(function(type) {
>+  >   var instance;
>+  >   eval('instance = new '+type.name);
>+  >   return Object.getPrototypeOf(instance) === type.prototype;
>+  > });
>+true
>+
>diff --git a/testsrc/doctests/object.keys.doctest b/testsrc/doctests/object.keys.doctest
>new file mode 100644
>index 0000000..8c31399
>--- /dev/null
>+++ b/testsrc/doctests/object.keys.doctest
>@@ -0,0 +1,56 @@
>+js> Object.keys;
>+function keys() { [native code for Object.keys, arity=1] }
>+js> Object.keys() === undefined;
>+true
>+js> [undefined, null, true, 1, "hello"].forEach(function(val) {
>+  >   try {
>+  >     Object.keys(val);
>+  >     print("Object.keys("+val+") should have thrown a TypeError");
>+  >   } catch (e if e instanceof TypeError) {
>+  >     // this was expected
>+  >   } catch (e) {
>+  >     print("Object.keys("+val+") should have thrown a TypeError, but instead threw "+e)
>+  >   }
>+  > })
>+
>+js> Object.keys({}).toSource();
>+[]
>+js> Object.keys({a:2}).toSource();
>+["a"]
>+js> Object.keys({a:1, b:2}).toSource();
>+["a", "b"]
>+js> Object.keys({'a.b':1, 'c d':2}).toSource();
>+["a.b", "c d"]
>+
>+js> Object.keys([]).toSource();
>+[]
>+js> Object.keys(['a', 'b', 'c']).toSource();
>+[0, 1, 2]
>+
>+js> function UserDefined() { this.a = 1; this.b = 2 };
>+js> var obj = new UserDefined()
>+js> Object.keys(obj).toSource()
>+["a", "b"]
>+
>+js> UserDefined.prototype.c = 3;
>+3
>+js> Object.keys(obj).toSource()
>+["a", "b"]
>+
>+js> // test properties of result are enumerable
>+js> for (var p in Object.keys({a:2, b:3})) print(p)
>+0
>+1
>+
>+js> // test that properties of result are writable
>+js> var k = Object.keys({a:2, b:3});
>+js> k[1] = 'c'; k.toSource();
>+["a", "c"]
>+
>+js> // test that properties of result are configurable
>+js> var k = Object.keys({a:2, b:3})
>+js> delete k[1];
>+true
>+js> k
>+a,
>+js> // TODO test that the attributes of the properties can be changed
>diff --git a/testsrc/org/mozilla/javascript/tests/Evaluator.java b/testsrc/org/mozilla/javascript/tests/Evaluator.java
>new file mode 100644
>index 0000000..0876c41
>--- /dev/null
>+++ b/testsrc/org/mozilla/javascript/tests/Evaluator.java
>@@ -0,0 +1,28 @@
>+package org.mozilla.javascript.tests;
>+import org.mozilla.javascript.*;
>+import java.util.Collections;
>+import java.util.Map;
>+
>+public class Evaluator {
>+
>+  public static Object eval(String source) {
>+    return eval(source, Collections.EMPTY_MAP);
>+  }
>+
>+  public static Object eval(String source, String id, Scriptable object) {
>+    return eval(source, Collections.singletonMap(id, object));
>+  }
>+
>+  public static Object eval(String source, Map<String, Scriptable> bindings) {
>+    Context cx = ContextFactory.getGlobal().enterContext();
>+    try {
>+      Scriptable scope = cx.initStandardObjects();
>+      for (String id : bindings.keySet()) {
>+        scope.put(id, scope, bindings.get(id));
>+      }
>+      return cx.evaluateString(scope, source, "source", 1, null);
>+    } finally {
>+      Context.exit();
>+    }
>+  }
>+}
>diff --git a/testsrc/org/mozilla/javascript/tests/es5/ObjectGetOwnPropertyDescriptorTest.java b/testsrc/org/mozilla/javascript/tests/es5/ObjectGetOwnPropertyDescriptorTest.java
>new file mode 100644
>index 0000000..9eaf7a9
>--- /dev/null
>+++ b/testsrc/org/mozilla/javascript/tests/es5/ObjectGetOwnPropertyDescriptorTest.java
>@@ -0,0 +1,32 @@
>+/*
>+ * Tests for the Object.getOwnPropertyDescriptor(obj, prop) method
>+ */
>+package org.mozilla.javascript.tests.es5;
>+import org.mozilla.javascript.*;
>+import static org.mozilla.javascript.tests.Evaluator.eval;
>+import static org.junit.Assert.assertEquals;
>+import org.junit.Test;
>+
>+public class ObjectGetOwnPropertyDescriptorTest {
>+
>+  @Test
>+  public void testContentsOfPropertyDescriptorShouldReflectAttributesOfProperty() {
>+    NativeObject descriptor;
>+    NativeObject object = new NativeObject();
>+    object.defineProperty("a", "1", ScriptableObject.EMPTY);
>+    object.defineProperty("b", "2", ScriptableObject.DONTENUM | ScriptableObject.READONLY | ScriptableObject.PERMANENT);
>+
>+    descriptor = (NativeObject) eval("Object.getOwnPropertyDescriptor(obj, 'a')", "obj", object);
>+    assertEquals("1",  descriptor.get("value"));
>+    assertEquals(true, descriptor.get("enumerable"));
>+    assertEquals(true, descriptor.get("writable"));
>+    assertEquals(true, descriptor.get("configurable"));
>+
>+    descriptor = (NativeObject) eval("Object.getOwnPropertyDescriptor(obj, 'b')", "obj", object);
>+    assertEquals("2",  descriptor.get("value"));
>+    assertEquals(false, descriptor.get("enumerable"));
>+    assertEquals(false, descriptor.get("writable"));
>+    assertEquals(false, descriptor.get("configurable"));
>+  }
>+
>+}
>diff --git a/testsrc/org/mozilla/javascript/tests/es5/ObjectGetOwnPropertyNamesTest.java b/testsrc/org/mozilla/javascript/tests/es5/ObjectGetOwnPropertyNamesTest.java
>new file mode 100644
>index 0000000..3ba1f9d
>--- /dev/null
>+++ b/testsrc/org/mozilla/javascript/tests/es5/ObjectGetOwnPropertyNamesTest.java
>@@ -0,0 +1,27 @@
>+/*
>+ * Tests for the Object.getOwnPropertyNames(obj) method
>+ */
>+package org.mozilla.javascript.tests.es5;
>+import org.mozilla.javascript.*;
>+import static org.mozilla.javascript.tests.Evaluator.eval;
>+import static org.junit.Assert.assertEquals;
>+import org.junit.Test;
>+
>+public class ObjectGetOwnPropertyNamesTest {
>+
>+  @Test
>+  public void testShouldReturnAllPropertiesOfArg() {
>+    NativeObject object = new NativeObject();
>+    object.defineProperty("a", "1", ScriptableObject.EMPTY);
>+    object.defineProperty("b", "2", ScriptableObject.DONTENUM);
>+
>+    Object result = eval("Object.getOwnPropertyNames(obj)", "obj", object);
>+
>+    NativeArray names = (NativeArray) result;
>+
>+    assertEquals(2, names.getLength());
>+    assertEquals("a", names.get(0, names));
>+    assertEquals("b", names.get(1, names));
>+  }
>+
>+}
>diff --git a/testsrc/org/mozilla/javascript/tests/es5/ObjectKeysTest.java b/testsrc/org/mozilla/javascript/tests/es5/ObjectKeysTest.java
>new file mode 100644
>index 0000000..9584c9f
>--- /dev/null
>+++ b/testsrc/org/mozilla/javascript/tests/es5/ObjectKeysTest.java
>@@ -0,0 +1,28 @@
>+/*
>+ * Tests for the Object.keys(obj) method
>+ */
>+package org.mozilla.javascript.tests.es5;
>+import org.mozilla.javascript.*;
>+import static org.mozilla.javascript.tests.Evaluator.eval;
>+import static org.junit.Assert.assertEquals;
>+import org.junit.Test;
>+
>+public class ObjectKeysTest {
>+
>+  @Test
>+  public void shouldReturnOnlyEnumerablePropertiesOfArg() {
>+    NativeObject object = new NativeObject();
>+    object.defineProperty("a", "1", ScriptableObject.EMPTY);
>+    object.defineProperty("b", "2", ScriptableObject.EMPTY);
>+    object.defineProperty("c", "3", ScriptableObject.DONTENUM);
>+
>+    Object result = eval("Object.keys(obj)", "obj", object);
>+
>+    NativeArray keys = (NativeArray) result;
>+
>+    assertEquals(2, keys.getLength());
>+    assertEquals("a", keys.get(0, keys));
>+    assertEquals("b", keys.get(1, keys));
>+  }
>+
>+}
>diff --git a/testsrc/org/mozilla/javascript/tests/es5/PropertyDescriptorTest.java b/testsrc/org/mozilla/javascript/tests/es5/PropertyDescriptorTest.java
>new file mode 100644
>index 0000000..15f767a
>--- /dev/null
>+++ b/testsrc/org/mozilla/javascript/tests/es5/PropertyDescriptorTest.java
>@@ -0,0 +1,120 @@
>+package org.mozilla.javascript.tests.es5;
>+
>+import org.mozilla.javascript.*;
>+import java.util.Arrays;
>+import static org.junit.Assert.assertEquals;
>+import static org.junit.Assert.assertTrue;
>+import static org.junit.Assert.assertFalse;
>+import static org.junit.Assert.fail;
>+import org.junit.Test;
>+
>+public class PropertyDescriptorTest {
>+  private final PropertyDescriptor blank = new PropertyDescriptor();
>+  private final Callable getter = new StubCallable();
>+  private final Callable setter = new StubCallable();
>+
>+  @Test
>+  public void shouldInitializeDataDescriptorThroughBuilderMethods() {
>+    PropertyDescriptor desc = blank.value("a").enumerable(true).writable(true).configurable(true);
>+    assertEquals("a", desc.getValue());
>+    assertEquals(true, desc.isEnumerable());
>+    assertEquals(true, desc.isWritable());
>+    assertEquals(true, desc.isConfigurable());
>+  }
>+
>+  @Test
>+  public void shouldInitialiseAccessorDescriptorThroughBuilderMethods() {
>+    PropertyDescriptor desc = blank.getter(getter).setter(setter).enumerable(true).configurable(true);
>+    assertEquals(getter, desc.getGetter());
>+    assertEquals(setter, desc.getSetter());
>+    assertEquals(true, desc.isEnumerable());
>+    assertEquals(true, desc.isConfigurable());
>+  }
>+
>+  @Test(expected = UnsupportedOperationException.class)
>+  public void shouldNotAllowGetterBeSetOnceValueHasBeenSet() {
>+    blank.value("a").getter(getter);
>+  }
>+  @Test(expected = UnsupportedOperationException.class)
>+  public void shouldNotAllowGetterToBeSetOnceWritableHasBeenSet() {
>+    blank.writable(true).getter(getter);
>+  }
>+  @Test(expected = UnsupportedOperationException.class)
>+  public void shouldNotAllowSetterBeSetOnceValueHasBeenSet() {
>+    blank.value("a").setter(setter);
>+  }
>+  @Test(expected = UnsupportedOperationException.class)
>+  public void shouldNotAllowSetterToBeSetOnceWritableHasBeenSet() {
>+    blank.writable(true).setter(setter);
>+  }
>+
>+  @Test(expected = UnsupportedOperationException.class)
>+  public void shouldNotAllowValueToBeSetOnceGetterHasBeenSet() {
>+    blank.getter(getter).value("a");
>+  }
>+  @Test(expected = UnsupportedOperationException.class)
>+  public void shouldNotAllowWritableToBeSetOnceGetterHasBeenSet() {
>+    blank.getter(getter).writable(true);
>+  }
>+  @Test(expected = UnsupportedOperationException.class)
>+  public void shouldNotAllowValueToBeSetOnceSetterHasBeenSet() {
>+    blank.setter(setter).value("a");
>+  }
>+  @Test(expected = UnsupportedOperationException.class)
>+  public void shouldNotAllowWritableToBeSetOnceSetterHasBeenSet() {
>+    blank.setter(setter).writable(true);
>+  }
>+
>+  @Test
>+  public void shouldBeDataDescriptorOnlyWhenValueOrWritableIsSet() {
>+    assertFalse(blank.isDataDescriptor());
>+    assertFalse(blank.enumerable(true).isDataDescriptor());
>+    assertFalse(blank.configurable(true).isDataDescriptor());
>+    assertFalse(blank.getter(getter).isDataDescriptor());
>+    assertFalse(blank.setter(setter).isDataDescriptor());
>+    assertTrue(blank.value("a").isDataDescriptor());
>+    assertTrue(blank.writable(true).isDataDescriptor());
>+  }
>+
>+  @Test
>+  public void shouldBeAccessorDescriptorOnlyWhenGetterOrSetterIsSet() {
>+    assertFalse(blank.isAccessorDescriptor());
>+    assertFalse(blank.enumerable(true).isAccessorDescriptor());
>+    assertFalse(blank.configurable(true).isAccessorDescriptor());
>+    assertFalse(blank.value("a").isAccessorDescriptor());
>+    assertFalse(blank.writable(true).isAccessorDescriptor());
>+    assertTrue(blank.getter(getter).isAccessorDescriptor());
>+    assertTrue(blank.setter(setter).isAccessorDescriptor());
>+  }
>+
>+  @Test
>+  public void shouldBeGenericDescriptorOnlyWhenNotDataOrAccessorDescriptor() {
>+    assertTrue(blank.isGenericDescriptor());
>+    assertTrue(blank.enumerable(true).isGenericDescriptor());
>+    assertTrue(blank.configurable(true).isGenericDescriptor());
>+    assertFalse(blank.value("a").isGenericDescriptor());
>+    assertFalse(blank.writable(true).isGenericDescriptor());
>+    assertFalse(blank.getter(getter).isGenericDescriptor());
>+    assertFalse(blank.setter(setter).isGenericDescriptor());
>+  }
>+
>+  @Test
>+  public void fromPropertyDescriptorShouldCreateValidObjectForDataDescriptor() {
>+    NativeObject expected = new NativeObject();
>+    expected.defineProperty("value", Integer.valueOf(1), ScriptableObject.EMPTY);
>+    expected.defineProperty("writable", Boolean.TRUE, ScriptableObject.EMPTY);
>+    expected.defineProperty("enumerable", Boolean.TRUE, ScriptableObject.EMPTY);
>+    expected.defineProperty("configurable", Boolean.TRUE, ScriptableObject.EMPTY);
>+
>+    PropertyDescriptor desc = new PropertyDescriptor().value(1).writable(true).enumerable(true).configurable(true);
>+    NativeObject actual = desc.fromPropertyDescriptor();
>+
>+    assertEquals(expected.entrySet(), actual.entrySet());
>+  }
>+
>+  private static class StubCallable implements Callable {
>+    public Object call(Context cx, Scriptable scope, Scriptable thisObj, Object[] args) {
>+      return null;
>+    }
>+  }
>+}

Comment 4

[rspeyer]

Attachment: updated patch

- replaced use of PropertyDescriptor with plain ScriptableObject's
- made Object.keys and Object.getOwnPropertyNames return ids of type string, not number
- changed error message for missing or wrong-typed arguments
- changed Object constructor methods to throw type error if they do not receive enough arguments

Comment 5

[Norris Boyd]

Thanks for the update!

I tried applying the patch but ran into conflicts, I think because I have committed changes for one of your bug fixes. Can you sync to CVS head and send me a new patch? If you were at CVS head let me know and I'll investigate further on my end.

Comment 6

[Norris Boyd]

I have it patched successfully now and it passed all regression tests. I'll review the code tomorrow.

Comment 7

[Norris Boyd]

Comment on attachment 379412 [details] [diff] [review]
updated patch

>diff --git a/src/org/mozilla/javascript/NativeObject.java b/src/org/mozilla/javascript/NativeObject.java
>index 39f4dd3..bdc0673 100644
>--- a/src/org/mozilla/javascript/NativeObject.java
>+++ b/src/org/mozilla/javascript/NativeObject.java
>@@ -71,6 +71,20 @@ public class NativeObject extends IdScriptableObject
>     }
> 
>     @Override
>+    protected void fillConstructorProperties(IdFunctionObject ctor)
>+    {
>+        addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_getPrototypeOf,
>+                "getPrototypeOf", 1);
>+        addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_keys,
>+                "keys", 1);
>+        addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_getOwnPropertyNames,
>+                "getOwnPropertyNames", 1);
>+        addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_getOwnPropertyDescriptor,
>+                "getOwnPropertyDescriptor", 2);
>+        super.fillConstructorProperties(ctor);
>+    }
>+
>+    @Override
>     protected void initPrototypeId(int id)
>     {
>         String s;
>@@ -256,6 +270,66 @@ public class NativeObject extends IdScriptableObject
>               }
>               return Undefined.instance;
> 
>+          case ConstructorId_getPrototypeOf:
>+              {
>+                if (args.length < 1)
>+                    throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.typeof(Undefined.instance));
>+
>+                Object arg = args[0];
>+
>+                if ( !(arg instanceof Scriptable) )
>+                    throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.typeof(arg));
>+
>+                Scriptable obj = (Scriptable) arg;
>+                return obj.getPrototype();
>+              }
>+          case ConstructorId_keys:
>+              {

How about 

Object arg = args.length < 1 ? Undefined.instance : args[0];

to replace through the declaration of "Object arg" below?

>+                if (args.length < 1)
>+                    throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.typeof(Undefined.instance));
>+
>+                Object arg = args[0];
>+
>+                if ( !(arg instanceof Scriptable) )
>+                    throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.typeof(arg));
>+
>+                Object[] ids = ((Scriptable) arg).getIds();
>+                for (int i = 0; i < ids.length; i += 1) {

Just use i++

>+                  ids[i] = ScriptRuntime.toString(ids[i]);
>+                }
>+                return cx.newArray(scope, ids);
>+              }
>+          case ConstructorId_getOwnPropertyNames:
>+              {
>+                if (args.length < 1)
>+                    throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.typeof(Undefined.instance));
>+
>+                Object arg = args[0];
>+
>+                if ( !(arg instanceof ScriptableObject) )
>+                    throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.typeof(arg));
>+
>+                Object[] ids = ((ScriptableObject) arg).getAllIds();
>+                for (int i = 0; i < ids.length; i += 1) {
>+                  ids[i] = ScriptRuntime.toString(ids[i]);
>+                }
>+                return cx.newArray(scope, ids);
>+              }
>+          case ConstructorId_getOwnPropertyDescriptor:
>+              {
>+                if (args.length < 2)
>+                    throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.typeof(Undefined.instance));

The second parameter can be any type as long as it can be converted to string, right? In fact, as an edge case, I think you could have a property named "undefined" that you could get the property descriptor for by calling Object.getOwnPropertyDescriptor(obj).

>+
>+                Object arg = args[0];
>+                if ( !(arg instanceof ScriptableObject) )
>+                    throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.typeof(arg));
>+                ScriptableObject obj = (ScriptableObject) arg;
>+                String name = ScriptRuntime.toString(args[1]);
>+
>+                Scriptable desc = obj.getOwnPropertyDescriptor(cx, name);
>+                return desc == null ? Undefined.instance : desc;
>+              }
>+
>           default:
>             throw new IllegalArgumentException(String.valueOf(id));
>         }
>@@ -303,6 +377,11 @@ public class NativeObject extends IdScriptableObject
>     }
> 
>     private static final int
>+        ConstructorId_getPrototypeOf = -1,
>+        ConstructorId_keys = -2,
>+        ConstructorId_getOwnPropertyNames = -3,
>+        ConstructorId_getOwnPropertyDescriptor = -4,
>+
>         Id_constructor           = 1,
>         Id_toString              = 2,
>         Id_toLocaleString        = 3,
>diff --git a/src/org/mozilla/javascript/ScriptableObject.java b/src/org/mozilla/javascript/ScriptableObject.java
>index 38bec1d..68f6e2f 100644
>--- a/src/org/mozilla/javascript/ScriptableObject.java
>+++ b/src/org/mozilla/javascript/ScriptableObject.java
>@@ -202,6 +202,15 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>             }
>         }
> 
>+        ScriptableObject getPropertyDescriptor(Context cx, Scriptable parent) {

s/parent/scope/. This is where the "Object" constructor is looked up.

>+          ScriptableObject desc = (ScriptableObject) cx.newObject(parent);

I steered you wrong before in suggesting you call this. The 8.10.4 #2 says "Let obj be the result of creating a new object as if by the expression new Object() where Object is the standard built-in constructor with that name." This code would allow someone to intercept that call by redefining "Object" in the global scope. Instead you should use

        ScriptableObject desc = new NativeObject();
        ScriptRuntime.setObjectProtoAndParent(desc, scope);

>+          if (value != null) desc.defineProperty("value", value, EMPTY);
>+          desc.defineProperty("writable",     (attributes & READONLY) == 0, EMPTY);
>+          desc.defineProperty("enumerable",   (attributes & DONTENUM) == 0, EMPTY);
>+          desc.defineProperty("configurable", (attributes & PERMANENT) == 0, EMPTY);
>+          return desc;
>+        }
>+
>     }
> 
>     private static final class GetterSlot extends Slot
>@@ -215,6 +224,16 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>         {
>             super(name, indexOrHash, attributes);
>         }
>+
>+        @Override
>+        ScriptableObject getPropertyDescriptor(Context cx, Scriptable parent) {
>+          ScriptableObject desc = super.getPropertyDescriptor(cx, parent);
>+          desc.delete("value");
>+          desc.delete("writable");
>+          if (getter != null) desc.defineProperty("get", getter, EMPTY);
>+          if (setter != null) desc.defineProperty("set", setter, EMPTY);
>+          return desc;
>+        }
>     }
> 
>     static void checkValidAttributes(int attributes)
>@@ -2523,6 +2542,11 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>         }
>     }
> 
>+    protected ScriptableObject getOwnPropertyDescriptor(Context cx, String name) {
>+      Slot slot = getSlot(name, 0, SLOT_QUERY);
>+      return (slot == null) ? null : slot.getPropertyDescriptor(cx, getParentScope());
>+    }
>+
>     // Methods and classes to implement java.util.Map interface
> 
>     public int size() {
>diff --git a/src/org/mozilla/javascript/resources/Messages.properties b/src/org/mozilla/javascript/resources/Messages.properties
>index a80e6a5..545dc87 100644
>--- a/src/org/mozilla/javascript/resources/Messages.properties
>+++ b/src/org/mozilla/javascript/resources/Messages.properties
>@@ -260,6 +260,9 @@ msg.bad.regexp.compile =\
>     Only one argument may be specified if the first argument to \
>     RegExp.prototype.compile is a RegExp object.
> 
>+msg.arg.not.object =\
>+    Expected argument of type object, but instead had type {0}
>+
> # Parser
> msg.got.syntax.errors = \
>     Compilation produced {0} syntax errors.
>diff --git a/testsrc/doctests/object.getownpropertydescriptor.doctest b/testsrc/doctests/object.getownpropertydescriptor.doctest
>new file mode 100644
>index 0000000..8a2cb8a
>--- /dev/null
>+++ b/testsrc/doctests/object.getownpropertydescriptor.doctest
>@@ -0,0 +1,43 @@
>+js> load('testsrc/doctests/util.js');
>+
>+js> Object.getOwnPropertyDescriptor;
>+function getOwnPropertyDescriptor() { [native code for Object.getOwnPropertyDescriptor, arity=2] }
>+
>+js> expectTypeError(function() { Object.getOwnPropertyDescriptor() })
>+js> expectTypeError(function() { Object.getOwnPropertyDescriptor({}) })
>+
>+js> [undefined, null, true, 1, 'hello'].forEach(function(value) { 
>+  >   expectTypeError(function() { Object.getOwnPropertyDescriptor(value, 'p') }) 
>+  > })
>+
>+js> Object.getOwnPropertyDescriptor({}, 'p') === undefined;
>+true
>+
>+js> var desc = Object.getOwnPropertyDescriptor({p:1}, 'p');
>+js> desc.value
>+1
>+js> desc.writable
>+true
>+js> desc.enumerable
>+true
>+js> desc.configurable
>+true
>+
>+js> var desc = Object.getOwnPropertyDescriptor({ get p() {}, set p() {} }, 'p');
>+js> desc.value === undefined;
>+true
>+js> desc.writable === undefined;
>+true
>+js> desc.get.toSource()
>+(function () {})
>+js> desc.set.toSource()
>+(function () {})
>+js> desc.enumerable
>+true
>+js> desc.configurable
>+true
>+
>+js> desc.__proto__ === Object.prototype
>+true
>+js> desc.__parent__;
>+[object global]
>diff --git a/testsrc/doctests/object.getownpropertynames.doctest b/testsrc/doctests/object.getownpropertynames.doctest
>new file mode 100644
>index 0000000..ddf91db
>--- /dev/null
>+++ b/testsrc/doctests/object.getownpropertynames.doctest
>@@ -0,0 +1,55 @@
>+js> load('testsrc/doctests/util.js');
>+
>+js> Object.getOwnPropertyNames;
>+function getOwnPropertyNames() { [native code for Object.getOwnPropertyNames, arity=1] }
>+
>+js> expectTypeError(function() { Object.getOwnPropertyNames() })
>+js> [undefined, null, true, 1, 'hello'].forEach(function(value) { 
>+  >   expectTypeError(function() { Object.getOwnPropertyNames(value) }) 
>+  > })
>+
>+js> Object.getOwnPropertyNames({}).toSource();
>+[]
>+js> Object.getOwnPropertyNames({a:2}).toSource();
>+["a"]
>+js> Object.getOwnPropertyNames({a:1, b:2}).toSource();
>+["a", "b"]
>+js> Object.getOwnPropertyNames({'a.b':1, 'c d':2}).toSource();
>+["a.b", "c d"]
>+
>+js> Object.getOwnPropertyNames([]).toSource();
>+["length"]
>+js> Object.getOwnPropertyNames(['a', 'b', 'c']).toSource();
>+["0", "1", "2", "length"]
>+
>+js> function UserDefined() { this.a = 1; this.b = 2 };
>+js> var obj = new UserDefined()
>+js> Object.getOwnPropertyNames(obj).toSource()
>+["a", "b"]
>+
>+js> UserDefined.prototype.c = 3;
>+3
>+js> Object.getOwnPropertyNames(obj).toSource()
>+["a", "b"]
>+
>+js> // test properties of result are enumerable
>+js> for (var p in Object.getOwnPropertyNames({a:2, b:3})) print(p)
>+0
>+1
>+
>+js> // test that properties of result are writable
>+js> var k = Object.getOwnPropertyNames({a:2, b:3});
>+js> k[1] = 'c'; k.toSource();
>+["a", "c"]
>+
>+js> // test that properties of result are configurable
>+js> var k = Object.getOwnPropertyNames({a:2, b:3})
>+js> delete k[1];
>+true
>+js> k
>+a,
>+js> // TODO test that the attributes of the properties can be changed
>+
>+js> var k = Object.getOwnPropertyNames({a:2, 5:6})
>+js> typeof k[1]
>+string
>diff --git a/testsrc/doctests/object.getprototypeof.doctest b/testsrc/doctests/object.getprototypeof.doctest
>new file mode 100644
>index 0000000..5514e14
>--- /dev/null
>+++ b/testsrc/doctests/object.getprototypeof.doctest
>@@ -0,0 +1,23 @@
>+js> load('testsrc/doctests/util.js');
>+
>+js> Object.getPrototypeOf;
>+function getPrototypeOf() { [native code for Object.getPrototypeOf, arity=1] }
>+
>+js> expectTypeError(function() { Object.getPrototypeOf() })
>+js> [undefined, null, true, 1, 'hello'].forEach(function(value) { 
>+  >   expectTypeError(function() { Object.getPrototypeOf(value) }) 
>+  > })
>+
>+js> [(function(){}), [], {}].every(function(obj) {
>+  >   return Object.getPrototypeOf(obj) === obj.__proto__;
>+  > });
>+true
>+
>+js> function UserDefined() {}
>+js> [Date, UserDefined].every(function(type) {
>+  >   var instance;
>+  >   eval('instance = new '+type.name);
>+  >   return Object.getPrototypeOf(instance) === type.prototype;
>+  > });
>+true
>+
>diff --git a/testsrc/doctests/object.keys.doctest b/testsrc/doctests/object.keys.doctest
>new file mode 100644
>index 0000000..e9aef8e
>--- /dev/null
>+++ b/testsrc/doctests/object.keys.doctest
>@@ -0,0 +1,55 @@
>+js> load('testsrc/doctests/util.js');
>+
>+js> Object.keys;
>+function keys() { [native code for Object.keys, arity=1] }
>+
>+js> expectTypeError(function() { Object.keys() })
>+js> [undefined, null, true, 1, 'hello'].forEach(function(value) { 
>+  >   expectTypeError(function() { Object.keys(value) }) 
>+  > })
>+
>+js> Object.keys({}).toSource();
>+[]
>+js> Object.keys({a:2}).toSource();
>+["a"]
>+js> Object.keys({a:1, b:2}).toSource();
>+["a", "b"]
>+js> Object.keys({'a.b':1, 'c d':2}).toSource();
>+["a.b", "c d"]
>+
>+js> Object.keys([]).toSource();
>+[]
>+js> Object.keys(['a', 'b', 'c']).toSource();
>+["0", "1", "2"]
>+
>+js> function UserDefined() { this.a = 1; this.b = 2 };
>+js> var obj = new UserDefined()
>+js> Object.keys(obj).toSource()
>+["a", "b"]
>+
>+js> UserDefined.prototype.c = 3;
>+3
>+js> Object.keys(obj).toSource()
>+["a", "b"]
>+
>+js> // test properties of result are enumerable
>+js> for (var p in Object.keys({a:2, b:3})) print(p)
>+0
>+1
>+
>+js> // test that properties of result are writable
>+js> var k = Object.keys({a:2, b:3});
>+js> k[1] = 'c'; k.toSource();
>+["a", "c"]
>+
>+js> // test that properties of result are configurable
>+js> var k = Object.keys({a:2, b:3})
>+js> delete k[1];
>+true
>+js> k
>+a,
>+js> // TODO test that the attributes of the properties can be changed
>+
>+js> var k = Object.keys({ a:1, 3:2 });
>+js> typeof k[1];
>+string
>diff --git a/testsrc/doctests/util.js b/testsrc/doctests/util.js
>new file mode 100644
>index 0000000..c17e4da
>--- /dev/null
>+++ b/testsrc/doctests/util.js
>@@ -0,0 +1,8 @@
>+function expectTypeError(code) {
>+  try {
>+    code();
>+    throw (code.toSource() + ' should have thrown a TypeError');
>+  } catch (e if e instanceof TypeError) {
>+    // all good
>+  }
>+}
>diff --git a/testsrc/org/mozilla/javascript/tests/Evaluator.java b/testsrc/org/mozilla/javascript/tests/Evaluator.java
>new file mode 100644
>index 0000000..491c0e7
>--- /dev/null
>+++ b/testsrc/org/mozilla/javascript/tests/Evaluator.java
>@@ -0,0 +1,30 @@
>+package org.mozilla.javascript.tests;
>+import org.mozilla.javascript.*;
>+import java.util.Collections;
>+import java.util.Map;
>+
>+public class Evaluator {
>+
>+  public static Object eval(String source) {
>+    return eval(source, Collections.EMPTY_MAP);
>+  }
>+
>+  public static Object eval(String source, String id, Scriptable object) {
>+    return eval(source, Collections.singletonMap(id, object));
>+  }
>+
>+  public static Object eval(String source, Map<String, Scriptable> bindings) {
>+    Context cx = ContextFactory.getGlobal().enterContext();
>+    try {
>+      Scriptable scope = cx.initStandardObjects();
>+      for (String id : bindings.keySet()) {
>+        Scriptable object = bindings.get(id);
>+        object.setParentScope(scope);
>+        scope.put(id, scope, object);
>+      }
>+      return cx.evaluateString(scope, source, "source", 1, null);
>+    } finally {
>+      Context.exit();
>+    }
>+  }
>+}
>diff --git a/testsrc/org/mozilla/javascript/tests/es5/ObjectGetOwnPropertyDescriptorTest.java b/testsrc/org/mozilla/javascript/tests/es5/ObjectGetOwnPropertyDescriptorTest.java
>new file mode 100644
>index 0000000..9eaf7a9
>--- /dev/null
>+++ b/testsrc/org/mozilla/javascript/tests/es5/ObjectGetOwnPropertyDescriptorTest.java
>@@ -0,0 +1,32 @@
>+/*
>+ * Tests for the Object.getOwnPropertyDescriptor(obj, prop) method
>+ */
>+package org.mozilla.javascript.tests.es5;
>+import org.mozilla.javascript.*;
>+import static org.mozilla.javascript.tests.Evaluator.eval;
>+import static org.junit.Assert.assertEquals;
>+import org.junit.Test;
>+
>+public class ObjectGetOwnPropertyDescriptorTest {
>+
>+  @Test
>+  public void testContentsOfPropertyDescriptorShouldReflectAttributesOfProperty() {
>+    NativeObject descriptor;
>+    NativeObject object = new NativeObject();
>+    object.defineProperty("a", "1", ScriptableObject.EMPTY);
>+    object.defineProperty("b", "2", ScriptableObject.DONTENUM | ScriptableObject.READONLY | ScriptableObject.PERMANENT);
>+
>+    descriptor = (NativeObject) eval("Object.getOwnPropertyDescriptor(obj, 'a')", "obj", object);
>+    assertEquals("1",  descriptor.get("value"));
>+    assertEquals(true, descriptor.get("enumerable"));
>+    assertEquals(true, descriptor.get("writable"));
>+    assertEquals(true, descriptor.get("configurable"));
>+
>+    descriptor = (NativeObject) eval("Object.getOwnPropertyDescriptor(obj, 'b')", "obj", object);
>+    assertEquals("2",  descriptor.get("value"));
>+    assertEquals(false, descriptor.get("enumerable"));
>+    assertEquals(false, descriptor.get("writable"));
>+    assertEquals(false, descriptor.get("configurable"));
>+  }
>+
>+}
>diff --git a/testsrc/org/mozilla/javascript/tests/es5/ObjectGetOwnPropertyNamesTest.java b/testsrc/org/mozilla/javascript/tests/es5/ObjectGetOwnPropertyNamesTest.java
>new file mode 100644
>index 0000000..3ba1f9d
>--- /dev/null
>+++ b/testsrc/org/mozilla/javascript/tests/es5/ObjectGetOwnPropertyNamesTest.java
>@@ -0,0 +1,27 @@
>+/*
>+ * Tests for the Object.getOwnPropertyNames(obj) method
>+ */
>+package org.mozilla.javascript.tests.es5;
>+import org.mozilla.javascript.*;
>+import static org.mozilla.javascript.tests.Evaluator.eval;
>+import static org.junit.Assert.assertEquals;
>+import org.junit.Test;
>+
>+public class ObjectGetOwnPropertyNamesTest {
>+
>+  @Test
>+  public void testShouldReturnAllPropertiesOfArg() {
>+    NativeObject object = new NativeObject();
>+    object.defineProperty("a", "1", ScriptableObject.EMPTY);
>+    object.defineProperty("b", "2", ScriptableObject.DONTENUM);
>+
>+    Object result = eval("Object.getOwnPropertyNames(obj)", "obj", object);
>+
>+    NativeArray names = (NativeArray) result;
>+
>+    assertEquals(2, names.getLength());
>+    assertEquals("a", names.get(0, names));
>+    assertEquals("b", names.get(1, names));
>+  }
>+
>+}
>diff --git a/testsrc/org/mozilla/javascript/tests/es5/ObjectKeysTest.java b/testsrc/org/mozilla/javascript/tests/es5/ObjectKeysTest.java
>new file mode 100644
>index 0000000..9584c9f
>--- /dev/null
>+++ b/testsrc/org/mozilla/javascript/tests/es5/ObjectKeysTest.java
>@@ -0,0 +1,28 @@
>+/*
>+ * Tests for the Object.keys(obj) method
>+ */
>+package org.mozilla.javascript.tests.es5;
>+import org.mozilla.javascript.*;
>+import static org.mozilla.javascript.tests.Evaluator.eval;
>+import static org.junit.Assert.assertEquals;
>+import org.junit.Test;
>+
>+public class ObjectKeysTest {
>+
>+  @Test
>+  public void shouldReturnOnlyEnumerablePropertiesOfArg() {
>+    NativeObject object = new NativeObject();
>+    object.defineProperty("a", "1", ScriptableObject.EMPTY);
>+    object.defineProperty("b", "2", ScriptableObject.EMPTY);
>+    object.defineProperty("c", "3", ScriptableObject.DONTENUM);
>+
>+    Object result = eval("Object.keys(obj)", "obj", object);
>+
>+    NativeArray keys = (NativeArray) result;
>+
>+    assertEquals(2, keys.getLength());
>+    assertEquals("a", keys.get(0, keys));
>+    assertEquals("b", keys.get(1, keys));
>+  }
>+
>+}

Comment 8

[Norris Boyd]

Comment on attachment 379412 [details] [diff] [review]
updated patch

>diff --git a/src/org/mozilla/javascript/NativeObject.java b/src/org/mozilla/javascript/NativeObject.java
>index 39f4dd3..bdc0673 100644
>--- a/src/org/mozilla/javascript/NativeObject.java
>+++ b/src/org/mozilla/javascript/NativeObject.java
>@@ -71,6 +71,20 @@ public class NativeObject extends IdScriptableObject
>     }
> 
>     @Override
>+    protected void fillConstructorProperties(IdFunctionObject ctor)
>+    {
>+        addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_getPrototypeOf,
>+                "getPrototypeOf", 1);
>+        addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_keys,
>+                "keys", 1);
>+        addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_getOwnPropertyNames,
>+                "getOwnPropertyNames", 1);
>+        addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_getOwnPropertyDescriptor,
>+                "getOwnPropertyDescriptor", 2);
>+        super.fillConstructorProperties(ctor);
>+    }
>+
>+    @Override
>     protected void initPrototypeId(int id)
>     {
>         String s;
>@@ -256,6 +270,66 @@ public class NativeObject extends IdScriptableObject
>               }
>               return Undefined.instance;
> 
>+          case ConstructorId_getPrototypeOf:
>+              {
>+                if (args.length < 1)
>+                    throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.typeof(Undefined.instance));
>+
>+                Object arg = args[0];
>+
>+                if ( !(arg instanceof Scriptable) )
>+                    throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.typeof(arg));
>+
>+                Scriptable obj = (Scriptable) arg;
>+                return obj.getPrototype();
>+              }
>+          case ConstructorId_keys:
>+              {

How about 

Object arg = args.length < 1 ? Undefined.instance : args[0];

to replace through the declaration of "Object arg" below?

>+                if (args.length < 1)
>+                    throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.typeof(Undefined.instance));
>+
>+                Object arg = args[0];
>+
>+                if ( !(arg instanceof Scriptable) )
>+                    throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.typeof(arg));
>+
>+                Object[] ids = ((Scriptable) arg).getIds();
>+                for (int i = 0; i < ids.length; i += 1) {

Just use i++

>+                  ids[i] = ScriptRuntime.toString(ids[i]);
>+                }
>+                return cx.newArray(scope, ids);
>+              }
>+          case ConstructorId_getOwnPropertyNames:
>+              {
>+                if (args.length < 1)
>+                    throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.typeof(Undefined.instance));
>+
>+                Object arg = args[0];
>+
>+                if ( !(arg instanceof ScriptableObject) )
>+                    throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.typeof(arg));
>+
>+                Object[] ids = ((ScriptableObject) arg).getAllIds();
>+                for (int i = 0; i < ids.length; i += 1) {
>+                  ids[i] = ScriptRuntime.toString(ids[i]);
>+                }
>+                return cx.newArray(scope, ids);
>+              }
>+          case ConstructorId_getOwnPropertyDescriptor:
>+              {
>+                if (args.length < 2)
>+                    throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.typeof(Undefined.instance));

The second parameter can be any type as long as it can be converted to string, right? In fact, as an edge case, I think you could have a property named "undefined" that you could get the property descriptor for by calling Object.getOwnPropertyDescriptor(obj).

>+
>+                Object arg = args[0];
>+                if ( !(arg instanceof ScriptableObject) )
>+                    throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.typeof(arg));
>+                ScriptableObject obj = (ScriptableObject) arg;
>+                String name = ScriptRuntime.toString(args[1]);
>+
>+                Scriptable desc = obj.getOwnPropertyDescriptor(cx, name);
>+                return desc == null ? Undefined.instance : desc;
>+              }
>+
>           default:
>             throw new IllegalArgumentException(String.valueOf(id));
>         }
>@@ -303,6 +377,11 @@ public class NativeObject extends IdScriptableObject
>     }
> 
>     private static final int
>+        ConstructorId_getPrototypeOf = -1,
>+        ConstructorId_keys = -2,
>+        ConstructorId_getOwnPropertyNames = -3,
>+        ConstructorId_getOwnPropertyDescriptor = -4,
>+
>         Id_constructor           = 1,
>         Id_toString              = 2,
>         Id_toLocaleString        = 3,
>diff --git a/src/org/mozilla/javascript/ScriptableObject.java b/src/org/mozilla/javascript/ScriptableObject.java
>index 38bec1d..68f6e2f 100644
>--- a/src/org/mozilla/javascript/ScriptableObject.java
>+++ b/src/org/mozilla/javascript/ScriptableObject.java
>@@ -202,6 +202,15 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>             }
>         }
> 
>+        ScriptableObject getPropertyDescriptor(Context cx, Scriptable parent) {

s/parent/scope/. This is where the "Object" constructor is looked up.

>+          ScriptableObject desc = (ScriptableObject) cx.newObject(parent);

I steered you wrong before in suggesting you call this. The 8.10.4 #2 says "Let obj be the result of creating a new object as if by the expression new Object() where Object is the standard built-in constructor with that name." This code would allow someone to intercept that call by redefining "Object" in the global scope. Instead you should use

        ScriptableObject desc = new NativeObject();
        ScriptRuntime.setObjectProtoAndParent(desc, scope);

>+          if (value != null) desc.defineProperty("value", value, EMPTY);
>+          desc.defineProperty("writable",     (attributes & READONLY) == 0, EMPTY);
>+          desc.defineProperty("enumerable",   (attributes & DONTENUM) == 0, EMPTY);
>+          desc.defineProperty("configurable", (attributes & PERMANENT) == 0, EMPTY);
>+          return desc;
>+        }
>+
>     }
> 
>     private static final class GetterSlot extends Slot
>@@ -215,6 +224,16 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>         {
>             super(name, indexOrHash, attributes);
>         }
>+
>+        @Override
>+        ScriptableObject getPropertyDescriptor(Context cx, Scriptable parent) {
>+          ScriptableObject desc = super.getPropertyDescriptor(cx, parent);
>+          desc.delete("value");
>+          desc.delete("writable");
>+          if (getter != null) desc.defineProperty("get", getter, EMPTY);
>+          if (setter != null) desc.defineProperty("set", setter, EMPTY);
>+          return desc;
>+        }
>     }
> 
>     static void checkValidAttributes(int attributes)
>@@ -2523,6 +2542,11 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>         }
>     }
> 
>+    protected ScriptableObject getOwnPropertyDescriptor(Context cx, String name) {
>+      Slot slot = getSlot(name, 0, SLOT_QUERY);
>+      return (slot == null) ? null : slot.getPropertyDescriptor(cx, getParentScope());
>+    }
>+
>     // Methods and classes to implement java.util.Map interface
> 
>     public int size() {
>diff --git a/src/org/mozilla/javascript/resources/Messages.properties b/src/org/mozilla/javascript/resources/Messages.properties
>index a80e6a5..545dc87 100644
>--- a/src/org/mozilla/javascript/resources/Messages.properties
>+++ b/src/org/mozilla/javascript/resources/Messages.properties
>@@ -260,6 +260,9 @@ msg.bad.regexp.compile =\
>     Only one argument may be specified if the first argument to \
>     RegExp.prototype.compile is a RegExp object.
> 
>+msg.arg.not.object =\
>+    Expected argument of type object, but instead had type {0}
>+
> # Parser
> msg.got.syntax.errors = \
>     Compilation produced {0} syntax errors.
>diff --git a/testsrc/doctests/object.getownpropertydescriptor.doctest b/testsrc/doctests/object.getownpropertydescriptor.doctest
>new file mode 100644
>index 0000000..8a2cb8a
>--- /dev/null
>+++ b/testsrc/doctests/object.getownpropertydescriptor.doctest
>@@ -0,0 +1,43 @@
>+js> load('testsrc/doctests/util.js');
>+
>+js> Object.getOwnPropertyDescriptor;
>+function getOwnPropertyDescriptor() { [native code for Object.getOwnPropertyDescriptor, arity=2] }
>+
>+js> expectTypeError(function() { Object.getOwnPropertyDescriptor() })
>+js> expectTypeError(function() { Object.getOwnPropertyDescriptor({}) })
>+
>+js> [undefined, null, true, 1, 'hello'].forEach(function(value) { 
>+  >   expectTypeError(function() { Object.getOwnPropertyDescriptor(value, 'p') }) 
>+  > })
>+
>+js> Object.getOwnPropertyDescriptor({}, 'p') === undefined;
>+true
>+
>+js> var desc = Object.getOwnPropertyDescriptor({p:1}, 'p');
>+js> desc.value
>+1
>+js> desc.writable
>+true
>+js> desc.enumerable
>+true
>+js> desc.configurable
>+true
>+
>+js> var desc = Object.getOwnPropertyDescriptor({ get p() {}, set p() {} }, 'p');
>+js> desc.value === undefined;
>+true
>+js> desc.writable === undefined;
>+true
>+js> desc.get.toSource()
>+(function () {})
>+js> desc.set.toSource()
>+(function () {})
>+js> desc.enumerable
>+true
>+js> desc.configurable
>+true
>+
>+js> desc.__proto__ === Object.prototype
>+true
>+js> desc.__parent__;
>+[object global]
>diff --git a/testsrc/doctests/object.getownpropertynames.doctest b/testsrc/doctests/object.getownpropertynames.doctest
>new file mode 100644
>index 0000000..ddf91db
>--- /dev/null
>+++ b/testsrc/doctests/object.getownpropertynames.doctest
>@@ -0,0 +1,55 @@
>+js> load('testsrc/doctests/util.js');
>+
>+js> Object.getOwnPropertyNames;
>+function getOwnPropertyNames() { [native code for Object.getOwnPropertyNames, arity=1] }
>+
>+js> expectTypeError(function() { Object.getOwnPropertyNames() })
>+js> [undefined, null, true, 1, 'hello'].forEach(function(value) { 
>+  >   expectTypeError(function() { Object.getOwnPropertyNames(value) }) 
>+  > })
>+
>+js> Object.getOwnPropertyNames({}).toSource();
>+[]
>+js> Object.getOwnPropertyNames({a:2}).toSource();
>+["a"]
>+js> Object.getOwnPropertyNames({a:1, b:2}).toSource();
>+["a", "b"]
>+js> Object.getOwnPropertyNames({'a.b':1, 'c d':2}).toSource();
>+["a.b", "c d"]
>+
>+js> Object.getOwnPropertyNames([]).toSource();
>+["length"]
>+js> Object.getOwnPropertyNames(['a', 'b', 'c']).toSource();
>+["0", "1", "2", "length"]
>+
>+js> function UserDefined() { this.a = 1; this.b = 2 };
>+js> var obj = new UserDefined()
>+js> Object.getOwnPropertyNames(obj).toSource()
>+["a", "b"]
>+
>+js> UserDefined.prototype.c = 3;
>+3
>+js> Object.getOwnPropertyNames(obj).toSource()
>+["a", "b"]
>+
>+js> // test properties of result are enumerable
>+js> for (var p in Object.getOwnPropertyNames({a:2, b:3})) print(p)
>+0
>+1
>+
>+js> // test that properties of result are writable
>+js> var k = Object.getOwnPropertyNames({a:2, b:3});
>+js> k[1] = 'c'; k.toSource();
>+["a", "c"]
>+
>+js> // test that properties of result are configurable
>+js> var k = Object.getOwnPropertyNames({a:2, b:3})
>+js> delete k[1];
>+true
>+js> k
>+a,
>+js> // TODO test that the attributes of the properties can be changed
>+
>+js> var k = Object.getOwnPropertyNames({a:2, 5:6})
>+js> typeof k[1]
>+string
>diff --git a/testsrc/doctests/object.getprototypeof.doctest b/testsrc/doctests/object.getprototypeof.doctest
>new file mode 100644
>index 0000000..5514e14
>--- /dev/null
>+++ b/testsrc/doctests/object.getprototypeof.doctest
>@@ -0,0 +1,23 @@
>+js> load('testsrc/doctests/util.js');
>+
>+js> Object.getPrototypeOf;
>+function getPrototypeOf() { [native code for Object.getPrototypeOf, arity=1] }
>+
>+js> expectTypeError(function() { Object.getPrototypeOf() })
>+js> [undefined, null, true, 1, 'hello'].forEach(function(value) { 
>+  >   expectTypeError(function() { Object.getPrototypeOf(value) }) 
>+  > })
>+
>+js> [(function(){}), [], {}].every(function(obj) {
>+  >   return Object.getPrototypeOf(obj) === obj.__proto__;
>+  > });
>+true
>+
>+js> function UserDefined() {}
>+js> [Date, UserDefined].every(function(type) {
>+  >   var instance;
>+  >   eval('instance = new '+type.name);
>+  >   return Object.getPrototypeOf(instance) === type.prototype;
>+  > });
>+true
>+
>diff --git a/testsrc/doctests/object.keys.doctest b/testsrc/doctests/object.keys.doctest
>new file mode 100644
>index 0000000..e9aef8e
>--- /dev/null
>+++ b/testsrc/doctests/object.keys.doctest
>@@ -0,0 +1,55 @@
>+js> load('testsrc/doctests/util.js');
>+
>+js> Object.keys;
>+function keys() { [native code for Object.keys, arity=1] }
>+
>+js> expectTypeError(function() { Object.keys() })
>+js> [undefined, null, true, 1, 'hello'].forEach(function(value) { 
>+  >   expectTypeError(function() { Object.keys(value) }) 
>+  > })
>+
>+js> Object.keys({}).toSource();
>+[]
>+js> Object.keys({a:2}).toSource();
>+["a"]
>+js> Object.keys({a:1, b:2}).toSource();
>+["a", "b"]
>+js> Object.keys({'a.b':1, 'c d':2}).toSource();
>+["a.b", "c d"]
>+
>+js> Object.keys([]).toSource();
>+[]
>+js> Object.keys(['a', 'b', 'c']).toSource();
>+["0", "1", "2"]
>+
>+js> function UserDefined() { this.a = 1; this.b = 2 };
>+js> var obj = new UserDefined()
>+js> Object.keys(obj).toSource()
>+["a", "b"]
>+
>+js> UserDefined.prototype.c = 3;
>+3
>+js> Object.keys(obj).toSource()
>+["a", "b"]
>+
>+js> // test properties of result are enumerable
>+js> for (var p in Object.keys({a:2, b:3})) print(p)
>+0
>+1
>+
>+js> // test that properties of result are writable
>+js> var k = Object.keys({a:2, b:3});
>+js> k[1] = 'c'; k.toSource();
>+["a", "c"]
>+
>+js> // test that properties of result are configurable
>+js> var k = Object.keys({a:2, b:3})
>+js> delete k[1];
>+true
>+js> k
>+a,
>+js> // TODO test that the attributes of the properties can be changed
>+
>+js> var k = Object.keys({ a:1, 3:2 });
>+js> typeof k[1];
>+string
>diff --git a/testsrc/doctests/util.js b/testsrc/doctests/util.js
>new file mode 100644
>index 0000000..c17e4da
>--- /dev/null
>+++ b/testsrc/doctests/util.js
>@@ -0,0 +1,8 @@
>+function expectTypeError(code) {
>+  try {
>+    code();
>+    throw (code.toSource() + ' should have thrown a TypeError');
>+  } catch (e if e instanceof TypeError) {
>+    // all good
>+  }
>+}
>diff --git a/testsrc/org/mozilla/javascript/tests/Evaluator.java b/testsrc/org/mozilla/javascript/tests/Evaluator.java
>new file mode 100644
>index 0000000..491c0e7
>--- /dev/null
>+++ b/testsrc/org/mozilla/javascript/tests/Evaluator.java
>@@ -0,0 +1,30 @@
>+package org.mozilla.javascript.tests;
>+import org.mozilla.javascript.*;
>+import java.util.Collections;
>+import java.util.Map;
>+
>+public class Evaluator {
>+
>+  public static Object eval(String source) {
>+    return eval(source, Collections.EMPTY_MAP);
>+  }
>+
>+  public static Object eval(String source, String id, Scriptable object) {
>+    return eval(source, Collections.singletonMap(id, object));
>+  }
>+
>+  public static Object eval(String source, Map<String, Scriptable> bindings) {
>+    Context cx = ContextFactory.getGlobal().enterContext();
>+    try {
>+      Scriptable scope = cx.initStandardObjects();
>+      for (String id : bindings.keySet()) {
>+        Scriptable object = bindings.get(id);
>+        object.setParentScope(scope);
>+        scope.put(id, scope, object);
>+      }
>+      return cx.evaluateString(scope, source, "source", 1, null);
>+    } finally {
>+      Context.exit();
>+    }
>+  }
>+}
>diff --git a/testsrc/org/mozilla/javascript/tests/es5/ObjectGetOwnPropertyDescriptorTest.java b/testsrc/org/mozilla/javascript/tests/es5/ObjectGetOwnPropertyDescriptorTest.java
>new file mode 100644
>index 0000000..9eaf7a9
>--- /dev/null
>+++ b/testsrc/org/mozilla/javascript/tests/es5/ObjectGetOwnPropertyDescriptorTest.java
>@@ -0,0 +1,32 @@
>+/*
>+ * Tests for the Object.getOwnPropertyDescriptor(obj, prop) method
>+ */
>+package org.mozilla.javascript.tests.es5;
>+import org.mozilla.javascript.*;
>+import static org.mozilla.javascript.tests.Evaluator.eval;
>+import static org.junit.Assert.assertEquals;
>+import org.junit.Test;
>+
>+public class ObjectGetOwnPropertyDescriptorTest {
>+
>+  @Test
>+  public void testContentsOfPropertyDescriptorShouldReflectAttributesOfProperty() {
>+    NativeObject descriptor;
>+    NativeObject object = new NativeObject();
>+    object.defineProperty("a", "1", ScriptableObject.EMPTY);
>+    object.defineProperty("b", "2", ScriptableObject.DONTENUM | ScriptableObject.READONLY | ScriptableObject.PERMANENT);
>+
>+    descriptor = (NativeObject) eval("Object.getOwnPropertyDescriptor(obj, 'a')", "obj", object);
>+    assertEquals("1",  descriptor.get("value"));
>+    assertEquals(true, descriptor.get("enumerable"));
>+    assertEquals(true, descriptor.get("writable"));
>+    assertEquals(true, descriptor.get("configurable"));
>+
>+    descriptor = (NativeObject) eval("Object.getOwnPropertyDescriptor(obj, 'b')", "obj", object);
>+    assertEquals("2",  descriptor.get("value"));
>+    assertEquals(false, descriptor.get("enumerable"));
>+    assertEquals(false, descriptor.get("writable"));
>+    assertEquals(false, descriptor.get("configurable"));
>+  }
>+
>+}
>diff --git a/testsrc/org/mozilla/javascript/tests/es5/ObjectGetOwnPropertyNamesTest.java b/testsrc/org/mozilla/javascript/tests/es5/ObjectGetOwnPropertyNamesTest.java
>new file mode 100644
>index 0000000..3ba1f9d
>--- /dev/null
>+++ b/testsrc/org/mozilla/javascript/tests/es5/ObjectGetOwnPropertyNamesTest.java
>@@ -0,0 +1,27 @@
>+/*
>+ * Tests for the Object.getOwnPropertyNames(obj) method
>+ */
>+package org.mozilla.javascript.tests.es5;
>+import org.mozilla.javascript.*;
>+import static org.mozilla.javascript.tests.Evaluator.eval;
>+import static org.junit.Assert.assertEquals;
>+import org.junit.Test;
>+
>+public class ObjectGetOwnPropertyNamesTest {
>+
>+  @Test
>+  public void testShouldReturnAllPropertiesOfArg() {
>+    NativeObject object = new NativeObject();
>+    object.defineProperty("a", "1", ScriptableObject.EMPTY);
>+    object.defineProperty("b", "2", ScriptableObject.DONTENUM);
>+
>+    Object result = eval("Object.getOwnPropertyNames(obj)", "obj", object);
>+
>+    NativeArray names = (NativeArray) result;
>+
>+    assertEquals(2, names.getLength());
>+    assertEquals("a", names.get(0, names));
>+    assertEquals("b", names.get(1, names));
>+  }
>+
>+}
>diff --git a/testsrc/org/mozilla/javascript/tests/es5/ObjectKeysTest.java b/testsrc/org/mozilla/javascript/tests/es5/ObjectKeysTest.java
>new file mode 100644
>index 0000000..9584c9f
>--- /dev/null
>+++ b/testsrc/org/mozilla/javascript/tests/es5/ObjectKeysTest.java
>@@ -0,0 +1,28 @@
>+/*
>+ * Tests for the Object.keys(obj) method
>+ */
>+package org.mozilla.javascript.tests.es5;
>+import org.mozilla.javascript.*;
>+import static org.mozilla.javascript.tests.Evaluator.eval;
>+import static org.junit.Assert.assertEquals;
>+import org.junit.Test;
>+
>+public class ObjectKeysTest {
>+
>+  @Test
>+  public void shouldReturnOnlyEnumerablePropertiesOfArg() {
>+    NativeObject object = new NativeObject();
>+    object.defineProperty("a", "1", ScriptableObject.EMPTY);
>+    object.defineProperty("b", "2", ScriptableObject.EMPTY);
>+    object.defineProperty("c", "3", ScriptableObject.DONTENUM);
>+
>+    Object result = eval("Object.keys(obj)", "obj", object);
>+
>+    NativeArray keys = (NativeArray) result;
>+
>+    assertEquals(2, keys.getLength());
>+    assertEquals("a", keys.get(0, keys));
>+    assertEquals("b", keys.get(1, keys));
>+  }
>+
>+}

Comment 9

[Norris Boyd]

Since the remaining edits were small, I made them myself and submitted:

Checking in src/org/mozilla/javascript/NativeObject.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/NativeObject.java,v  <--  NativeObject.java
new revision: 1.49; previous revision: 1.48
done
Checking in src/org/mozilla/javascript/ScriptableObject.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/ScriptableObject.java,v  <--  ScriptableObject.java
new revision: 1.148; previous revision: 1.147
done
Checking in src/org/mozilla/javascript/resources/Messages.properties;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/resources/Messages.properties,v  <--  Messages.properties
new revision: 1.93; previous revision: 1.92
done
RCS file: /cvsroot/mozilla/js/rhino/testsrc/doctests/object.getownpropertydescriptor.doctest,v
done
Checking in testsrc/doctests/object.getownpropertydescriptor.doctest;
/cvsroot/mozilla/js/rhino/testsrc/doctests/object.getownpropertydescriptor.doctest,v  <--  object.getownpropertydescriptor.doctest
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/rhino/testsrc/doctests/object.getownpropertynames.doctest,v
done
Checking in testsrc/doctests/object.getownpropertynames.doctest;
/cvsroot/mozilla/js/rhino/testsrc/doctests/object.getownpropertynames.doctest,v  <--  object.getownpropertynames.doctest
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/rhino/testsrc/doctests/object.getprototypeof.doctest,v
done
Checking in testsrc/doctests/object.getprototypeof.doctest;
/cvsroot/mozilla/js/rhino/testsrc/doctests/object.getprototypeof.doctest,v  <--  object.getprototypeof.doctest
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/rhino/testsrc/doctests/object.keys.doctest,v
done
Checking in testsrc/doctests/object.keys.doctest;
/cvsroot/mozilla/js/rhino/testsrc/doctests/object.keys.doctest,v  <--  object.keys.doctest
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/rhino/testsrc/doctests/util.js,v
done
Checking in testsrc/doctests/util.js;
/cvsroot/mozilla/js/rhino/testsrc/doctests/util.js,v  <--  util.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/rhino/testsrc/org/mozilla/javascript/tests/Evaluator.java,v
done
Checking in testsrc/org/mozilla/javascript/tests/Evaluator.java;
/cvsroot/mozilla/js/rhino/testsrc/org/mozilla/javascript/tests/Evaluator.java,v  <--  Evaluator.java
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/rhino/testsrc/org/mozilla/javascript/tests/es5/ObjectGetOwnPropertyDescriptorTest.java,v
done
Checking in testsrc/org/mozilla/javascript/tests/es5/ObjectGetOwnPropertyDescriptorTest.java;
/cvsroot/mozilla/js/rhino/testsrc/org/mozilla/javascript/tests/es5/ObjectGetOwnPropertyDescriptorTest.java,v  <--  ObjectGetOwnPropertyDescriptorTest.java
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/rhino/testsrc/org/mozilla/javascript/tests/es5/ObjectGetOwnPropertyNamesTest.java,v
done
Checking in testsrc/org/mozilla/javascript/tests/es5/ObjectGetOwnPropertyNamesTest.java;
/cvsroot/mozilla/js/rhino/testsrc/org/mozilla/javascript/tests/es5/ObjectGetOwnPropertyNamesTest.java,v  <--  ObjectGetOwnPropertyNamesTest.java
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/rhino/testsrc/org/mozilla/javascript/tests/es5/ObjectKeysTest.java,v
done
Checking in testsrc/org/mozilla/javascript/tests/es5/ObjectKeysTest.java;
/cvsroot/mozilla/js/rhino/testsrc/org/mozilla/javascript/tests/es5/ObjectKeysTest.java,v  <--  ObjectKeysTest.java
initial revision: 1.1
done

Comment 10

[rspeyer]

Attachment: makes junit run new tests

Sorry, I left this out of the previous patch. It just gets junit to execute the new test files which are currently under an es5 subpackage.

Comment 11

[Norris Boyd]

Checking in testsrc/build.xml;
/cvsroot/mozilla/js/rhino/testsrc/build.xml,v  <--  build.xml
new revision: 1.15; previous revision: 1.14
done

Comment 12

[rspeyer]

Attachment: patch for isExtensible, preventExtensions and defineProperty

Initial implementation for isExtensible, preventExtensions and defineProperty

Comment 13

[rspeyer]

Attachment: new patch for isExtensible, preventExtensions and defineProperty

This is a slight revision of patch 380379 such that defineProperty now throws a TypeError when defining a new property on a non-extensible object, rather than just failing silently.

Comment 14

[Norris Boyd]

Comment on attachment 380698 [details] [diff] [review]
new patch for isExtensible, preventExtensions and defineProperty

>diff --git a/src/org/mozilla/javascript/NativeObject.java b/src/org/mozilla/javascript/NativeObject.java
>index 46cc101..b08aa68 100644
>--- a/src/org/mozilla/javascript/NativeObject.java
>+++ b/src/org/mozilla/javascript/NativeObject.java
>@@ -81,6 +81,12 @@ public class NativeObject extends IdScriptableObject
>                 "getOwnPropertyNames", 1);
>         addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_getOwnPropertyDescriptor,
>                 "getOwnPropertyDescriptor", 2);
>+        addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_defineProperty,
>+                "defineProperty", 3);
>+        addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_isExtensible,
>+                "isExtensible", 1);
>+        addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_preventExtensions,
>+                "preventExtensions", 1);
>         super.fillConstructorProperties(ctor);
>     }
> 
>@@ -273,21 +279,14 @@ public class NativeObject extends IdScriptableObject
>           case ConstructorId_getPrototypeOf:
>               {
>                 Object arg = args.length < 1 ? Undefined.instance : args[0];
>-                if (!(arg instanceof Scriptable)) {
>-                    throw ScriptRuntime.typeError1("msg.arg.not.object",
>-                                                   ScriptRuntime.typeof(arg));
>-                }
>-                Scriptable obj = (Scriptable) arg;
>+                Scriptable obj = ensureScriptable(arg);
>                 return obj.getPrototype();
>               }
>           case ConstructorId_keys:
>               {
>                 Object arg = args.length < 1 ? Undefined.instance : args[0];
>-                if (!(arg instanceof Scriptable)) {
>-                    throw ScriptRuntime.typeError1("msg.arg.not.object",
>-                                                   ScriptRuntime.typeof(arg));
>-                }
>-                Object[] ids = ((Scriptable) arg).getIds();
>+                Scriptable obj = ensureScriptable(arg);
>+                Object[] ids = obj.getIds();
>                 for (int i = 0; i < ids.length; i++) {
>                   ids[i] = ScriptRuntime.toString(ids[i]);
>                 }
>@@ -296,11 +295,8 @@ public class NativeObject extends IdScriptableObject
>           case ConstructorId_getOwnPropertyNames:
>               {
>                 Object arg = args.length < 1 ? Undefined.instance : args[0];
>-                if (!(arg instanceof Scriptable)) {
>-                    throw ScriptRuntime.typeError1("msg.arg.not.object",
>-                                                   ScriptRuntime.typeof(arg));
>-                }
>-                Object[] ids = ((ScriptableObject) arg).getAllIds();
>+                ScriptableObject obj = ensureScriptableObject(arg);
>+                Object[] ids = obj.getAllIds();
>                 for (int i = 0; i < ids.length; i++) {
>                   ids[i] = ScriptRuntime.toString(ids[i]);
>                 }
>@@ -309,25 +305,56 @@ public class NativeObject extends IdScriptableObject
>           case ConstructorId_getOwnPropertyDescriptor:
>               {
>                 Object arg = args.length < 1 ? Undefined.instance : args[0];
>-                if (!(arg instanceof ScriptableObject)) {
>                     // TODO(norris): There's a deeper issue here if
>                     // arg instanceof Scriptable. Should we create a new
>                     // interface to admit the new ECMAScript 5 operations?
>-                    throw ScriptRuntime.typeError1("msg.arg.not.object",
>-                                                   ScriptRuntime.typeof(arg));
>-                }
>-                ScriptableObject obj = (ScriptableObject) arg;
>+                ScriptableObject obj = ensureScriptableObject(arg);
>                 Object nameArg = args.length < 2 ? Undefined.instance : args[1];
>                 String name = ScriptRuntime.toString(nameArg);
>                 Scriptable desc = obj.getOwnPropertyDescriptor(cx, name);
>                 return desc == null ? Undefined.instance : desc;
>               }
>+          case ConstructorId_defineProperty:
>+              {
>+                Object arg = args.length < 1 ? Undefined.instance : args[0];
>+                ScriptableObject obj = ensureScriptableObject(arg);
>+                Object nameArg = args.length < 2 ? Undefined.instance : args[1];
>+                String name = ScriptRuntime.toString(nameArg);
>+                Object descArg = args.length < 3 ? Undefined.instance : args[2];
>+                ScriptableObject desc = ensureScriptableObject(descArg);
>+                obj.defineOwnProperty(name, desc);
>+                return obj;
>+              }
>+          case ConstructorId_isExtensible:
>+              {
>+                Object arg = args.length < 1 ? Undefined.instance : args[0];
>+                ScriptableObject obj = ensureScriptableObject(arg);
>+                return obj.isExtensible();
>+              }
>+          case ConstructorId_preventExtensions:
>+              {
>+                Object arg = args.length < 1 ? Undefined.instance : args[0];
>+                ScriptableObject obj = ensureScriptableObject(arg);
>+                obj.preventExtensions();
>+                return obj;
>+              }
> 
>           default:
>             throw new IllegalArgumentException(String.valueOf(id));
>         }
>     }
> 
>+    private Scriptable ensureScriptable(Object arg) {
>+      if ( !(arg instanceof Scriptable) )
>+        throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.typeof(arg));
>+      return (Scriptable) arg;
>+    }

Add 1 line between

>+    private ScriptableObject ensureScriptableObject(Object arg) {
>+      if ( !(arg instanceof ScriptableObject) )
>+        throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.typeof(arg));
>+      return (ScriptableObject) arg;
>+    }
>+
> // #string_id_map#
> 
>     @Override
>@@ -374,6 +401,9 @@ public class NativeObject extends IdScriptableObject
>         ConstructorId_keys = -2,
>         ConstructorId_getOwnPropertyNames = -3,
>         ConstructorId_getOwnPropertyDescriptor = -4,
>+        ConstructorId_defineProperty = -5,
>+        ConstructorId_isExtensible = -6,
>+        ConstructorId_preventExtensions = -7,
> 
>         Id_constructor           = 1,
>         Id_toString              = 2,
>diff --git a/src/org/mozilla/javascript/ScriptableObject.java b/src/org/mozilla/javascript/ScriptableObject.java
>index 87984f0..2044eb8 100644
>--- a/src/org/mozilla/javascript/ScriptableObject.java
>+++ b/src/org/mozilla/javascript/ScriptableObject.java
>@@ -154,6 +154,9 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>     private static final int SLOT_REMOVE = 3;
>     private static final int SLOT_MODIFY_GETTER_SETTER = 4;
>     private static final int SLOT_MODIFY_CONST = 5;
>+    private static final int SLOT_CONVERT_ACCESSOR_TO_DATA = 6;
>+
>+    private boolean isExtensible = true;

In the current Rhino, "sealed" is pretty much equivalent to !extensible in es5. I don't think it makes sense to have both concepts running around independently, especially since now there's a new "sealed" in es5 that means something different!

Thinking about it, go ahead and add this boolean and I'll think about how to handle this change in the most backwards compatible way.

> 
>     private static class Slot implements Serializable
>     {
>@@ -574,13 +577,30 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>     public void setGetterOrSetter(String name, int index,
>                                   Callable getterOrSetter, boolean isSetter)
>     {
>+        setGetterOrSetter(name, index, getterOrSetter, isSetter, false);
>+    }
>+
>+    private void setGetterOrSetter(String name, int index, Callable getterOrSetter, boolean isSetter, boolean force)
>+    {
>         if (name != null && index != 0)
>             throw new IllegalArgumentException(name);
> 
>+        if (!force) {
>         checkNotSealed(name, index);
>-        GetterSlot gslot = (GetterSlot)getSlot(name, index,
>-                                               SLOT_MODIFY_GETTER_SETTER);
>+        }
>+
>+        final GetterSlot gslot;
>+        if (isExtensible()) {
>+          gslot = (GetterSlot)getSlot(name, index, SLOT_MODIFY_GETTER_SETTER);
>+        } else {
>+          gslot = (GetterSlot)getSlot(name, index, SLOT_QUERY);
>+          if (gslot == null)
>+            return;
>+        }
>+        
>+        if (!force) {
>         gslot.checkNotReadonly();
>+        }
>         if (isSetter) {
>             gslot.setter = getterOrSetter;
>         } else {
>@@ -1476,6 +1496,117 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>         gslot.setter = setterBox;
>     }
> 
>+    public void defineOwnProperty(String name, ScriptableObject desc) {

Add javadoc, at least with reference to ES5 spec section.

>+      Slot slot = getSlot(name, 0, SLOT_QUERY);
>+      final int attributes;
>+
>+      if (slot == null) { // new slot
>+        if (!isExtensible()) throw ScriptRuntime.typeError("msg.not.extensible");
>+        slot = getSlot(name, 0, SLOT_MODIFY);
>+        attributes = applyDescriptorToAttributeBitset(DONTENUM|READONLY|PERMANENT, desc);
>+      } else {
>+        checkLegalPropertyRedefinition(name, desc);
>+        attributes = applyDescriptorToAttributeBitset(getAttributes(name), desc);
>+      }
>+
>+      defineOwnProperty(slot, desc, attributes);
>+    }
>+
>+    private void defineOwnProperty(Slot slot, ScriptableObject desc, int attributes) {
>+      if (isAccessorDescriptor(desc)) {
>+        if ( !(slot instanceof GetterSlot) ) 
>+          slot = getSlot(slot.name, 0, SLOT_MODIFY_GETTER_SETTER);
>+
>+        Object getter = desc.get("get");
>+        Object setter = desc.get("set");
>+
>+        if (getter != null && !(getter instanceof Callable) ) {

If not found, get() returns NOT_FOUND, not null (since null is a valid value).

>+          throw ScriptRuntime.notFunctionError(getter);
>+        }
>+        if (setter != null && !(setter instanceof Callable) ) {
>+          throw ScriptRuntime.notFunctionError(setter);
>+        }
>+
>+        defineOwnAccessorProperty( (GetterSlot) slot, (Callable) getter, (Callable) setter, attributes);
>+      } else  {
>+        if (slot instanceof GetterSlot && isDataDescriptor(desc)) {
>+          slot = getSlot(slot.name, 0, SLOT_CONVERT_ACCESSOR_TO_DATA);
>+        }
>+        defineOwnDataProperty(slot, desc.get("value"), attributes);
>+      }
>+    }
>+
>+    private void defineOwnDataProperty(Slot slot, Object value, int attributes) {
>+      if (value != null) {
>+        slot.value = value;
>+      }
>+      slot.setAttributes(attributes);
>+    }
>+
>+    private void defineOwnAccessorProperty(GetterSlot slot, Callable getter, Callable setter, int attributes) {
>+      if (getter != null) {
>+        slot.getter = getter;
>+      } if (setter != null) {
>+        slot.setter = setter;
>+      }
>+      slot.value = Undefined.instance;
>+      slot.setAttributes(attributes);
>+    }
>+
>+    private void checkLegalPropertyRedefinition(String name, ScriptableObject desc) {
>+      ScriptableObject current = getOwnPropertyDescriptor(Context.getContext(), name);
>+      if (Boolean.FALSE.equals(current.get("configurable"))) {

From reading 8.10.5, a couple of changes are needed:
* [[Get]] is not equivalent to ScriptableObject.get. [[Get]] will look in the prototype chain if need be, and will call getters if need be. I think getProperty will do what is needed here, but double check that getters are honored. There are other places in this patch that need to use a method other than get() as well.
* Step 4b specifies that ToBoolean is called. So use ScriptRuntime.toBoolean rather than comparing with Boolean.FALSE.

This is as far as I got tonight, but it'll give you something to work on.

>+
>+        if (Boolean.TRUE.equals(desc.get("configurable"))) 
>+          throw ScriptRuntime.typeError1("msg.change.configurable.false.to.true", name);
>+        if (desc.has("enumerable", desc) && !current.get("enumerable").equals(desc.get("enumerable")))
>+          throw ScriptRuntime.typeError1("msg.change.enumerable.with.configurable.false", name);
>+
>+        if (isGenericDescriptor(desc)) {
>+          // no further validation required
>+        } else if (isDataDescriptor(desc) && isDataDescriptor(current)) {
>+          if (Boolean.FALSE.equals(current.get("writable"))) {
>+            if (Boolean.TRUE.equals(desc.get("writable")))
>+              throw ScriptRuntime.typeError1("msg.change.writable.false.to.true.with.configurable.false", name);
>+
>+            if (desc.has("value", desc) && !ScriptRuntime.shallowEq(current.get("value"), desc.get("value"))) 
>+              throw ScriptRuntime.typeError1("msg.change.value.with.writable.false", name);
>+          }
>+        } else if (isAccessorDescriptor(desc) && isAccessorDescriptor(current)) {
>+          if (desc.has("set", desc) && !ScriptRuntime.shallowEq(current.get("set"), desc.get("set")))
>+            throw ScriptRuntime.typeError1("msg.change.setter.with.configurable.false", name);
>+
>+          if (desc.has("get", desc) && !ScriptRuntime.shallowEq(current.get("get"), desc.get("get"))) 
>+            throw ScriptRuntime.typeError1("msg.change.getter.with.configurable.false", name);
>+        } else {
>+          throw ScriptRuntime.typeError1("msg.change.descriptor.type.with.configurable.false", name);
>+        }
>+      }
>+    }
>+
>+    private int applyDescriptorToAttributeBitset(int attributes, ScriptableObject desc) {
>+      Boolean enumerable = (Boolean) desc.get("enumerable");
>+      if (enumerable != null) attributes = (enumerable ? attributes & ~DONTENUM : attributes | DONTENUM);
>+
>+      Boolean writable = (Boolean) desc.get("writable");
>+      if (writable != null) attributes = (writable ? attributes & ~READONLY : attributes | READONLY);
>+
>+      Boolean configurable = (Boolean) desc.get("configurable");
>+      if (configurable != null) attributes = (configurable ? attributes & ~PERMANENT : attributes | PERMANENT);
>+
>+      return attributes;
>+    }
>+
>+    private boolean isDataDescriptor(ScriptableObject desc) {
>+      return desc.has("value", desc) || desc.has("writable", desc);
>+    }
>+    private boolean isAccessorDescriptor(ScriptableObject desc) {
>+      return desc.has("get", desc) || desc.has("set", desc);
>+    }
>+    private boolean isGenericDescriptor(ScriptableObject desc) {
>+      return !isDataDescriptor(desc) && !isAccessorDescriptor(desc);
>+    }
>+
>     /**
>      * Search for names in a class, adding the resulting methods
>      * as properties.
>@@ -1576,6 +1707,14 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>         }
>     }
> 
>+    public boolean isExtensible() {
>+      return isExtensible;
>+    }
>+
>+    public void preventExtensions() {
>+      isExtensible = false;
>+    }
>+
>     /**
>      * Seal this object.
>      *
>@@ -2082,6 +2221,11 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>             if (slot == null) {
>                 return false;
>             }
>+        } else if (!isExtensible()) {
>+            slot = getSlot(name, index, SLOT_QUERY);
>+            if (slot == null) {
>+                return true;
>+            }
>         } else {
>             checkNotSealed(name, index);
>             // either const hoisted declaration or initialization
>@@ -2188,6 +2332,10 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>                 !(slot instanceof GetterSlot))
>                 break lastAccessCheck;
> 
>+            if (accessType == SLOT_CONVERT_ACCESSOR_TO_DATA &&
>+                (slot instanceof GetterSlot))
>+                break lastAccessCheck;
>+
>             return slot;
>         }
> 
>@@ -2206,7 +2354,8 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>         if (accessType == SLOT_QUERY ||
>             accessType == SLOT_MODIFY ||
>             accessType == SLOT_MODIFY_CONST ||
>-            accessType == SLOT_MODIFY_GETTER_SETTER)
>+            accessType == SLOT_MODIFY_GETTER_SETTER ||
>+            accessType == SLOT_CONVERT_ACCESSOR_TO_DATA)
>         {
>             // Check the hashtable without using synchronization
> 
>@@ -2249,6 +2398,9 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>                 } else if (accessType == SLOT_MODIFY_CONST) {
>                     if (slot != null)
>                         return slot;
>+                } else if (accessType == SLOT_CONVERT_ACCESSOR_TO_DATA) {
>+                    if ( !(slot instanceof GetterSlot) )
>+                        return slot;
>                 }
>             }
> 
>@@ -2288,11 +2440,19 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>                         // implementation it is harmless with the only
>                         // complication is the need to replace the added slot
>                         // if we need GetterSlot and the old one is not.
>-                        if (accessType == SLOT_MODIFY_GETTER_SETTER &&
>-                            !(slot instanceof GetterSlot))
>-                        {
>-                            GetterSlot newSlot = new GetterSlot(name,
>-                                    indexOrHash, slot.getAttributes());
>+
>+                        Slot newSlot;
>+
>+                        if (accessType == SLOT_MODIFY_GETTER_SETTER && !(slot instanceof GetterSlot)) {
>+                            newSlot = new GetterSlot(name, indexOrHash, slot.getAttributes());
>+                        } else if (accessType == SLOT_CONVERT_ACCESSOR_TO_DATA && (slot instanceof GetterSlot)) {
>+                            newSlot = new Slot(name, indexOrHash, slot.getAttributes());
>+                        } else if (accessType == SLOT_MODIFY_CONST) {
>+                          return null;
>+                        } else {
>+                          return slot;
>+                        }
>+
>                             newSlot.value = slot.value;
>                             newSlot.next = slot.next;
>                             // add new slot to linked list
>@@ -2314,13 +2474,8 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>                             if (slot == lastAccess) {
>                                 lastAccess = REMOVED;
>                             }
>-                            slot = newSlot;
>-                        } else if (accessType == SLOT_MODIFY_CONST) {
>-                            return null;
>-                        }
>-                        return slot;
>-                    }
>-
>+                        return newSlot;
>+                    } else {
>                     // Check if the table is not too full before inserting.
>                     if (4 * (count + 1) > 3 * slotsLocalRef.length) {
>                         slotsLocalRef = new Slot[slotsLocalRef.length * 2 + 1];
>@@ -2330,7 +2485,7 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>                                 indexOrHash);
>                     }
>                 }
>-
>+                }
>                 Slot newSlot = (accessType == SLOT_MODIFY_GETTER_SETTER
>                                 ? new GetterSlot(name, indexOrHash, 0)
>                                 : new Slot(name, indexOrHash, 0));
>diff --git a/src/org/mozilla/javascript/resources/Messages.properties b/src/org/mozilla/javascript/resources/Messages.properties
>index 545dc87..6ad9097 100644
>--- a/src/org/mozilla/javascript/resources/Messages.properties
>+++ b/src/org/mozilla/javascript/resources/Messages.properties
>@@ -667,6 +667,32 @@ msg.modify.sealed =\
> msg.modify.readonly =\
>     Cannot modify readonly property: {0}.
> 
>+msg.change.configurable.false.to.true =\
>+    Cannot change the configurable attribute of "{0}" from false to true.
>+
>+msg.change.enumerable.with.configurable.false =\
>+    Cannot change the enumerable attribute of "{0}" because configurable is false.
>+
>+msg.change.writable.false.to.true.with.configurable.false =\
>+    Cannot change the writable attribute of "{0}" from false to true because configurable is false.
>+
>+# TODO should we be using msg.modify.readonly here instead?
>+msg.change.value.with.writable.false =\
>+    Cannot change the value of attribute "{0}" because writable is false.
>+
>+msg.change.getter.with.configurable.false =\
>+    Cannot change the get attribute of "{0}" because configurable is false.
>+
>+msg.change.setter.with.configurable.false =\
>+    Cannot change the set attribute of "{0}" because configurable is false.
>+
>+# TODO not sure about how descriptive this message is
>+msg.change.descriptor.type.with.configurable.false =\
>+    Cannot change type of property "{0}" because configurable is false.
>+
>+msg.not.extensible =\
>+    Cannot add properties to this object because extensible is false.
>+
> # TokenStream
> msg.missing.exponent =\
>     missing exponent
>diff --git a/testsrc/doctests/480758.doctest b/testsrc/doctests/480758.doctest
>index 675483a..3893400 100755
>diff --git a/testsrc/doctests/object.defineproperty.doctest b/testsrc/doctests/object.defineproperty.doctest
>new file mode 100644
>index 0000000..e40e972
>--- /dev/null
>+++ b/testsrc/doctests/object.defineproperty.doctest
>@@ -0,0 +1,169 @@
>+js> load('testsrc/doctests/util.js');
>+
>+js> Object.defineProperty
>+function defineProperty() { [native code for Object.defineProperty, arity=3] }
>+
>+js> expectTypeError(function() { Object.defineProperty() });
>+js> expectTypeError(function() { Object.defineProperty({}) });
>+js> expectTypeError(function() { Object.defineProperty({}, 'p') });
>+js> [undefined, null, true, 1, 'hello'].forEach(function(value) { 
>+  >   expectTypeError(function() { Object.defineProperty(value, 'p', {}) }) 
>+  > })
>+
>+js> Object.defineProperty({}, 'p', {value:1}).p
>+1
>+js> var obj = {}
>+js> Object.defineProperty(obj, 'a', {
>+  >   value: 1,
>+  >   enumerable: false,
>+  >   writable: false,
>+  >   configurable: false
>+  > })  
>+[object Object]
>+js> for (var p in obj) print(p); // check it has no enumerable properties
>+js> obj.a = 2; obj.a; // check that the property is not writable
>+1
>+js> delete obj.a; obj.a // check that the property is not deletable
>+1
>+
>+js> var define = Object.defineProperty;
>+js> var describe = Object.getOwnPropertyDescriptor;
>+
>+js> // when define new property with empty descriptor then default values are used for the descriptor
>+js> var obj = define({}, 'a', {});
>+js> describe(obj, 'a').toSource()
>+({writable:false, enumerable:false, configurable:false})
>+
>+js> // when define new property with data descriptor then those values are used for the descriptor
>+js> var obj = define({}, 'a', { value: 2, writable: true });
>+js> var {value:v, writable:w} = describe(obj, 'a'); [v, w].toSource();
>+[2, true]
>+js> obj.a
>+2
>+
>+js> // when define new property with accessor descriptor then those values are used for the descriptor
>+js> var obj = define({}, 'a', { get: function() { return 3; }, set: function(value) {} });
>+js> var {get:g, set:s} = describe(obj, 'a'); [g, s].toSource();
>+[(function () {return 3;}), (function (value) {})]
>+js> obj.a
>+3
>+
>+js> // when define existing property with empty descriptor then descriptor is left unchanged
>+js> var descriptor = {value:1, writable:true, enumerable:true, configurable:true};
>+js> var obj = define({},  'a', descriptor);
>+js> var obj = define(obj, 'a', {});
>+js> describe(obj, 'a').toSource()
>+({value:1, writable:true, enumerable:true, configurable:true})
>+
>+js> // when define existing property with same descriptor then descriptor is left unchanged
>+js> var descriptor = {value:1, writable:true, enumerable:true, configurable:true};
>+js> var obj = define({},  'a', descriptor);
>+js> var obj = define(obj, 'a', descriptor);
>+js> describe(obj, 'a').toSource()
>+({value:1, writable:true, enumerable:true, configurable:true})
>+
>+js> // may not change configurable from false to true
>+js> expectTypeError(function() {
>+  >   var obj = define({}, 'a', {configurable : false});
>+  >   define(obj, 'a',          {configurable : true});
>+  > });
>+
>+js> // may not change enumerable when configurable is false
>+js> expectTypeError(function() {
>+  >   var obj = define({}, 'a', {enumerable : true, configurable:false});
>+  >   define(obj, 'a',          {enumerable : false});
>+  > });
>+
>+js> // may not change writable from false to true when configurable is false
>+js> expectTypeError(function() {
>+  >   var obj = define({}, 'a', {writable : false, configurable: false});
>+  >   define(obj, 'a',          {writable : true});
>+  > });
>+
>+js> // may not change value when writable is false
>+js> expectTypeError(function() {
>+  >   var obj = define({}, 'a', {value : 1, writable:false});
>+  >   define(obj, 'a',          {value : 2});
>+  > });
>+
>+js> // may not change getter when configurable is false
>+js> expectTypeError(function() {
>+  >   var obj = define({}, 'a', {get: function() { return 1 }, configurable:false});
>+  >   define(obj, 'a',          {get: function() { return 1 }});
>+  > });
>+
>+js> // may not change setter when configurable is false
>+js> expectTypeError(function() {
>+  >   var obj = define({}, 'a', {set: function(val) {}, configurable:false});
>+  >   define(obj, 'a',          {set: function(val) {}});
>+  > });
>+
>+js> // may not change from data property to accessor property when configurable is false
>+js> expectTypeError(function() {
>+  >   var obj = define({}, 'a', {value : 1, configurable:false});
>+  >   define(obj, 'a',          {get   : function() { return 1 }});
>+  > });
>+
>+js> // may not change from accessor property to data property when configurable is false
>+js> expectTypeError(function() {
>+  >   var obj = define({}, 'a', {get   : function() { return 1 }, configurable:false});
>+  >   define(obj, 'a',          {value : 1});
>+  > });
>+
>+js> // can change writable from true to false when configurable is false
>+js> var obj = define({},  'a', {writable:true, configurable:false});
>+js> var obj = define(obj, 'a', {writable:false});
>+
>+js> // can set enumerable to the same value when configuable is false
>+js> var obj = define({},  'a', {enumerable:true, configurable:false});
>+js> var obj = define(obj, 'a', {enumerable:true});
>+
>+js> // can change from data property to accessor property when configurable is true
>+js> var obj = define({},  'a', {value : 1, configurable: true});
>+js> var obj = define(obj, 'a', {get   : function() { return 4 }});
>+js> obj.a
>+4
>+js> describe(obj, 'a').toSource()
>+({enumerable:false, configurable:true, get:(function () {return 4;})})
>+
>+js> // can change from accessor property to data property when configurable is true
>+js> var obj = define({},  'a', {get   : function() { return 2 }, configurable:true});
>+js> var obj = define(obj, 'a', {value : 5});
>+js> obj.a
>+5
>+js> describe(obj, 'a').toSource()
>+({value:5, writable:false, enumerable:false, configurable:true})
>+
>+js> // can change enumerable and writable to true when configurable is true
>+js> var obj = define({},  'a', {writable : false, enumerable : false, configurable:true});
>+js> var obj = define(obj, 'a', {writable : true,  enumerable : true, configurable:true});
>+
>+js> // can change the value if writable is true
>+js> var obj = define({},  'a', {value:6, writable:true})
>+js> obj.a
>+6
>+js> var obj = define(obj, 'a', {value:7})
>+js> obj.a
>+7
>+
>+js> // defining a new property should fail loudly when object is not extensible
>+js> var obj = Object.preventExtensions({});
>+js> expectTypeError(function() { define(obj, 'a', {value:1}) })
>+
>+js> // defining new property should succeed when object is extensible
>+js> var obj = {}
>+js> Object.isExtensible(obj);
>+true
>+js> obj.a = 8; obj.a
>+8
>+
>+js> // changing existing property should succeed when object is not extensible
>+js> var obj = define({},  'a', {value:1, writable:true});
>+js> var obj = Object.preventExtensions(obj);
>+js> obj.a = 9; obj.a
>+9
>+
>+js> // defined getters and setters must be functions
>+js> expectTypeError(function() { define({}, 'a', {get:1}); })
>+js> expectTypeError(function() { define({}, 'a', {set:1}); })
>+
>diff --git a/testsrc/doctests/object.extensible.doctest b/testsrc/doctests/object.extensible.doctest
>new file mode 100644
>index 0000000..841511d
>--- /dev/null
>+++ b/testsrc/doctests/object.extensible.doctest
>@@ -0,0 +1,30 @@
>+js> load('testsrc/doctests/util.js');
>+
>+js> Object.isExtensible;
>+function isExtensible() { [native code for Object.isExtensible, arity=1] }
>+js> expectTypeError(function() { Object.isExtensible() });
>+js> [undefined, null, true, 1, 'hello'].forEach(function(value) { 
>+  >   expectTypeError(function() { Object.isExtensible(value) }) 
>+  > })
>+
>+js> Object.preventExtensions;
>+function preventExtensions() { [native code for Object.preventExtensions, arity=1] }
>+js> expectTypeError(function() { Object.preventExtensions() });
>+js> [undefined, null, true, 1, 'hello'].forEach(function(value) { 
>+  >   expectTypeError(function() { Object.preventExtensions(value) }) 
>+  > })
>+
>+js> var x = {};
>+js> Object.isExtensible(x);
>+true
>+js> var y = Object.preventExtensions(x);
>+js> y === x;
>+true
>+js> Object.isExtensible(x);
>+false
>+
>+js> x.a = 1; x.a
>+js>
>+
>+js> x.__defineGetter__('b', function() { return 1 }); x.b
>+js>

Comment 15

[rspeyer]

Attachment: updates from comment 14 (https://bugzilla.mozilla.org/show_bug.cgi?id=489329#c14)

- added line between methods
- Added javadoc for ScriptableObject#defineOwnProperty
- replaced use of `get` with `getProperty` and `has` with `hasProperty` when dealing with property descriptors
- called ScriptRuntime.toBoolean on the boolean properties of any property descriptors passed in

Comment 16

[Norris Boyd]

Comment on attachment 381099 [details] [diff] [review]
updates from comment 14 (https://bugzilla.mozilla.org/show_bug.cgi?id=489329#c14)

>diff --git a/src/org/mozilla/javascript/NativeObject.java b/src/org/mozilla/javascript/NativeObject.java
>index 46cc101..f9f3bdd 100644
>--- a/src/org/mozilla/javascript/NativeObject.java
>+++ b/src/org/mozilla/javascript/NativeObject.java
>@@ -81,6 +81,12 @@ public class NativeObject extends IdScriptableObject
>                 "getOwnPropertyNames", 1);
>         addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_getOwnPropertyDescriptor,
>                 "getOwnPropertyDescriptor", 2);
>+        addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_defineProperty,
>+                "defineProperty", 3);
>+        addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_isExtensible,
>+                "isExtensible", 1);
>+        addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_preventExtensions,
>+                "preventExtensions", 1);
>         super.fillConstructorProperties(ctor);
>     }
> 
>@@ -273,21 +279,14 @@ public class NativeObject extends IdScriptableObject
>           case ConstructorId_getPrototypeOf:
>               {
>                 Object arg = args.length < 1 ? Undefined.instance : args[0];
>-                if (!(arg instanceof Scriptable)) {
>-                    throw ScriptRuntime.typeError1("msg.arg.not.object",
>-                                                   ScriptRuntime.typeof(arg));
>-                }
>-                Scriptable obj = (Scriptable) arg;
>+                Scriptable obj = ensureScriptable(arg);
>                 return obj.getPrototype();
>               }
>           case ConstructorId_keys:
>               {
>                 Object arg = args.length < 1 ? Undefined.instance : args[0];
>-                if (!(arg instanceof Scriptable)) {
>-                    throw ScriptRuntime.typeError1("msg.arg.not.object",
>-                                                   ScriptRuntime.typeof(arg));
>-                }
>-                Object[] ids = ((Scriptable) arg).getIds();
>+                Scriptable obj = ensureScriptable(arg);
>+                Object[] ids = obj.getIds();
>                 for (int i = 0; i < ids.length; i++) {
>                   ids[i] = ScriptRuntime.toString(ids[i]);
>                 }
>@@ -296,11 +295,8 @@ public class NativeObject extends IdScriptableObject
>           case ConstructorId_getOwnPropertyNames:
>               {
>                 Object arg = args.length < 1 ? Undefined.instance : args[0];
>-                if (!(arg instanceof Scriptable)) {
>-                    throw ScriptRuntime.typeError1("msg.arg.not.object",
>-                                                   ScriptRuntime.typeof(arg));
>-                }
>-                Object[] ids = ((ScriptableObject) arg).getAllIds();
>+                ScriptableObject obj = ensureScriptableObject(arg);
>+                Object[] ids = obj.getAllIds();
>                 for (int i = 0; i < ids.length; i++) {
>                   ids[i] = ScriptRuntime.toString(ids[i]);
>                 }
>@@ -309,25 +305,57 @@ public class NativeObject extends IdScriptableObject
>           case ConstructorId_getOwnPropertyDescriptor:
>               {
>                 Object arg = args.length < 1 ? Undefined.instance : args[0];
>-                if (!(arg instanceof ScriptableObject)) {
>-                    // TODO(norris): There's a deeper issue here if
>-                    // arg instanceof Scriptable. Should we create a new
>-                    // interface to admit the new ECMAScript 5 operations?
>-                    throw ScriptRuntime.typeError1("msg.arg.not.object",
>-                                                   ScriptRuntime.typeof(arg));
>-                }
>-                ScriptableObject obj = (ScriptableObject) arg;
>+                // TODO(norris): There's a deeper issue here if
>+                // arg instanceof Scriptable. Should we create a new
>+                // interface to admit the new ECMAScript 5 operations?
>+                ScriptableObject obj = ensureScriptableObject(arg);
>                 Object nameArg = args.length < 2 ? Undefined.instance : args[1];
>                 String name = ScriptRuntime.toString(nameArg);
>                 Scriptable desc = obj.getOwnPropertyDescriptor(cx, name);
>                 return desc == null ? Undefined.instance : desc;
>               }
>+          case ConstructorId_defineProperty:
>+              {
>+                Object arg = args.length < 1 ? Undefined.instance : args[0];
>+                ScriptableObject obj = ensureScriptableObject(arg);
>+                Object nameArg = args.length < 2 ? Undefined.instance : args[1];
>+                String name = ScriptRuntime.toString(nameArg);
>+                Object descArg = args.length < 3 ? Undefined.instance : args[2];
>+                ScriptableObject desc = ensureScriptableObject(descArg);
>+                obj.defineOwnProperty(name, desc);
>+                return obj;
>+              }
>+          case ConstructorId_isExtensible:
>+              {
>+                Object arg = args.length < 1 ? Undefined.instance : args[0];
>+                ScriptableObject obj = ensureScriptableObject(arg);
>+                return obj.isExtensible();
>+              }
>+          case ConstructorId_preventExtensions:
>+              {
>+                Object arg = args.length < 1 ? Undefined.instance : args[0];
>+                ScriptableObject obj = ensureScriptableObject(arg);
>+                obj.preventExtensions();
>+                return obj;
>+              }
> 
>           default:
>             throw new IllegalArgumentException(String.valueOf(id));
>         }
>     }
> 
>+    private Scriptable ensureScriptable(Object arg) {
>+      if ( !(arg instanceof Scriptable) )
>+        throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.typeof(arg));
>+      return (Scriptable) arg;
>+    }
>+
>+    private ScriptableObject ensureScriptableObject(Object arg) {
>+      if ( !(arg instanceof ScriptableObject) )
>+        throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.typeof(arg));
>+      return (ScriptableObject) arg;
>+    }
>+
> // #string_id_map#
> 
>     @Override
>@@ -374,6 +402,9 @@ public class NativeObject extends IdScriptableObject
>         ConstructorId_keys = -2,
>         ConstructorId_getOwnPropertyNames = -3,
>         ConstructorId_getOwnPropertyDescriptor = -4,
>+        ConstructorId_defineProperty = -5,
>+        ConstructorId_isExtensible = -6,
>+        ConstructorId_preventExtensions = -7,
> 
>         Id_constructor           = 1,
>         Id_toString              = 2,
>diff --git a/src/org/mozilla/javascript/ScriptableObject.java b/src/org/mozilla/javascript/ScriptableObject.java
>index 87984f0..a4146d0 100644
>--- a/src/org/mozilla/javascript/ScriptableObject.java
>+++ b/src/org/mozilla/javascript/ScriptableObject.java
>@@ -154,6 +154,9 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>     private static final int SLOT_REMOVE = 3;
>     private static final int SLOT_MODIFY_GETTER_SETTER = 4;
>     private static final int SLOT_MODIFY_CONST = 5;
>+    private static final int SLOT_CONVERT_ACCESSOR_TO_DATA = 6;
>+
>+    private boolean isExtensible = true;
> 
>     private static class Slot implements Serializable
>     {
>@@ -574,13 +577,30 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>     public void setGetterOrSetter(String name, int index,
>                                   Callable getterOrSetter, boolean isSetter)
>     {
>+        setGetterOrSetter(name, index, getterOrSetter, isSetter, false);
>+    }
>+
>+    private void setGetterOrSetter(String name, int index, Callable getterOrSetter, boolean isSetter, boolean force)
>+    {
>         if (name != null && index != 0)
>             throw new IllegalArgumentException(name);
> 
>-        checkNotSealed(name, index);
>-        GetterSlot gslot = (GetterSlot)getSlot(name, index,
>-                                               SLOT_MODIFY_GETTER_SETTER);
>-        gslot.checkNotReadonly();
>+        if (!force) {
>+          checkNotSealed(name, index);
>+        }
>+
>+        final GetterSlot gslot;
>+        if (isExtensible()) {
>+          gslot = (GetterSlot)getSlot(name, index, SLOT_MODIFY_GETTER_SETTER);
>+        } else {
>+          gslot = (GetterSlot)getSlot(name, index, SLOT_QUERY);
>+          if (gslot == null)
>+            return;
>+        }
>+        
>+        if (!force) {
>+          gslot.checkNotReadonly();
>+        }
>         if (isSetter) {
>             gslot.setter = getterOrSetter;
>         } else {
>@@ -588,7 +608,7 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>         }
>         gslot.value = Undefined.instance;
>     }
>-    
>+
>     /**
>      * Get the getter or setter for a given property. Used by __lookupGetter__
>      * and __lookupSetter__.
>@@ -1477,6 +1497,130 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>     }
> 
>     /**
>+     * Defines a property on an object
>+     *
>+     * Based on [[DefineOwnProperty]] from 8.12.10 of the spec
>+     *
>+     * @param name the name of the property
>+     * @param desc the new property descriptor, as described in 8.6.1
>+     */
>+    public void defineOwnProperty(String name, ScriptableObject desc) {
>+      Slot slot = getSlot(name, 0, SLOT_QUERY);
>+      final int attributes;
>+
>+      if (slot == null) { // new slot
>+        if (!isExtensible()) throw ScriptRuntime.typeError("msg.not.extensible");
>+        slot = getSlot(name, 0, SLOT_MODIFY);
>+        attributes = applyDescriptorToAttributeBitset(DONTENUM|READONLY|PERMANENT, desc);
>+      } else {
>+        checkLegalPropertyRedefinition(name, desc);
>+        attributes = applyDescriptorToAttributeBitset(getAttributes(name), desc);
>+      }
>+
>+      defineOwnProperty(slot, desc, attributes);
>+    }
>+
>+    private void defineOwnProperty(Slot slot, ScriptableObject desc, int attributes) {
>+      if (isAccessorDescriptor(desc)) {
>+        if ( !(slot instanceof GetterSlot) ) 
>+          slot = getSlot(slot.name, 0, SLOT_MODIFY_GETTER_SETTER);
>+
>+        GetterSlot gslot = (GetterSlot) slot;
>+
>+        if (hasProperty(desc, "get")) {
>+          Object getter = getProperty(desc, "get");

Both hasProperty and getProperty traverse the prototype chain looking for the property. So it's more efficient to call getProperty once and compare the result to NOT_FOUND.

>+          if ( !(getter instanceof Callable) ) {
>+            throw ScriptRuntime.notFunctionError(getter);
>+          } else {
>+            gslot.getter = getter;
>+          }
>+        }
>+        if (hasProperty(desc, "set")) {
>+          Object setter = getProperty(desc, "set");
>+          if ( !(setter instanceof Callable) ) {
>+            throw ScriptRuntime.notFunctionError(setter);
>+          } else {
>+            gslot.setter = setter;
>+          }
>+        }
>+
>+        gslot.value = Undefined.instance;
>+        gslot.setAttributes(attributes);
>+      } else {
>+        if (slot instanceof GetterSlot && isDataDescriptor(desc)) {
>+          slot = getSlot(slot.name, 0, SLOT_CONVERT_ACCESSOR_TO_DATA);
>+        }
>+
>+        if (hasProperty(desc, "value")) {
>+          slot.value = getProperty(desc, "value");
>+        }
>+        slot.setAttributes(attributes);
>+      }
>+    }
>+
>+
>+
>+
>+    private void checkLegalPropertyRedefinition(String name, ScriptableObject desc) {
>+      ScriptableObject current = getOwnPropertyDescriptor(Context.getContext(), name);
>+      if (Boolean.FALSE.equals(getProperty(current, "configurable"))) {

I assume you're not calling toBoolean on the properties of current because you know that they will
be Boolean objects. Seems like you'd also know that they'd be defined in current rather than in its prototype chain you could call get(). Come to think of it, you don't have to create current at all
but instead could just compare against the bits in attributes.

>+        if (hasProperty(desc, "configurable") && ScriptRuntime.toBoolean(getProperty(desc, "configurable")))
>+          throw ScriptRuntime.typeError1("msg.change.configurable.false.to.true", name);
>+        if (hasProperty(desc, "enumerable") && !getProperty(current, "enumerable").equals(ScriptRuntime.toBoolean(getProperty(desc, "enumerable"))))
>+          throw ScriptRuntime.typeError1("msg.change.enumerable.with.configurable.false", name);
>+
>+        if (isGenericDescriptor(desc)) {
>+          // no further validation required
>+        } else if (isDataDescriptor(desc) && isDataDescriptor(current)) {
>+          if (Boolean.FALSE.equals(getProperty(current, "writable"))) {
>+            if (hasProperty(desc, "writable") && ScriptRuntime.toBoolean(getProperty(desc, "writable")))
>+              throw ScriptRuntime.typeError1("msg.change.writable.false.to.true.with.configurable.false", name);
>+
>+            if (hasProperty(desc, "value") && !ScriptRuntime.shallowEq(getProperty(current, "value"), getProperty(desc, "value"))) 
>+              throw ScriptRuntime.typeError1("msg.change.value.with.writable.false", name);
>+          }
>+        } else if (isAccessorDescriptor(desc) && isAccessorDescriptor(current)) {
>+          if (hasProperty(desc, "set") && !ScriptRuntime.shallowEq(getProperty(current, "set"), getProperty(desc, "set")))
>+            throw ScriptRuntime.typeError1("msg.change.setter.with.configurable.false", name);
>+
>+          if (hasProperty(desc, "get") && !ScriptRuntime.shallowEq(getProperty(current, "get"), getProperty(desc, "get"))) 
>+            throw ScriptRuntime.typeError1("msg.change.getter.with.configurable.false", name);
>+        } else {
>+          throw ScriptRuntime.typeError1("msg.change.descriptor.type.with.configurable.false", name);

What does this correspond to in the spec? I'm not sure where this comes from.

>+        }
>+      }
>+    }
>+
>+    private int applyDescriptorToAttributeBitset(int attributes, ScriptableObject desc) {
>+      if (hasProperty(desc, "enumerable")) {
>+        boolean enumerable = ScriptRuntime.toBoolean(getProperty(desc, "enumerable"));
>+        attributes = (enumerable ? attributes & ~DONTENUM : attributes | DONTENUM);
>+      }
>+
>+      if (hasProperty(desc, "writable")) {
>+        boolean writable = ScriptRuntime.toBoolean(getProperty(desc, "writable"));
>+        attributes = (writable ? attributes & ~READONLY : attributes | READONLY);
>+      }
>+
>+      if (hasProperty(desc, "configurable")) {
>+        boolean configurable = ScriptRuntime.toBoolean(getProperty(desc, "configurable"));
>+        attributes = (configurable ? attributes & ~PERMANENT : attributes | PERMANENT);
>+      }
>+
>+      return attributes;
>+    }
>+
>+    private boolean isDataDescriptor(ScriptableObject desc) {
>+      return hasProperty(desc, "value") || hasProperty(desc, "writable");
>+    }
>+    private boolean isAccessorDescriptor(ScriptableObject desc) {
>+      return hasProperty(desc, "get") || hasProperty(desc, "set");
>+    }
>+    private boolean isGenericDescriptor(ScriptableObject desc) {
>+      return !isDataDescriptor(desc) && !isAccessorDescriptor(desc);
>+    }
>+
>+    /**
>      * Search for names in a class, adding the resulting methods
>      * as properties.
>      *
>@@ -1576,6 +1720,14 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>         }
>     }
> 
>+    public boolean isExtensible() {
>+      return isExtensible;
>+    }
>+
>+    public void preventExtensions() {
>+      isExtensible = false;
>+    }
>+
>     /**
>      * Seal this object.
>      *
>@@ -2082,6 +2234,11 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>             if (slot == null) {
>                 return false;
>             }
>+        } else if (!isExtensible()) {
>+            slot = getSlot(name, index, SLOT_QUERY);
>+            if (slot == null) {
>+                return true;
>+            }
>         } else {
>             checkNotSealed(name, index);
>             // either const hoisted declaration or initialization
>@@ -2188,6 +2345,10 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>                 !(slot instanceof GetterSlot))
>                 break lastAccessCheck;
> 
>+            if (accessType == SLOT_CONVERT_ACCESSOR_TO_DATA &&
>+                (slot instanceof GetterSlot))
>+                break lastAccessCheck;
>+
>             return slot;
>         }
> 
>@@ -2206,7 +2367,8 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>         if (accessType == SLOT_QUERY ||
>             accessType == SLOT_MODIFY ||
>             accessType == SLOT_MODIFY_CONST ||
>-            accessType == SLOT_MODIFY_GETTER_SETTER)
>+            accessType == SLOT_MODIFY_GETTER_SETTER ||
>+            accessType == SLOT_CONVERT_ACCESSOR_TO_DATA)
>         {
>             // Check the hashtable without using synchronization
> 
>@@ -2249,6 +2411,9 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>                 } else if (accessType == SLOT_MODIFY_CONST) {
>                     if (slot != null)
>                         return slot;
>+                } else if (accessType == SLOT_CONVERT_ACCESSOR_TO_DATA) {
>+                    if ( !(slot instanceof GetterSlot) )
>+                        return slot;
>                 }
>             }
> 
>@@ -2288,49 +2453,52 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>                         // implementation it is harmless with the only
>                         // complication is the need to replace the added slot
>                         // if we need GetterSlot and the old one is not.
>-                        if (accessType == SLOT_MODIFY_GETTER_SETTER &&
>-                            !(slot instanceof GetterSlot))
>-                        {
>-                            GetterSlot newSlot = new GetterSlot(name,
>-                                    indexOrHash, slot.getAttributes());
>-                            newSlot.value = slot.value;
>-                            newSlot.next = slot.next;
>-                            // add new slot to linked list
>-                            if (lastAdded != null)
>-                                lastAdded.orderedNext = newSlot;
>-                            if (firstAdded == null)
>-                                firstAdded = newSlot;
>-                            lastAdded = newSlot;
>-                            // add new slot to hash table
>-                            if (prev == slot) {
>-                                slotsLocalRef[insertPos] = newSlot;
>-                            } else {
>-                                prev.next = newSlot;
>-                            }
>-                            // other housekeeping
>-                            slot.wasDeleted = true;
>-                            slot.value = null;
>-                            slot.name = null;
>-                            if (slot == lastAccess) {
>-                                lastAccess = REMOVED;
>-                            }
>-                            slot = newSlot;
>+
>+                        Slot newSlot;
>+
>+                        if (accessType == SLOT_MODIFY_GETTER_SETTER && !(slot instanceof GetterSlot)) {
>+                            newSlot = new GetterSlot(name, indexOrHash, slot.getAttributes());
>+                        } else if (accessType == SLOT_CONVERT_ACCESSOR_TO_DATA && (slot instanceof GetterSlot)) {
>+                            newSlot = new Slot(name, indexOrHash, slot.getAttributes());
>                         } else if (accessType == SLOT_MODIFY_CONST) {
>-                            return null;
>+                          return null;
>+                        } else {
>+                          return slot;
>                         }
>-                        return slot;
>-                    }
> 
>-                    // Check if the table is not too full before inserting.
>-                    if (4 * (count + 1) > 3 * slotsLocalRef.length) {
>-                        slotsLocalRef = new Slot[slotsLocalRef.length * 2 + 1];
>-                        copyTable(slots, slotsLocalRef, count);
>-                        slots = slotsLocalRef;
>-                        insertPos = getSlotIndex(slotsLocalRef.length,
>-                                indexOrHash);
>+                        newSlot.value = slot.value;
>+                        newSlot.next = slot.next;
>+                        // add new slot to linked list
>+                        if (lastAdded != null)
>+                            lastAdded.orderedNext = newSlot;
>+                        if (firstAdded == null)
>+                            firstAdded = newSlot;
>+                        lastAdded = newSlot;
>+                        // add new slot to hash table
>+                        if (prev == slot) {
>+                            slotsLocalRef[insertPos] = newSlot;
>+                        } else {
>+                            prev.next = newSlot;
>+                        }
>+                        // other housekeeping
>+                        slot.wasDeleted = true;
>+                        slot.value = null;
>+                        slot.name = null;
>+                        if (slot == lastAccess) {
>+                            lastAccess = REMOVED;
>+                        }
>+                        return newSlot;
>+                    } else {
>+                      // Check if the table is not too full before inserting.
>+                      if (4 * (count + 1) > 3 * slotsLocalRef.length) {
>+                          slotsLocalRef = new Slot[slotsLocalRef.length * 2 + 1];
>+                          copyTable(slots, slotsLocalRef, count);
>+                          slots = slotsLocalRef;
>+                          insertPos = getSlotIndex(slotsLocalRef.length,
>+                                  indexOrHash);
>+                      }
>                     }
>                 }
>-
>                 Slot newSlot = (accessType == SLOT_MODIFY_GETTER_SETTER
>                                 ? new GetterSlot(name, indexOrHash, 0)
>                                 : new Slot(name, indexOrHash, 0));
>diff --git a/src/org/mozilla/javascript/resources/Messages.properties b/src/org/mozilla/javascript/resources/Messages.properties
>index 545dc87..6ad9097 100644
>--- a/src/org/mozilla/javascript/resources/Messages.properties
>+++ b/src/org/mozilla/javascript/resources/Messages.properties
>@@ -667,6 +667,32 @@ msg.modify.sealed =\
> msg.modify.readonly =\
>     Cannot modify readonly property: {0}.
> 
>+msg.change.configurable.false.to.true =\
>+    Cannot change the configurable attribute of "{0}" from false to true.
>+
>+msg.change.enumerable.with.configurable.false =\
>+    Cannot change the enumerable attribute of "{0}" because configurable is false.
>+
>+msg.change.writable.false.to.true.with.configurable.false =\
>+    Cannot change the writable attribute of "{0}" from false to true because configurable is false.
>+
>+# TODO should we be using msg.modify.readonly here instead?

No, it's fine to have a separate message given that it's coming from a new ES5 function

>+msg.change.value.with.writable.false =\
>+    Cannot change the value of attribute "{0}" because writable is false.
>+
>+msg.change.getter.with.configurable.false =\
>+    Cannot change the get attribute of "{0}" because configurable is false.
>+
>+msg.change.setter.with.configurable.false =\
>+    Cannot change the set attribute of "{0}" because configurable is false.
>+
>+# TODO not sure about how descriptive this message is
>+msg.change.descriptor.type.with.configurable.false =\
>+    Cannot change type of property "{0}" because configurable is false.
>+
>+msg.not.extensible =\
>+    Cannot add properties to this object because extensible is false.
>+
> # TokenStream
> msg.missing.exponent =\
>     missing exponent
>diff --git a/testsrc/doctests/object.defineproperty.doctest b/testsrc/doctests/object.defineproperty.doctest
>new file mode 100644
>index 0000000..e40e972
>--- /dev/null
>+++ b/testsrc/doctests/object.defineproperty.doctest
>@@ -0,0 +1,169 @@
>+js> load('testsrc/doctests/util.js');
>+
>+js> Object.defineProperty
>+function defineProperty() { [native code for Object.defineProperty, arity=3] }
>+
>+js> expectTypeError(function() { Object.defineProperty() });
>+js> expectTypeError(function() { Object.defineProperty({}) });
>+js> expectTypeError(function() { Object.defineProperty({}, 'p') });
>+js> [undefined, null, true, 1, 'hello'].forEach(function(value) { 
>+  >   expectTypeError(function() { Object.defineProperty(value, 'p', {}) }) 
>+  > })
>+
>+js> Object.defineProperty({}, 'p', {value:1}).p
>+1
>+js> var obj = {}
>+js> Object.defineProperty(obj, 'a', {
>+  >   value: 1,
>+  >   enumerable: false,
>+  >   writable: false,
>+  >   configurable: false
>+  > })  
>+[object Object]
>+js> for (var p in obj) print(p); // check it has no enumerable properties
>+js> obj.a = 2; obj.a; // check that the property is not writable
>+1
>+js> delete obj.a; obj.a // check that the property is not deletable
>+1
>+
>+js> var define = Object.defineProperty;
>+js> var describe = Object.getOwnPropertyDescriptor;
>+
>+js> // when define new property with empty descriptor then default values are used for the descriptor
>+js> var obj = define({}, 'a', {});
>+js> describe(obj, 'a').toSource()
>+({writable:false, enumerable:false, configurable:false})
>+
>+js> // when define new property with data descriptor then those values are used for the descriptor
>+js> var obj = define({}, 'a', { value: 2, writable: true });
>+js> var {value:v, writable:w} = describe(obj, 'a'); [v, w].toSource();
>+[2, true]
>+js> obj.a
>+2
>+
>+js> // when define new property with accessor descriptor then those values are used for the descriptor
>+js> var obj = define({}, 'a', { get: function() { return 3; }, set: function(value) {} });
>+js> var {get:g, set:s} = describe(obj, 'a'); [g, s].toSource();
>+[(function () {return 3;}), (function (value) {})]
>+js> obj.a
>+3
>+
>+js> // when define existing property with empty descriptor then descriptor is left unchanged
>+js> var descriptor = {value:1, writable:true, enumerable:true, configurable:true};
>+js> var obj = define({},  'a', descriptor);
>+js> var obj = define(obj, 'a', {});
>+js> describe(obj, 'a').toSource()
>+({value:1, writable:true, enumerable:true, configurable:true})
>+
>+js> // when define existing property with same descriptor then descriptor is left unchanged
>+js> var descriptor = {value:1, writable:true, enumerable:true, configurable:true};
>+js> var obj = define({},  'a', descriptor);
>+js> var obj = define(obj, 'a', descriptor);
>+js> describe(obj, 'a').toSource()
>+({value:1, writable:true, enumerable:true, configurable:true})
>+
>+js> // may not change configurable from false to true
>+js> expectTypeError(function() {
>+  >   var obj = define({}, 'a', {configurable : false});
>+  >   define(obj, 'a',          {configurable : true});
>+  > });
>+
>+js> // may not change enumerable when configurable is false
>+js> expectTypeError(function() {
>+  >   var obj = define({}, 'a', {enumerable : true, configurable:false});
>+  >   define(obj, 'a',          {enumerable : false});
>+  > });
>+
>+js> // may not change writable from false to true when configurable is false
>+js> expectTypeError(function() {
>+  >   var obj = define({}, 'a', {writable : false, configurable: false});
>+  >   define(obj, 'a',          {writable : true});
>+  > });
>+
>+js> // may not change value when writable is false
>+js> expectTypeError(function() {
>+  >   var obj = define({}, 'a', {value : 1, writable:false});
>+  >   define(obj, 'a',          {value : 2});
>+  > });
>+
>+js> // may not change getter when configurable is false
>+js> expectTypeError(function() {
>+  >   var obj = define({}, 'a', {get: function() { return 1 }, configurable:false});
>+  >   define(obj, 'a',          {get: function() { return 1 }});
>+  > });
>+
>+js> // may not change setter when configurable is false
>+js> expectTypeError(function() {
>+  >   var obj = define({}, 'a', {set: function(val) {}, configurable:false});
>+  >   define(obj, 'a',          {set: function(val) {}});
>+  > });
>+
>+js> // may not change from data property to accessor property when configurable is false
>+js> expectTypeError(function() {
>+  >   var obj = define({}, 'a', {value : 1, configurable:false});
>+  >   define(obj, 'a',          {get   : function() { return 1 }});
>+  > });
>+
>+js> // may not change from accessor property to data property when configurable is false
>+js> expectTypeError(function() {
>+  >   var obj = define({}, 'a', {get   : function() { return 1 }, configurable:false});
>+  >   define(obj, 'a',          {value : 1});
>+  > });
>+
>+js> // can change writable from true to false when configurable is false
>+js> var obj = define({},  'a', {writable:true, configurable:false});
>+js> var obj = define(obj, 'a', {writable:false});
>+
>+js> // can set enumerable to the same value when configuable is false
>+js> var obj = define({},  'a', {enumerable:true, configurable:false});
>+js> var obj = define(obj, 'a', {enumerable:true});
>+
>+js> // can change from data property to accessor property when configurable is true
>+js> var obj = define({},  'a', {value : 1, configurable: true});
>+js> var obj = define(obj, 'a', {get   : function() { return 4 }});
>+js> obj.a
>+4
>+js> describe(obj, 'a').toSource()
>+({enumerable:false, configurable:true, get:(function () {return 4;})})
>+
>+js> // can change from accessor property to data property when configurable is true
>+js> var obj = define({},  'a', {get   : function() { return 2 }, configurable:true});
>+js> var obj = define(obj, 'a', {value : 5});
>+js> obj.a
>+5
>+js> describe(obj, 'a').toSource()
>+({value:5, writable:false, enumerable:false, configurable:true})
>+
>+js> // can change enumerable and writable to true when configurable is true
>+js> var obj = define({},  'a', {writable : false, enumerable : false, configurable:true});
>+js> var obj = define(obj, 'a', {writable : true,  enumerable : true, configurable:true});
>+
>+js> // can change the value if writable is true
>+js> var obj = define({},  'a', {value:6, writable:true})
>+js> obj.a
>+6
>+js> var obj = define(obj, 'a', {value:7})
>+js> obj.a
>+7
>+
>+js> // defining a new property should fail loudly when object is not extensible
>+js> var obj = Object.preventExtensions({});
>+js> expectTypeError(function() { define(obj, 'a', {value:1}) })
>+
>+js> // defining new property should succeed when object is extensible
>+js> var obj = {}
>+js> Object.isExtensible(obj);
>+true
>+js> obj.a = 8; obj.a
>+8
>+
>+js> // changing existing property should succeed when object is not extensible
>+js> var obj = define({},  'a', {value:1, writable:true});
>+js> var obj = Object.preventExtensions(obj);
>+js> obj.a = 9; obj.a
>+9
>+
>+js> // defined getters and setters must be functions
>+js> expectTypeError(function() { define({}, 'a', {get:1}); })
>+js> expectTypeError(function() { define({}, 'a', {set:1}); })
>+
>diff --git a/testsrc/doctests/object.extensible.doctest b/testsrc/doctests/object.extensible.doctest
>new file mode 100644
>index 0000000..841511d
>--- /dev/null
>+++ b/testsrc/doctests/object.extensible.doctest
>@@ -0,0 +1,30 @@
>+js> load('testsrc/doctests/util.js');
>+
>+js> Object.isExtensible;
>+function isExtensible() { [native code for Object.isExtensible, arity=1] }
>+js> expectTypeError(function() { Object.isExtensible() });
>+js> [undefined, null, true, 1, 'hello'].forEach(function(value) { 
>+  >   expectTypeError(function() { Object.isExtensible(value) }) 
>+  > })
>+
>+js> Object.preventExtensions;
>+function preventExtensions() { [native code for Object.preventExtensions, arity=1] }
>+js> expectTypeError(function() { Object.preventExtensions() });
>+js> [undefined, null, true, 1, 'hello'].forEach(function(value) { 
>+  >   expectTypeError(function() { Object.preventExtensions(value) }) 
>+  > })
>+
>+js> var x = {};
>+js> Object.isExtensible(x);
>+true
>+js> var y = Object.preventExtensions(x);
>+js> y === x;
>+true
>+js> Object.isExtensible(x);
>+false
>+
>+js> x.a = 1; x.a
>+js>
>+
>+js> x.__defineGetter__('b', function() { return 1 }); x.b
>+js>

Comment 17

[rspeyer]

(In reply to comment #16)

...snip...

> >+        if (isGenericDescriptor(desc)) {
> >+          // no further validation required
> >+        } else if (isDataDescriptor(desc) && isDataDescriptor(current)) {
> >+          if (Boolean.FALSE.equals(getProperty(current, "writable"))) {
> >+            if (hasProperty(desc, "writable") && ScriptRuntime.toBoolean(getProperty(desc, "writable")))
> >+              throw ScriptRuntime.typeError1("msg.change.writable.false.to.true.with.configurable.false", name);
> >+
> >+            if (hasProperty(desc, "value") && !ScriptRuntime.shallowEq(getProperty(current, "value"), getProperty(desc, "value"))) 
> >+              throw ScriptRuntime.typeError1("msg.change.value.with.writable.false", name);
> >+          }
> >+        } else if (isAccessorDescriptor(desc) && isAccessorDescriptor(current)) {
> >+          if (hasProperty(desc, "set") && !ScriptRuntime.shallowEq(getProperty(current, "set"), getProperty(desc, "set")))
> >+            throw ScriptRuntime.typeError1("msg.change.setter.with.configurable.false", name);
> >+
> >+          if (hasProperty(desc, "get") && !ScriptRuntime.shallowEq(getProperty(current, "get"), getProperty(desc, "get"))) 
> >+            throw ScriptRuntime.typeError1("msg.change.getter.with.configurable.false", name);
> >+        } else {
> >+          throw ScriptRuntime.typeError1("msg.change.descriptor.type.with.configurable.false", name);
> 
> What does this correspond to in the spec? I'm not sure where this comes from.
> 

This comes from to 8.10.12 Step 9.a

...snip...

Comment 18

[Norris Boyd]

(In reply to comment #17)
> (In reply to comment #16)
> 
> ...snip...
> 
> > >+        if (isGenericDescriptor(desc)) {
> > >+          // no further validation required
> > >+        } else if (isDataDescriptor(desc) && isDataDescriptor(current)) {
> > >+          if (Boolean.FALSE.equals(getProperty(current, "writable"))) {
> > >+            if (hasProperty(desc, "writable") && ScriptRuntime.toBoolean(getProperty(desc, "writable")))
> > >+              throw ScriptRuntime.typeError1("msg.change.writable.false.to.true.with.configurable.false", name);
> > >+
> > >+            if (hasProperty(desc, "value") && !ScriptRuntime.shallowEq(getProperty(current, "value"), getProperty(desc, "value"))) 
> > >+              throw ScriptRuntime.typeError1("msg.change.value.with.writable.false", name);
> > >+          }
> > >+        } else if (isAccessorDescriptor(desc) && isAccessorDescriptor(current)) {
> > >+          if (hasProperty(desc, "set") && !ScriptRuntime.shallowEq(getProperty(current, "set"), getProperty(desc, "set")))
> > >+            throw ScriptRuntime.typeError1("msg.change.setter.with.configurable.false", name);
> > >+
> > >+          if (hasProperty(desc, "get") && !ScriptRuntime.shallowEq(getProperty(current, "get"), getProperty(desc, "get"))) 
> > >+            throw ScriptRuntime.typeError1("msg.change.getter.with.configurable.false", name);
> > >+        } else {
> > >+          throw ScriptRuntime.typeError1("msg.change.descriptor.type.with.configurable.false", name);
> > 
> > What does this correspond to in the spec? I'm not sure where this comes from.
> > 
> 
> This comes from to 8.10.12 Step 9.a
> 
> ...snip...

Ah, thanks. The "type" confused me there as I was thinking runtime types. Perhaps a better error message would be something like "can't add or remove getters or setters because configurable is false" (Does that error message cover all the cases?)

Comment 19

[rspeyer]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > 
> > ...snip...
> > 
> > > >+        if (isGenericDescriptor(desc)) {
> > > >+          // no further validation required
> > > >+        } else if (isDataDescriptor(desc) && isDataDescriptor(current)) {
> > > >+          if (Boolean.FALSE.equals(getProperty(current, "writable"))) {
> > > >+            if (hasProperty(desc, "writable") && ScriptRuntime.toBoolean(getProperty(desc, "writable")))
> > > >+              throw ScriptRuntime.typeError1("msg.change.writable.false.to.true.with.configurable.false", name);
> > > >+
> > > >+            if (hasProperty(desc, "value") && !ScriptRuntime.shallowEq(getProperty(current, "value"), getProperty(desc, "value"))) 
> > > >+              throw ScriptRuntime.typeError1("msg.change.value.with.writable.false", name);
> > > >+          }
> > > >+        } else if (isAccessorDescriptor(desc) && isAccessorDescriptor(current)) {
> > > >+          if (hasProperty(desc, "set") && !ScriptRuntime.shallowEq(getProperty(current, "set"), getProperty(desc, "set")))
> > > >+            throw ScriptRuntime.typeError1("msg.change.setter.with.configurable.false", name);
> > > >+
> > > >+          if (hasProperty(desc, "get") && !ScriptRuntime.shallowEq(getProperty(current, "get"), getProperty(desc, "get"))) 
> > > >+            throw ScriptRuntime.typeError1("msg.change.getter.with.configurable.false", name);
> > > >+        } else {
> > > >+          throw ScriptRuntime.typeError1("msg.change.descriptor.type.with.configurable.false", name);
> > > 
> > > What does this correspond to in the spec? I'm not sure where this comes from.
> > > 
> > 
> > This comes from to 8.10.12 Step 9.a
> > 
> > ...snip...
> 
> Ah, thanks. The "type" confused me there as I was thinking runtime types.
> Perhaps a better error message would be something like "can't add or remove
> getters or setters because configurable is false" (Does that error message
> cover all the cases?)

Well in principle they could just be defining a writable attribute to an existing accessor property, e.g.

js> var obj = Object.defineProperty({}, 'a', {configurable:false, get:function() { return 1 }})
js> Object.defineProperty(obj, 'a', {writable:false})

What about something more explicit:
'Cannot change property "a" from an accessor property to a data property because configurable is false" 
for the accessor -> data case, and 
'Cannot change property "a" from a data property to an accessor property because configurable is false" 
for the other?

Or even:
'Cannot set value or writable on property "a" because it is currently a getter/setter property and configurable is false'
and
'Cannot set getter or setter on property "a" because it is currently a value/writable property and configurable is false'
?

Comment 20

[rspeyer]

Attachment: updates from comment 16, 18, 19

- replaced calls to hasProperty, with checks for for NOT_FOUND
- replaced calls to getProperty(current, ...) with current.get(...)
- didn't replace current.get() with direct slot attribute checks as that seemed to require duplication and reduced readability of the code, is it worthwhile for efficiency?
- made error messages more descriptive for changing property data->accessor or accessor->data when configurable is false

Comment 21

[rspeyer]

Attachment: updates from comment 16, 18, 19 again

- same as previous but also removed hasProperty/getProperty pairing from applyDescriptorToAttributeBitset
- have not attempted to optimise calls to isAccessorProperty/isDataProperty/isGenericProperty. worthwhile?

Comment 22

[Norris Boyd]

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > (In reply to comment #16)
> > > 
> > > ...snip...
> > > 
> > > > >+        if (isGenericDescriptor(desc)) {
> > > > >+          // no further validation required
> > > > >+        } else if (isDataDescriptor(desc) && isDataDescriptor(current)) {
> > > > >+          if (Boolean.FALSE.equals(getProperty(current, "writable"))) {
> > > > >+            if (hasProperty(desc, "writable") && ScriptRuntime.toBoolean(getProperty(desc, "writable")))
> > > > >+              throw ScriptRuntime.typeError1("msg.change.writable.false.to.true.with.configurable.false", name);
> > > > >+
> > > > >+            if (hasProperty(desc, "value") && !ScriptRuntime.shallowEq(getProperty(current, "value"), getProperty(desc, "value"))) 
> > > > >+              throw ScriptRuntime.typeError1("msg.change.value.with.writable.false", name);
> > > > >+          }
> > > > >+        } else if (isAccessorDescriptor(desc) && isAccessorDescriptor(current)) {
> > > > >+          if (hasProperty(desc, "set") && !ScriptRuntime.shallowEq(getProperty(current, "set"), getProperty(desc, "set")))
> > > > >+            throw ScriptRuntime.typeError1("msg.change.setter.with.configurable.false", name);
> > > > >+
> > > > >+          if (hasProperty(desc, "get") && !ScriptRuntime.shallowEq(getProperty(current, "get"), getProperty(desc, "get"))) 
> > > > >+            throw ScriptRuntime.typeError1("msg.change.getter.with.configurable.false", name);
> > > > >+        } else {
> > > > >+          throw ScriptRuntime.typeError1("msg.change.descriptor.type.with.configurable.false", name);
> > > > 
> > > > What does this correspond to in the spec? I'm not sure where this comes from.
> > > > 
> > > 
> > > This comes from to 8.10.12 Step 9.a
> > > 
> > > ...snip...
> > 
> > Ah, thanks. The "type" confused me there as I was thinking runtime types.
> > Perhaps a better error message would be something like "can't add or remove
> > getters or setters because configurable is false" (Does that error message
> > cover all the cases?)
> 
> Well in principle they could just be defining a writable attribute to an
> existing accessor property, e.g.
> 
> js> var obj = Object.defineProperty({}, 'a', {configurable:false,
> get:function() { return 1 }})
> js> Object.defineProperty(obj, 'a', {writable:false})
> 
> What about something more explicit:
> 'Cannot change property "a" from an accessor property to a data property
> because configurable is false" 
> for the accessor -> data case, and 
> 'Cannot change property "a" from a data property to an accessor property
> because configurable is false" 
> for the other?

Yes, I like these, thanks.

> 
> Or even:
> 'Cannot set value or writable on property "a" because it is currently a
> getter/setter property and configurable is false'
> and
> 'Cannot set getter or setter on property "a" because it is currently a
> value/writable property and configurable is false'
> ?

Comment 23

[Norris Boyd]

Submitted:

Checking in src/org/mozilla/javascript/NativeObject.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/NativeObject.java,v  <--  NativeObject.java
new revision: 1.50; previous revision: 1.49
done
Checking in src/org/mozilla/javascript/ScriptableObject.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/ScriptableObject.java,v  <--  ScriptableObject.java
new revision: 1.149; previous revision: 1.148
done
Checking in src/org/mozilla/javascript/resources/Messages.properties;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/resources/Messages.properties,v  <--  Messages.properties
new revision: 1.94; previous revision: 1.93
done
RCS file: /cvsroot/mozilla/js/rhino/testsrc/doctests/object.defineproperty.doctest,v
done
Checking in testsrc/doctests/object.defineproperty.doctest;
/cvsroot/mozilla/js/rhino/testsrc/doctests/object.defineproperty.doctest,v  <--  object.defineproperty.doctest
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/rhino/testsrc/doctests/object.extensible.doctest,vdone
Checking in testsrc/doctests/object.extensible.doctest;
/cvsroot/mozilla/js/rhino/testsrc/doctests/object.extensible.doctest,v  <--  object.extensible.doctest
initial revision: 1.1
done

Comment 24

[rspeyer]

Attachment: patch for Object.create and Object.defineProperties

- added Object.defineProperties and Object.defineProperties
- moved all validation of new properties into a separated method so it could be done independently
- small change to ScriptableObject#getOwnPropertyDescriptor so that if the current parent scope is null, it uses this as descriptor's scope
- also added small change to DoctestsTest so that it also prints out failing tests

Comment 25

[Norris Boyd]

Comment on attachment 381889 [details] [diff] [review]
patch for Object.create and Object.defineProperties

>diff --git a/src/org/mozilla/javascript/NativeObject.java b/src/org/mozilla/javascript/NativeObject.java
>index f9f3bdd..4be509e 100644
>--- a/src/org/mozilla/javascript/NativeObject.java
>+++ b/src/org/mozilla/javascript/NativeObject.java
>@@ -40,6 +40,8 @@
>  * ***** END LICENSE BLOCK ***** */
> 
> package org.mozilla.javascript;
>+import java.util.Map;
>+import java.util.LinkedHashMap;
> 
> /**
>  * This class implements the Object native object.
>@@ -87,6 +89,10 @@ public class NativeObject extends IdScriptableObject
>                 "isExtensible", 1);
>         addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_preventExtensions,
>                 "preventExtensions", 1);
>+        addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_defineProperties,
>+                "defineProperties", 2);
>+        addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_create,
>+                "create", 2);
>         super.fillConstructorProperties(ctor);
>     }
> 
>@@ -338,22 +344,36 @@ public class NativeObject extends IdScriptableObject
>                 obj.preventExtensions();
>                 return obj;
>               }
>-
>-          default:
>-            throw new IllegalArgumentException(String.valueOf(id));
>+          case ConstructorId_defineProperties:
>+              {
>+                Object arg = args.length < 1 ? Undefined.instance : args[0];
>+                ScriptableObject obj = ensureScriptableObject(arg);
>+                Object propsObj = args.length < 2 ? Undefined.instance : args[1];
>+                Scriptable props = Context.toObject(propsObj, getParentScope());
>+                obj.defineOwnProperties(ensureScriptableObject(props));
>+                return obj;
>         }
>+          case ConstructorId_create:
>+              {
>+                Object arg = args.length < 1 ? Undefined.instance : args[0];
>+                Scriptable obj = ensureScriptable(arg);
>+                Object propsObj = args.length < 2 ? Undefined.instance : args[1];
>+
>+                ScriptableObject newObject = new NativeObject();
>+                newObject.setParentScope(this.getParentScope());
>+                newObject.setPrototype(obj);
>+
>+                if (propsObj != Undefined.instance) {

Seems clearer to just check against args.length and define propsObj inside the if.

>+                    Scriptable props = Context.toObject(propsObj, getParentScope());
>+                    newObject.defineOwnProperties(ensureScriptableObject(props));
>     }
> 
>-    private Scriptable ensureScriptable(Object arg) {
>-      if ( !(arg instanceof Scriptable) )
>-        throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.typeof(arg));
>-      return (Scriptable) arg;
>+                return newObject;
>     }
> 
>-    private ScriptableObject ensureScriptableObject(Object arg) {
>-      if ( !(arg instanceof ScriptableObject) )
>-        throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.typeof(arg));
>-      return (ScriptableObject) arg;
>+          default:
>+            throw new IllegalArgumentException(String.valueOf(id));
>+        }
>     }
> 
> // #string_id_map#
>@@ -405,6 +425,8 @@ public class NativeObject extends IdScriptableObject
>         ConstructorId_defineProperty = -5,
>         ConstructorId_isExtensible = -6,
>         ConstructorId_preventExtensions = -7,
>+        ConstructorId_defineProperties= -8,
>+        ConstructorId_create = -9,
> 
>         Id_constructor           = 1,
>         Id_toString              = 2,
>diff --git a/src/org/mozilla/javascript/ScriptableObject.java b/src/org/mozilla/javascript/ScriptableObject.java
>index 9ac6840..1021cdf 100644
>--- a/src/org/mozilla/javascript/ScriptableObject.java
>+++ b/src/org/mozilla/javascript/ScriptableObject.java
>@@ -1496,6 +1496,21 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>         gslot.setter = setterBox;
>     }
> 
>+    public void defineOwnProperties(ScriptableObject props) {
>+        Object[] ids = props.getIds();
>+        for (Object id : ids) {
>+            String name = ScriptRuntime.toString(id);
>+            Object descObj = props.get(id);
>+            ScriptableObject desc = ensureScriptableObject(descObj);
>+            checkValidPropertyDefinition(getSlot(name, 0, SLOT_QUERY), desc);

Hmm. Does getSlot actually need "name" here rather than "id"? In other words, does this work with integer "names"?

>+        }
>+        for (Object id : ids) {
>+            String name = ScriptRuntime.toString(id);
>+            ScriptableObject desc = (ScriptableObject) props.get(id);
>+            defineOwnProperty(name, desc, false);
>+        }
>+    }
>+
>     /**
>      * Defines a property on an object
>      *
>@@ -1505,15 +1520,20 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>      * @param desc the new property descriptor, as described in 8.6.1
>      */
>     public void defineOwnProperty(String name, ScriptableObject desc) {
>+      defineOwnProperty(name, desc, true);
>+    }
>+
>+    private void defineOwnProperty(String name, ScriptableObject desc, boolean checkValid) {
>       Slot slot = getSlot(name, 0, SLOT_QUERY);
>       final int attributes;
> 
>+      if (checkValid)
>+        checkValidPropertyDefinition(slot, desc);
>+      
>       if (slot == null) { // new slot
>-        if (!isExtensible()) throw ScriptRuntime.typeError("msg.not.extensible");
>         slot = getSlot(name, 0, SLOT_MODIFY);
>         attributes = applyDescriptorToAttributeBitset(DONTENUM|READONLY|PERMANENT, desc);
>       } else {
>-        checkLegalPropertyRedefinition(name, desc);
>         attributes = applyDescriptorToAttributeBitset(getAttributes(name), desc);
>       }
> 
>@@ -1529,20 +1549,12 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
> 
>         Object getter = getProperty(desc, "get");
>         if (getter != NOT_FOUND) {
>-          if ( !(getter instanceof Callable) ) {
>-            throw ScriptRuntime.notFunctionError(getter);
>-          } else {
>             gslot.getter = getter;
>           }
>-        }
>         Object setter = getProperty(desc, "set");
>         if (setter != NOT_FOUND) {
>-          if ( !(setter instanceof Callable) ) {
>-            throw ScriptRuntime.notFunctionError(setter);
>-          } else {
>             gslot.setter = setter;
>           }
>-        }
> 
>         gslot.value = Undefined.instance;
>         gslot.setAttributes(attributes);
>@@ -1559,7 +1571,20 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>       }
>     }
> 
>-    private void checkLegalPropertyRedefinition(String name, ScriptableObject desc) {
>+    private void checkValidPropertyDefinition(Slot slot, ScriptableObject desc) {
>+      Object getter = getProperty(desc, "get");
>+      if (getter != NOT_FOUND && !(getter instanceof Callable) ) {
>+        throw ScriptRuntime.notFunctionError(getter);
>+      }
>+      Object setter = getProperty(desc, "set");
>+      if (setter != NOT_FOUND && !(setter instanceof Callable) ) {
>+        throw ScriptRuntime.notFunctionError(setter);
>+      }
>+
>+      if (slot == null) { // new property
>+        if (!isExtensible()) throw ScriptRuntime.typeError("msg.not.extensible");
>+      } else {
>+        String name = slot.name;
>       ScriptableObject current = getOwnPropertyDescriptor(Context.getContext(), name);
>       if (Boolean.FALSE.equals(current.get("configurable")) ) {
>         if (Boolean.TRUE.equals(tryBoolean(getProperty(desc, "configurable")))) 
>@@ -1578,10 +1603,10 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>               throw ScriptRuntime.typeError1("msg.change.value.with.writable.false", name);
>           }
>         } else if (isAccessorDescriptor(desc) && isAccessorDescriptor(current)) {
>-          if (changes(current.get("set"), getProperty(desc, "set")))
>+            if (changes(current.get("set"), setter))
>             throw ScriptRuntime.typeError1("msg.change.setter.with.configurable.false", name);
> 
>-          if (changes(current.get("get"), getProperty(desc, "get"))) 
>+            if (changes(current.get("get"), getter)) 
>             throw ScriptRuntime.typeError1("msg.change.getter.with.configurable.false", name);
>         } else {
>           if (isDataDescriptor(current))
>@@ -1591,6 +1616,7 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>         }
>       }
>     }
>+    }
> 
>     private static Object tryBoolean(Object value) {
>       if (value == NOT_FOUND) return NOT_FOUND;
>@@ -1636,6 +1662,18 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>       return !isDataDescriptor(desc) && !isAccessorDescriptor(desc);
>     }
> 
>+    protected Scriptable ensureScriptable(Object arg) {
>+      if ( !(arg instanceof Scriptable) )
>+        throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.typeof(arg));
>+      return (Scriptable) arg;
>+    }
>+
>+    protected ScriptableObject ensureScriptableObject(Object arg) {
>+      if ( !(arg instanceof ScriptableObject) )
>+        throw ScriptRuntime.typeError1("msg.arg.not.object", ScriptRuntime.typeof(arg));
>+      return (ScriptableObject) arg;
>+    }
>+
>     /**
>      * Search for names in a class, adding the resulting methods
>      * as properties.
>@@ -2729,7 +2767,8 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
> 
>     protected ScriptableObject getOwnPropertyDescriptor(Context cx, String name) {
>       Slot slot = getSlot(name, 0, SLOT_QUERY);
>-      return (slot == null) ? null : slot.getPropertyDescriptor(cx, getParentScope());
>+      Scriptable scope = getParentScope();
>+      return (slot == null) ? null : slot.getPropertyDescriptor(cx, (scope == null ? this : scope));
>     }
> 
>     // Methods and classes to implement java.util.Map interface
>diff --git a/testsrc/doctests/object.create.doctest b/testsrc/doctests/object.create.doctest
>new file mode 100644
>index 0000000..3ba51d5
>--- /dev/null
>+++ b/testsrc/doctests/object.create.doctest
>@@ -0,0 +1,30 @@
>+js> load('testsrc/doctests/util.js');
>+
>+js> Object.create;
>+function create() { [native code for Object.create, arity=2] }
>+
>+js> expectTypeError(function() { Object.create() });
>+js> [undefined, null, true, 1, 'hello'].forEach(function(value) { 
>+  >   expectTypeError(function() { Object.create(value) }) 
>+  > })
>+js> expectTypeError(function() { Object.create({}, null) }) 
>+
>+js> var obj = Object.create({});
>+js> var obj = Object.create({}, {});
>+js> var obj = Object.create({}, undefined);
>+
>+js> var orig = {}
>+js> var next = Object.create(orig);
>+js> Object.getPrototypeOf(next) === orig;
>+true
>+
>+js> var obj = Object.create({}, {a: {value:1}, b: {value:2}});
>+js> [obj.a, obj.b].toSource();
>+[1, 2]
>+
>+js> var orig = {a:1};
>+js> var obj = Object.create(orig, {a:{value:2}, b:{value:3}});
>+js> [obj.a, obj.b].toSource()
>+[2, 3]
>+
>+js> expectTypeError(function() { Object.create({}, {b: {value:1}, c:1}) });
>diff --git a/testsrc/doctests/object.defineproperties.doctest b/testsrc/doctests/object.defineproperties.doctest
>new file mode 100644
>index 0000000..0541f75
>--- /dev/null
>+++ b/testsrc/doctests/object.defineproperties.doctest
>@@ -0,0 +1,39 @@
>+js> load('testsrc/doctests/util.js');
>+
>+js> Object.defineProperties
>+function defineProperties() { [native code for Object.defineProperties, arity=2] }
>+
>+js> expectTypeError(function() { Object.defineProperties() });
>+js> expectTypeError(function() { Object.defineProperties({}) });
>+js> [undefined, null, true, 1, 'hello'].forEach(function(value) { 
>+  >   expectTypeError(function() { Object.defineProperties(value, {}) }) 
>+  > })
>+
>+js> Object.defineProperties({}, {p: {value:1}}).p
>+1
>+
>+js> var obj = Object.defineProperties({}, {a: {value:1}, b: {value:2}});
>+js> [obj.a, obj.b].toSource();
>+[1, 2]
>+
>+js> Object.defineProperties({}, {'wierd name': {value:1}})['wierd name']
>+1
>+
>+js> Object.defineProperties({}, {}).toSource()
>+({})
>+
>+js> var obj = {a:1};
>+js> var obj = Object.defineProperties(obj, {a:{value:2}, b:{value:3}});
>+js> [obj.a, obj.b].toSource()
>+[2, 3]
>+
>+js> expectTypeError(function() { Object.defineProperties({}, {a: null}) })
>+js> expectTypeError(function() { Object.defineProperties({}, {a: 1}) })
>+js> expectTypeError(function() { Object.defineProperties({}, {a: {get: 1}}) })
>+
>+js> var obj = {a:1}
>+js> expectTypeError(function() { 
>+  >   obj = Object.defineProperties(obj, {b: {value:1}, c:1});
>+  > });
>+js> obj.b
>+js>
>diff --git a/testsrc/org/mozilla/javascript/tests/DoctestsTest.java b/testsrc/org/mozilla/javascript/tests/DoctestsTest.java
>index f03343f..8081253 100644
>--- a/testsrc/org/mozilla/javascript/tests/DoctestsTest.java
>+++ b/testsrc/org/mozilla/javascript/tests/DoctestsTest.java
>@@ -93,6 +93,9 @@ public class DoctestsTest {
>             System.out.println(name + "(" + optimizationLevel + "): " +
>                     testsPassed + " passed.");
>             assertTrue(testsPassed > 0);
>+        } catch (Exception ex) {
>+          System.out.println(name + "(" + optimizationLevel + "): FAILED due to "+ex);
>+          throw ex;
>         } finally {
>             Context.exit();
>         }

Comment 26

[rspeyer]

Attachment: create, defineProperty and defineProperties, and proper handling of numerical ids

fixed methods to handle numerical property ids correctly

Comment 27

[Norris Boyd]

Submitted to CVS HEAD. Thanks!

Comment 28

[rspeyer]

Attachment: patch for seal, isSealed, freeze and isFrozen

Implements Object.seal, Object.isSealed, Object.freeze and Object.isFrozen

Comment 29

[Norris Boyd]

Comment on attachment 383057 [details] [diff] [review]
patch for seal, isSealed, freeze and isFrozen

>diff --git a/src/org/mozilla/javascript/NativeObject.java b/src/org/mozilla/javascript/NativeObject.java
>index a46af60..325395f 100644
>--- a/src/org/mozilla/javascript/NativeObject.java
>+++ b/src/org/mozilla/javascript/NativeObject.java
>@@ -93,6 +93,14 @@ public class NativeObject extends IdScriptableObject
>                 "defineProperties", 2);
>         addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_create,
>                 "create", 2);
>+        addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_isSealed,
>+                "isSealed", 1);
>+        addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_isFrozen,
>+                "isFrozen", 1);
>+        addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_seal,
>+                "seal", 1);
>+        addIdFunctionProperty(ctor, OBJECT_TAG, ConstructorId_freeze,
>+                "freeze", 1);
>         super.fillConstructorProperties(ctor);
>     }
> 
>@@ -364,10 +372,73 @@ public class NativeObject extends IdScriptableObject
>                 if (args.length > 1 && args[1] != Undefined.instance) {
>                   Scriptable props = Context.toObject(args[1], getParentScope());
>                   newObject.defineOwnProperties(cx, ensureScriptableObject(props));
>-    }
>+                }
> 
>                 return newObject;
>-    }
>+              }
>+
>+          case ConstructorId_isSealed:
>+              {
>+                Object arg = args.length < 1 ? Undefined.instance : args[0];
>+                ScriptableObject obj = ensureScriptableObject(arg);
>+
>+                for (Object name: obj.getAllIds()) {
>+                  Object configurable = obj.getOwnPropertyDescriptor(cx, name).get("configurable");
>+                  if (Boolean.TRUE.equals(configurable)) 
>+                    return false;
>+                }
>+
>+                return !obj.isExtensible();

How about testing this first since it's cheap to test?

>+              }
>+          case ConstructorId_isFrozen:
>+              {
>+                Object arg = args.length < 1 ? Undefined.instance : args[0];
>+                ScriptableObject obj = ensureScriptableObject(arg);
>+
>+                for (Object name: obj.getAllIds()) {
>+                  ScriptableObject desc = obj.getOwnPropertyDescriptor(cx, name);
>+                  if (Boolean.TRUE.equals(desc.get("configurable"))) 
>+                    return false;
>+                  if (isDataDescriptor(desc) && Boolean.TRUE.equals(desc.get("writable")))
>+                    return false;
>+                }
>+
>+                return !obj.isExtensible();

Here too.

>+              }
>+          case ConstructorId_seal:
>+              {
>+                Object arg = args.length < 1 ? Undefined.instance : args[0];
>+                ScriptableObject obj = ensureScriptableObject(arg);
>+
>+                for (Object name: obj.getAllIds()) {
>+                  ScriptableObject desc = obj.getOwnPropertyDescriptor(cx, name);
>+                  if (Boolean.TRUE.equals(desc.get("configurable"))) {
>+                    desc.put("configurable", desc, false);
>+                    obj.defineOwnProperty(cx, name, desc); 
>+                  }
>+                }
>+                obj.preventExtensions();

The spec has wording "The above algorithm is specified as a set of sequential steps that include the possibility of an exception being thrown at various intermediate points. Rather than failing after a partial update of O, this function must be implemented such that it either atomically completes all property updates successfully or fails without making any update to the properties of object O." Do you know where the exceptions could be thrown, and do you avoid partial updates?

Same question for freeze.

>+
>+                return obj;
>+              }
>+          case ConstructorId_freeze:
>+              {
>+                Object arg = args.length < 1 ? Undefined.instance : args[0];
>+                ScriptableObject obj = ensureScriptableObject(arg);
>+
>+                for (Object name: obj.getAllIds()) {
>+                  ScriptableObject desc = obj.getOwnPropertyDescriptor(cx, name);
>+                  if (isDataDescriptor(desc) && Boolean.TRUE.equals(desc.get("writable")))
>+                    desc.put("writable", desc, false);
>+                  if (Boolean.TRUE.equals(desc.get("configurable")))
>+                    desc.put("configurable", desc, false);
>+                  obj.defineOwnProperty(cx, name, desc);
>+                }
>+                obj.preventExtensions();
>+
>+                return obj;
>+              }
>+
> 
>           default:
>             throw new IllegalArgumentException(String.valueOf(id));
>@@ -425,6 +496,10 @@ public class NativeObject extends IdScriptableObject
>         ConstructorId_preventExtensions = -7,
>         ConstructorId_defineProperties= -8,
>         ConstructorId_create = -9,
>+        ConstructorId_isSealed = -10,
>+        ConstructorId_isFrozen = -11,
>+        ConstructorId_seal = -12,
>+        ConstructorId_freeze = -13,
> 
>         Id_constructor           = 1,
>         Id_toString              = 2,
>diff --git a/src/org/mozilla/javascript/ScriptableObject.java b/src/org/mozilla/javascript/ScriptableObject.java
>index 15fe398..31ab96c 100644
>--- a/src/org/mozilla/javascript/ScriptableObject.java
>+++ b/src/org/mozilla/javascript/ScriptableObject.java
>@@ -1653,15 +1653,15 @@ public abstract class ScriptableObject implements Scriptable, Serializable,
>       return attributes;
>     }
> 
>-    private boolean isDataDescriptor(ScriptableObject desc) {
>+    protected boolean isDataDescriptor(ScriptableObject desc) {
>       return hasProperty(desc, "value") || hasProperty(desc, "writable");
>     }
>-    
>-    private boolean isAccessorDescriptor(ScriptableObject desc) {
>+
>+    protected boolean isAccessorDescriptor(ScriptableObject desc) {
>       return hasProperty(desc, "get") || hasProperty(desc, "set");
>     }
>-    
>-    private boolean isGenericDescriptor(ScriptableObject desc) {
>+
>+    protected boolean isGenericDescriptor(ScriptableObject desc) {
>       return !isDataDescriptor(desc) && !isAccessorDescriptor(desc);
>     }
> 
>diff --git a/testsrc/doctests/object.freeze.doctest b/testsrc/doctests/object.freeze.doctest
>new file mode 100644
>index 0000000..db3f023
>--- /dev/null
>+++ b/testsrc/doctests/object.freeze.doctest
>@@ -0,0 +1,27 @@
>+js> load('testsrc/doctests/util.js');
>+
>+js> Object.freeze;
>+function freeze() { [native code for Object.freeze, arity=1] }
>+
>+js> [undefined, null, true, 1, 'hello'].forEach(function(value) { 
>+  >   expectTypeError(function() { Object.freeze(value) }) 
>+  > })
>+js> expectTypeError(function() { Object.freeze() })
>+
>+js> var x = {}
>+js> var y = Object.freeze(x)
>+js> x === y
>+true
>+
>+js> var obj = Object.defineProperty({}, 'a', {configurable:true, writable:true})
>+js> var _ = Object.freeze(obj)
>+js> var a = Object.getOwnPropertyDescriptor(obj, 'a');
>+js> a.configurable
>+false
>+js> a.writable
>+false
>+js> Object.isExtensible(obj)
>+false
>+
>+js> Object.isFrozen(obj)
>+true
>diff --git a/testsrc/doctests/object.isfrozen.doctest b/testsrc/doctests/object.isfrozen.doctest
>new file mode 100644
>index 0000000..b2206f9
>--- /dev/null
>+++ b/testsrc/doctests/object.isfrozen.doctest
>@@ -0,0 +1,37 @@
>+js> load('testsrc/doctests/util.js');
>+
>+js> Object.isFrozen
>+function isFrozen() { [native code for Object.isFrozen, arity=1] }
>+
>+js> expectTypeError(function() { Object.isFrozen() });
>+js> [undefined, null, true, 1, 'hello'].forEach(function(value) { 
>+  >   expectTypeError(function() { Object.isFrozen(value) }) 
>+  > })
>+
>+js> Object.isFrozen({})
>+false
>+
>+js> var obj = Object.preventExtensions({});
>+js> Object.isFrozen(obj);
>+true
>+
>+js> var obj = Object.defineProperty({}, 'a', {configurable:true, writable:false})
>+js> var _ = Object.preventExtensions(obj);
>+js> Object.isFrozen(obj);
>+false
>+
>+js> var obj = Object.defineProperty({}, 'a', {configurable:false, writable:true})
>+js> var _ = Object.preventExtensions(obj);
>+js> Object.isFrozen(obj);
>+false
>+
>+js> var obj = Object.defineProperty({}, 'a', {configurable:false, writable:false})
>+js> var _ = Object.preventExtensions(obj);
>+js> Object.isFrozen(obj);
>+true
>+
>+js> var obj = Object.defineProperty({}, 'a', {configurable:false, set: function(){} })
>+js> var _ = Object.preventExtensions(obj);
>+js> Object.isFrozen(obj);
>+true
>+
>diff --git a/testsrc/doctests/object.issealed.doctest b/testsrc/doctests/object.issealed.doctest
>new file mode 100644
>index 0000000..275b4ac
>--- /dev/null
>+++ b/testsrc/doctests/object.issealed.doctest
>@@ -0,0 +1,26 @@
>+js> load('testsrc/doctests/util.js');
>+
>+js> Object.isSealed
>+function isSealed() { [native code for Object.isSealed, arity=1] }
>+
>+js> expectTypeError(function() { Object.isSealed() });
>+js> [undefined, null, true, 1, 'hello'].forEach(function(value) { 
>+  >   expectTypeError(function() { Object.isSealed(value) }) 
>+  > })
>+
>+js> Object.isSealed({})
>+false
>+
>+js> var obj = Object.preventExtensions({});
>+js> Object.isSealed(obj);
>+true
>+
>+js> var obj = Object.defineProperty({}, 'a', {configurable:false});
>+js> var _ = Object.preventExtensions(obj);
>+js> Object.isSealed(obj);
>+true
>+
>+js> var obj = Object.defineProperty({}, 'a', {configurable:true});
>+js> var _ = Object.preventExtensions(obj);
>+js> Object.isSealed(obj);
>+false
>diff --git a/testsrc/doctests/object.seal.doctest b/testsrc/doctests/object.seal.doctest
>new file mode 100644
>index 0000000..2f50804
>--- /dev/null
>+++ b/testsrc/doctests/object.seal.doctest
>@@ -0,0 +1,24 @@
>+js> load('testsrc/doctests/util.js');
>+
>+js> Object.seal;
>+function seal() { [native code for Object.seal, arity=1] }
>+
>+js> [undefined, null, true, 1, 'hello'].forEach(function(value) { 
>+  >   expectTypeError(function() { Object.seal(value) }) 
>+  > })
>+js> expectTypeError(function() { Object.seal() })
>+
>+js> var x = {}
>+js> var y = Object.seal(x)
>+js> x === y
>+true
>+
>+js> var obj = Object.defineProperty({}, 'a', {configurable:true})
>+js> var _ = Object.seal(obj)
>+js> Object.getOwnPropertyDescriptor(obj, 'a').configurable
>+false
>+js> Object.isExtensible(obj)
>+false
>+
>+js> Object.isSealed(obj)
>+true

Comment 30

[rspeyer]

(In reply to comment #29)
> (From update of attachment 383057 [details] [diff] [review])
...
> >+          case ConstructorId_seal:
> >+              {
> >+                Object arg = args.length < 1 ? Undefined.instance : args[0];
> >+                ScriptableObject obj = ensureScriptableObject(arg);
> >+
> >+                for (Object name: obj.getAllIds()) {
> >+                  ScriptableObject desc = obj.getOwnPropertyDescriptor(cx, name);
> >+                  if (Boolean.TRUE.equals(desc.get("configurable"))) {
> >+                    desc.put("configurable", desc, false);
> >+                    obj.defineOwnProperty(cx, name, desc); 
> >+                  }
> >+                }
> >+                obj.preventExtensions();
> 
> The spec has wording "The above algorithm is specified as a set of sequential
> steps that include the possibility of an exception being thrown at various
> intermediate points. Rather than failing after a partial update of O, this
> function must be implemented such that it either atomically completes all
> property updates successfully or fails without making any update to the
> properties of object O." Do you know where the exceptions could be thrown, and
> do you avoid partial updates?
> 
> Same question for freeze.
...

By my reading of 8.12.10, this algorithm shouldn't throw any exceptions, and since we create the descriptor ourselves with data properties, the descriptors' setters won't throw. All the properties we're defining attributes on come from getAllIds, so we shouldn't have to worry about a null descriptor (unless another thread in application code deletes a property part-way through, should we worry about that?).

Comment 31

[Norris Boyd]

No, we shouldn't worry about the multithreaded case. I'll submit these changes. Thanks!

Comment 32

[rspeyer]

Attachment: make isSealed and isFrozen slightly more efficient in the common case

check isExtensible at the beginning of the isSealed and isFrozen implementations, as it is fast that will typically be true, making checking all the attributes unnecessary.

Comment 33

[rspeyer]

Attachment: make getOwnPropertyDescriptor and defineProperty work property for arrays

Allows getting and setting attributes on index properties when the array is in dense mode.

Comment 34

[Norris Boyd]

I saw the org.hamcrest.* references. Are they sufficiently useful with a good license to merit including them in Rhino?

Comment 35

[rspeyer]

The particular hamcrest packages that are used (Is and IsNot) are included in junit. See http://junit.sourceforge.net/javadoc/org/hamcrest/core/package-summary.html

(In reply to comment #34)
> I saw the org.hamcrest.* references. Are they sufficiently useful with a good
> license to merit including them in Rhino?

Comment 36

[Norris Boyd]

Thanks. They're not working for me inside eclipse. I'll investigate.

Comment 37

[Norris Boyd]

(In reply to comment #32)
> Created an attachment (id=386879) [details]
> make isSealed and isFrozen slightly more efficient in the common case
> 
> check isExtensible at the beginning of the isSealed and isFrozen
> implementations, as it is fast that will typically be true, making checking all
> the attributes unnecessary.

Committed this patch:

Checking in src/org/mozilla/javascript/NativeObject.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/NativeObject.java,v  <--  NativeObject.java
new revision: 1.53; previous revision: 1.52
done

Comment 38

[Norris Boyd]

(In reply to comment #36)
> Thanks. They're not working for me inside eclipse. I'll investigate.

I've resolved the Eclipse issues, run the regression tests, and committed the code:

Checking in src/org/mozilla/javascript/NativeArray.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/NativeArray.java,v  <--  NativeArray.java
new revision: 1.102; previous revision: 1.101
done
RCS file: /cvsroot/mozilla/js/rhino/testsrc/doctests/arrays.doctest,v
done
Checking in testsrc/doctests/arrays.doctest;
/cvsroot/mozilla/js/rhino/testsrc/doctests/arrays.doctest,v  <--  arrays.doctest
initial revision: 1.1
done
Checking in testsrc/doctests/object.defineproperty.doctest;
/cvsroot/mozilla/js/rhino/testsrc/doctests/object.defineproperty.doctest,v  <--  object.defineproperty.doctest
new revision: 1.3; previous revision: 1.2
done
Checking in testsrc/doctests/object.getownpropertydescriptor.doctest;
/cvsroot/mozilla/js/rhino/testsrc/doctests/object.getownpropertydescriptor.doctest,v  <--  object.getownpropertydescriptor.doctest
new revision: 1.3; previous revision: 1.2
done
RCS file: /cvsroot/mozilla/js/rhino/testsrc/org/mozilla/javascript/tests/NativeArrayTest.java,v
done
Checking in testsrc/org/mozilla/javascript/tests/NativeArrayTest.java;
/cvsroot/mozilla/js/rhino/testsrc/org/mozilla/javascript/tests/NativeArrayTest.java,v  <--  NativeArrayTest.java
initial revision: 1.1
done

Comment 39

[rspeyer]

Attachment: Allow Object.create to accept null as a first argument

Allow Object.create to accept null as a first argument

This reflects updates in the spec since Object.create was first implemented.

Comment 40

[rspeyer]

Attachment: Make Object.getOwnPropertyDescriptor work for members of builtin objects

Previously Object.getOwnPropertyDescriptor(Math, "PI") returned undefined. 

This patch makes the members of builtin objects have descriptors with 
Writable=true, Enumerable=false, Configurable=true for function properties, and
Writable=false, Enumerable=false, Configurable=false for non-function properties

Comment 41

[rspeyer]

Attachment: adding JSON object

Added JSON object. Includes some new files (JsonLexer and JsonParser) for parsing JSON source.

Comment 42

[rspeyer]

Comment on attachment 389384 [details] [diff] [review]
adding JSON object

Moved to bug 505211

Comment 43

[Norris Boyd]

Submitted patches from #39 and #40.

Comment 44

[rspeyer]

Attachment: corrected the descriptor properties of built-ins

In attachment 388878 [details] [diff] [review] I had some of the logic inverted. This is fix for that.

Comment 45

[rspeyer]

Attachment: ensure that getters/setters defined through defineProperty are actually used

Comment 46

[Norris Boyd]

(In reply to comment #45)
> Created an attachment (id=390629) [details]
> ensure that getters/setters defined through defineProperty are actually used

Checking in src/org/mozilla/javascript/ScriptableObject.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/ScriptableObject.java,v  <--  ScriptableObject.java
new revision: 1.153; previous revision: 1.152
done
Checking in testsrc/doctests/object.defineproperty.doctest;
/cvsroot/mozilla/js/rhino/testsrc/doctests/object.defineproperty.doctest,v  <--  object.defineproperty.doctest
new revision: 1.4; previous revision: 1.3
done

Comment 47

[rspeyer]

Attachment: allow getOwnPropertyDescriptor to be called for objects with no scope

e.g. functions returned from Function.bind

Comment 48

[Norris Boyd]

(In reply to comment #44)
> Created an attachment (id=390393) [details]
> corrected the descriptor properties of built-ins
> 
> In attachment 388878 [details] [diff] [review] I had some of the logic inverted. This is fix for that.

It's writable and configurable only if it is a method? That seems backward, and
I couldn't see where in the spec it mentioned different behavior for function
properties. Can you describe more?

Comment 49

[rspeyer]

(In reply to comment #48)
> (In reply to comment #44)
> > Created an attachment (id=390393) [details] [details]
> > corrected the descriptor properties of built-ins
> > 
> > In attachment 388878 [details] [diff] [review] [details] I had some of the logic inverted. This is fix for that.
> 
> It's writable and configurable only if it is a method? That seems backward, and
> I couldn't see where in the spec it mentioned different behavior for function
> properties. Can you describe more?

Well the last paragraph of section 15 (before 15.1) says: 

"In every case, the length property of a built-in Function object described in this section has the attributes 
{ [[Writable]]: false, [[Enumerable]]: false, [[Configurable]]: false } (and no others). Every other property 
 
described in this section has the attributes { [[Writable]]: true, [[Enumerable]]: false, [[Configurable]]: true } 
unless otherwise specified."

Looking through the other properties specified on objects in section 15, it seems that all the non-function properties are "otherwise specified" to have all-false descriptors, while function properties all seem to take the default above.

Comment 50

[Norris Boyd]

Checking in src/org/mozilla/javascript/IdScriptableObject.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/IdScriptableObject.java,v  <--  IdScriptableObject.java
new revision: 1.14; previous revision: 1.13
done
(In reply to comment #49)
> (In reply to comment #48)
> > (In reply to comment #44)
> > > Created an attachment (id=390393) [details] [details] [details]
> > > corrected the descriptor properties of built-ins
> > > 
> > > In attachment 388878 [details] [diff] [review] [details] [details] I had some of the logic inverted. This is fix for that.
> > 
> > It's writable and configurable only if it is a method? That seems backward, and
> > I couldn't see where in the spec it mentioned different behavior for function
> > properties. Can you describe more?
> 
> Well the last paragraph of section 15 (before 15.1) says: 
> 
> "In every case, the length property of a built-in Function object described in
> this section has the attributes 
> { [[Writable]]: false, [[Enumerable]]: false, [[Configurable]]: false } (and no
> others). Every other property 
> 
> described in this section has the attributes { [[Writable]]: true,
> [[Enumerable]]: false, [[Configurable]]: true } 
> unless otherwise specified."
> 
> Looking through the other properties specified on objects in section 15, it
> seems that all the non-function properties are "otherwise specified" to have
> all-false descriptors, while function properties all seem to take the default
> above.

Ok, thanks.

Checking in src/org/mozilla/javascript/IdScriptableObject.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/IdScriptableObject.java,v  <--  IdScriptableObject.java
new revision: 1.14; previous revision: 1.13
done

Comment 51

[Norris Boyd]

(In reply to comment #47)
> Created an attachment (id=390825) [details]
> allow getOwnPropertyDescriptor to be called for objects with no scope
> 
> e.g. functions returned from Function.bind

Checking in src/org/mozilla/javascript/IdScriptableObject.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/IdScriptableObject.java,v  <--  IdScriptableObject.java
new revision: 1.15; previous revision: 1.14

Comment 52

[rspeyer]

Attachment: make defineProperty throw a TypeError when attributes is both data and accessor descriptor

As per 8.10.5 step 9 of the spec.

Comment 53

[rspeyer]

Attachment: For getters and setters of accessor propeties, make being absent is equivalent to having value undefined

As per 8.6.1 Table 3.

Assumes attachment 391263 [details] [diff] [review] has been applied.

Comment 54

[rspeyer]

Attachment: made getOwnPropertDescriptor use the actual attributes for builtins, rather than the isMethod heuristic

Comment 55

[rspeyer]

Attachment: make Object.defineProperty work for builtin properties

Such as Math.pow, or JSON.stringify

Comment 56

[Norris Boyd]

(In reply to comment #52)
> Created an attachment (id=391263) [details]
> make defineProperty throw a TypeError when attributes is both data and accessor
> descriptor
> 
> As per 8.10.5 step 9 of the spec.

Checking in src/org/mozilla/javascript/ScriptableObject.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/ScriptableObject.java,v  <--  ScriptableObject.java
new revision: 1.154; previous revision: 1.153
done
Checking in src/org/mozilla/javascript/resources/Messages.properties;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/resources/Messages.properties,v  <--  Messages.properties
new revision: 1.98; previous revision: 1.97
done

Comment 57

[Norris Boyd]

(In reply to comment #53)
> Created an attachment (id=391276) [details]
> For getters and setters of accessor propeties, make being absent is equivalent
> to having value undefined
> 
> As per 8.6.1 Table 3.
> 
> Assumes attachment 391263 [details] [diff] [review] has been applied.

Checking in src/org/mozilla/javascript/ScriptableObject.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/ScriptableObject.java,v  <--  ScriptableObject.java
new revision: 1.155; previous revision: 1.154
done
Checking in testsrc/doctests/object.defineproperty.doctest;
/cvsroot/mozilla/js/rhino/testsrc/doctests/object.defineproperty.doctest,v  <--  object.defineproperty.doctest
new revision: 1.5; previous revision: 1.4
done

Comment 58

[Norris Boyd]

(In reply to comment #54)
> Created an attachment (id=391299) [details]
> made getOwnPropertDescriptor use the actual attributes for builtins, rather
> than the isMethod heuristic

Checking in src/org/mozilla/javascript/IdScriptableObject.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/IdScriptableObject.java,v  <--  IdScriptableObject.java
new revision: 1.16; previous revision: 1.15
done
Checking in src/org/mozilla/javascript/ScriptableObject.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/ScriptableObject.java,v  <--  ScriptableObject.java
new revision: 1.156; previous revision: 1.155
done
Checking in testsrc/doctests/object.defineproperty.doctest;
/cvsroot/mozilla/js/rhino/testsrc/doctests/object.defineproperty.doctest,v  <--  object.defineproperty.doctest
new revision: 1.6; previous revision: 1.5
done

Comment 59

[Norris Boyd]

(In reply to comment #55)
> Created an attachment (id=391768) [details]
> make Object.defineProperty work for builtin properties
> 
> Such as Math.pow, or JSON.stringify

Checking in src/org/mozilla/javascript/IdScriptableObject.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/IdScriptableObject.java,v  <--  IdScriptableObject.java
new revision: 1.17; previous revision: 1.16
done
Checking in src/org/mozilla/javascript/ScriptableObject.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/ScriptableObject.java,v  <--  ScriptableObject.java
new revision: 1.157; previous revision: 1.156
done
Checking in testsrc/doctests/object.defineproperty.doctest;
/cvsroot/mozilla/js/rhino/testsrc/doctests/object.defineproperty.doctest,v  <--  object.defineproperty.doctest
new revision: 1.7; previous revision: 1.6
done

Comment 60

[rspeyer]

Attachment: make Object.freeze work with functions, arrays, regexp

setInstanceIdValue, called by defineOwnProperty, called by freeze, was not handing all id-identifiable properties, and so when freeze tried to freeze them all, setInstanceIdValue was deferring onto IdScriptableObject.setInstanceIdValue, which just throws an IllegalStateException.

I made setInstanceIdValue always handle all the ids that getInstanceIdValue does.

Comment 61

[Norris Boyd]

Committed:

Checking in src/org/mozilla/javascript/BaseFunction.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/BaseFunction.java,v  <--  BaseFunction.java
new revision: 1.72; previous revision: 1.71
done
Checking in src/org/mozilla/javascript/regexp/NativeRegExp.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/regexp/NativeRegExp.java,v  <--  NativeRegExp.java
new revision: 1.112; previous revision: 1.111
done
Checking in src/org/mozilla/javascript/regexp/NativeRegExpCtor.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/regexp/NativeRegExpCtor.java,v  <--  NativeRegExpCtor.java
new revision: 1.22; previous revision: 1.21
done
Checking in testsrc/doctests/object.freeze.doctest;
/cvsroot/mozilla/js/rhino/testsrc/doctests/object.freeze.doctest,v  <--  object.freeze.doctest
new revision: 1.2; previous revision: 1.1
done