[webkit-reviews] review denied: [Bug 103655] Add PlugInOriginHash : [Attachment 177451] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Jon Lee <jonlee at apple.com>'s request
for review:
Bug 103655: Add PlugInOriginHash
https://bugs.webkit.org/show_bug.cgi?id=103655

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=177451&action=review


review- because there is a much better way to do this hash correctly

> Source/WebCore/ChangeLog:19
> +	   benefit either, since retrieving the host from the KURL creates a
brand new string every time. We shift
> +	   the page's host hash, since otherwise, in the case of where the
plug-in is loading from the same domain
> +	   as the page, it would lead to a combined hash of 0.

I don’t understand why a right shift of 1 is the right way to solve that
problem. It throws away 1/32 of the host hash. I’d think a rotate would be
better than a shift.

> Source/WebCore/plugins/PlugInOriginHash.cpp:44
> +static inline unsigned computeCaseFoldedHash(const String& string)
> +{
> +    if (string.is8Bit())
> +	   return StringHasher::computeHash<LChar,
CaseFoldingHash::foldCase<LChar> >(string.characters8(), string.length());
> +    return StringHasher::computeHash<UChar, CaseFoldingHash::foldCase<UChar>
>(string.characters16(), string.length());
> +}

The fact that this is not a duplicate of the CaseFoldingHash::hash function
from StringHash.h is a subtle detail that is not explained here. The change log
mentions that we don’t want to mask the top 8 bits, but it’s easy to overlook
that so we need a comment explaining that, not just change log. And I also
think we should consider putting this function into StringHash.h in the future.
We just need to figure out the right name for it.

> Source/WebCore/plugins/PlugInOriginHash.cpp:51
> +    String plugInSrcHost = plugInURL.host();

Why put this into a local variable, but not put plugInElement->serviceType()
into a local variable?

> Source/WebCore/plugins/PlugInOriginHash.cpp:53
> +    return (computeCaseFoldedHash(pageHost) >> 1) ^
computeCaseFoldedHash(plugInSrcHost) ^
computeCaseFoldedHash(plugInElement->serviceType());

I’m not sure that exclusive or is a good way to combine these three hashes;
normally we hash the hashes, or make a single hash. And the need to shift the
hash of the page host and the source host is sufficiently non-obvious that it
needs to be in a comment, not just change log.

I think I know a much better way to do this. Here’s my idea. First, we refactor
the StringHasher::computeHash static member function into a new
StringHasher::addCharacters(const T* data, unsigned length) non-static member
function, so we have a function we can use on an existing StringHasher multiple
times. Then we write a function to add a string to a hash named
addCaseFoldedCharacters, which calls addCharacters. Then we write this:

    StringHasher hasher;
    addCaseFoldedCharacters(hasher,
page->mainFrame()->document()->baseURL().host());
    hasher.addCharacter(0);
    addCaseFoldedCharacters(hasher, plugInURL.host());
    hasher.addCharacter(0);
    addCaseFoldedCharacters(hasher, plugInElement->serviceType());
    return hasher.hash();

This way, we won’t have to do any tricky shifting or worry about how well
exclusive or works.

> Source/WebCore/plugins/PlugInOriginHash.h:35
> +    static unsigned hash(HTMLPlugInImageElement* plugInElement, const KURL&
plugInURL);

Normally we need an equality function to go along with a hash function. Are we
actually going to rely on these hashes without also doing equality checks?