Bug 31470

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.

style-queue ran check-webkit-style on attachment 44621 [details] without any errors.

The patch seems to be fine.

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])

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.

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.

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.

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.

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.

Created attachment 47319 [details] IDN support through glib Style checker issue fixed.

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?

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 on attachment 47335 [details] IDN support through glib LGTM.

Comment on attachment 47335 [details] IDN support through glib Clearing flags on attachment: 47335 Committed r53940: <http://trac.webkit.org/changeset/53940>

All reviewed patches have been landed. Closing bug.

Created attachment 48722 [details] patch