Bug 93255 – [GTK] Webkit 1.8.2 fails to build with MinGW with spellchecking enabled

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

Created attachment 156656 [details]
proposed patch

Webkit 1.8.2 fails to build out of the box with MinGW with spellchecking enabled

[WebKit Review Bot]

Attachment 156656 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 156664 [details]
proposed patch

[WebKit Review Bot]

Attachment 156664 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1
Source/WebKit/gtk/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 156956 [details]
patch with style fix

[Eric Seidel (no email)]

Comment on attachment 156956 [details]
patch with style fix

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

> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:129
> +    iface->check_spelling_of_string = checkSpellingOfString;
> +    iface->get_guesses_for_word = getGuessesForWord;
> +    iface->update_spell_checking_languages = updateSpellCheckingLanguages;
> +    iface->get_autocorrect_suggestions_for_misspelled_word = getAutocorrectSuggestionsForMisspelledWord;
> +    iface->learn_word = learnWord;
> +    iface->ignore_word = ignoreWord;

Why would you move away from using an english word?

[Martin Robinson]

Comment on attachment 156956 [details]
patch with style fix

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

> Source/WebKit/gtk/WebCoreSupport/TextCheckerClientGtk.cpp:64
> -    GOwnPtr<gchar> utf8Text(g_utf16_to_utf8(const_cast<gunichar2*>(text), length, 0, 0, 0));
> +    GOwnPtr<gchar> utf8Text(g_utf16_to_utf8(reinterpret_cast<const gunichar2*>(text), length, 0, 0, 0));

I mentioned this to pfor via IRC, but I'll leave it here for posterity: I think it might be necessary here to use  g_ucs4_to_utf8 instead of simply casting 32-bit characters to 16-bit.

> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:122
> -static void webkit_spell_checker_enchant_spell_checker_interface_init(WebKitSpellCheckerInterface* interface)
> +static void webkit_spell_checker_enchant_spell_checker_interface_init(WebKitSpellCheckerInterface* iface)

Yeah, I agree with Eric here. If we can't use interface, we should use something like spellCheckerInterface.

[Kalev Lember]

(In reply to comment #6)
> > Source/WebKit/gtk/WebCoreSupport/TextCheckerClientGtk.cpp:64
> > -    GOwnPtr<gchar> utf8Text(g_utf16_to_utf8(const_cast<gunichar2*>(text), length, 0, 0, 0));
> > +    GOwnPtr<gchar> utf8Text(g_utf16_to_utf8(reinterpret_cast<const gunichar2*>(text), length, 0, 0, 0));
> 
> I mentioned this to pfor via IRC, but I'll leave it here for posterity: I think it might be necessary here to use  g_ucs4_to_utf8 instead of simply casting 32-bit characters to 16-bit.

I don't think the cast actually loses precision. It is from UChar* to gunichar2*; I believe these are both always defined as 16 bit types.

From Source/JavaScriptCore/icu/unicode/umachine.h, which defines UChar for the ICU backend:

#if U_SIZEOF_WCHAR_T==2
    typedef wchar_t UChar;
#elif U_GNUC_UTF16_STRING
#if defined _GCC_
    typedef __CHAR16_TYPE__ char16_t;
#endif
    typedef char16_t UChar;
#else
    typedef uint16_t UChar;
#endif

e.g. if wchar_t is 16 bit wide, try to use this; otherwise fall back to another 16 bit type.

The case is even simpler for the glib unicode backend. UnicodeGLib.h:
typedef uint16_t UChar;

Again, a 16 bit type.

And same goes for gunichar2 which is defined in glib's gunicode.h header:
typedef guint16 gunichar2;

[Martin Robinson]

(In reply to comment #7)
> (In reply to comment #6)
> > > Source/WebKit/gtk/WebCoreSupport/TextCheckerClientGtk.cpp:64
> > > -    GOwnPtr<gchar> utf8Text(g_utf16_to_utf8(const_cast<gunichar2*>(text), length, 0, 0, 0));
> > > +    GOwnPtr<gchar> utf8Text(g_utf16_to_utf8(reinterpret_cast<const gunichar2*>(text), length, 0, 0, 0));
> > 
> > I mentioned this to pfor via IRC, but I'll leave it here for posterity: I think it might be necessary here to use  g_ucs4_to_utf8 instead of simply casting 32-bit characters to 16-bit.
> 
> I don't think the cast actually loses precision. It is from UChar* to gunichar2*; I believe these are both always defined as 16 bit types.
> 
> From Source/JavaScriptCore/icu/unicode/umachine.h, which defines UChar for the ICU backend:
> 
> #if U_SIZEOF_WCHAR_T==2
>     typedef wchar_t UChar;
> #elif U_GNUC_UTF16_STRING
> #if defined _GCC_
>     typedef __CHAR16_TYPE__ char16_t;
> #endif
>     typedef char16_t UChar;
> #else
>     typedef uint16_t UChar;
> #endif

Okay. This sounds reasonable. I'm a bit surprised that pointers of the same size require a reinterpet_cast to switch between them though. Do you understand why it's necessary here? Can you double-check by printing sizeof for both types?

[Kalev Lember]

(In reply to comment #8)
> Okay. This sounds reasonable. I'm a bit surprised that pointers of the same size require a reinterpet_cast to switch between them though. Do you understand why it's necessary here?

I believe reinterpret_cast is needed because static_cast doesn't cast between unrelated pointer types, even if the sizes of the types pointed to match. On Linux, UChar is an integer type and static_cast works because it's casting from one type of integer to another, which are naturally convertible. But on Windows, UChar is a wchar_t and it's not implicitly convertible to an integer.


> Can you double-check by printing sizeof for both types?
I don't currently have a webkitgtk win32 build at hand to instrument the source code there, but a quick test with sizeof:

> $ cat kala.cpp
> #include <glib.h>
> #include <iostream>
> 
> int main()
> {
>     std::cout << "sizeof wchar_t: " << sizeof (wchar_t) << std::endl;
>     std::cout << "sizeof guint16: " << sizeof (guint16) << std::endl;
>     return 0;
> }
> 
> $ i686-w64-mingw32-g++ kala.cpp `i686-w64-mingw32-pkg-config --cflags glib-2.0` -o kala.exe
> $ wine kala.exe
> sizeof wchar_t: 2
> sizeof guint16: 2

[Martin Robinson]

Comment on attachment 156956 [details]
patch with style fix

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

>> Source/WebKit/gtk/WebCoreSupport/TextCheckerClientGtk.cpp:64
>> +    GOwnPtr<gchar> utf8Text(g_utf16_to_utf8(reinterpret_cast<const gunichar2*>(text), length, 0, 0, 0));
> 
> I mentioned this to pfor via IRC, but I'll leave it here for posterity: I think it might be necessary here to use  g_ucs4_to_utf8 instead of simply casting 32-bit characters to 16-bit.

I think it's possible to simply avoid the casts altogether by using the WTFString API:

String textAsString(text, length);
webkit_spell_checker_check_spelling_of_string(m_spellChecker.get(), textAsString.utf8().data(), misspellingLocation, misspellingLength);

If that's possible let's do that. That's both a little cleaner and uses less casting, which can be potentially dangerous.

Created attachment 165648 [details]
Updated patch

Thanks very much for suggestions.

Attached patch is updated according to those.
Tested with WebKitGTK 1.8.3

[WebKit Review Bot]

Comment on attachment 165648 [details]
Updated patch

Clearing flags on attachment: 165648

Committed r129537: <http://trac.webkit.org/changeset/129537>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.