Bug 96244

Summary: Add AtomicString::number() and use it
Product: WebKit Reporter: Patrick R. Gansterer <paroga>
Component: Web Template FrameworkAssignee: Nobody <webkit-unassigned>
Status: NEW    
Severity: Normal CC: ap, benjamin, cmarcelo, eric.carlson, feature-media-reviews, kling, koivisto, mifenton, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch benjamin: review-

Patrick R. Gansterer
Reported 2012-09-10 01:07:43 PDT
Add AtomicString::number() and use it
Attachments
Patch (16.98 KB, patch)
2012-09-10 01:41 PDT, Patrick R. Gansterer
benjamin: review-
Patrick R. Gansterer
Comment 1 2012-09-10 01:41:39 PDT
Alexey Proskuryakov
Comment 2 2012-09-10 10:15:49 PDT
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.
Benjamin Poulain
Comment 3 2012-09-10 12:56:31 PDT
Comment on attachment 163065 [details] Patch 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.
Benjamin Poulain
Comment 4 2013-01-29 20:18:24 PST
Do you have any plan to update this patch? If not, I might take the bug.
Note You need to log in before you can comment on or make changes to this bug.