Bug 39014 – [chromium] Improve spacing support for complex text on Linux

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Adam Langley]

Previously, our complex text support ignored word-spacing, justification and letter-spacing. This fixes the first two issues and allows us to render Scribd's HTML5 documents much better.

[Adam Langley]

Created attachment 55885 [details]
patch

I get differing opinions on what the current policy on adding layout tests is, but I'm doing what I /think/ is correct here.

[Tony Chang]

I think you're adding a layout test correctly.  There should probably be at least the Chromium Linux results in LayoutTests/platform/chromium-linux.  I assume you're planning on getting the other platform results from the bots after you commit.

[Adam Langley]

Created attachment 56259 [details]
patch

[Gustavo Noronha (kov)]

Out of curiosity, what are the first two issues?

[Adam Langley]

(In reply to comment #4)
> Out of curiosity, what are the first two issues?

word-spacing and justification. See the comments in the patch about why we still don't handle letter spacing.

(Note that Scribd is still a little broken on Windows, although I think I know what the issue is and will hopefully get it fixed after this patch.)

[Kent Tamura]

Comment on attachment 56259 [details]
patch

I'm not familiar with this part of code, but
 - Update your WebKit revision and remake the patch
 - The patch lacks complex-text-word-spacing-expected.png content
 - The test should have an RTL case.

[Xiaomei Ji]

*** Bug 42111 has been marked as a duplicate of this bug. ***

[Adam Langley]

Created attachment 64142 [details]
Patch

Comment on attachment 64142 [details]
Patch

> +        if (numWordBreaks)
> +            m_padPerWordBreak = ceilf(static_cast<float>(m_padding) / numWordBreaks);

[…]

> +                if (m_padding) {
> +                    unsigned toPad = m_padPerWordBreak;
> +                    if (m_padding < toPad)
> +                        toPad = m_padding;
> +                    m_padding -= toPad;

Looks like you’re introducing the kind of behavior described in bug 36105.

[Adam Langley]

Comment on attachment 64142 [details]
Patch

(In reply to comment #9)
> Looks like you’re introducing the kind of behavior described in bug 36105.

Very good point. Thank you, sir. I'll fix that now.

[Adam Langley]

Created attachment 64153 [details]
Patch

[Adam Langley]

Created attachment 64155 [details]
Patch

[Evan Martin]

Looks fine to me.

[Tony Chang]

Comment on attachment 64155 [details]
Patch

> diff --git a/WebCore/platform/graphics/chromium/FontLinux.cpp b/WebCore/platform/graphics/chromium/FontLinux.cpp
>          , m_run(getTextRun(run))
>          , m_iterateBackwards(m_run.rtl())
> +        , m_wordSpacingAdjustment(0)
> +        , m_padding(0)
> +        , m_padError(0)

Do you need to initialize m_padPerWordBreak and m_letterSpacing (well, I guess this isn't used yet so it doesn't matter) here?

[WebKit Review Bot]

http://trac.webkit.org/changeset/65428 might have broken SnowLeopard Intel Release (Tests)

[Adam Langley]

r65428

(SnowLeopard Intel Release (Tests) was red long before this commit.)