[webkit-reviews] review granted: [Bug 133990] Refactor to remove unnecessary code from the string hashing functions : [Attachment 233248] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Adam Treat <manyoso at yahoo.com>'s
request for review:
Bug 133990: Refactor to remove unnecessary code from the string hashing
functions
https://bugs.webkit.org/show_bug.cgi?id=133990

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

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=233248&action=review


> Source/WTF/ChangeLog:6
> +	   Refactor to remove unnecessary code from the string hashing
functions
> +	   https://bugs.webkit.org/show_bug.cgi?id=133990
> +
> +	   Previously, StringHasher contained methods for calculating the hash
of a pure C

We have an convention of referring to the original Blink commit when merging a
Blink patch into WebKit by adding a remark to the ChangeLog entry/commit
message of the form:

Merged from Blink:
https://src.chromium.org/viewvc/blink?revision=175826&view=revision

You can see an example of ChangeLog entry/commit messages that reference this
at <http://trac.webkit.org/changeset/166668>.

> Source/WTF/ChangeLog:17
> +	   Reviewed by NOBODY (OOPS!).

Please move the Reviewed by line to be on its own line under the WebKit bug
URL.

> Source/WTF/wtf/text/AtomicString.cpp:94
> -    return addToStringTable<const LChar*, CStringTranslator>(c);
> +    return add(c, strlen(reinterpret_cast<const char*>(c)));

This seems like a weird and indirect approach because AtomicString::add(const
LChar*, unsigned) duplicates the precondition checks done in this function and
otherwise calls through to AtomicString::addToStringTable<LCharBuffer,
LCharBufferTranslator>(). We should take a similar approach as
AtomicString::add(const UChar*) and stack allocate an LCharBuffer with the
specified string and computed length and directly call
AtomicString::addToStringTable<LCharBuffer, LCharBufferTranslator>(...), which
may be inlined.

> Tools/ChangeLog:6
> +	   Refactor to remove unnecessary code from the string hashing
functions
> +	   https://bugs.webkit.org/show_bug.cgi?id=133990
> +
> +	   Previously, StringHasher contained methods for calculating the hash
of a pure C

Please also update this ChangeLog entry.