Bug 239127 - Replace AtomString(const char*) with AtomString::fromLatin1(const char*)
Summary: Replace AtomString(const char*) with AtomString::fromLatin1(const char*)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-04-12 08:39 PDT by Chris Dumez
Modified: 2022-04-13 09:10 PDT (History)
45 users (show)

See Also:

Attachments
Patch (81.28 KB, patch)
2022-04-12 08:49 PDT, Chris Dumez 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (108.64 KB, patch)
2022-04-12 09:10 PDT, Chris Dumez 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (109.58 KB, patch)
2022-04-12 09:40 PDT, Chris Dumez 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (111.19 KB, patch)
2022-04-12 10:16 PDT, Chris Dumez 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (113.91 KB, patch)
2022-04-12 10:41 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (113.93 KB, patch)
2022-04-12 10:57 PDT, Chris Dumez 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (114.72 KB, patch)
2022-04-12 12:46 PDT, Chris Dumez 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (117.01 KB, patch)
2022-04-12 13:43 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (117.55 KB, patch)
2022-04-12 19:26 PDT, Chris Dumez 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (117.56 KB, patch)
2022-04-12 20:49 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Show Obsolete (9) 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 Chris Dumez 2022-04-12 08:39:38 PDT 
Replace AtomString(const char*) with AtomString::fromLatin1(const char*) for consistency with String.
Comment 1 Chris Dumez 2022-04-12 08:49:54 PDT 
Created attachment 457333 [details]
Patch
Comment 2 Chris Dumez 2022-04-12 09:10:46 PDT 
Created attachment 457334 [details]
Patch
Comment 3 Chris Dumez 2022-04-12 09:40:54 PDT 
Created attachment 457336 [details]
Patch
Comment 4 Chris Dumez 2022-04-12 10:16:12 PDT 
Created attachment 457340 [details]
Patch
Comment 5 Chris Dumez 2022-04-12 10:41:10 PDT 
Created attachment 457343 [details]
Patch
Comment 6 Chris Dumez 2022-04-12 10:57:46 PDT 
Created attachment 457348 [details]
Patch
Comment 7 Chris Dumez 2022-04-12 12:46:31 PDT 
Created attachment 457377 [details]
Patch
Comment 8 Chris Dumez 2022-04-12 13:43:59 PDT 
Created attachment 457410 [details]
Patch
Comment 9 Darin Adler 2022-04-12 18:07:48 PDT 
Comment on attachment 457410 [details]
Patch

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

> Source/WTF/wtf/text/AtomString.h:171
> +    AtomString(const char*);

Might even mark this explicit.

> Source/WebCore/dom/DocumentInlines.h:42
> +inline AtomString Document::encoding() const { return AtomString::fromLatin1(textEncoding().domName()); }

I wonder if this is better AtomString than String; worth thinking about at some point, now obviously good to change just this.

> Source/WebCore/editing/HTMLInterchange.h:39
> +#define ApplePasteAsQuotation     "Apple-paste-as-quotation"_s
> +#define AppleStyleSpanClass       "Apple-style-span"_s
> +#define AppleTabSpanClass         "Apple-tab-span"_s
> +#define WebKitMSOListQuirksStyle  "WebKit-mso-list-quirks-style"_s

If we’re changing it this much, maybe we can use constexpr instead of #define

> Source/WebCore/page/Quirks.cpp:504
> +            if (auto* paneContainer = node->treeScope().getElementById(AtomString("paneContainer"_s))) {

Does this require an explicit conversion?
Comment 10 Chris Dumez 2022-04-12 19:26:28 PDT 
Created attachment 457500 [details]
Patch
Comment 11 Chris Dumez 2022-04-12 20:49:16 PDT 
Created attachment 457504 [details]
Patch
Comment 12 Chris Dumez 2022-04-12 20:50:07 PDT 
(In reply to Darin Adler from comment #9)
> Comment on attachment 457410 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457410&action=review
> 
> > Source/WTF/wtf/text/AtomString.h:171
> > +    AtomString(const char*);
> 
> Might even mark this explicit.
> 
> > Source/WebCore/dom/DocumentInlines.h:42
> > +inline AtomString Document::encoding() const { return AtomString::fromLatin1(textEncoding().domName()); }
> 
> I wonder if this is better AtomString than String; worth thinking about at
> some point, now obviously good to change just this.
> 
> > Source/WebCore/editing/HTMLInterchange.h:39
> > +#define ApplePasteAsQuotation     "Apple-paste-as-quotation"_s
> > +#define AppleStyleSpanClass       "Apple-style-span"_s
> > +#define AppleTabSpanClass         "Apple-tab-span"_s
> > +#define WebKitMSOListQuirksStyle  "WebKit-mso-list-quirks-style"_s
> 
> If we’re changing it this much, maybe we can use constexpr instead of #define
> 
> > Source/WebCore/page/Quirks.cpp:504
> > +            if (auto* paneContainer = node->treeScope().getElementById(AtomString("paneContainer"_s))) {
> 
> Does this require an explicit conversion?

Yes, it is ambiguous without it because there is a getElementById() that takes in a String and one that takes in an AtomString.
Comment 13 Chris Dumez 2022-04-13 07:47:44 PDT 
Comment on attachment 457504 [details]
Patch

Clearing flags on attachment: 457504

Committed r292810 (249592@trunk): <https://commits.webkit.org/249592@trunk>
Comment 14 Chris Dumez 2022-04-13 07:47:49 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2022-04-13 07:48:17 PDT 
<rdar://problem/91689002>
Comment 16 Darin Adler 2022-04-13 09:10:03 PDT 
(In reply to Chris Dumez from comment #12)
> (In reply to Darin Adler from comment #9)
> > > Source/WebCore/page/Quirks.cpp:504
> > > +            if (auto* paneContainer = node->treeScope().getElementById(AtomString("paneContainer"_s))) {
> > 
> > Does this require an explicit conversion?
> 
> Yes, it is ambiguous without it because there is a getElementById() that
> takes in a String and one that takes in an AtomString.

Side note: If we wanted an efficient algorithm for this in hot code paths, we would call a "find an AtomString only if it already exists" variant rather than the AtomString constructor. It’s wasteful to allocate memory for the AtomString, when we know that in that case there is no element with that ID.

One of the ways to do that would be an overload that takes an ASCIILiteral. Another would be some AtomString construction functions that followed this "only if already exists" rule, which could be useful for various kinds of AtomString-based lookup scenarios.