Lots of comments. Needs a second iteration.
http://codereview.chromium.org/463040/diff/5/1006 File src/messages.js (right): http://codereview.chromium.org/463040/diff/5/1006#newcode162 src/messages.js:162: value_and_getter_setter: "Invalid property. 'value' present on property with getter or setter", This sounds confusing. How about something along the lines of "A property cannot have both a value and accessors". http://codereview.chromium.org/463040/diff/5/1006#newcode163 src/messages.js:163: proto_object_or_null: 'Object prototype may only be an Object or null', Inconsistent string delimiters. http://codereview.chromium.org/463040/diff/5/1005 File src/v8natives.js (right): http://codereview.chromium.org/463040/diff/5/1005#newcode286 src/v8natives.js:286: if (desc === void 0) return false; If you're deliberately avoiding IS_UNDEFINED please document why. Otherwise you should use IS_UNDEFINED. http://codereview.chromium.org/463040/diff/5/1005#newcode312 src/v8natives.js:312: throw MakeTypeError(property_desc_object, [obj]); You mean "property_desc_object" right, with quotes? http://codereview.chromium.org/463040/diff/5/1005#newcode316 src/v8natives.js:316: var enumerable = obj.enumerable; This doesn't seem to match what's in the spec. Shouldn't it be something like var enum = false; if ('enumerable' in obj) { enum = ToBoolean(obj.enumerable); } Ditto, with variations, for every block below. In any case |enumerable in obj| certainly seems incorrect, it should be |"enumerable" in obj|. http://codereview.chromium.org/463040/diff/5/1005#newcode336 src/v8natives.js:336: if (get !== void 0 && typeof(get) !== 'function') { Use IS_FUNCTION. Applies throughout. http://codereview.chromium.org/463040/diff/5/1005#newcode360 src/v8natives.js:360: this.value_ = void 0; The spec says nothing about these properties being present on property descriptors. This is both a problem in and of itself but it also allows people to put a property descriptor into an inconsistent state. Also, I can't find the style guide right now but I believe variables should be lowerCamelCase. This applies throughout. http://codereview.chromium.org/463040/diff/5/1005#newcode373 src/v8natives.js:373: PropertyDescriptor.prototype.set_value = function(value) { Naming convention should be lowerCamelCase. http://codereview.chromium.org/463040/diff/5/1005#newcode415 src/v8natives.js:415: if (typeof(desc.get_) === 'function') %DefineAccessor(obj, p, GETTER, desc.get_, flag); Use IS_FUNCTION. http://codereview.chromium.org/463040/diff/5/1005#newcode440 src/v8natives.js:440: for (var key in props) { This will include properties in the prototype chain of 'properties', only own properties should be included (15.2.3.7 step 3). http://codereview.chromium.org/463040/diff/5/1004 File test/mjsunit/object-create.js (right): http://codereview.chromium.org/463040/diff/5/1004#newcode43 test/mjsunit/object-create.js:43: var Foo = { foo: function() { ctr++; }}; Peculiar naming convention -- Foo is not a function. http://codereview.chromium.org/463040/diff/5/1004#newcode184 test/mjsunit/object-create.js:184: print("ok"); Excuse me? http://codereview.chromium.org/463040 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
