Bug 195346 – [Win] Remove -DUCHAR_TYPE=wchar_t stopgap and learn to live with char16_t.

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

[Ross Kirsling]

[Win] Remove -DUCHAR_TYPE=wchar_t stopgap and learn to live with char16_t.

[Ross Kirsling]

Created attachment 363715 [details]
Patch

[Ross Kirsling]

Created attachment 363719 [details]
Patch

[Fujii Hironori]

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?

[Fujii Hironori]

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.

[Fujii Hironori]

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.

[Fujii Hironori]

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

[Ross Kirsling]

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

[Ross Kirsling]

Created attachment 363792 [details]
Patch

[Ross Kirsling]

Created attachment 363807 [details]
Patch

[Ross Kirsling]

Created attachment 363811 [details]
Patch

[Ross Kirsling]

Created attachment 363815 [details]
Patch

[Fujii Hironori]

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.

[Ross Kirsling]

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

[Fujii Hironori]

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.

[Ross Kirsling]

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

[Fujii Hironori]

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

[Ross Kirsling]

Created attachment 363841 [details]
Patch

[Ross Kirsling]

Created attachment 363843 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 363843 [details]
Patch

Clearing flags on attachment: 363843

Committed r242592: <https://trac.webkit.org/changeset/242592>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48667048>

[Fujii Hironori]

Committed r242632: <https://trac.webkit.org/changeset/242632>

[Darin Adler]

I’m thinking we might want to move from UChar to char16_t across all of WebKit at some point.