[webkit-reviews] review requested: [Bug 52934] Use WTF::StringHasher in WebCore : [Attachment 79873] Patch

[bugzilla-daemon at webkit.org]

Patrick R. Gansterer <paroga at paroga.com> has asked  for review:
Bug 52934: Use WTF::StringHasher in WebCore
https://bugs.webkit.org/show_bug.cgi?id=52934

Attachment 79873: Patch
https://bugs.webkit.org/attachment.cgi?id=79873&action=review

------- Additional Comments from Patrick R. Gansterer <paroga at paroga.com>
(In reply to comment #5)
> (From update of attachment 79823 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=79823&action=review
> 
> review- because of the WTF prefix issue. Other than this and the function
naming that we decided to handle outside the scope of this patch, this seems
fine.
> 
> > Source/JavaScriptCore/wtf/StringHasher.h:146
> > +	     ASSERT(!(size % 4));
> 
> Where does the number 4 come from here? The hasher can handle any object with
a size that’s multiple of 2. It is true that 4 is the size of int and the size
of pointers on 32-bit systems, but it seems a bit arbitrary to specifically
support only ranges of memory that are multiples of 4.
Fixed.

> > Source/WebCore/loader/appcache/ApplicationCacheStorage.cpp:92
> > -	 return
AlreadyHashed::avoidDeletedValue(StringImpl::computeHash(url.string().character
s() + hostStart, hostEnd - hostStart));
> > +	 return
AlreadyHashed::avoidDeletedValue(WTF::StringHasher::createHash(url.string().cha
racters() + hostStart, hostEnd - hostStart));
> 
> There should be no need to use the namespace prefix WTF. The WTF header files
make use of using declarations, so files that use things defined in the WTF
namespace should almost never need to explicitly include the WTF:: prefix.
Is ok to adress this too in the follow up patch? StringHaser.h has no using
WTF:StringHasher, and we use it already with the WTF prefix everywhere. see
http://trac.webkit.org/changeset/70705