Bug 92098

This bug removes Pango from WebKit-Gtk spelling implementation (https://bugs.webkit.org/show_bug.cgi?id=94320).

I've enabled this locally and I notice some differences in pixel output that look a bit like regressions. For instance, the Arabic characters in LayoutTests/platform/efl/fast/text/complex-text-opacity-expected.png are not elided properly.

(In reply to comment #2) > I've enabled this locally and I notice some differences in pixel output that look a bit like regressions. For instance, the Arabic characters in LayoutTests/platform/efl/fast/text/complex-text-opacity-expected.png are not elided properly. Does this change depending on the Harfbuzz version? Looking at their mailing list, there's lots of changes going on.

(In reply to comment #3) > (In reply to comment #2) > > I've enabled this locally and I notice some differences in pixel output that look a bit like regressions. For instance, the Arabic characters in LayoutTests/platform/efl/fast/text/complex-text-opacity-expected.png are not elided properly. > > Does this change depending on the Harfbuzz version? Looking at their mailing list, there's lots of changes going on. I see this with the latest harfbuzz release (0.9.5).

(In reply to comment #4) > I see this with the latest harfbuzz release (0.9.5). The issue seems to be with the platform harfbuzz code. Instead of splitting complex text into runs and choosing a font appropriately, the behavior I see is that the text uses the default font. For each character of the complex text that is not in the default font we are falling back to FontCache::getFontDataForCharacters. In the end harfbuzz is not combining those glyphs properly, because they are fallback glyphs. I guess we should be choosing the appropriate fallback font earlier in the process. bashi, do you think you can give more information about how this works for Chromium with the HarfbuzzNG path?

Adding behdad. (In reply to comment #5) > (In reply to comment #4) > > > I see this with the latest harfbuzz release (0.9.5). > > The issue seems to be with the platform harfbuzz code. Instead of splitting complex text into runs and choosing a font appropriately, the behavior I see is that the text uses the default font. For each character of the complex text that is not in the default font we are falling back to FontCache::getFontDataForCharacters. In the end harfbuzz is not combining those glyphs properly, because they are fallback glyphs. > > I guess we should be choosing the appropriate fallback font earlier in the process. > > bashi, do you think you can give more information about how this works for Chromium with the HarfbuzzNG path? (In reply to comment #5) > (In reply to comment #4) > > > I see this with the latest harfbuzz release (0.9.5). > > The issue seems to be with the platform harfbuzz code. Instead of splitting complex text into runs and choosing a font appropriately, the behavior I see is that the text uses the default font. For each character of the complex text that is not in the default font we are falling back to FontCache::getFontDataForCharacters. In the end harfbuzz is not combining those glyphs properly, because they are fallback glyphs. > > I guess we should be choosing the appropriate fallback font earlier in the process. > > bashi, do you think you can give more information about how this works for Chromium with the HarfbuzzNG path? Are you trying to use HarfBuzzShaper on GTK? If so, HarfBuzzShaper::collectHarfBuzzRuns() doesn't select fonts properly?

Mystery solved (I hope). When the Fontconfig FontCache returns a fallback font, it returns a different font each time -- even if the Fontconfig pattern returns the same font. This is quite bad, as it means that we're keeping many duplicate copies of fallback fonts in the cache. I've fixed this locally and the results look a lot better. The reason this was causing a problem for Harfbuzz was that HarfBuzzShaper saw each fallback font as a different font and created a separate text run for each character.

Created attachment 171729 [details] Patch

I didn't include the new layout test results because the made the patch a bit too big.

It looks like the bot is missing ICU pkg-config files.

(In reply to comment #10) > It looks like the bot is missing ICU pkg-config files. The bot has libicu-dev 4.8.1.1-9. Seems like the harfbuzz headers are not found instead when building WebCore. Is a clean build required?

(In reply to comment #9) > I didn't include the new layout test results because the made the patch a bit too big. Glad you found the reason. You may ping our gardener on IRC once you land this, so that we can take care of the EFL rebaselining. http://trac.webkit.org/wiki/EFLWebKitBuildBots

(In reply to comment #11) > (In reply to comment #10) > > It looks like the bot is missing ICU pkg-config files. > > The bot has libicu-dev 4.8.1.1-9. Seems like the harfbuzz headers are not found instead when building WebCore. > > Is a clean build required? In Debian, up until very recently, the libicu-dev package did not install pkg-config files. See here: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=687339 I'm not sure what the best way to resolve this is. Locally I install libicu manually.

(In reply to comment #13) > I'm not sure what the best way to resolve this is. I'm proposing http://lists.freedesktop.org/archives/harfbuzz/2012-November/002612.html Since we need to bump the version in bug 101323 as well.

Comment on attachment 171729 [details] Patch LGTM, and I think the bots have all been updated some time ago, haven't they?

Committed r137146: <http://trac.webkit.org/changeset/137146>

Looks life this patch affected the results for EFL port? About 50 test cases seem to require rebaselining after this patch? Was that expected?

Ok, I see from https://bugs.webkit.org/show_bug.cgi?id=92098#c12 that the rebaseline was expected. I guess I'll take care of it then.

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

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