Summary: | Move CFStringRef and NSString support from StringBuilder into StringConcatenateCF | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||||||||||||||||||||||
Component: | Web Template Framework | Assignee: | Darin Adler <darin> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | andersca, annulen, benjamin, cdumez, changseok, cmarcelo, dino, eric.carlson, esprehn+autocc, ews-watchlist, fmalita, fred.wang, glenn, gyuyoung.kim, hi, jamesr, jer.noble, joepeck, keith_miller, kondapallykalyan, luiz, mark.lam, mifenton, msaboff, pdr, philipj, ryuan.choi, saam, sabouhallawa, sam, schenney, sergio, tonikitoo, tzagallo, webkit-bug-importer | ||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=226497 | ||||||||||||||||||||||||||||
Attachments: |
|
Description
Darin Adler
2021-05-14 20:36:31 PDT
Created attachment 428707 [details]
Patch
Created attachment 428709 [details]
Patch
Created attachment 428710 [details]
Patch
Created attachment 428715 [details]
Patch
Created attachment 428721 [details]
Patch
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. Created attachment 428749 [details]
Patch
Created attachment 428752 [details]
Patch
Created attachment 428759 [details]
Patch
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? 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. Created attachment 428916 [details]
Patch
Created attachment 428964 [details]
Patch
Created attachment 428970 [details]
Patch
Created attachment 428977 [details]
Patch
Committed r277744 (237916@main): <https://commits.webkit.org/237916@main> |