Bug 172729 – Fix build of Windows-specific code with ICU 59.1

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Konstantin Tokarev]

ICU 59.1 has broken source compatibility. Now it defines UChar as char16_t, which does not allow automatic type conversion from wchar_t as it used to be.

Added WCharStringExtras.h header to WTF to hold new helper functions.

[Konstantin Tokarev]

Created attachment 311541 [details]
Patch

[Konstantin Tokarev]

Created attachment 311591 [details]
Patch

[Darin Adler]

Comment on attachment 311591 [details]
Patch

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

It‘s fine to create these new function helpers as free functions; helps keep the String class cleaner and contain fewer platform specifics. But note that this is not the pattern we have used in WTF::String up until this point. We have been using member functions for this sort of thing, with examples like the String::createCFString function and the String::operator NSString * function. But I think the explicit function approach is likely superior.

If we take this path, we should also remove the following functions, which I believe were created for use on the Windows platform, and could be replaced by the new functions:

    String::String(const UChar*), which takes a null-terminated 16-bit wide string
    String::charactersWithNullTermination(), which creates a null-terminated 16-bit wide string

There’s nothing about these functions that makes them hard to implement in non-member functions.

> Source/WTF/wtf/text/WCharStringExtras.h:36
> +inline wchar_t* stringToNullTerminatedWChar(const String& string)
> +{
> +    return reinterpret_cast<wchar_t*>(string.charactersWithNullTermination().data());
> +}

This is wrong. This creates a vector, takes a pointer to its content, then deletes that vector and returns the dangling pointer.

[Konstantin Tokarev]

Should I move Source/WTF/wtf/text/WCharStringExtras.h to Source/WTF/wtf/text/win?

[Darin Adler]

(In reply to Konstantin Tokarev from comment #4)
> Should I move Source/WTF/wtf/text/WCharStringExtras.h to
> Source/WTF/wtf/text/win?

Sure, that would be OK. Not sure it’s important.

[Konstantin Tokarev]

I've found that String::String(const UChar*) is used in a few places outside of Windows-specific code. In particular, it's used in SQLiteStatement.cpp as

    return String(reinterpret_cast<const UChar*>(sqlite3_column_name16(m_statement, col)));

[Konstantin Tokarev]

Created attachment 311921 [details]
Patch

[Konstantin Tokarev]

Created attachment 311925 [details]
Patch

[Konstantin Tokarev]

Created attachment 311926 [details]
Patch

[Konstantin Tokarev]

Created attachment 311928 [details]
Patch

[Konstantin Tokarev]

Created attachment 311931 [details]
Patch

[Konstantin Tokarev]

Created attachment 311934 [details]
Patch

[Konstantin Tokarev]

Created attachment 311937 [details]
Patch

[Konstantin Tokarev]

Comment on attachment 311937 [details]
Patch

I haven't managed to remove String::String(UChar*) and String::charactersWithNullTermination() in this patch, because they are used in many places that aren't touched in this patch otherwise, however it would be possible to remove them later.

[WebKit Commit Bot]

Comment on attachment 311937 [details]
Patch

Clearing flags on attachment: 311937

Committed r217771: <http://trac.webkit.org/changeset/217771>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yusuke Suzuki]

It seems that this patch causes JSC test failures due to the failure of opening files (I think this is because of jsc.cpp's path name change)
https://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/builds/937

[Konstantin Tokarev]

Thanks for spotting, indeed modules tests fail with 'Could not open file'

[Konstantin Tokarev]

Fixed in http://trac.webkit.org/changeset/218067