Whitespace causes font-variant: all-small-caps to synthesize
Created attachment 272820 [details] Patch
Comment on attachment 272820 [details] Patch Attachment 272820 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/920110 New failing tests: fast/text/all-small-caps-whitespace.html
Created attachment 272823 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 272820 [details] Patch Attachment 272820 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/920135 New failing tests: fast/text/all-small-caps-whitespace.html
Created attachment 272824 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 272820 [details] Patch Attachment 272820 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/920185 New failing tests: fast/text/all-small-caps-whitespace.html
Created attachment 272825 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
<rdar://problem/24630796>
Created attachment 272828 [details] Patch
Comment on attachment 272828 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272828&action=review > Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:309 > + bool result = nextFont && nextFont != Font::systemFallback(); I suggest using various early return false statements instead of the bool and all the &&-ing. > Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:311 > + result &= !u_isUWhiteSpace(baseCharacter); Is this really the check we want? I know, for example, that it’s not a particularly fast function. Since what we are really doing is working around a bug in a font’s coverage data, it might be more efficient to just check for the space character, or maybe isASCIISpace. Using u_isUWhiteSpace seems both too broad and too narrow. I’m not sure I understand why whitespace is special. Maybe all uncased characters are special?
Comment on attachment 272828 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272828&action=review >> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:311 >> + result &= !u_isUWhiteSpace(baseCharacter); > > Is this really the check we want? I know, for example, that it’s not a particularly fast function. Since what we are really doing is working around a bug in a font’s coverage data, it might be more efficient to just check for the space character, or maybe isASCIISpace. Using u_isUWhiteSpace seems both too broad and too narrow. > > I’m not sure I understand why whitespace is special. Maybe all uncased characters are special? The difference is that smcp/c2sc does cover uncased characters (and it needs to in order for the uncased character not to look out of place). It doesn't, however, need to cover whitespace (presumably because, since there is no outline data, shrinking it would be a no-op)
Comment on attachment 272828 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272828&action=review >>> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:311 >>> + result &= !u_isUWhiteSpace(baseCharacter); >> >> Is this really the check we want? I know, for example, that it’s not a particularly fast function. Since what we are really doing is working around a bug in a font’s coverage data, it might be more efficient to just check for the space character, or maybe isASCIISpace. Using u_isUWhiteSpace seems both too broad and too narrow. >> >> I’m not sure I understand why whitespace is special. Maybe all uncased characters are special? > > The difference is that smcp/c2sc does cover uncased characters (and it needs to in order for the uncased character not to look out of place). It doesn't, however, need to cover whitespace (presumably because, since there is no outline data, shrinking it would be a no-op) It still should, though, because it isn't really a no-op - the glyph's advance should change too.
Committed r197573: <http://trac.webkit.org/changeset/197573>