Summary: | [Win] Remove -DUCHAR_TYPE=wchar_t stopgap and learn to live with char16_t. | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ross Kirsling <ross.kirsling> | ||||||||||||||||||
Component: | New Bugs | Assignee: | Ross Kirsling <ross.kirsling> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | achristensen, annulen, bfulgham, commit-queue, darin, don.olmstead, Hironori.Fujii, pvollan, 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=181004 | ||||||||||||||||||||
Attachments: |
|
Description
Ross Kirsling
2019-03-05 17:19:12 PST
Created attachment 363715 [details]
Patch
Created attachment 363719 [details]
Patch
Comment on attachment 363719 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363719&action=review > Source/WTF/wtf/text/win/WCharStringExtras.h:40 > +inline wchar_t* ucharAsWCharMutable(UChar* characters) You can overload. const wchar_t* ucharAsWChar(const UChar* characters) wchar_t* ucharAsWChar(UChar* characters) And, I prefer wcharFrom. > Source/WebKitLegacy/win/DOMCSSClasses.cpp:95 > + WTF::String cssTextString = nullTerminatedWCharToString(cssText); WTF::String has platform specific ctors, String(CFStringRef) and String(NSString *). We should have String(BSTR). > Source/WebKitLegacy/win/WebCoreSupport/WebChromeClient.cpp:668 > + String file = nullTerminatedWCharToString(files); Why do need nullTerminatedWCharToString here? WTF::String has a ctor from UChar*. // Construct a string with UTF-16 data, from a null-terminated source. WTF_EXPORT_PRIVATE String(const UChar*); First of all, nullTerminatedWCharToString is needed? Comment on attachment 363719 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363719&action=review >> Source/WebKitLegacy/win/WebCoreSupport/WebChromeClient.cpp:668 >> + String file = nullTerminatedWCharToString(files); > > Why do need nullTerminatedWCharToString here? > WTF::String has a ctor from UChar*. > > // Construct a string with UTF-16 data, from a null-terminated source. > WTF_EXPORT_PRIVATE String(const UChar*); > > First of all, nullTerminatedWCharToString is needed? I understand. Ignore this comment. We need platfrom specific ctor, String(wchat_t*). But we can't have this until AppleWin port update ICU. Comment on attachment 363719 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363719&action=review >> Source/WebKitLegacy/win/DOMCSSClasses.cpp:95 >> + WTF::String cssTextString = nullTerminatedWCharToString(cssText); > > WTF::String has platform specific ctors, String(CFStringRef) and String(NSString *). > We should have String(BSTR). Oh, no. BSTR is just a typedef of wchar_t. We can't do that until AppleWin port updates ICU. Comment on attachment 363719 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363719&action=review >>> Source/WebKitLegacy/win/DOMCSSClasses.cpp:95 >>> + WTF::String cssTextString = nullTerminatedWCharToString(cssText); >> >> WTF::String has platform specific ctors, String(CFStringRef) and String(NSString *). >> We should have String(BSTR). > > Oh, no. BSTR is just a typedef of wchar_t. We can't do that until AppleWin port updates ICU. How about this idea? #if OS(WINDOWS) && U_ICU_VERSION_MAJOR_NUM >= 59 WTF_EXPORT_PRIVATE String(const wchar_t*) #endif (In reply to Fujii Hironori from comment #6) > Comment on attachment 363719 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=363719&action=review > > >>> Source/WebKitLegacy/win/DOMCSSClasses.cpp:95 > >>> + WTF::String cssTextString = nullTerminatedWCharToString(cssText); > >> > >> WTF::String has platform specific ctors, String(CFStringRef) and String(NSString *). > >> We should have String(BSTR). > > > > Oh, no. BSTR is just a typedef of wchar_t. We can't do that until AppleWin port updates ICU. > > How about this idea? > > #if OS(WINDOWS) && U_ICU_VERSION_MAJOR_NUM >= 59 > WTF_EXPORT_PRIVATE String(const wchar_t*) > #endif Damn, I did all this without realizing that AppleWin is still behind. That seems like a good plan though. Created attachment 363792 [details]
Patch
Created attachment 363807 [details]
Patch
Created attachment 363811 [details]
Patch
Created attachment 363815 [details]
Patch
Comment on attachment 363815 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363815&action=review > Source/WebKitLegacy/win/Plugins/PluginDatabaseWin.cpp:355 > + directories.append(String(macromediaDirectoryStr)); directories.append take a String as a argument. And, you added non-explicit ctor. So, you can omit explicit String ctor call. directories.append(macromediaDirectoryStr); I don't have a strong opinion. > Source/WebKitLegacy/win/WebFrame.cpp:735 > + Frame* foundFrame = coreFrame->tree().find(AtomicString(ucharFrom(name), SysStringLen(name)), *coreFrame); Let's do same for AtomicString. Then, you can remove these patch hunks. (In reply to Fujii Hironori from comment #12) > Comment on attachment 363815 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=363815&action=review > > > Source/WebKitLegacy/win/Plugins/PluginDatabaseWin.cpp:355 > > + directories.append(String(macromediaDirectoryStr)); > > directories.append take a String as a argument. And, you added non-explicit > ctor. So, you can omit explicit String ctor call. > directories.append(macromediaDirectoryStr); > I don't have a strong opinion. Sure, I can prune as many of these as possible. > > Source/WebKitLegacy/win/WebFrame.cpp:735 > > + Frame* foundFrame = coreFrame->tree().find(AtomicString(ucharFrom(name), SysStringLen(name)), *coreFrame); > > Let's do same for AtomicString. Then, you can remove these patch hunks. Will do -- looks like there are CFStringRef/NSString ctors for AtomicString after all. This is just my idea. There are a lot of conversion code from WTF::String to wchar_t*. > wcharFrom(title.charactersWithNullTermination().data()) Looks very long. This can be simplified if we have, for example WinString class. > WinString(title) class WinString { WinString(const String&); operator wchar_t*() const; }; WebCore already has WebCore::BString class. But, it is using SysAllocString. I think WinString should use fastMalloc. (In reply to Fujii Hironori from comment #14) > There are a lot of conversion code from WTF::String to wchar_t*. > > > wcharFrom(title.charactersWithNullTermination().data()) > > Looks very long. I agree -- I've removed stringToNullTerminatedWChar here (because it's a copy-paste of charactersWithNullTermination with a return type of Vector<wchar_t> and I thought it would be bad to have the definitions get out of sync) but we still kind of need a replacement. Since we're already adding String(const wchar_t*), I think it could be reasonable to add a Vector<wchar_t> method too, e.g.: > WTF_EXPORT_PRIVATE Vector<wchar_t> wideCharacters() const; (In reply to Ross Kirsling from comment #15) > Since we're already adding String(const wchar_t*), I think it could be > reasonable to add a Vector<wchar_t> method too, e.g.: > > WTF_EXPORT_PRIVATE Vector<wchar_t> wideCharacters() const; Sounds good to me. Created attachment 363841 [details]
Patch
Created attachment 363843 [details]
Patch
Comment on attachment 363843 [details] Patch Clearing flags on attachment: 363843 Committed r242592: <https://trac.webkit.org/changeset/242592> All reviewed patches have been landed. Closing bug. Committed r242632: <https://trac.webkit.org/changeset/242632> I’m thinking we might want to move from UChar to char16_t across all of WebKit at some point. |