Bug 97093 - [Chromium] Improve glyph positioning of HarfBuzzShaper
Summary: [Chromium] Improve glyph positioning of 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:
 
Reported: 2012-09-19 03:08 PDT by Kenichi Ishibashi
Modified: 2012-09-19 17:00 PDT (History)
2 users (show)

See Also:


Attachments
Patch (14.82 KB, patch)
2012-09-19 03:12 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch for landing (14.82 KB, patch)
2012-09-19 15:25 PDT, Kenichi Ishibashi
no flags Details | Formatted Diff | Diff
Patch for landing (14.84 KB, patch)
2012-09-19 15:54 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-09-19 03:08:57 PDT
There are two problems in glyph positioning of HarfBuzzShaper:
- If the shaper produces multiple HarfBuzzRuns, the position of first glyph of second and following runs could be wrong if the offsets of the precedence glyph (that is the last glyph of the previous run) aren't zero.
- Letter spacing and word spacing are added at the right side of characters/words even for RTL run. Spacing should be added at the left side of characters/words.

We should fix these problems.
Comment 1 Kenichi Ishibashi 2012-09-19 03:12:20 PDT
Created attachment 164705 [details]
Patch
Comment 2 Kenichi Ishibashi 2012-09-19 03:13:06 PDT
Hi Tony,

Could you take a look?
Comment 3 Tony Chang 2012-09-19 10:27:30 PDT
Comment on attachment 164705 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=164705&action=review

FWIW, if you uploaded before and after png results for the tests that change as a separate attachment to the bug, it would help me understand the change.

> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzShaper.cpp:319
> +    // HarfBuzz returns the shaping result in visual order. We need not to flip for RTL

Nit: Missing period at the end of the sentence.
Comment 4 Kenichi Ishibashi 2012-09-19 15:25:28 PDT
Created attachment 164786 [details]
Patch for landing
Comment 5 Kenichi Ishibashi 2012-09-19 15:26:37 PDT
(In reply to comment #3)
> (From update of attachment 164705 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164705&action=review
> 
> FWIW, if you uploaded before and after png results for the tests that change as a separate attachment to the bug, it would help me understand the change.
> 

I should have done so. I'll do it next time.

> > Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzShaper.cpp:319
> > +    // HarfBuzz returns the shaping result in visual order. We need not to flip for RTL
> 
> Nit: Missing period at the end of the sentence.

Done.
Comment 6 WebKit Review Bot 2012-09-19 15:47:15 PDT
Comment on attachment 164786 [details]
Patch for landing

Rejecting attachment 164786 [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:
kitpy/layout_tests/models/test_expectations_unittest.py
Failed to merge in the changes.
Patch failed at 0001 implement first part of support for the new TestExpectations syntax

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.

Full output: http://queues.webkit.org/results/13905607
Comment 7 Kenichi Ishibashi 2012-09-19 15:54:47 PDT
Created attachment 164791 [details]
Patch for landing
Comment 8 WebKit Review Bot 2012-09-19 17:00:24 PDT
Comment on attachment 164791 [details]
Patch for landing

Clearing flags on attachment: 164791

Committed r129074: <http://trac.webkit.org/changeset/129074>
Comment 9 WebKit Review Bot 2012-09-19 17:00:28 PDT
All reviewed patches have been landed.  Closing bug.