[webkit-reviews] review denied: [Bug 9508] JavaScript Comparing a form element group to itself returns false. : [Attachment 12621] proposed fix

[bugzilla-request-daemon at macosforge.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 9508: JavaScript Comparing a form element group to itself returns false.
http://bugs.webkit.org/show_bug.cgi?id=9508

Attachment 12621: proposed fix
http://bugs.webkit.org/attachment.cgi?id=12621&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
There's no need for two virtual functions here. Once we're taking the overhead
of a virtual function call there's no reason to not just call it isEqual and do
the pointer comparison in the base JSObject implementation. I don't see the
need for the word "deep" in the name.

Since both objects are known to be ObjectType already, we should be doing a
cast to JSObject, not a call to getObject -- the getObject call is much more
expensive and does not add any value. In fact, I think the "equal" function
would read better if the end part was a switch statement.

I don't think the change to strictEqual is correct. That's for the "==="
operation, which should almost certainly distinguish two objects with identical
contents like these. So I believe that function doesn't need to change.

+    const DOMNamedNodesCollection* d = reinterpret_cast<const
DOMNamedNodesCollection*>(rhs);    

This should be a static_cast, not a reinterpret_cast.

+    // For performance, we can assume that two collections which have the same
size and start with the
+    // same nodes are equivalent, as opposed to iterating through the entire
vectors.

No. That's wrong. We do need to compare the vectors. This code should just say:


    return rhs->isObject(&DOMNamedNodesCollection::info)
	&& m_nodes == static_cast<const
DOMNamedNodesCollection*>(rhs)->m_nodes;