Bug 53051 – [Chromium] Support small caps in complex text on linux

• Component: New Bugs
• Status: RESOLVED FIXED

[James Simonsen]

[Chromium] Support small caps in complex text on linux

[James Simonsen]

Created attachment 79994 [details]
Patch

[James Simonsen]

This fixes 2 more complex text layout tests on linux.

[Evan Martin]

Comment on attachment 79994 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.cpp:145
> +bool isLowerCase(UChar c) { return c != u_toupper(c); }

Is there really not an ICU builtin for this?

> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.cpp:170
> +        if (m_font->isSmallCaps() && isLowerCase(m_item.string[m_item.item.pos]) != isLowerCase(m_item.string[m_item.item.pos + endOfRun]))

Maybe we should factor out m_item.string[m_item.item.pos + endOfRun] from these two lines.

> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.cpp:195
> +    if (m_font->isSmallCaps() && isLowerCase(m_item.string[m_item.item.pos])) {

Why do you check isLowerCase here?

[James Simonsen]

(In reply to comment #3)
> (From update of attachment 79994 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79994&action=review
> 
> > Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.cpp:145
> > +bool isLowerCase(UChar c) { return c != u_toupper(c); }
> 
> Is there really not an ICU builtin for this?

Ugh. You're right. I'd just copied it from the mac port.

Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.cpp:170
> > +        if (m_font->isSmallCaps() && isLowerCase(m_item.string[m_item.item.pos]) != isLowerCase(m_item.string[m_item.item.pos + endOfRun]))
> 
> Maybe we should factor out m_item.string[m_item.item.pos + endOfRun] from these two lines.

Sure. Done.

Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.cpp:195
> > +    if (m_font->isSmallCaps() && isLowerCase(m_item.string[m_item.item.pos])) {
> 
> Why do you check isLowerCase here?

We don't want to use SmallCapsVariant for uppercase text. The uppercase text is supposed to be taller than the small caps.

[James Simonsen]

Created attachment 80087 [details]
Patch

[Evan Martin]

Comment on attachment 80087 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.cpp:169
> +        if (m_font->isSmallCaps() && u_islower(m_item.string[m_item.item.pos]) != u_islower(nextCharacter))

Should we hoist the first islower test out of the loop?  (I expect it to be moderately expensive, given the size of Unicode, but more that it would be more readable.)

> Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.cpp:194
> +    if (m_font->isSmallCaps() && u_islower(m_item.string[m_item.item.pos])) {

I still don't quite get this.  Why do we only consider the first character of the string for making this decision?

[James Simonsen]

Thanks for taking the time to look at this!

(In reply to comment #6)
> (From update of attachment 80087 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80087&action=review
> 
> > Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.cpp:169
> > +        if (m_font->isSmallCaps() && u_islower(m_item.string[m_item.item.pos]) != u_islower(nextCharacter))
> 
> Should we hoist the first islower test out of the loop?  (I expect it to be moderately expensive, given the size of Unicode, but more that it would be more readable.)

Sure. Done.

> 
> > Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.cpp:194
> > +    if (m_font->isSmallCaps() && u_islower(m_item.string[m_item.item.pos])) {
> 
> I still don't quite get this.  Why do we only consider the first character of the string for making this decision?

In nextScriptRun(), the loop to determine the length of the run will break early if we ever change case. So in setupFontForScriptRun(), we can be confident that this entire run is the same case.

I've refactored this a bit. Does that improve the readability?

[James Simonsen]

Created attachment 80089 [details]
Patch

[Evan Martin]

Comment on attachment 80089 [details]
Patch

Thanks for explaining!  This looks good to me, though I am not a reviewer.

[James Simonsen]

Created attachment 80122 [details]
Patch

[James Simonsen]

(In reply to comment #7)
> > > Source/WebCore/platform/graphics/chromium/ComplexTextControllerLinux.cpp:194
> > > +    if (m_font->isSmallCaps() && u_islower(m_item.string[m_item.item.pos])) {
> > 
> > I still don't quite get this.  Why do we only consider the first character of the string for making this decision?
> 
> In nextScriptRun(), the loop to determine the length of the run will break early if we ever change case. So in setupFontForScriptRun(), we can be confident that this entire run is the same case.
> 
> I've refactored this a bit. Does that improve the readability?

Bah. This refactoring wasn't correct either. We still need to end the script run when an uppercase run becomes lowercase.

I've undone the refactoring and simply added a comment containing roughly what we discussed in this thread.

Let me know if you'd like me to change it. Otherwise, I'll land it later today.

[Evan Martin]

I like it better without the member variable, and the comments look good too.

[Tony Chang]

Comment on attachment 80122 [details]
Patch

> Bah. This refactoring wasn't correct either. We still need to end the script run when an uppercase run becomes lowercase.

It would be nice to add a layout test for this case (could be a follow up patch).

[James Simonsen]

Committed r76644

I'll set up a new test case this evening.

[James Simonsen]

(In reply to comment #13)
> (From update of attachment 80122 [details])
> > Bah. This refactoring wasn't correct either. We still need to end the script run when an uppercase run becomes lowercase.
> 
> It would be nice to add a layout test for this case (could be a follow up patch).

Hum. I tried to create a test case, then stepped through the code for selecting FontData. I don't see any situation where my concern can actually occur. Every time we change case, we're guaranteed to select a different FontData because we have to switch to the scaled small caps FontData. So, the first if() in nextScriptRun() should be sufficient.

I'll send out a new patch to clear this up.