Bug 31470 - [Gtk] For removing ICU, implement IDN support by means of libidn
Summary: [Gtk] For removing ICU, implement IDN support by means of libidn
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 15914
  Show dependency treegraph
 
Reported: 2009-11-13 05:54 PST by Dominik Röttsches (drott)
Modified: 2010-02-14 08:37 PST (History)
8 users (show)

See Also:


Attachments
IDN support through libidn (3.02 KB, patch)
2009-12-10 08:57 PST, Dominik Röttsches (drott)
xan.lopez: review-
Details | Formatted Diff | Diff
IDN support through libidn (2.85 KB, patch)
2010-01-15 01:46 PST, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
IDN support through glib (1.65 KB, patch)
2010-01-22 08:04 PST, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
IDN support through glib (1.66 KB, patch)
2010-01-25 00:00 PST, Dominik Röttsches (drott)
xan.lopez: review-
Details | Formatted Diff | Diff
IDN support through glib (1.71 KB, patch)
2010-01-25 04:24 PST, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
patch (457 bytes, text/plain)
2010-02-14 08:33 PST, Alexander Butenko
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dominik Röttsches (drott) 2009-11-13 05:54:16 PST
On GTK, when removing ICU, support for IDN needs to be shifted from ICU backend to libidn.
Comment 1 Dominik Röttsches (drott) 2009-12-10 08:57:26 PST
Created attachment 44621 [details]
IDN support through libidn

IDN support through libidn when using GLib unicode backend.

http://www.gnu.org/software/libidn/manual/html_node/Autoconf-tests.html and own experience seems to indicate that testing for libidn availability with header and linkcheck is more stable than using pkg-config for the purpose. I don't know what the project's preference for these tests is. If required, I can surely change it to the pkg-config based approach.
Comment 2 WebKit Review Bot 2009-12-10 09:01:08 PST
style-queue ran check-webkit-style on attachment 44621 [details] without any errors.
Comment 3 Holger Freyther 2009-12-12 22:20:42 PST
The patch seems to be fine.
Comment 4 Xan Lopez 2010-01-14 04:13:47 PST
Comment on attachment 44621 [details]
IDN support through libidn

>+#elif USE(GLIB_UNICODE)
>+    // first translate to ucs4, libidn is expecting code points 
>+    GOwnPtr<gunichar> ucs4Hostname;
>+    GOwnPtr<GError> ucs4Err;
>+    ucs4Hostname.set(g_utf16_to_ucs4(str, strLen, 0, 0, &ucs4Err.outPtr()));
>+    if (ucs4Err)
>+        return;
>+    char* encodedHostname = 0;
>+    int err;
>+    err = idna_to_ascii_4z(ucs4Hostname.get(), &encodedHostname, IDNA_ALLOW_UNASSIGNED);
>+    if (err == IDNA_SUCCESS)

A nitpick, but I guess retValue or something like that would be better than 'err', since the variable does not necessarily denote an error.

>+        buffer.append(encodedHostname, strlen(encodedHostname));
>+    free(encodedHostname);
> #endif
> }
> 
>Index: autotools/webkit.m4
>===================================================================
>--- autotools/webkit.m4	(revision 51948)
>+++ autotools/webkit.m4	(working copy)
>@@ -166,6 +166,15 @@ fi
> 
> if test "$with_unicode_backend" = "glib"; then
> 	PKG_CHECK_MODULES([UNICODE], [glib-2.0 pango >= 1.21.0])
>+
>+	AC_MSG_CHECKING([for libidn])
>+	AC_CHECK_HEADER(idna.h,
>+		AC_CHECK_LIB(idn, stringprep_check_version,
>+			[libidn=yes UNICODE_LIBS="${UNICODE_LIBS} -lidn"], libidn=no),
>+		libidn=no)
>+	if test "$libidn" = "no" ; then
>+		AC_MSG_ERROR([Libidn not found, needed for GLib unicode backend.])
>+	fi
> fi

I very much prefer to rely on pkg-config for this, as we do everywhere else.

Also, is there any chance this could be folded into glib at some point? Depending on a new library for a single function is kind of silly.

> 
> AC_SUBST([UNICODE_CFLAGS])
Comment 5 Dominik Röttsches (drott) 2010-01-15 01:46:27 PST
Created attachment 46657 [details]
IDN support through libidn

(In reply to comment #4)
> (From update of attachment 44621 [details])

> A nitpick, but I guess retValue or something like that would be better than
> 'err', since the variable does not necessarily denote an error.
> [...]
> I very much prefer to rely on pkg-config for this, as we do everywhere else.

Comments addressed.

> Also, is there any chance this could be folded into glib at some point?
> Depending on a new library for a single function is kind of silly.

I agree - but don't know about such plans. I could file it as an issue for glib.
Comment 6 Alexander Butenko 2010-01-19 12:57:17 PST
04:37:11 PM) avb: kov: https://bugs.webkit.org/show_bug.cgi?id=31470
(04:37:24 PM) avb: no chances that this will be commited till 1.1.19?
(04:39:07 PM) avb: the only thing probably this could be handled by libsoup without libidn 
(04:39:11 PM) avb: danw: 
16:40
(04:40:04 PM) danw: glib has idn support. g_hostname_to_unicode() and g_hostname_to_ascii()
16:55
(04:55:30 PM) avb: danw: so no need in libidn
(04:55:38 PM) danw: right
(04:55:56 PM) avb: ill copy paste this chat into the bug


so seems there is a way to avoid new dependency.
Comment 7 Dominik Röttsches (drott) 2010-01-20 00:41:02 PST
Cancelled the review for the current patch. Thanks for the glib info regarding IDN, Alex - didn't know that. Will try to incorporate it soon, but could take a while, unfortunately not much time for working on this one at the moment.
Comment 8 Dominik Röttsches (drott) 2010-01-22 08:04:38 PST
Created attachment 47205 [details]
IDN support through glib

Now using glib functions for the same purpose. No extra dependencies and easier memory management through GOwnPtr.
Comment 9 WebKit Review Bot 2010-01-22 08:09:13 PST
Attachment 47205 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/KURL.cpp:1414:  Missing space before ( in if(  [whitespace/parens] [5]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Dominik Röttsches (drott) 2010-01-25 00:00:14 PST
Created attachment 47319 [details]
IDN support through glib

Style checker issue fixed.
Comment 11 Xan Lopez 2010-01-25 00:43:16 PST
Comment on attachment 47319 [details]
IDN support through glib

Sorry to be a pain, but could you update the ChangeLog to reflect that you are not using libidn anymore?
Comment 12 Dominik Röttsches (drott) 2010-01-25 04:24:12 PST
Created attachment 47335 [details]
IDN support through glib

(In reply to comment #11)
> (From update of attachment 47319 [details])
> Sorry to be a pain, but could you update the ChangeLog to reflect that you are
> not using libidn anymore?

Of course, sorry I missed it. Hope it reflects the conclusion appropriately now.
Comment 13 Xan Lopez 2010-01-26 23:17:07 PST
Comment on attachment 47335 [details]
IDN support through glib

LGTM.
Comment 14 WebKit Commit Bot 2010-01-27 10:19:45 PST
Comment on attachment 47335 [details]
IDN support through glib

Clearing flags on attachment: 47335

Committed r53940: <http://trac.webkit.org/changeset/53940>
Comment 15 WebKit Commit Bot 2010-01-27 10:19:53 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Alexander Butenko 2010-02-14 08:33:13 PST
Created attachment 48722 [details]
patch