[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
bugzilla-request-daemon at macosforge.org
Tue Jan 23 08:03:19 PST 2007
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;
More information about the webkit-reviews
mailing list