Support for -webkit-font-feature-settings: 'kern' (seemingly only enabled on CrMac) was preventing us from enabling the great performance win of only measuring text once during prefs-widths calculation instead of again for line layout. Catching this case specifically fixes the problem and gives this straggler platform the 10% perf win!
Created attachment 194933 [details] Patch
Comment on attachment 194933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194933&action=review > Source/WebCore/rendering/RenderBlockLineLayout.cpp:785 > +static inline bool hasKerningFeatureSetting(const Font& font) We must have other code which depends on being able to get this out of the font, no? > Source/WebCore/rendering/RenderBlockLineLayout.cpp:787 > + FontFeatureSettings* settings = font.fontDescription().featureSettings(); So this is http://dev.w3.org/csswg/css3-fonts/#propdef-font-feature-settings, correct? > Source/WebCore/rendering/RenderBlockLineLayout.cpp:792 > + for (unsigned i = 0; i < numFeatures; ++i) { Shouldn't this loop just be on FontFeatureSettings::hasKerning? Then it can know the magic of that this is a 4-char string, and do the AtomicString constant all nicely, etc. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:793 > + if (settings->at(i).tag() == "kern") I assume tag() is an AtomicString? We should probably be using a DEFINE_STATIC_LOCAL(AtomicString, kernAtom, "kern") to make this compare faster. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:828 > bool kerningIsEnabled = font.typesettingFeatures() & Kerning; So previously we were always ignoring kerning, but in order to partake of this optimization we need this code to be kerning aware, correct? > Source/WebCore/rendering/RenderBlockLineLayout.cpp:830 > + kerningIsEnabled = kerningIsEnabled || hasKerningFeatureSetting(font); So this then teaches this code to be aware of both types of kerning. One which comes from optimize-legibility, and one which comes from font-feature-settings? Is that correct? Previously this code only understood optimize-legibility, thus turning on this optimization introduced a new dependency on thsi coe which was not font-feature-settings-aware? correct?
Comment on attachment 194933 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194933&action=review >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:785 >> +static inline bool hasKerningFeatureSetting(const Font& font) > > We must have other code which depends on being able to get this out of the font, no? Sadly no... we pipe this directly into HarfBuzz. See: HarfBuzzShaper::setFontFeatures() >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:787 >> + FontFeatureSettings* settings = font.fontDescription().featureSettings(); > > So this is http://dev.w3.org/csswg/css3-fonts/#propdef-font-feature-settings, correct? Yup. >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:792 >> + for (unsigned i = 0; i < numFeatures; ++i) { > > Shouldn't this loop just be on FontFeatureSettings::hasKerning? Then it can know the magic of that this is a 4-char string, and do the AtomicString constant all nicely, etc. Yeah. Right now FontFeatureSettings has effectively no functionality to do lookups, but this one clearly needs to be in there. >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:793 >> + if (settings->at(i).tag() == "kern") > > I assume tag() is an AtomicString? We should probably be using a DEFINE_STATIC_LOCAL(AtomicString, kernAtom, "kern") to make this compare faster. Thank you! :) >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:828 >> bool kerningIsEnabled = font.typesettingFeatures() & Kerning; > > So previously we were always ignoring kerning, but in order to partake of this optimization we need this code to be kerning aware, correct? Before the optimization, we always measured the text here and didn't have a problem. We still end up measuring here when there's kerning, but we were missing the fact that kerning was enabled for exactly the reasons you mention below.
Created attachment 195347 [details] Patch
Comment on attachment 195347 [details] Patch Attachment 195347 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17345107 New failing tests: fast/text/shaping/shaping-selection-rect.html
Created attachment 195360 [details] Archive of layout-test-results from gce-cr-linux-02 for chromium-linux-x86_64 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-02 Port: chromium-linux-x86_64 Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
So after more testing, it turns out this occurs whenever there are any font-feature-settings defined, which isn't too surprising given it uses complex text features. The surprising thing is that it only effects Mac. For now, I'm teaching this bit of code that this doesn't use the simple font path, but I imagine we'll find more places where the lack of plumbing this info throughout the rendering code may get us in trouble.
Comment on attachment 195347 [details] Patch LGTM.
Created attachment 195632 [details] Patch for landing
Comment on attachment 195632 [details] Patch for landing Clearing flags on attachment: 195632 Committed r147156: <http://trac.webkit.org/changeset/147156>
All reviewed patches have been landed. Closing bug.