Bug 137086 - Web Replay: use static Strings instead of AtomicStrings for replay input type tags
Summary: Web Replay: use static Strings instead of AtomicStrings for replay input type...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brian Burg
URL:
Keywords:
Depends on:
Blocks: WebReplay
  Show dependency tree / graph
 
Reported: 2014-09-24 16:36 PDT by Brian Burg
Modified: 2014-09-30 12:21 PDT (History)
10 users (show)

See Also:

Attachments
WIP (25.57 KB, patch)
2014-09-24 16:37 PDT, Brian Burg 		no flags Details | Formatted Diff | Diff
Patch (53.24 KB, patch)
2014-09-26 00:24 PDT, Brian Burg 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2014-09-24 16:36:10 PDT 
.
Comment 1 Brian Burg 2014-09-24 16:37:26 PDT 
Created attachment 238626 [details]
WIP
Comment 2 Timothy Hatcher 2014-09-24 21:29:20 PDT 
Comment on attachment 238626 [details]
WIP

Looks fine.
Comment 3 Anders Carlsson 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?
Comment 4 Brian Burg 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>.)
Comment 5 Anders Carlsson 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).
Comment 6 Brian Burg 2014-09-26 00:24:52 PDT 
Created attachment 238702 [details]
Patch
Comment 7 WebKit Commit Bot 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`)
Comment 8 WebKit Commit Bot 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.
Comment 9 Joseph Pecoraro 2014-09-30 11:42:00 PDT 
Comment on attachment 238702 [details]
Patch

r=me
Comment 10 WebKit Commit Bot 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>
Comment 11 WebKit Commit Bot 2014-09-30 12:21:35 PDT 
All reviewed patches have been landed.  Closing bug.