[webkit-reviews] review granted: [Bug 9950] property name array changes : [Attachment 9483] JS property name fixes

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 9950: property name array changes
http://bugzilla.opendarwin.org/show_bug.cgi?id=9950

Attachment 9483: JS property name fixes
http://bugzilla.opendarwin.org/attachment.cgi?id=9483&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
+	 PropertyNameArray() {}

This line of code is not needed.

+	 void deallocateVector();

This declaration is not needed.

+	 typedef HashSet<UString::Impl*, PtrHash<UString::Impl*> >
IdentifierSet;

Why do you specify PtrHash explicitly? Isn't that the default?

+    virtual void getPropertyNames(ExecState *exec, PropertyNameArray&
propertyNames);

I'd write the above as:

    virtual void getPropertyNames(ExecState*, PropertyNameArray&);

I can tell that you want to renaming UString::Rep to UString::Impl -- we should
do that! Not sure why you wanted to create the synonym just for this patch
though.

+"This tests that for/in statements don't report properties that are in both an
object ant its prototype more than once."

Lets say "and" rather than "ant" here.

+ *  Copyright (C) 2005 Apple Computer, Inc

Should be 2006.

+#ifndef KJS_IDENTIFIER_SEQUENCED_SET_H

Since you chose to call this PropertyNameArray, I recommend making the header
guard say that too.

I think JSPropertyNameArray is a fine name for the API. But internally, maybe
this should be JSPropertyNameVector. I know it's a dual vector/set, but it can
also be thought of as a vector with a special "add" operation. The use of array
in the API is really just to be consistent with CFArray, and our C++ equivalent
of CFArray is Vector.

r=me