Bug 91864 – [Cairo] Add complex font drawing using HarfbuzzNG

• Component: Platform
• Status: RESOLVED FIXED

[Dominik Röttsches (drott)]

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.

[Dominik Röttsches (drott)]

Created attachment 154062 [details]
HarfbuzzNG support based on cairo-ft.

[Dominik Röttsches (drott)]

Created attachment 154064 [details]
HarfbuzzNG support based on cairo and freetype.

[Dominik Röttsches (drott)]

This is enabling the code for EFL right away, GTK support is tracked in bug 92098.

[Thiago Marcos P. Santos]

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.

[Gyuyoung Kim]

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

[Dominik Röttsches (drott)]

Created attachment 154090 [details]
HarfbuzzNG support based on cairo and freetype, v2.

[Dominik Röttsches (drott)]

(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.

[Dominik Röttsches (drott)]

*** Bug 91858 has been marked as a duplicate of this bug. ***

[Kenneth Rohde Christiansen]

Simon, maybe this is interesting for you?

[Kenneth Rohde Christiansen]

(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?

[Dominik Röttsches (drott)]

(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.

[Kenneth Rohde Christiansen]

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 ...

[Gyuyoung Kim]

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

[Dominik Röttsches (drott)]

Created attachment 154126 [details]
HarfbuzzNG support based on cairo and freetype, v3

[Dominik Röttsches (drott)]

(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.

[Dominik Röttsches (drott)]

(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.

[Raphael Kubo da Costa (:rakuco)]

(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.

[Raphael Kubo da Costa (:rakuco)]

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.

[Gyuyoung Kim]

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

[Raphael Kubo da Costa (:rakuco)]

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/

[Dominik Röttsches (drott)]

Created attachment 154294 [details]
HarfbuzzNG support based on cairo and freetype, v4

[Dominik Röttsches (drott)]

(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

[Simon Hausmann]

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?

[Dominik Röttsches (drott)]

Created attachment 154338 [details]
HarfbuzzNG support based on cairo and freetype, v5

[Dominik Röttsches (drott)]

(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).

[Raphael Kubo da Costa (:rakuco)]

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.

[Raphael Kubo da Costa (:rakuco)]

(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 ;)

[Dominik Röttsches (drott)]

Created attachment 154433 [details]
HarfbuzzNG support based on cairo and freetype, v6

[Dominik Röttsches (drott)]

(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?

[Simon Hausmann]

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.

[Martin Robinson]

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.

[Dominik Röttsches (drott)]

Created attachment 154877 [details]
HarfbuzzNG support based on cairo and freetype, final.

[Dominik Röttsches (drott)]

(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.

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dominik Röttsches (drott)]

Reopening to attach new patch.

[Dominik Röttsches (drott)]

Created attachment 155271 [details]
Using cairo_scaled_font approach, cleanup for FindHarfbuzz

[Dominik Röttsches (drott)]

(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.

[Dominik Röttsches (drott)]

(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.

[Dominik Röttsches (drott)]

Simon, Martin - would you have time to r+ the recent change?

[Martin Robinson]

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.

[Dominik Röttsches (drott)]

Created attachment 156067 [details]
Using cairo_scaled_font approach, review comments addressed

[Dominik Röttsches (drott)]

Thanks a lot Martin - that was very quick :-)

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.