Bug 116978 - [GTK] Needs to check for harfbuzz-icu
Summary: [GTK] Needs to check for harfbuzz-icu
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-29 11:02 PDT by Emilio Pozuelo Monfort
Modified: 2013-05-30 07:55 PDT (History)
5 users (show)

See Also:

Attachments
Patch (1.61 KB, patch)
2013-05-30 03:47 PDT, Alberto Garcia 		no flags Details | Formatted Diff | Diff
Patch (1.59 KB, patch)
2013-05-30 06:17 PDT, Alberto Garcia 		mrobinson: review+
Details | Formatted Diff | Diff
Patch (1.60 KB, patch)
2013-05-30 07:04 PDT, Alberto Garcia 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Emilio Pozuelo Monfort 2013-05-29 11:02:17 PDT 
Latest harfbuzz has splitted hb_icu into its own library libharfbuzz-icu.so. Therefore webkit needs to check for harfbuzz-icu in addition to harfbuzz to a) make sure harfbuzz-icu is available and b) get the necessary cflags / libs.
Comment 1 Alberto Garcia 2013-05-30 03:00:12 PDT 
I'll take a look.
Comment 2 Alberto Garcia 2013-05-30 03:47:47 PDT 
Created attachment 203339 [details]
Patch

Here's the patch. Emilio, can you double check if it works for you?
Comment 3 Emilio Pozuelo Monfort 2013-05-30 04:19:12 PDT 
I don't have harfbuzz 0.9.18 yet so not now.

The patch looks good. Something you could do is abort the build if harfbuzz-icu is not available but harfbuzz is >= 0.9.18, with:

if pkg-config --atleast-version 0.9.18 harfbuzz; then
    PKG_CHECK_MODULES(HARFBUZZ_ICU, harfbuzz-icu >= $harfbuzz_required_version)
    FREETYPE_CFLAGS+=" $HARFBUZZ_ICU_CFLAGS"
    FREETYPE_LIBS+=" $HARFBUZZ_ICU_LIBS"
fi

This is useful in case somebody builds harfbuzz but doesn't have libicu installed, as harfbuzz-icu will be automatically disabled and he may not notice.
Comment 4 Alberto Garcia 2013-05-30 06:11:34 PDT 
(In reply to comment #3)
> The patch looks good. Something you could do is abort the build if
> harfbuzz-icu is not available but harfbuzz is >= 0.9.18

Yes, I was also considering doing that. I thought it's actually a
better option in case someone builds harfbuzz without icu support.

I'll upload a new patch.
Comment 5 Alberto Garcia 2013-05-30 06:17:38 PDT 
Created attachment 203347 [details]
Patch
Comment 6 Emilio Pozuelo Monfort 2013-05-30 06:54:27 PDT 
Patch looks good. I should be able to test it tomorrow in case that's still useful by then.
Comment 7 Martin Robinson 2013-05-30 07:00:01 PDT 
Comment on attachment 203347 [details]
Patch

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

> Source/autotools/FindDependencies.m4:379
> +# Since we support earlier HarfBuzz versions we keep this conditional by now.

English nit: by now -> for now

> Source/autotools/FindDependencies.m4:380
> +if $PKG_CONFIG --atleast-version 0.9.18 harfbuzz ; then

Nit: extra space after 'harfbuzz'
Comment 8 Alberto Garcia 2013-05-30 07:04:28 PDT 
Created attachment 203352 [details]
Patch

I fixed the nits that Martin pointed out.
Comment 9 Xan Lopez 2013-05-30 07:27:54 PDT 
Comment on attachment 203352 [details]
Patch

r=me
Comment 10 WebKit Commit Bot 2013-05-30 07:55:35 PDT 
Comment on attachment 203352 [details]
Patch

Clearing flags on attachment: 203352

Committed r150963: <http://trac.webkit.org/changeset/150963>
Comment 11 WebKit Commit Bot 2013-05-30 07:55:37 PDT 
All reviewed patches have been landed.  Closing bug.