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

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> 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 176800: Patch
https://bugs.webkit.org/attachment.cgi?id=176800&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=176800&action=review


r- for layering violation.

> Source/WebCore/ChangeLog:18
> +	       In the case where the host has an empty string (like a file
URL), we substitute in "localhost".

Why is that desirable?

> Source/WebCore/platform/PlugInOriginHash.cpp:13
> + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY

What's "Apple Computer, Inc."?

> Source/WebCore/platform/PlugInOriginHash.cpp:41
> +static inline uint64_t codeFoldedHash(const String& string)
> +{
> +    if (string.is8Bit())
> +	   return StringHasher::computeHash<LChar,
CaseFoldingHash::foldCase<LChar> >(string.characters8(), string.length());

The function you are calling is named "compute hash", so why does the new
function not have this verb in its name?

> Source/WebCore/platform/PlugInOriginHash.h:34
> +typedef uint64_t PlugInOriginHash;

Our existing types named "Hash" are classes, see e.g. ones in StringHash.h.
It's quite confusing to have a drastically different one for plug-ins.

> Source/WebCore/platform/PlugInOriginHash.h:36
> +PlugInOriginHash plugInOriginHash(const Page*, const KURL& srcURL);

Code in WebCore/platform cannot use Page or Frame classes. It's either (1)
platform API wrappers that do not know about Web classes or concepts or (2)
platform level code that we build from scratch.

Even having "plugin" in the name is quite suspect.