[webkit-reviews] review denied: [Bug 20928] Speed up JS property enumeration by caching entire PropertyNameArray : [Attachment 23556] cache entire PropertyNameArray

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Sam Weinig <sam at webkit.org>'s request
for review:
Bug 20928: Speed up JS property enumeration by caching entire PropertyNameArray
https://bugs.webkit.org/show_bug.cgi?id=20928

Attachment 23556: cache entire PropertyNameArray
https://bugs.webkit.org/attachment.cgi?id=23556&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
JSPropertyNameIterator::invalidate is so simple now it should probably be
inlined. Maybe also use clear() instead of assigning 0?

+    , m_data(0)

Don't need that for RefPtr.

+inline JSPropertyNameIterator::JSPropertyNameIterator(JSObject* object,
PropertyNameArrayData* propertyNameArrayData)

Should PropertyNameArrayData* should be PassRefPtr, since this takes ownership.


+	 void setCachedPrototypeChain(StructureIDChain* cachedPrototypeChain) {
m_cachedPrototypeChain = cachedPrototypeChain; }

+	 void setData(PropertyNameArrayData* data) { m_data = data; } 

Should be PassRefPtr since these take ownership.

+	 if ((*a).get() != (*b).get())
+	     return false;

These would read better with -> but can't you just omit the get() altogether?

+	 if (!(*a).get())
+	     return true;

This would read better with -> but can't you just omit the get() altogether?

+    ASSERT_NOT_REACHED();
+    return false;

This will just create a warning on Windows and make the build fail. Leave it
out.

+    // FIXME: This breaks the optimization for the JSGlobalObject. 

I think you should write a clearer comment. Does this really need a FIXME?

I'm going to say review- because of the number of actionable comments.