Bug 239127

Created attachment 457333 [details]
Patch

Created attachment 457334 [details]
Patch

Created attachment 457336 [details]
Patch

Created attachment 457340 [details]
Patch

Created attachment 457343 [details]
Patch

Created attachment 457348 [details]
Patch

Created attachment 457377 [details]
Patch

Created attachment 457410 [details]
Patch

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?

Created attachment 457500 [details]
Patch

Created attachment 457504 [details]
Patch

(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 on attachment 457504 [details]
Patch

Clearing flags on attachment: 457504

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

All reviewed patches have been landed.  Closing bug.

<rdar://problem/91689002>

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