Bug 90951 - [Chromium] Fix bugs in HarfBuzzShaper
Summary: [Chromium] Fix bugs in HarfBuzzShaper
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kenichi Ishibashi
URL:
Keywords:
Depends on:
Blocks: 96902
  Show dependency treegraph
 
Reported: 2012-07-11 01:23 PDT by Kenichi Ishibashi
Modified: 2012-09-17 00:39 PDT (History)
3 users (show)

See Also:


Attachments
Patch (24.18 KB, patch)
2012-07-11 01:35 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch V2 (32.90 KB, patch)
2012-07-11 22:32 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch for landing (32.54 KB, patch)
2012-07-12 16:52 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch for landing (32.46 KB, patch)
2012-07-12 19:20 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenichi Ishibashi 2012-07-11 01:23:08 PDT
This is the leftover of https://bugs.webkit.org/show_bug.cgi?id=90362. The current implementation has some problems. Will post patch soon.
Comment 1 Kenichi Ishibashi 2012-07-11 01:35:23 PDT
Created attachment 151644 [details]
Patch
Comment 2 Tony Chang 2012-07-11 10:41:37 PDT
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?
Comment 3 Kenichi Ishibashi 2012-07-11 22:32:10 PDT
Created attachment 151858 [details]
Patch V2
Comment 4 Kenichi Ishibashi 2012-07-11 22:38:58 PDT
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 5 Tony Chang 2012-07-12 10:24:08 PDT
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.
Comment 6 Kenichi Ishibashi 2012-07-12 16:52:58 PDT
Created attachment 152098 [details]
Patch for landing
Comment 7 Kenichi Ishibashi 2012-07-12 16:54:46 PDT
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 8 WebKit Review Bot 2012-07-12 18:28:39 PDT
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
Comment 9 Kenichi Ishibashi 2012-07-12 19:20:28 PDT
Created attachment 152132 [details]
Patch for landing
Comment 10 WebKit Review Bot 2012-07-13 04:17:03 PDT
Comment on attachment 152132 [details]
Patch for landing

Clearing flags on attachment: 152132

Committed r122562: <http://trac.webkit.org/changeset/122562>
Comment 11 WebKit Review Bot 2012-07-13 04:17:08 PDT
All reviewed patches have been landed.  Closing bug.