| Summary: | Web Replay: use static Strings instead of AtomicStrings for replay input type tags | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Brian Burg <burg> | ||||||
| Component: | WebCore Misc. | Assignee: | Brian Burg <burg> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | andersca, commit-queue, gyuyoung.kim, joepeck, kling, rakuco, ryuan.choi, sam, sergio, timothy | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 136290 | ||||||||
| Attachments: |
|
||||||||
|
Description
Brian Burg
2014-09-24 16:36:10 PDT
Created attachment 238626 [details]
WIP
Comment on attachment 238626 [details]
WIP
Looks fine.
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? (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>.) (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). Created attachment 238702 [details]
Patch
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`) 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 on attachment 238702 [details]
Patch
r=me
Comment on attachment 238702 [details] Patch Clearing flags on attachment: 238702 Committed r174113: <http://trac.webkit.org/changeset/174113> All reviewed patches have been landed. Closing bug. |