Bug 95313 – Convert more static Strings to use ASCIILiteral

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Convert more static Strings to use ASCIILiteral

[Adam Barth]

Created attachment 161161 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 161161 [details]
Patch

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

> Source/WebCore/Modules/mediasource/MediaSource.h:49
> +    static const String& openKeyword();
> +    static const String& closedKeyword();
> +    static const String& endedKeyword();

Thanks for saving the linker some work. :)

> Source/WebCore/css/CSSWrapShapes.cpp:127
> +            result.appendLiteral(", ");

I assume the compiler uniques these strings?  It or the linker must... for sanity's sake.

> Source/WebCore/css/StyleSheetContents.cpp:294
> +        if (baseURL().string().endsWith("/KHTMLFixes.css") && !sheetText.isNull() && mediaWikiKHTMLFixesStyleSheet.startsWith(sheetText)

The web it an amazing place.

> Source/WebCore/editing/markup.cpp:203
>  const String StyledMarkupAccumulator::styleNodeCloseTag(bool isBlock)

This could be const String&

> Source/WebCore/editing/markup.cpp:555
>          return "";

Oh noes.

> Source/WebCore/platform/KURLWTFURL.cpp:116
> +    if (isNull())
> +        return String();

Returning a reference to a temporary seems like a bad idea?  How does the compiler allow this?

> Source/WebCore/platform/MIMETypeRegistry.cpp:641
> +    mimeTypeMap->add(ASCIILiteral("image/x-ms-bmp"), ASCIILiteral("image/bmp")));

I would have created an addLiterals(map, key, value) helper if I had writen this.

> Source/WebCore/svg/SVGAnimatedBoolean.cpp:37
> +    animtedType->boolean() = (string == "true"); // wat?

Yes.  Wat?

[Adam Barth]

(In reply to comment #2)
> (From update of attachment 161161 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161161&action=review
> 
> > Source/WebCore/platform/KURLWTFURL.cpp:116
> > +    if (isNull())
> > +        return String();
> 
> Returning a reference to a temporary seems like a bad idea?  How does the compiler allow this?

Hum...  You're right.  We should use nullAtom.

[Adam Barth]

Comment on attachment 161161 [details]
Patch

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

>> Source/WebCore/css/CSSWrapShapes.cpp:127
>> +            result.appendLiteral(", ");
> 
> I assume the compiler uniques these strings?  It or the linker must... for sanity's sake.

Yes.

[Adam Barth]

(In reply to comment #2)
> (From update of attachment 161161 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161161&action=review
> 
> > Source/WebCore/platform/MIMETypeRegistry.cpp:641
> > +    mimeTypeMap->add(ASCIILiteral("image/x-ms-bmp"), ASCIILiteral("image/bmp")));
> 
> I would have created an addLiterals(map, key, value) helper if I had writen this.

Yeah, the issue is that mimeTypeMap is a WTF template object and that sort of thing only makes sense for String.  We could make a subclass, but this seemed easier.

[Adam Barth]

Created attachment 161246 [details]
Patch

[Eric Seidel (no email)]

(In reply to comment #5)
> (In reply to comment #2)
> > (From update of attachment 161161 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=161161&action=review
> > 
> > > Source/WebCore/platform/MIMETypeRegistry.cpp:641
> > > +    mimeTypeMap->add(ASCIILiteral("image/x-ms-bmp"), ASCIILiteral("image/bmp")));
> > 
> > I would have created an addLiterals(map, key, value) helper if I had writen this.
> 
> Yeah, the issue is that mimeTypeMap is a WTF template object and that sort of thing only makes sense for String.  We could make a subclass, but this seemed easier.

I meant a static inline helper function. :)

[Eric Seidel (no email)]

Comment on attachment 161246 [details]
Patch

LGTM.

[Adam Barth]

(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #2)
> > > (From update of attachment 161161 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=161161&action=review
> > > 
> > > > Source/WebCore/platform/MIMETypeRegistry.cpp:641
> > > > +    mimeTypeMap->add(ASCIILiteral("image/x-ms-bmp"), ASCIILiteral("image/bmp")));
> > > 
> > > I would have created an addLiterals(map, key, value) helper if I had writen this.
> > 
> > Yeah, the issue is that mimeTypeMap is a WTF template object and that sort of thing only makes sense for String.  We could make a subclass, but this seemed easier.
> 
> I meant a static inline helper function. :)

Ah...  Ok.

[Benjamin Poulain]

> >> Source/WebCore/css/CSSWrapShapes.cpp:127
> >> +            result.appendLiteral(", ");
> > 
> > I assume the compiler uniques these strings?  It or the linker must... for sanity's sake.
> 
> Yes.

I can confirm this, I verified that a while ago.

[WebKit Review Bot]

Comment on attachment 161246 [details]
Patch

Clearing flags on attachment: 161246

Committed r127062: <http://trac.webkit.org/changeset/127062>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.