[Chromium] Support small caps in complex text on linux
Created attachment 79994 [details] Patch
This fixes 2 more complex text layout tests on linux.
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?
(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.
Created attachment 80087 [details] Patch
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?
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?
Created attachment 80089 [details] Patch
Comment on attachment 80089 [details] Patch Thanks for explaining! This looks good to me, though I am not a reviewer.
Created attachment 80122 [details] Patch
(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.
I like it better without the member variable, and the comments look good too.
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).
Committed r76644 I'll set up a new test case this evening.
(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.