Bug 97676

Created attachment 165792 [details] WIP

Created attachment 165993 [details] Patch

Hi Tony, Could you take a look? Instead using ipafont(c.f. https://bugs.webkit.org/show_bug.cgi?id=97682#c4), I'd like to add a fast path for fonts which have vhea/vmtx table when drawing vertical text. This patch doesn't change behavior on LayoutTests.

Comment on attachment 165993 [details] Patch Attachment 165993 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14001061 New failing tests: fast/text/international/text-spliced-font.html

Comment on attachment 165993 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165993&action=review The EWS bot failure looks real. > Source/WebCore/platform/graphics/harfbuzz/FontHarfBuzz.cpp:100 > + canvas->drawTextOnPath(glyphs + i, 2, path, 0, paint); Can we use sizeof(GlyphBufferGlyph) instead of hardcoding 2? > Source/WebCore/platform/graphics/harfbuzz/FontHarfBuzz.cpp:121 > + canvas->drawTextOnPath(glyphs + i, 2, path, 0, paint); Ditto.

Comment on attachment 165993 [details] Patch Attachment 165993 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14060007 New failing tests: fast/text/international/text-spliced-font.html

Created attachment 166079 [details] Patch for landing

Created attachment 166082 [details] Patch for landing

Comment on attachment 166082 [details] Patch for landing Rejecting attachment 166082 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/mac/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/14038829

Comment on attachment 165993 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165993&action=review Thank you for review! I should have disabled setVerticalText() in drawComplexText() because we don't support vertical text on complex path now. It's will be supported after hb-ng transition. >> Source/WebCore/platform/graphics/harfbuzz/FontHarfBuzz.cpp:100 >> + canvas->drawTextOnPath(glyphs + i, 2, path, 0, paint); > > Can we use sizeof(GlyphBufferGlyph) instead of hardcoding 2? Done. >> Source/WebCore/platform/graphics/harfbuzz/FontHarfBuzz.cpp:121 >> + canvas->drawTextOnPath(glyphs + i, 2, path, 0, paint); > > Ditto. Done.

Comment on attachment 166082 [details] Patch for landing Rejecting attachment 166082 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/mac/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/14034956

Created attachment 166086 [details] Patch for landing

Comment on attachment 166086 [details] Patch for landing Rejecting attachment 166086 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/mac/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/14066002

(In reply to comment #13) > (From update of attachment 166086 [details]) > Rejecting attachment 166086 [details] from commit-queue. > > Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 > > ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/mac/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). > > Full output: http://queues.webkit.org/results/14066002 Hmm, we saw the same thing on a different patch. Maybe a bug in the commit queue?

(In reply to comment #14) > Hmm, we saw the same thing on a different patch. Maybe a bug in the commit queue? That might be.. I'll land the patch by hand.

Yes, the bot is having trouble today for some reason.

Committed r129823: <http://trac.webkit.org/changeset/129823>

This patch caused a bunch of image failures on Chromium Linux. They look like they just need rebaselines. Bashi, mind taking a look?

Actually, on closer inspection, the new results look bad to me. The glyphs are oddly compressed and small. I'm going to rollout as I haven't gotten a hold of bashi.

Re-opened since this is blocked by bug 97837

I couldn't reproduce the failures on my linux. I need the fonts which are used by chromium canary bots to investigate the failures.

Not sure. Tony or Adam probably know how we deal with fonts on Linux DRT. I'm pretty sure we include a fixed set of fonts. Maybe we just need to add this font to the list. Does it have a license that we can use?

(In reply to comment #22) > Not sure. Tony or Adam probably know how we deal with fonts on Linux DRT. I'm pretty sure we include a fixed set of fonts. Maybe we just need to add this font to the list. Does it have a license that we can use? Sorry, my previous comment wasn't enough. I believe Linux DRT uses "kochi mincho" and "kochi gothic" for vertical text tests. I'm guessing that kochi mincho font on bots would be different from mine. I need the fonts which are actually used on the bots to investigate the cause.

(In reply to comment #23) > (In reply to comment #22) > > Not sure. Tony or Adam probably know how we deal with fonts on Linux DRT. I'm pretty sure we include a fixed set of fonts. Maybe we just need to add this font to the list. Does it have a license that we can use? > > Sorry, my previous comment wasn't enough. I believe Linux DRT uses "kochi mincho" and "kochi gothic" for vertical text tests. I'm guessing that kochi mincho font on bots would be different from mine. I need the fonts which are actually used on the bots to investigate the cause. Are you using Lucid or Precise? All the bots run Lucid and running install-build-deps.sh should get the right version of the fonts. Also, I suspect that the failure of fast/text/international/text-spliced-font.html on the EWS bot is real. Ojan, did more than that one test fail?

(In reply to comment #24) > Ojan, did more than that one test fail? Nevermind, I found the list of failing tests on the other bug.

It looks like the glyphs are too far to the left and getting cut off: http://build.chromium.org/f/chromium/layout_test_results/Webkit_Linux/results/layout-test-results/editing/selection/vertical-rl-ltr-extend-line-forward-p-actual.png

I checked on the build.chromium.org bots. They have: ii ttf-kochi-gothic 20030809-6 Kochi Subst Gothic Japanese TrueType font wi ii ttf-kochi-mincho 20030809-6 Kochi Subst Mincho Japanese TrueType font wi Which is the same as on my machine.

(In reply to comment #27) > I checked on the build.chromium.org bots. They have: > ii ttf-kochi-gothic 20030809-6 Kochi Subst Gothic Japanese TrueType font wi > ii ttf-kochi-mincho 20030809-6 Kochi Subst Mincho Japanese TrueType font wi > > Which is the same as on my machine. I'm using Lucid and the version of ttf-kochi-{gothic,mincho} is the same. Hmm.. I wonder why the failures appears on chromium canary bots but don't appear on EWS bots. How can I get the skia version the bots are using? Skia guys has improved vertical text rendering (e.g. r5677)

(In reply to comment #28) > (In reply to comment #27) > > I checked on the build.chromium.org bots. They have: > > ii ttf-kochi-gothic 20030809-6 Kochi Subst Gothic Japanese TrueType font wi > > ii ttf-kochi-mincho 20030809-6 Kochi Subst Mincho Japanese TrueType font wi > > > > Which is the same as on my machine. > > I'm using Lucid and the version of ttf-kochi-{gothic,mincho} is the same. Hmm.. > I wonder why the failures appears on chromium canary bots but don't appear on EWS bots. > > How can I get the skia version the bots are using? Skia guys has improved vertical text rendering (e.g. r5677) You can look at the update step on the bots. E.g., on http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux/builds/31607/steps/update/logs/stdio , the skia revision is in this line: 122>________ running 'svn update /mnt/data/b/build/slave/Webkit_Linux/build/src/third_party/skia/gyp --revision 5746 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/data/b/build/slave/Webkit_Linux/build' The build.chromium.org use a full chromium checkout while the EWS bots use a WebKit only checkout. Maybe that mattered at the time? You could try uploading again or landing again to see if things are different now.

(In reply to comment #29) > You can look at the update step on the bots. E.g., on http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux/builds/31607/steps/update/logs/stdio , the skia revision is in this line: > > 122>________ running 'svn update /mnt/data/b/build/slave/Webkit_Linux/build/src/third_party/skia/gyp --revision 5746 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/data/b/build/slave/Webkit_Linux/build' > > The build.chromium.org use a full chromium checkout while the EWS bots use a WebKit only checkout. Maybe that mattered at the time? You could try uploading again or landing again to see if things are different now. Thank you for the information. I used tryserver and confirmed the patch doesn't pass the vertical text tests. I'm investigating the cause.