[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 bugzilla-request-daemon at opendarwin.org
Sat Aug 5 16:18:56 PDT 2006

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

Attachment 9894: Now with testcase

------- 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

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

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.

More information about the webkit-reviews mailing list