Bug 45191 – [chromium] Fix complex text word spacing on Linux.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Adam Langley]

I broke complex text word spacing with r66689. I misnamed a variable in the original code |glyphIndex| when it was actually indexing code points. That meant that I compared it against the wrong limit when working around Harfbuzz issues and neatly disabled word spacing.

[Adam Langley]

Created attachment 66530 [details]
Patch

[Tony Chang]

Can you also remove the line from test_expectations.txt where I marked the test as failing?

[Adam Langley]

Comment on attachment 66530 [details]
Patch

Will land on Monday (with the test-expectations changing included): landing on a Friday afternoon is tempting fate.

[Adam Langley]

r66882

[Tony Chang]

(In reply to comment #4)
> r66882

Hmm, it still looks a bit off.  It looks like the initial spacing is lost.

Expected:
http://trac.webkit.org/export/66888/trunk/LayoutTests/platform/chromium-linux/fast/text/atsui-spacing-features-expected.png

Actual:
http://build.chromium.org/buildbot/layout_test_results/webkit-rel-linux-webkit-org/results/layout-test-results/fast/text/atsui-spacing-features-actual.png

[Tony Chang]

Reopening

[Adam Langley]

(In reply to comment #5)
> Hmm, it still looks a bit off.  It looks like the initial spacing is lost.

I checked by running the layout tests. However, I now imagine that the line in test_expectations.txt would have suppressed the failure, right?

I'm not sure that either result is currently more 'right', but we should probably not change it for change's sake so I'll take another look.

[Tony Chang]

(In reply to comment #7)
> (In reply to comment #5)
> > Hmm, it still looks a bit off.  It looks like the initial spacing is lost.
> 
> I checked by running the layout tests. However, I now imagine that the line in test_expectations.txt would have suppressed the failure, right?

Yes, if you ran the test before updating test_expectations.txt.

> I'm not sure that either result is currently more 'right', but we should probably not change it for change's sake so I'll take another look.

Thanks, Adam!  Sorry this is such a yak shave . . .

[Adam Langley]

Rebaselined in r67400.

I played with a few different things but nothing was clearly better. The new baseline doesn't include word-spacing at the right-hand-side of RTL text, which is better (I think).