[webkit-reviews] review denied: [Bug 10230] SVGDOMImplementation should die (and be rolled into DOMImplementation) : [Attachment 9894] Now with testcase

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 10230: SVGDOMImplementation should die (and be rolled into
DOMImplementation)
http://bugzilla.opendarwin.org/show_bug.cgi?id=10230

Attachment 9894: Now with testcase
http://bugzilla.opendarwin.org/attachment.cgi?id=9894&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
(In reply to comment #12)
> Could you provide a bit more detail about what's wrong with the hash
additions?

This change:

     template<> struct StrHash<WebCore::StringImpl*> {
+	 static unsigned hash(const WebCore::String& key) { return
key.impl()->hash(); }
	 static unsigned hash(const WebCore::StringImpl* key) { return
key->hash(); }

should be unnecessary. That's a string hash for StringImpl* and it doesn't make
sense to also add a hash to it for String, nor should it be needed.

The two equal function overloads inside CaseInsensitiveHash should be OK. It's
reasonable to try to make CaseInsensitiveHash work for both String and
StringImpl

But this one is definitely wrong:

+	 static const WebCore::String& deletedValue() {
+	     static WebCore::String null;
+	     return null;
+	 }

It's going to create a separate global variable for each translation unit that
it's used in. It's also going to make empty values and deleted values be the
same, since emptyValueIsZero causes a String with all values 0 (any null
string) be the empty value. The hash code goes completely awry if empty values
and deleted values are the same.