This is the leftover of https://bugs.webkit.org/show_bug.cgi?id=90362. The current implementation has some problems. Will post patch soon.
Created attachment 151644 [details] Patch
Comment on attachment 151644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151644&action=review r- for lack of tests. > Source/WebCore/ChangeLog:14 > + - Cannot render RTL text if the TextRun is divided into more than two > + HarfBuzzRun. > + - Script handling in TextRun partitioning is incorrect. > + - Inaccurate calculation of selection rect. > + - Wrong rendering position when the first glyph of the TextRun have > + non-zero offsets in terms of HarfBuzz. Why are there no tests? Doesn't this code already run on Chromium Mac? We should have a test showing that each of these are fixed. > Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzShaper.cpp:230 > + return m_harfbuzzRuns.size() > 0; !m_harfbuzzRuns.isEmpty() > Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzShaper.cpp:256 > + hb_buffer_destroy(harfbuzzBuffer); Should we make a scoped pointer type that calls hb_buffer_destroy in the destructor? Maybe for hb_font_t* too? > Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzShaper.cpp:260 > + hb_shape(harfbuzzFont, harfbuzzBuffer, m_features.size() > 0 ? m_features.data() : 0, m_features.size()); Nit: m_features.isEmpty() > Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzShaper.h:111 > + float m_startOffsetX; > + float m_startOffsetY; Maybe use a FloatPoint here instead?
Created attachment 151858 [details] Patch V2
Comment on attachment 151644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151644&action=review Thank you so much for review! >> Source/WebCore/ChangeLog:14 >> + non-zero offsets in terms of HarfBuzz. > > Why are there no tests? Doesn't this code already run on Chromium Mac? We should have a test showing that each of these are fixed. Added some tests. Chromium Mac only uses this shaper iff -webkit-font-feature-settings is specified and corresponding font is an opentype font. Tests looks weird because of this. Anyway, all fix of this change will be covered by existing tests once we switch to use harfbuzz-ng on Linux. >> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzShaper.cpp:230 >> + return m_harfbuzzRuns.size() > 0; > > !m_harfbuzzRuns.isEmpty() Done. >> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzShaper.cpp:256 >> + hb_buffer_destroy(harfbuzzBuffer); > > Should we make a scoped pointer type that calls hb_buffer_destroy in the destructor? Maybe for hb_font_t* too? Done. >> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzShaper.cpp:260 >> + hb_shape(harfbuzzFont, harfbuzzBuffer, m_features.size() > 0 ? m_features.data() : 0, m_features.size()); > > Nit: m_features.isEmpty() Done. >> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzShaper.h:111 >> + float m_startOffsetY; > > Maybe use a FloatPoint here instead? Done.
Comment on attachment 151858 [details] Patch V2 View in context: https://bugs.webkit.org/attachment.cgi?id=151858&action=review > LayoutTests/ChangeLog:13 > + * fast/text/harfbuzz-script-order.html: Added. > + * fast/text/harfbuzz-selection.html: Added. It's a little weird that the tests have the name harfbuzz in them since they might not always use harfbuzz. Maybe we should use a different name like complex-text-script-order.html or shaping-script-order.html. You might want to just make a subdirectory for these tests (fast/text/complex or fast/text/shaping or fast/text/font-feature-setting) and just skip the directory on other platforms.
Created attachment 152098 [details] Patch for landing
Thank you for review. (In reply to comment #5) > It's a little weird that the tests have the name harfbuzz in them since they might not always use harfbuzz. Maybe we should use a different name like complex-text-script-order.html or shaping-script-order.html. You might want to just make a subdirectory for these tests (fast/text/complex or fast/text/shaping or fast/text/font-feature-setting) and just skip the directory on other platforms. Created fast/text/shaping subdirectory and moved these tests into the subdirectory.
Comment on attachment 152098 [details] Patch for landing Rejecting attachment 152098 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: hunk FAILED -- saving rejects to file LayoutTests/platform/qt/TestExpectations.rej patching file LayoutTests/platform/win/Skipped Hunk #1 succeeded at 1944 (offset -1 lines). patching file LayoutTests/platform/wincairo/Skipped Hunk #1 succeeded at 2126 (offset -1 lines). patching file LayoutTests/platform/wk2/Skipped Hunk #1 succeeded at 1419 (offset -2 lines). Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/13206920
Created attachment 152132 [details] Patch for landing
Comment on attachment 152132 [details] Patch for landing Clearing flags on attachment: 152132 Committed r122562: <http://trac.webkit.org/changeset/122562>
All reviewed patches have been landed. Closing bug.