Bug 91864

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

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

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

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

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

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

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

Simon, maybe this is interesting for you?

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

All reviewed patches have been landed. Closing bug.

Reopening to attach new patch.

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

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

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

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

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.

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

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

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>

All reviewed patches have been landed. Closing bug.