Bug 195276 – Rename AtomicString to AtomString

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Michael Catanzaro]

On webkit-dev we discussed confusion caused by the name AtomicString. Many developers over the years have mistaken AtomicString for a threadsafe/synchronized string class to be shared across threads. In fact, it's an interned ("atom string") class where all strings are stored in a global string table, so not only can AtomicStrings not be shared across threads, they cannot be created or used on secondary threads at all. Turns out many developers (myself included) have never heard of "atom strings," while others have never heard of "interned strings" (the term I'm familiar with, from both GLib and Java).

After several possible names were considered (the best: "Stringleton", thanks for that one, Simon) we seem to have consensus to rename this class from AtomicString to AtomString.

It's an intimidating rename, so I don't plan to work on it myself (at least not now), but I wanted to file a bug at least.

Discussed at:

https://lists.webkit.org/pipermail/webkit-dev/2018-December/030355.html
https://lists.webkit.org/pipermail/webkit-dev/2019-January/030411.html

And older:

https://lists.webkit.org/pipermail/webkit-dev/2013-June/024988.html

[Darin Adler]

We’ll do it with the do-webcore-rename script, which makes it really easy.

[Darin Adler]

Although we might need to do some trial runs and fix bugs in the script.

I’ve done similarly-broad giant renames with that script before.

[Michael Catanzaro]

(In reply to Darin Adler from comment #2)
> Although we might need to do some trial runs and fix bugs in the script.
> 
> I’ve done similarly-broad giant renames with that script before.

Cool script. Note its output when not given anything new to rename:

diff --git a/Source/WTF/wtf/Expected.h b/Source/WTF/wtf/Expected.h
index e9b0ec9f9e6..b64f69ecdab 100644
--- a/Source/WTF/wtf/Expected.h
+++ b/Source/WTF/wtf/Expected.h
@@ -214,7 +214,7 @@ template<class E>
 class bad_expected_access : public bad_expected_access<void> {
 public:
     explicit bad_expected_access(E val) : val(val) { }
-    virtual const char* what() const noexcept override { return std::exception::what(); }
+    const char* what() const noexcept override { return std::exception::what(); }
     E& error() & { return val; }
     const E& error() const& { return val; }
     E&& error() && { return std::move(val); }

(A code style error.)

diff --git a/Source/WTF/wtf/Platform.h b/Source/WTF/wtf/Platform.h
index 94037ce8e29..369d2344d98 100644
--- a/Source/WTF/wtf/Platform.h
+++ b/Source/WTF/wtf/Platform.h
@@ -353,7 +353,7 @@
 #endif
 
 /* ==== OS() - underlying operating system; only to be used for mandated low-level services like 
-   virtual memory, not to choose a GUI toolkit ==== */
+   memory, not to choose a GUI toolkit ==== */
 
 /* OS(AIX) - AIX */
 #ifdef _AIX

(A do-webcore-rename issue.)

[Darin Adler]

It removed the word "virtual"?

[Darin Adler]

One tricky, but maybe obvious, thing about do-webcore-rename -- it usually has the "old renaming" list checked in, so you have to delete that and replace it with the new set of things to rename.

[Darin Adler]

Created attachment 372205 [details]
Patch

[Darin Adler]

Note for anyone reviewing the patch: All the changes were done by the do-webcore-rename script, except for the changes to do-webcore-rename itself, and the changes to AtomString.h to add a temporary AtomicString synonym. Still worth looking over the changes to see if you can spot any mistakes, but it's not like a person made a choice about each change.

Accordingly, I didn't try to fix the style mistakes mentioned by the style checker in this patch.

[Darin Adler]

Comment on attachment 372205 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372205&action=review

> Tools/Scripts/do-webcore-rename:270
> +            $newContents =~ s/\b$from\b/$renames{$from}/g; # this " unconfuses Xcode syntax highlighting

I can take out that comment, since it doesn’t apply any more.

[EWS Watchlist]

Attachment 372205 [details] did not pass style-queue:


ERROR: Source/WebCore/domjit/DOMJITIDLTypeFilter.h:43:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/domjit/DOMJITIDLTypeFilter.h:44:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/domjit/DOMJITIDLTypeFilter.h:68:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/domjit/DOMJITIDLTypeFilter.h:69:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/dom/QualifiedName.cpp:29:  l is incorrectly named. Don't use the single letter 'l' as an identifier name.  [readability/naming] [4]
ERROR: Source/WTF/wtf/text/AtomStringHash.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/text/AtomStringHash.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/AtomString.cpp:28:  You should not add a blank line before implementation file's own header.  [build/include_order] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/AtomString.cpp:36:  Consider using ASSERT_EQ instead of ASSERT_TRUE(a == b)  [readability/check] [2]
ERROR: Source/WebCore/dom/Event.h:147:  The parameter name "isTrusted" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/text/AtomString.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/text/AtomString.h:79:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomString.h:80:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomString.h:85:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomString.h:100:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WebCore/domjit/DOMJITIDLType.h:42:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/domjit/DOMJITIDLType.h:43:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/VectorTraits.h:31:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/PointerEvent.h:128:  The parameter name "isCancelable" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/xml/XPathStep.h:58:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/xml/XPathStep.h:59:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/TouchEvent.cpp:59:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/SpaceSplitString.cpp:161:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 23 in 1257 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

I think this is ready to land! Review, anyone?

[Darin Adler]

Maybe someone needs to re-create the patch using Subversion directly so the renames work correctly?

[Michael Catanzaro]

Comment on attachment 372205 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372205&action=review

Thanks Darin! I certainly haven't looked at the whole diff, but it passes my sanity check.

I suspect the Bugzilla review tool just isn't smart enough to notice renames? Renames generally appear as renames on trac.

> Source/WTF/wtf/text/AtomString.h:180
> +using AtomicString = AtomString;

This deserves a FIXME to remove it when Apple is able to do that, right?

[Darin Adler]

(In reply to Michael Catanzaro from comment #12)
> This deserves a FIXME to remove it when Apple is able to do that, right?

Sure, but that's going to be really soon.

[Darin Adler]

(In reply to Michael Catanzaro from comment #12)
> I suspect the Bugzilla review tool just isn't smart enough to notice
> renames? Renames generally appear as renames on trac.

At one time, the review tool did the right thing when passed a patch generated by Subversion but not a patch generated but git. I don’t know about what will or won’t work when actually applying the patch.

[WebKit Commit Bot]

Comment on attachment 372205 [details]
Patch

Clearing flags on attachment: 372205

Committed r246490: <https://trac.webkit.org/changeset/246490>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51791157>