Bug 90951 – [Chromium] Fix bugs in HarfBuzzShaper

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Kenichi Ishibashi]

This is the leftover of https://bugs.webkit.org/show_bug.cgi?id=90362. The current implementation has some problems. Will post patch soon.

[Kenichi Ishibashi]

Created attachment 151644 [details]
Patch

[Tony Chang]

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?

[Kenichi Ishibashi]

Created attachment 151858 [details]
Patch V2

[Kenichi Ishibashi]

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.

[Tony Chang]

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.

[Kenichi Ishibashi]

Created attachment 152098 [details]
Patch for landing

[Kenichi Ishibashi]

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.

[WebKit Review Bot]

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

[Kenichi Ishibashi]

Created attachment 152132 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 152132 [details]
Patch for landing

Clearing flags on attachment: 152132

Committed r122562: <http://trac.webkit.org/changeset/122562>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.