Bug 91864 - [Cairo] Add complex font drawing using HarfbuzzNG
Summary: [Cairo] Add complex font drawing using HarfbuzzNG
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dominik Röttsches (drott)
URL:
Keywords:
: 91858 (view as bug list)
Depends on:
Blocks: 85606 91853 92120
  Show dependency treegraph
 
Reported: 2012-07-20 08:03 PDT by Dominik Röttsches (drott)
Modified: 2012-08-02 07:21 PDT (History)
12 users (show)

See Also:


Attachments
HarfbuzzNG support based on cairo-ft. (29.32 KB, patch)
2012-07-24 07:59 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
HarfbuzzNG support based on cairo and freetype. (29.31 KB, patch)
2012-07-24 08:04 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
HarfbuzzNG support based on cairo and freetype, v2. (29.93 KB, patch)
2012-07-24 09:55 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
HarfbuzzNG support based on cairo and freetype, v3 (29.83 KB, patch)
2012-07-24 13:06 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
HarfbuzzNG support based on cairo and freetype, v4 (31.46 KB, patch)
2012-07-25 01:59 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
HarfbuzzNG support based on cairo and freetype, v5 (30.86 KB, patch)
2012-07-25 06:30 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
HarfbuzzNG support based on cairo and freetype, v6 (31.04 KB, patch)
2012-07-25 13:45 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
HarfbuzzNG support based on cairo and freetype, final. (31.10 KB, patch)
2012-07-27 01:40 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
Using cairo_scaled_font approach, cleanup for FindHarfbuzz (4.08 KB, patch)
2012-07-30 05:40 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff
Using cairo_scaled_font approach, review comments addressed (45.92 KB, patch)
2012-08-02 06:28 PDT, Dominik Röttsches (drott)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominik Röttsches (drott) 2012-07-20 08:03:09 PDT
Similar to Chromium mac, we can add complex font rendering path to cairo using Harfbuzz NG shaper + drawGlyphBuffer.
I propose to add a new file FontCairoComplex.cpp that implements this.
Comment 1 Dominik Röttsches (drott) 2012-07-24 07:59:57 PDT
Created attachment 154062 [details]
HarfbuzzNG support based on cairo-ft.
Comment 2 Dominik Röttsches (drott) 2012-07-24 08:04:26 PDT
Created attachment 154064 [details]
HarfbuzzNG support based on cairo and freetype.
Comment 3 Dominik Röttsches (drott) 2012-07-24 08:14:42 PDT
This is enabling the code for EFL right away, GTK support is tracked in bug 92098.
Comment 4 Thiago Marcos P. Santos 2012-07-24 08:46:21 PDT
Comment on attachment 154064 [details]
HarfbuzzNG support based on cairo and freetype.

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

In addition to the comments, I would also like to seggest you to try building with pango backend and also a --minimal build.

> Source/WebCore/ChangeLog:11
> +        No new tests, complex font support is covered by existing tests.

I guess you might have some tests to unskip or rebaseline.

> Source/WebCore/PlatformEfl.cmake:170
> +      platform/graphics/harfbuzz/HarfBuzzShaperBase.cpp
> +      platform/graphics/harfbuzz/ng/HarfBuzzNGFace.cpp
> +      platform/graphics/harfbuzz/ng/HarfBuzzNGFaceCairo.cpp
> +      platform/graphics/harfbuzz/ng/HarfBuzzShaper.cpp
> +    )
> +    LIST(APPEND WebCore_LIBRARIES
> +      ${HARFBUZZ_LIBRARIES}
>      )

Maybe this should be conditoned to DWTF_USE_HARFBUZZ_NG. Usually the .cpp are always added to the build system and the contents protected by the flags.

> Source/WebCore/platform/graphics/cairo/FontCairoComplex.cpp:48
> +    if (shaper.shape(&glyphBuffer)) {
> +        drawGlyphBuffer(context, run, glyphBuffer, point);
> +        return;
> +    }
> +    ASSERT_NOT_REACHED();

Isn't better to write as ASSERT(shaper.shape(&glyphBuffer)) and remove the ASSERT_NOT_REACHED()?

Looks to me that this code will do a unnecessary if you build release.

> Source/WebCore/platform/graphics/cairo/FontCairoComplex.cpp:71
> +    if (shaper.shape())
> +        return shaper.totalWidth();
> +    ASSERT_NOT_REACHED();

Ditto.

> Source/WebCore/platform/graphics/cairo/FontCairoComplex.cpp:90
> +    if (shaper.shape())
> +        return shaper.selectionRect(point, h, from, to);
> +    ASSERT_NOT_REACHED();
> +    return FloatRect();

Ditto.
Comment 5 Gyuyoung Kim 2012-07-24 09:03:05 PDT
Comment on attachment 154064 [details]
HarfbuzzNG support based on cairo and freetype.

Attachment 154064 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13324622
Comment 6 Dominik Röttsches (drott) 2012-07-24 09:55:00 PDT
Created attachment 154090 [details]
HarfbuzzNG support based on cairo and freetype, v2.
Comment 7 Dominik Röttsches (drott) 2012-07-24 09:59:34 PDT
(In reply to comment #4)
> (From update of attachment 154064 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154064&action=review
> 
> In addition to the comments, I would also like to seggest you to try building with pango backend and also a --minimal build.

--minimal build works.
I plan to remove the pango backend, see bug 92102.

> > Source/WebCore/ChangeLog:11
> > +        No new tests, complex font support is covered by existing tests.
> 
> I guess you might have some tests to unskip or rebaseline.

Yes, I plan to handle that separately in bug 92120.

> > Source/WebCore/PlatformEfl.cmake:170
> > +      platform/graphics/harfbuzz/HarfBuzzShaperBase.cpp
> > +      platform/graphics/harfbuzz/ng/HarfBuzzNGFace.cpp
> > +      platform/graphics/harfbuzz/ng/HarfBuzzNGFaceCairo.cpp
> > +      platform/graphics/harfbuzz/ng/HarfBuzzShaper.cpp
> > +    )
> > +    LIST(APPEND WebCore_LIBRARIES
> > +      ${HARFBUZZ_LIBRARIES}
> >      )
> 
> Maybe this should be conditoned to DWTF_USE_HARFBUZZ_NG. Usually the .cpp are always added to the build system and the contents protected by the flags.

The idea is that when freetype is the backend, Harfbuzz is mandatory. The WTF_USE_HARFBUZZ_NG define is only needed for cross-port compatibility with Chromium. So, the files are compiled completely and the library always needs to be added.


> > Source/WebCore/platform/graphics/cairo/FontCairoComplex.cpp:48
> > +    if (shaper.shape(&glyphBuffer)) {
> > +        drawGlyphBuffer(context, run, glyphBuffer, point);
> > +        return;
> > +    }
> > +    ASSERT_NOT_REACHED();
> 
> Isn't better to write as ASSERT(shaper.shape(&glyphBuffer)) and remove the ASSERT_NOT_REACHED()?

I removed the assertions and decided to do error logging instead. It's not improbably that something goes wrong inside the shaper due to encoding issues in the page. We shouldn't completely bail out for that.

In addition, build issues on EFL and GTK EWS (hopefully) fixed.
Comment 8 Dominik Röttsches (drott) 2012-07-24 10:02:43 PDT
*** Bug 91858 has been marked as a duplicate of this bug. ***
Comment 9 Kenneth Rohde Christiansen 2012-07-24 10:33:25 PDT
Simon, maybe this is interesting for you?
Comment 10 Kenneth Rohde Christiansen 2012-07-24 10:34:00 PDT
(In reply to comment #0)
> Similar to Chromium mac, we can add complex font rendering path to cairo using Harfbuzz NG shaper + drawGlyphBuffer.
> I propose to add a new file FontCairoComplex.cpp that implements this.

Wouldn't FontComplexCairo make more sense?
Comment 11 Dominik Röttsches (drott) 2012-07-24 10:39:10 PDT
(In reply to comment #10)
> Wouldn't FontComplexCairo make more sense?

There is FontCairo.cpp already. For consistency and given the fact that FontCairoComplex only implements the additional methods of class Font which deal with the complex path, I thought the "Complex" suffix was a good idea.
Comment 12 Kenneth Rohde Christiansen 2012-07-24 10:43:51 PDT
Comment on attachment 154090 [details]
HarfbuzzNG support based on cairo and freetype, v2.

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

> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzNGFaceCairo.cpp:106
> +    cairo_scaled_font_text_to_glyphs(scaledFont, 0, 0, utf8Codepoint.data(), utf8Codepoint.length(), &glyphs, &numGlyphs, 0, 0, 0);
> +    if (status != CAIRO_STATUS_SUCCESS)
> +        return false;
> +    ASSERT(numGlyphs > 0);

it is common to add newlines before and after blocks and returns.

> ChangeLog:10
> +        * Source/cmake/FindHarfBuzz.cmake: Added. Pkgconfig based discovery of HarfBuzz.

Added pkgconfig ...
Comment 13 Gyuyoung Kim 2012-07-24 10:44:49 PDT
Comment on attachment 154090 [details]
HarfbuzzNG support based on cairo and freetype, v2.

Attachment 154090 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13325521
Comment 14 Dominik Röttsches (drott) 2012-07-24 13:06:01 PDT
Created attachment 154126 [details]
HarfbuzzNG support based on cairo and freetype, v3
Comment 15 Dominik Röttsches (drott) 2012-07-24 13:08:10 PDT
(In reply to comment #12)
> (From update of attachment 154090 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154090&action=review
> 
> > Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzNGFaceCairo.cpp:106
> > +    cairo_scaled_font_text_to_glyphs(scaledFont, 0, 0, utf8Codepoint.data(), utf8Codepoint.length(), &glyphs, &numGlyphs, 0, 0, 0);
> > +    if (status != CAIRO_STATUS_SUCCESS)
> > +        return false;
> > +    ASSERT(numGlyphs > 0);
> 
> it is common to add newlines before and after blocks and returns.

Added newline after return, removed one unnecessary newline a bit further up. So it's even. :-)

> > ChangeLog:10
> > +        * Source/cmake/FindHarfBuzz.cmake: Added. Pkgconfig based discovery of HarfBuzz.
> 
> Added pkgconfig ...

Fixed.
Comment 16 Dominik Röttsches (drott) 2012-07-24 13:09:04 PDT
(In reply to comment #13)
> (From update of attachment 154090 [details])
> Attachment 154090 [details] did not pass efl-ews (efl):
> Output: http://queues.webkit.org/results/13325521

I believe this failure is an EWS artifact due to my touching jhbuild.modules here. I couldn't reproduce this failure after trying clean builds on two different machines.
Comment 17 Raphael Kubo da Costa (:rakuco) 2012-07-24 15:19:26 PDT
(In reply to comment #16)
> (In reply to comment #13)
> > (From update of attachment 154090 [details] [details])
> > Attachment 154090 [details] [details] did not pass efl-ews (efl):
> > Output: http://queues.webkit.org/results/13325521
> 
> I believe this failure is an EWS artifact due to my touching jhbuild.modules here. I couldn't reproduce this failure after trying clean builds on two different machines.

The error message looks pretty real:

/usr/bin/ld: ../../lib/libwebcore_efl.a(HarfBuzzShaper.cpp.o): undefined reference to symbol 'hb_buffer_get_length'
/usr/bin/ld: note: 'hb_buffer_get_length' is defined in DSO /mnt/eflews/git/webkit/WebKitBuild/Dependencies/Root/lib64/libharfbuzz.so.0 so try adding it to the linker command line
/mnt/eflews/git/webkit/WebKitBuild/Dependencies/Root/lib64/libharfbuzz.so.0: could not read symbols: Invalid operation

Looking at your FindHarfBuzz, I see you use PKG_CHECK_MODULES, which is case-sentitive. This means harfbuzz_LIBRARIES is being set instead of HARFBUZZ_LIBRARIES, so you end up not linking to harfbuzz at all in WebCore's PlatformEfl.cmake.
Comment 18 Raphael Kubo da Costa (:rakuco) 2012-07-24 15:32:46 PDT
Comment on attachment 154126 [details]
HarfbuzzNG support based on cairo and freetype, v3

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

> Source/cmake/FindHarfBuzz.cmake:3
> +INCLUDE(FindPkgConfig)
> +
> +PKG_CHECK_MODULES (harfbuzz REQUIRED harfbuzz>=0.9.0)

Comments with my build system hat:

 - First of all, this file should have a license/usage header (see FindGStreamer.cmake, for example).

 - Relying solely on pkg-config is also not very good, the main reason being that pkg-config will simply report paths (such as include and library locations) in their absolute form. This is why we currently have those ugly LDFLAGS variables in our build system files. You should instead use pkg-config as an additional helper tool, and use CMake's FIND_PATH and FIND_LIBRARY calls to actually find the things you are looking for. The general form of a simple FindFoo.cmake file would be something like this:

  FIND_PACKAGE(PkgConfig)

  PKG_CHECK_MODULES(PC_HARFBUZZ harfbuzz>=0.9.0)

  FIND_PATH(HARFBUZZ_INCLUDE_DIRS NAMES harfbuzz.h # whatever header you are looking for
      HINTS ${PC_HARFBUZZ_INCLUDE_DIRS} ${PC_HARFBUZZ_INCLUDEDIR}
  )

  FIND_LIBRARY(HARFBUZZ_LIBRARIES NAMES harfbuzz # "lib" is prepended automatically
      HINTS ${PC_HARFBUZZ_LIBRARY_DIRS} ${PC_HARFBUZZ_LIBDIR}
  )

  INCLUDE(FindPackageHandleStandardArgs)
  FIND_PACKAGE_HANDLE_STANDARD_ARGS(HarfBuzz DEFAULT_MSG HARFBUZZ_INCLUDE_DIRS HARFBUZZ_LIBRARIES)

By using CMake's own commands to find files and libraries, the resulting variables will have the full paths set. This means, on its turn, that you can use INCLUDE_DIRECTORIES(${HARFBUZZ_INCLUDE_DIRS}) to add the header's path to the compiler's include path, and using ${HARFBUZZ_LIBRARIES} in TARGET_LINK_LIBRARIES() will pass -L/full/path/to/libharfbuzz.so to the linker instead of -lharfbuzz.
Comment 19 Gyuyoung Kim 2012-07-24 15:33:43 PDT
Comment on attachment 154126 [details]
HarfbuzzNG support based on cairo and freetype, v3

Attachment 154126 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13324737
Comment 20 Raphael Kubo da Costa (:rakuco) 2012-07-24 16:01:47 PDT
Comment on attachment 154126 [details]
HarfbuzzNG support based on cairo and freetype, v3

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

Two additional comments:
 - Do you need to add the platform/graphics/harfbuzz includes to Tools/WTR and WebKit2 even if you do not link against it? Plus, do you need to link against HB in WebKit itself?
 - I couldn't find harfbuzz packages for Debian, Ubuntu, ArchLinux or FreeBSD (not with that name at least). Are we expected to externally depend on it or should it be bundled with the port's code (in a tarball, for example)?

> Source/WebKit/PlatformEfl.cmake:59
> +  LIST(APPEND WebCore_LIBRARIES

s/WebCore/WebKit/
Comment 21 Dominik Röttsches (drott) 2012-07-25 01:59:17 PDT
Created attachment 154294 [details]
HarfbuzzNG support based on cairo and freetype, v4
Comment 22 Dominik Röttsches (drott) 2012-07-25 02:06:22 PDT
(In reply to comment #18)
> (From update of attachment 154126 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154126&action=review
> 
> > Source/cmake/FindHarfBuzz.cmake:3
> > +INCLUDE(FindPkgConfig)
> > +
> > +PKG_CHECK_MODULES (harfbuzz REQUIRED harfbuzz>=0.9.0)
> 
> Comments with my build system hat:
>  - First of all, this file should have a license/usage header (see FindGStreamer.cmake, for example).

Fixed. I guess looking at FindEFL.cmake as an example was not a good idea for many reasons. Noone with a build system hat seems to have reviewed that one...

>  - Relying solely on pkg-config is also not very good, the main reason being that pkg-config will simply report paths (such as include and library locations) in their absolute form. This is why we currently have those ugly LDFLAGS variables in our build system files. You should instead use pkg-config as an additional helper tool, and use CMake's FIND_PATH and FIND_LIBRARY calls to actually find the things you are looking for. The general form of a simple FindFoo.cmake file would be something like this:
> [...]
> By using CMake's own commands to find files and libraries, the resulting variables will have the full paths set. This means, on its turn, that you can use INCLUDE_DIRECTORIES(${HARFBUZZ_INCLUDE_DIRS}) to add the header's path to the compiler's include path, and using ${HARFBUZZ_LIBRARIES} in TARGET_LINK_LIBRARIES() will pass -L/full/path/to/libharfbuzz.so to the linker instead of -lharfbuzz.

Thanks for the CMAKE lesson. Incorporated your suggestions, may the BSD gods have mercy. Still don't fully understand why it's better - but we can take that as a discussion offline. You're saying bad thing about pkg-config is that it results in "absolute form" paths, the improvement using the CMAKE methodology being that "the resulting variables will have the full paths set". What's the difference between full and absolute? :-)

(In reply to comment #20)
> (From update of attachment 154126 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154126&action=review
> 
> Two additional comments:
>  - Do you need to add the platform/graphics/harfbuzz includes to Tools/WTR and WebKit2 even if you do not link against it? Plus, do you need to link against HB in WebKit itself?

@Includes: Yes, needed since the HarfBuzzNGFace.h gets included.
@Linking: No, it's probably enough to link WebCore against Harfbuzz, hence...

> > Source/WebKit/PlatformEfl.cmake:59
> > +  LIST(APPEND WebCore_LIBRARIES
> 
> s/WebCore/WebKit/

removed.

>  - I couldn't find harfbuzz packages for Debian, Ubuntu, ArchLinux or FreeBSD (not with that name at least). Are we expected to externally depend on it or should it be bundled with the port's code (in a tarball, for example)?

Added a note in https://trac.webkit.org/wiki/EFLWebKit#Dependencies
Comment 23 Simon Hausmann 2012-07-25 05:18:54 PDT
Comment on attachment 154294 [details]
HarfbuzzNG support based on cairo and freetype, v4

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

> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzNGFaceCairo.cpp:93
> +static hb_bool_t harfbuzzGetGlyph(hb_font_t* hbFont, void* fontData, hb_codepoint_t unicode, hb_codepoint_t variationSelector, hb_codepoint_t* glyph, void* userData)

I think that this function is going to be a hot code path.

If you look at the CoreText implementation for example, then you can see that it avoids any allocations.

Is there a lower-level API that you can use?

> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzNGFaceCairo.cpp:105
> +    cairo_status_t status = CAIRO_STATUS_SUCCESS;
> +    cairo_glyph_t* glyphs = 0;
> +    int numGlyphs = 0;
> +    CString utf8Codepoint = UTF8Encoding().encode(reinterpret_cast<UChar*>(&unicode), 1, QuestionMarksForUnencodables);
> +    cairo_scaled_font_text_to_glyphs(scaledFont, 0, 0, utf8Codepoint.data(), utf8Codepoint.length(), &glyphs, &numGlyphs, 0, 0, 0);
> +    if (status != CAIRO_STATUS_SUCCESS)
> +        return false;

It looks like "status" is an unused variable. Did you mean to initialize it from the return value of cairo_scaled_font_text_to_glyphs?
Comment 24 Dominik Röttsches (drott) 2012-07-25 06:30:20 PDT
Created attachment 154338 [details]
HarfbuzzNG support based on cairo and freetype, v5
Comment 25 Dominik Röttsches (drott) 2012-07-25 06:32:55 PDT
(In reply to comment #23)
> (From update of attachment 154294 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154294&action=review
> 
> > Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzNGFaceCairo.cpp:93
> > +static hb_bool_t harfbuzzGetGlyph(hb_font_t* hbFont, void* fontData, hb_codepoint_t unicode, hb_codepoint_t variationSelector, hb_codepoint_t* glyph, void* userData)
> 
> I think that this function is going to be a hot code path.
> If you look at the CoreText implementation for example, then you can see that it avoids any allocations.
> Is there a lower-level API that you can use?

Yes, I wasn't exactly happy with the UTF16->UTF8 conversion here either. Thanks for pointing me at it again - we can actually use the corresponding Freetype function here directly (inspired by the Harfbuzz freetype implementation).
Comment 26 Raphael Kubo da Costa (:rakuco) 2012-07-25 07:00:53 PDT
Comment on attachment 154338 [details]
HarfbuzzNG support based on cairo and freetype, v5

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

> Source/cmake/FindHarfBuzz.cmake:27
> +#

This is not a requirement, but rather a friendly heads-up: FindFoo.cmake files normally include some documentation stating what they do and what variables are set (see FindGStreamer.cmake or http://cmake.org/gitweb?p=cmake.git;a=blob;f=Modules/readme.txt;h=7d61d8d27377733b6722d40e7c86843277b8e871;hb=HEAD , the latter for a much lengthier explanation).

> Source/cmake/FindHarfBuzz.cmake:31
> +PKG_CHECK_MODULES (harfbuzz REQUIRED harfbuzz>=0.9.0)

This seems to be a leftover.
Comment 27 Raphael Kubo da Costa (:rakuco) 2012-07-25 07:06:32 PDT
(In reply to comment #22)
> >  - Relying solely on pkg-config is also not very good, the main reason being that pkg-config will simply report paths (such as include and library locations) in their absolute form. This is why we currently have those ugly LDFLAGS variables in our build system files. You should instead use pkg-config as an additional helper tool, and use CMake's FIND_PATH and FIND_LIBRARY calls to actually find the things you are looking for. The general form of a simple FindFoo.cmake file would be something like this:
> > [...]
> > By using CMake's own commands to find files and libraries, the resulting variables will have the full paths set. This means, on its turn, that you can use INCLUDE_DIRECTORIES(${HARFBUZZ_INCLUDE_DIRS}) to add the header's path to the compiler's include path, and using ${HARFBUZZ_LIBRARIES} in TARGET_LINK_LIBRARIES() will pass -L/full/path/to/libharfbuzz.so to the linker instead of -lharfbuzz.
> 
> Thanks for the CMAKE lesson. Incorporated your suggestions, may the BSD gods have mercy. Still don't fully understand why it's better - but we can take that as a discussion offline. You're saying bad thing about pkg-config is that it results in "absolute form" paths, the improvement using the CMAKE methodology being that "the resulting variables will have the full paths set". What's the difference between full and absolute? :-)

Oops :-) What I meant is that CMake's PKG_CHECK_MODULES() will read the pkg-config .pc files, which normally have lines such as "Libs: -lfoo" which are then used in our TARGET_LINK_LIBRARIES() calls. In the end, the linker gets called with a command line that only says "ld -lfoo ...", which relies on libfoo.so being in a standard linker path (or -L being passed in the right order to the linker by other means). If, on the other hand, CMake's FIND_LIBRARY() is used, the linker will be called as "ld -l/opt/foo/bar/libfoo.so", which is safer.

So I guess there's no difference between full and absolute, it's pkg-config that does not report absolute paths (and me being terrible at giving lessons of any form ;)
Comment 28 Dominik Röttsches (drott) 2012-07-25 13:45:39 PDT
Created attachment 154433 [details]
HarfbuzzNG support based on cairo and freetype, v6
Comment 29 Dominik Röttsches (drott) 2012-07-25 13:48:53 PDT
(In reply to comment #26)
> (From update of attachment 154338 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154338&action=review
> 
> > Source/cmake/FindHarfBuzz.cmake:27
> > +#
> 
> This is not a requirement, but rather a friendly heads-up: FindFoo.cmake files normally include some documentation stating what they do and what variables are set (see FindGStreamer.cmake or 

Even though the FindHarfBuzz name here is rather self-evident, I put some sort documentation inside the file. :-) Thanks for the friendly heads-up.

> > Source/cmake/FindHarfBuzz.cmake:31
> > +PKG_CHECK_MODULES (harfbuzz REQUIRED harfbuzz>=0.9.0)
> 
> This seems to be a leftover.

Indeed, sorry. Removed.

Simon, Martin, how's the patch looking to you now? Can we get this landed?
Comment 30 Simon Hausmann 2012-07-26 08:29:01 PDT
Comment on attachment 154433 [details]
HarfbuzzNG support based on cairo and freetype, v6

r=me. I'm not super familiar with the cmake stuff, so I won't set cq+ just yet but give others time to comment.
Comment 31 Martin Robinson 2012-07-26 08:50:26 PDT
Comment on attachment 154433 [details]
HarfbuzzNG support based on cairo and freetype, v6

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

Glad to see this work! Sorry that I didn't pick it up when I said I would.

> Source/WebCore/platform/graphics/cairo/FontCairoComplex.cpp:3
> +/*
> + * Copyright (C) 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
> + * Copyright (C) 2012 Intel Corporation

How about FontCairoHarfbuzzNG to match the naming scheme of the other files? FontCairoComplex seems too general, as it doesn't cover the Windows port (for example).

> Source/WebCore/platform/graphics/cairo/FontCairoComplex.cpp:37
> +#include "SimpleFontData.h"
> +
> +#include <cairo.h>

No newline is required here.
Comment 32 Dominik Röttsches (drott) 2012-07-27 01:40:00 PDT
Created attachment 154877 [details]
HarfbuzzNG support based on cairo and freetype, final.
Comment 33 Dominik Röttsches (drott) 2012-07-27 01:40:59 PDT
(In reply to comment #31)

> How about FontCairoHarfbuzzNG to match the naming scheme of the other files? FontCairoComplex seems too general, as it doesn't cover the Windows port (for example).

Good idea, thanks - done.

> No newline is required here.

Removed.
Comment 34 WebKit Review Bot 2012-07-27 04:33:07 PDT
Comment on attachment 154877 [details]
HarfbuzzNG support based on cairo and freetype, final.

Clearing flags on attachment: 154877

Committed r123864: <http://trac.webkit.org/changeset/123864>
Comment 35 WebKit Review Bot 2012-07-27 04:33:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 36 Dominik Röttsches (drott) 2012-07-30 05:40:17 PDT
Reopening to attach new patch.
Comment 37 Dominik Röttsches (drott) 2012-07-30 05:40:30 PDT
Created attachment 155271 [details]
Using cairo_scaled_font approach, cleanup for FindHarfbuzz
Comment 38 Dominik Röttsches (drott) 2012-07-30 05:41:33 PDT
(In reply to comment #37)
> Created an attachment (id=155271) [details]
> Using cairo_scaled_font approach, cleanup for FindHarfbuzz

Actually, the Freetype based harfbuzzGetGlyph implementation produces incorrect results for

fast/dom/52776.html
fast/text/atsui-negative-spacing-features.html
fast/text/atsui-spacing-features.html

I would like to revert to the cairo based implementation for now and leave the optimization to switch to a lower level library for a later step.

Also, cleaning up an accidental leftover in FindHarfbuzz.cmake.
Comment 39 Dominik Röttsches (drott) 2012-07-30 05:44:32 PDT
(In reply to comment #38)

> I would like to revert to the cairo based implementation for now and leave the optimization to switch to a lower level library for a later step.

Created Bug 92641 to track this.
Comment 40 Dominik Röttsches (drott) 2012-08-01 11:32:22 PDT
Simon, Martin - would you have time to r+ the recent change?
Comment 41 Martin Robinson 2012-08-02 04:37:14 PDT
Comment on attachment 155271 [details]
Using cairo_scaled_font approach, cleanup for FindHarfbuzz

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

> Source/WebCore/ChangeLog:9
> +        fails to procude correct results for some tests.

procude -> produce :)

> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzNGFaceCairo.cpp:103
> +    status = cairo_scaled_font_text_to_glyphs(scaledFont, 0, 0, utf8Codepoint.data(), utf8Codepoint.length(), &glyphs, &numGlyphs, 0, 0, 0);

Why not just do:

if (CAIRO_STATUS_SUCCESS != cairo_scaled_font_text_to_glyphs(scaledFont, 0, 0, utf8Codepoint.data(), utf8Codepoint.length(), &glyphs, &numGlyphs, 0, 0, 0))
    return false;

> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzNGFaceCairo.cpp:107
> +    if (status != CAIRO_STATUS_SUCCESS)
> +        return false;
> +
> +    ASSERT(numGlyphs > 0);

It's a bit odd to ASSERT against the return value of third-party software. Better here to mistrust it and handle that situation:

if (!numGlyphs)
    return false;

> Source/cmake/FindHarfBuzz.cmake:31
> +# Try to find Harfbuzz include and library directories.
>  #
> +# After successful discovery, this will set for inclusion where needed:
> +# HARFBUZZ_INCLUDE_DIRS - containg the HarfBuzz headers
> +# HARFBUZZ_LIBRARIES - containg the HarfBuzz library

This change seems unrelated, so perhaps it could be split into another patch.
Comment 42 Dominik Röttsches (drott) 2012-08-02 06:28:53 PDT
Created attachment 156067 [details]
Using cairo_scaled_font approach, review comments addressed
Comment 43 Dominik Röttsches (drott) 2012-08-02 06:35:20 PDT
Thanks a lot Martin - that was very quick :-)
Comment 44 WebKit Review Bot 2012-08-02 07:21:28 PDT
Comment on attachment 156067 [details]
Using cairo_scaled_font approach, review comments addressed

Clearing flags on attachment: 156067

Committed r124454: <http://trac.webkit.org/changeset/124454>
Comment 45 WebKit Review Bot 2012-08-02 07:21:36 PDT
All reviewed patches have been landed.  Closing bug.