[webkit-reviews] review denied: [Bug 96244] Add AtomicString::number() and use it : [Attachment 163065] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Patrick R. Gansterer
<paroga at paroga.com>'s request for review:
Bug 96244: Add AtomicString::number() and use it
https://bugs.webkit.org/show_bug.cgi?id=96244

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
This is great, it should avoid creating StringImpl temporarily.

r- because I would like some WebKit API tests for the change.
It would be nice to have benchmark numbers in the ChangeLog. A microbenchmark
should be able to point out changes visible from JavaScript.


If you have time, there is a nice improvement possible for this
AtomicString::number() - (for a separate patch).
Currently, you use string hash() on the converted number. You could design a
hash() function homomorphic to the hash for string(). Depending on the
operations, the homomorphic hash() could be faster (or slower...) than string
hash on the converted value.


(In reply to comment #2)
> I'm wondering how much sense it makes for attribute values to be atomic
strings. I know that this is how it's been for a long time, but anyway.

This is a valid point.

I am in favor of going forward with this patch. Changing the type of the values
is a important change. This tiny patch reduces nicely the need for extraneous
StringImpl without adding any complexity to the codebase.