Bug 172729 - Fix build of Windows-specific code with ICU 59.1
Summary: Fix build of Windows-specific code with ICU 59.1
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Konstantin Tokarev
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-05-30 13:43 PDT by Konstantin Tokarev
Modified: 2017-06-11 04:33 PDT (History)
7 users (show)

See Also:


Attachments
Patch (19.63 KB, patch)
2017-05-30 13:53 PDT, Konstantin Tokarev
no flags Details | Formatted Diff | Diff
Patch (19.77 KB, patch)
2017-05-31 08:24 PDT, Konstantin Tokarev
no flags Details | Formatted Diff | Diff
Patch (26.68 KB, patch)
2017-06-03 06:50 PDT, Konstantin Tokarev
no flags Details | Formatted Diff | Diff
Patch (26.69 KB, patch)
2017-06-03 07:07 PDT, Konstantin Tokarev
no flags Details | Formatted Diff | Diff
Patch (26.71 KB, patch)
2017-06-03 07:14 PDT, Konstantin Tokarev
no flags Details | Formatted Diff | Diff
Patch (29.64 KB, patch)
2017-06-03 08:39 PDT, Konstantin Tokarev
no flags Details | Formatted Diff | Diff
Patch (30.39 KB, patch)
2017-06-03 09:44 PDT, Konstantin Tokarev
no flags Details | Formatted Diff | Diff
Patch (23.73 KB, patch)
2017-06-03 11:14 PDT, Konstantin Tokarev
no flags Details | Formatted Diff | Diff
Patch (23.86 KB, patch)
2017-06-03 12:18 PDT, Konstantin Tokarev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Konstantin Tokarev 2017-05-30 13:43:57 PDT
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.
Comment 1 Konstantin Tokarev 2017-05-30 13:53:01 PDT
Created attachment 311541 [details]
Patch
Comment 2 Konstantin Tokarev 2017-05-31 08:24:50 PDT
Created attachment 311591 [details]
Patch
Comment 3 Darin Adler 2017-06-01 09:12:19 PDT
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.
Comment 4 Konstantin Tokarev 2017-06-02 13:37:55 PDT
Should I move Source/WTF/wtf/text/WCharStringExtras.h to Source/WTF/wtf/text/win?
Comment 5 Darin Adler 2017-06-02 14:09:00 PDT
(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.
Comment 6 Konstantin Tokarev 2017-06-02 16:06:00 PDT
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)));
Comment 7 Konstantin Tokarev 2017-06-03 06:50:03 PDT
Created attachment 311921 [details]
Patch
Comment 8 Konstantin Tokarev 2017-06-03 07:07:50 PDT
Created attachment 311925 [details]
Patch
Comment 9 Konstantin Tokarev 2017-06-03 07:14:09 PDT
Created attachment 311926 [details]
Patch
Comment 10 Konstantin Tokarev 2017-06-03 08:39:24 PDT
Created attachment 311928 [details]
Patch
Comment 11 Konstantin Tokarev 2017-06-03 09:44:51 PDT
Created attachment 311931 [details]
Patch
Comment 12 Konstantin Tokarev 2017-06-03 11:14:33 PDT
Created attachment 311934 [details]
Patch
Comment 13 Konstantin Tokarev 2017-06-03 12:18:02 PDT
Created attachment 311937 [details]
Patch
Comment 14 Konstantin Tokarev 2017-06-03 14:37:34 PDT
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.
Comment 15 WebKit Commit Bot 2017-06-04 09:38:43 PDT
Comment on attachment 311937 [details]
Patch

Clearing flags on attachment: 311937

Committed r217771: <http://trac.webkit.org/changeset/217771>
Comment 16 WebKit Commit Bot 2017-06-04 09:38:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Yusuke Suzuki 2017-06-09 10:06:52 PDT
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
Comment 18 Konstantin Tokarev 2017-06-09 10:40:33 PDT
Thanks for spotting, indeed modules tests fail with 'Could not open file'
Comment 19 Konstantin Tokarev 2017-06-11 04:33:21 PDT
Fixed in http://trac.webkit.org/changeset/218067