Bug 137086

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:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

136290

Attachments:

Description	Flags
WIP	none
Patch	none

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	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 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.