Bug 225839 – Move CFStringRef and NSString support from StringBuilder into StringConcatenateCF

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

[Darin Adler]

Move CFStringRef and NSString support from StringBuilder into StringConcatenateCF

[Darin Adler]

Created attachment 428707 [details]
Patch

[Darin Adler]

Created attachment 428709 [details]
Patch

[Darin Adler]

Created attachment 428710 [details]
Patch

[Darin Adler]

Created attachment 428715 [details]
Patch

[Darin Adler]

Created attachment 428721 [details]
Patch

[Darin Adler]

Comment on attachment 428721 [details]
Patch

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

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:439
> +        ", ", InjectedBundlePage::responseHeaderCount(response), " headers");

Missing “>” on the end of this line.

[Darin Adler]

Created attachment 428749 [details]
Patch

[Darin Adler]

Created attachment 428752 [details]
Patch

[Darin Adler]

Created attachment 428759 [details]
Patch

[Sam Weinig]

Comment on attachment 428759 [details]
Patch

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

> Source/WTF/wtf/text/StringBuilder.h:124
> +    LChar* m_bufferCharacters { nullptr };

What is the intent of removing the union here and replacing its uses with reinterpret_casts<>? An alternative formulation we could consider (though it may have downsides I am not considering) is using a Variant<LChar*, UChar*> and removing the m_is8Bit bool.

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:327
>      Color textColor = captionsTextColor(important);

auto?

[Darin Adler]

Comment on attachment 428759 [details]
Patch

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

>> Source/WTF/wtf/text/StringBuilder.h:124
>> +    LChar* m_bufferCharacters { nullptr };
> 
> What is the intent of removing the union here and replacing its uses with reinterpret_casts<>? An alternative formulation we could consider (though it may have downsides I am not considering) is using a Variant<LChar*, UChar*> and removing the m_is8Bit bool.

Your suggestion is excellent. To be honest I do not know why I chose to make the change I did, on reflection now. At one point I had changed it to a void* and was using static_cast.

I should probably switch back to the union.

I am tempted to switch to the Variant.

But one reason not to is the possibility of getting rid of m_is8Bit because m_string and m_buffer both already can answer that question, and there is no need to store the state redundantly.

It occurs to me that these bufferCharacters pointers don’t really have much value; they are used to optimize the fast path for append but I am not convinced the benefit is significant over calling m_buffer->characters() and chasing one more pointer; after all, we are already chasing the pointer to check m_buffer->length().

So likely could and should remove both m_bufferCharacters and m_is8Bit.

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:312
> +    Color textColor(adoptCF(MACaptionAppearanceCopyForegroundColor(kMACaptionAppearanceDomainUser, &behavior)).get());

Should use = style here instead of construction syntax I think.

>> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:327
>>      Color textColor = captionsTextColor(important);
> 
> auto?

Will do.

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:417
> +    auto name = adoptCF(CTFontDescriptorCopyAttribute(font.get(), kCTFontNameAttribute));

Should probably move the cast to CFStringRef up here because this is where we call the function guaranteed to return that type for this key.

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:428
>              auto fontCascadeName = adoptCF(CTFontDescriptorCopyAttribute(fontCascade, kCTFontNameAttribute));

Here too. As is done just above for CFArrayRef.

[Darin Adler]

Created attachment 428916 [details]
Patch

[Darin Adler]

Created attachment 428964 [details]
Patch

[Darin Adler]

Created attachment 428970 [details]
Patch

[Darin Adler]

Created attachment 428977 [details]
Patch

[Darin Adler]

Committed r277744 (237916@main): <https://commits.webkit.org/237916@main>

[Radar WebKit Bug Importer]

<rdar://problem/78218765>