First cut at bug 992

src/v8natives.js

Index: src/v8natives.js
diff --git a/src/v8natives.js b/src/v8natives.js
index 9fd2162686d65ae878107a5969ed1723e629070f..a645a4d886480d786ee6939624d44981da0ca4d9 100644
--- a/src/v8natives.js
+++ b/src/v8natives.js
@@ -545,10 +545,12 @@ function DefineOwnProperty(obj, p, desc, should_throw) {
   if (IS_UNDEFINED(current) && !extensible)
     throw MakeTypeError("define_disallowed", ["defineProperty"]);
 
-  if (!IS_UNDEFINED(current) && !current.isConfigurable()) {
+  if (!IS_UNDEFINED(current)) {
     // Step 5 and 6
-    if ((!desc.hasEnumerable() ||
-         SameValue(desc.isEnumerable() && current.isEnumerable())) &&
+    if ((IsGenericDescriptor(desc) || 

[Rico, 2011/01/05 13:37:01]

Remove space at the end of the line

[Peter Hallam, 2011/01/05 23:50:31]

Done.

+         IsDataDescriptor(desc) == IsDataDescriptor(current)) &&
+        (!desc.hasEnumerable() ||
+         SameValue(desc.isEnumerable(), current.isEnumerable())) &&
         (!desc.hasConfigurable() ||
          SameValue(desc.isConfigurable(), current.isConfigurable())) &&
         (!desc.hasWritable() ||
@@ -561,29 +563,35 @@ function DefineOwnProperty(obj, p, desc, should_throw) {
          SameValue(desc.getSet(), current.getSet()))) {
       return true;
     }
-
-    // Step 7
-    if (desc.isConfigurable() || desc.isEnumerable() != current.isEnumerable())
-      throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
-    // Step 9
-    if (IsDataDescriptor(current) != IsDataDescriptor(desc))
-      throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
-    // Step 10
-    if (IsDataDescriptor(current) && IsDataDescriptor(desc)) {
-      if (!current.isWritable() && desc.isWritable())
-        throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
-      if (!current.isWritable() && desc.hasValue() &&
-          !SameValue(desc.getValue(), current.getValue())) {
-        throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
-      }
-    }
-    // Step 11
-    if (IsAccessorDescriptor(desc) && IsAccessorDescriptor(current)) {
-      if (desc.hasSetter() && !SameValue(desc.getSet(), current.getSet())){
+    if (!current.isConfigurable()) {
+      // Step 7
+      if (desc.isConfigurable() || 

[Rico, 2011/01/05 13:37:01]

Remove space at the end of the line

[Peter Hallam, 2011/01/05 23:50:31]

Done.

+          (desc.hasEnumerable() && 

[Rico, 2011/01/05 13:37:01]

Remove space at the end of the line

[Peter Hallam, 2011/01/05 23:50:31]

Done.

+           desc.isEnumerable() != current.isEnumerable()))
         throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
+      // Step 8
+      if (!IsGenericDescriptor(desc)) {
+        // Step 9a
+        if (IsDataDescriptor(current) != IsDataDescriptor(desc))
+          throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
+        // Step 10a
+        if (IsDataDescriptor(current) && IsDataDescriptor(desc)) {
+          if (!current.isWritable() && desc.isWritable())
+            throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
+          if (!current.isWritable() && desc.hasValue() &&
+              !SameValue(desc.getValue(), current.getValue())) {
+            throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
+          }
+        }
+        // Step 11
+        if (IsAccessorDescriptor(desc) && IsAccessorDescriptor(current)) {
+          if (desc.hasSetter() && !SameValue(desc.getSet(), current.getSet())){
+            throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
+          }
+          if (desc.hasGetter() && !SameValue(desc.getGet(),current.getGet()))
+            throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
+        }
       }
-      if (desc.hasGetter() && !SameValue(desc.getGet(),current.getGet()))
-        throw MakeTypeError("redefine_disallowed", ["defineProperty"]);
     }
   }
 
@@ -607,7 +615,16 @@ function DefineOwnProperty(obj, p, desc, should_throw) {
   } else
     flag |= DONT_DELETE;
 
-  if (IsDataDescriptor(desc) || IsGenericDescriptor(desc)) {
+  if (IsDataDescriptor(desc) || 

[Rico, 2011/01/05 13:37:01]

Remove space at the end of the line

[Peter Hallam, 2011/01/05 23:50:31]

Done.

+      (IsGenericDescriptor(desc) && 

[Rico, 2011/01/05 13:37:01]

Remove space at the end of the line

[Peter Hallam, 2011/01/05 23:50:31]

Done.

+       (IS_UNDEFINED(current) || IsDataDescriptor(current)))) {
+    // There are 3 cases that lead here:
+    // Step 4a - defining a new data property

[Rico, 2011/01/05 13:37:01]

Period at end of comment + below

[Peter Hallam, 2011/01/05 23:50:31]

Done.

+    // Steps 9b & 12 - replacing an existing accessor property with a data 

[Rico, 2011/01/05 13:37:01]

Remove space at the end of the line

[Peter Hallam, 2011/01/05 23:50:31]

Done.

+    //                 property
+    // Step 12 - updating an existing data property with a data or generic
+    //           descriptor
+
     if (desc.hasWritable()) {
       flag |= desc.isWritable() ? 0 : READ_ONLY;
     } else if (!IS_UNDEFINED(current)) {
@@ -615,22 +632,39 @@ function DefineOwnProperty(obj, p, desc, should_throw) {
     } else {
       flag |= READ_ONLY;
     }
+
     var value = void 0;  // Default value is undefined.
     if (desc.hasValue()) {
       value = desc.getValue();
-    } else if (!IS_UNDEFINED(current)) {
+    } else if (!IS_UNDEFINED(current) && IsDataDescriptor(current)) {
       value = current.getValue();
     }
+    

[Rico, 2011/01/05 13:37:01]

Remove spaces from the empty line

[Peter Hallam, 2011/01/05 23:50:31]

Done.

     %DefineOrRedefineDataProperty(obj, p, value, flag);
   } else {

[Rico, 2011/01/05 13:37:01]

How about:
} else if (IsGenericDescriptor(desc)){
    // Update a genereic accessor with a generic descriptor.
    %DefineOrRedefineAccessorProperty(obj, p, GETTER, current.getGet(), flag);
  } else {
    // There are 3 cases that lead here:
    // Step 4b - defining a new accessor property.
    // Steps 9c & 12 - replacing an existing data property with an accessor
    //                 property.
    // Step 12 - updating an existing accessor property with an accessor.

    if (desc.hasGetter()) {
      %DefineOrRedefineAccessorProperty(obj, p, GETTER, desc.getGet(), flag);
    }

    if (desc.hasSetter()) {
      %DefineOrRedefineAccessorProperty(obj, p, SETTER, desc.getSet(), flag);
    }
  }

This saves us an extra call to runtime in case a generic descriptor is passed or
if we are defining or redefining either just the setter or just the getter

[Peter Hallam, 2011/01/05 23:50:31]

Done.

-    if (desc.hasGetter() &&
-        (IS_FUNCTION(desc.getGet()) || IS_UNDEFINED(desc.getGet()))) {
-       %DefineOrRedefineAccessorProperty(obj, p, GETTER, desc.getGet(), flag);
+    // There are 3 cases that lead here:
+    // Step 4b - defining a new accessor property

[Rico, 2011/01/05 13:37:01]

period at the end of the comment + below

[Peter Hallam, 2011/01/05 23:50:31]

Done.

+    // Steps 9c & 12 - replacing an existing data property with an accessor
+    //                 property
+    // Step 12 - updating an existing accessor property with an accessor or
+    //           generic descriptor
+
+    var getter = void 0;
+    if (desc.hasGetter()) {
+      getter = desc.getGet();
+    } else if (!IS_UNDEFINED(current) && IsAccessorDescriptor(current)) {
+      getter = current.getGet();
     }
-    if (desc.hasSetter() &&
-        (IS_FUNCTION(desc.getSet()) || IS_UNDEFINED(desc.getSet()))) {
-      %DefineOrRedefineAccessorProperty(obj, p, SETTER, desc.getSet(), flag);
+
+    var setter = void 0;
+    if (desc.hasSetter()) {
+      setter = desc.getSet();
+    } else if (!IS_UNDEFINED(current) && IsAccessorDescriptor(current)) {
+      setter = current.getSet();
     }
+
+    %DefineOrRedefineAccessorProperty(obj, p, GETTER, getter, flag);
+    %DefineOrRedefineAccessorProperty(obj, p, SETTER, setter, flag);
   }
   return true;
 }