Bug 239127 – Replace AtomString(const char*) with AtomString::fromLatin1(const char*)

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

[Chris Dumez]

Replace AtomString(const char*) with AtomString::fromLatin1(const char*) for consistency with String.

[Chris Dumez]

Created attachment 457333 [details]
Patch

[Chris Dumez]

Created attachment 457334 [details]
Patch

[Chris Dumez]

Created attachment 457336 [details]
Patch

[Chris Dumez]

Created attachment 457340 [details]
Patch

[Chris Dumez]

Created attachment 457343 [details]
Patch

[Chris Dumez]

Created attachment 457348 [details]
Patch

[Chris Dumez]

Created attachment 457377 [details]
Patch

[Chris Dumez]

Created attachment 457410 [details]
Patch

[Darin Adler]

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?

[Chris Dumez]

Created attachment 457500 [details]
Patch

[Chris Dumez]

Created attachment 457504 [details]
Patch

[Chris Dumez]

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

[Chris Dumez]

Comment on attachment 457504 [details]
Patch

Clearing flags on attachment: 457504

Committed r292810 (249592@trunk): <https://commits.webkit.org/249592@trunk>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/91689002>

[Darin Adler]

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