RESOLVED FIXED 137086
Web Replay: use static Strings instead of AtomicStrings for replay input type tags 
https://bugs.webkit.org/show_bug.cgi?id=137086
Summary Web Replay: use static Strings instead of AtomicStrings for replay input type...
Brian Burg
Reported 2014-09-24 16:36:10 PDT
.
Attachments
WIP (25.57 KB, patch)
2014-09-24 16:37 PDT, Brian Burg 		no flags
DetailsFormatted DiffDiff
Patch (53.24 KB, patch)
2014-09-26 00:24 PDT, Brian Burg 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Brian Burg
Comment 1 2014-09-24 16:37:26 PDT
Created attachment 238626 [details] WIP
Timothy Hatcher
Comment 2 2014-09-24 21:29:20 PDT
Comment on attachment 238626 [details] WIP Looks fine.
Anders Carlsson
Comment 3 2014-09-25 14:25:52 PDT
I don't think it's a good idea to use NeverDestroyed with AtomicString - is it really that important to be able to do fast compares or would it be enough to just use normal strings here?
Brian Burg
Comment 4 2014-09-25 14:29:10 PDT
(In reply to comment #3) > I don't think it's a good idea to use NeverDestroyed with AtomicString - is it really that important to be able to do fast compares or would it be enough to just use normal strings here? This is not a performance bottleneck, a String would be fine. (Why is NeverDestroyed<AtomicString> not a good idea? I was ambivalent so I turned to grep, and found about 2x more NeverDestroyed<AtomicString> than NeverDestroyed<String>.)
Anders Carlsson
Comment 5 2014-09-25 14:39:46 PDT
(In reply to comment #4) > (In reply to comment #3) > > I don't think it's a good idea to use NeverDestroyed with AtomicString - is it really that important to be able to do fast compares or would it be enough to just use normal strings here? > > This is not a performance bottleneck, a String would be fine. > > (Why is NeverDestroyed<AtomicString> not a good idea? I was ambivalent so I turned to grep, and found about 2x more NeverDestroyed<AtomicString> than NeverDestroyed<String>.) Creating static AtomicStrings is bad in general since if the function that returns the AtomicString is ever called from more than one thread, you're going to return a string that is only atomic on one thread (the thread that created it).
Brian Burg
Comment 6 2014-09-26 00:24:52 PDT
Created attachment 238702 [details] Patch
WebKit Commit Bot
Comment 7 2014-09-26 00:27:13 PDT
This patch modifies the WEB_REPLAY inputs generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-input-generator-tests --reset-results`)
WebKit Commit Bot
Comment 8 2014-09-26 00:27:27 PDT
Attachment 238702 [details] did not pass style-queue: ERROR: Source/WebCore/replay/SerializationMethods.cpp:171: Multi line control clauses should use braces. [whitespace/braces] [4] ERROR: Source/WebCore/replay/SerializationMethods.cpp:195: Multi line control clauses should use braces. [whitespace/braces] [4] Total errors found: 2 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.
Joseph Pecoraro
Comment 9 2014-09-30 11:42:00 PDT
Comment on attachment 238702 [details] Patch r=me
WebKit Commit Bot
Comment 10 2014-09-30 12:21:29 PDT
Comment on attachment 238702 [details] Patch Clearing flags on attachment: 238702 Committed r174113: <http://trac.webkit.org/changeset/174113>
WebKit Commit Bot
Comment 11 2014-09-30 12:21:35 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.