Bug 93255 - [GTK] Webkit 1.8.2 fails to build with MinGW with spellchecking enabled
Summary: [GTK] Webkit 1.8.2 fails to build with MinGW with spellchecking enabled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-06 04:24 PDT by tuxator
Modified: 2012-09-25 12:11 PDT (History)
5 users (show)

See Also:


Attachments
proposed patch (2.69 KB, patch)
2012-08-06 04:24 PDT, tuxator
no flags Details | Formatted Diff | Diff
proposed patch (3.68 KB, patch)
2012-08-06 04:56 PDT, tuxator
no flags Details | Formatted Diff | Diff
patch with style fix (3.67 KB, patch)
2012-08-07 08:57 PDT, tuxator
mrobinson: review-
Details | Formatted Diff | Diff
Updated patch (3.94 KB, patch)
2012-09-25 11:36 PDT, tuxator
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description tuxator 2012-08-06 04:24:21 PDT
Created attachment 156656 [details]
proposed patch

Webkit 1.8.2 fails to build out of the box with MinGW with spellchecking enabled
Comment 1 WebKit Review Bot 2012-08-06 04:26:38 PDT
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.
Comment 2 tuxator 2012-08-06 04:56:42 PDT
Created attachment 156664 [details]
proposed patch
Comment 3 WebKit Review Bot 2012-08-06 04:58:48 PDT
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.
Comment 4 tuxator 2012-08-07 08:57:48 PDT
Created attachment 156956 [details]
patch with style fix
Comment 5 Eric Seidel (no email) 2012-08-07 15:17:34 PDT
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?
Comment 6 Martin Robinson 2012-08-07 15:31:31 PDT
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.
Comment 7 Kalev Lember 2012-09-23 06:09:16 PDT
(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;
Comment 8 Martin Robinson 2012-09-24 08:18:02 PDT
(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?
Comment 9 Kalev Lember 2012-09-24 09:10:17 PDT
(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
Comment 10 Martin Robinson 2012-09-24 09:15:41 PDT
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.
Comment 11 tuxator 2012-09-25 11:36:03 PDT
Created attachment 165648 [details]
Updated patch

Thanks very much for suggestions.

Attached patch is updated according to those.
Tested with WebKitGTK 1.8.3
Comment 12 WebKit Review Bot 2012-09-25 12:11:11 PDT
Comment on attachment 165648 [details]
Updated patch

Clearing flags on attachment: 165648

Committed r129537: <http://trac.webkit.org/changeset/129537>
Comment 13 WebKit Review Bot 2012-09-25 12:11:15 PDT
All reviewed patches have been landed.  Closing bug.