Summary: | Only measure text once instead of twice when performing line layout | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Enrica Casucci <enrica> | ||||||
Component: | Layout and Rendering | Assignee: | Enrica Casucci <enrica> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, adele, bashi, eric, hyatt, jshin, koivisto, leviw, mitz, ned, ossy, schenney, tony, webkit.review.bot, xji | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | 98545, 98826, 98876 | ||||||||
Bug Blocks: | 100794 | ||||||||
Attachments: |
|
Description
Enrica Casucci
2012-10-03 16:07:31 PDT
Created attachment 167001 [details]
Patch
Attachment 167001 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/rendering/RenderBlockLineLayout.cpp:686: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/rendering/RenderBlockLineLayout.cpp:833: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 2 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Do we have any data on how much of an epic perf win this is? :) I remember https://github.com/dglazkov/performance-tests showing a bunch of time spent in width(), similarly I think http://trac.webkit.org/browser/trunk/PerformanceTests/Layout/line-layout.html does as well. run-perf-tests Layout/line-layout.html may show you want you want to see. :) Comment on attachment 167001 [details] Patch Attachment 167001 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14135867 New failing tests: editing/selection/vertical-rl-rtl-extend-line-backward-br.html (In reply to comment #3) > Do we have any data on how much of an epic perf win this is? :) > > I remember https://github.com/dglazkov/performance-tests showing a bunch of time spent in width(), similarly I think http://trac.webkit.org/browser/trunk/PerformanceTests/Layout/line-layout.html does as well. > > run-perf-tests Layout/line-layout.html may show you want you want to see. :) I measured a speedup of about 10%. (In reply to comment #4) > (From update of attachment 167001 [details]) > Attachment 167001 [details] did not pass mac-ews (mac): > Output: http://queues.webkit.org/results/14135867 > > New failing tests: > editing/selection/vertical-rl-rtl-extend-line-backward-br.html I noticed that this new method of measuring text uncovers an issue with system fallback fonts on the Mac platform. I'll file a bug and block this one on it. Comment on attachment 167001 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167001&action=review > Source/WebCore/ChangeLog:13 > + be consumed in setLogicalWidthForText run where we used to measure the typo: “setLogicalWidthForText run” > Source/WebCore/ChangeLog:16 > + about the start and end offset in the run, the renderer, the meaasured with “start and end offset of the run” or “start and end offset in the RenderText” typos: “meaasured with” > Source/WebCore/ChangeLog:24 > + (Font): Added fallback fonts parameter to the static width method/ typo: “/”. I’d call it “static member function” rather than “method”. > Source/WebCore/ChangeLog:29 > + (WebCore::Font::width): Added fallback fonts parameter. Perhaps you can call out the fact that you’re adding the fallback fonts parameter to the TextLayout versions of these functions (as I was reading this, I was confused because I was thinking “don’t these functions already support fallback fonts?” not realizing these were the TextLayout-enabled overloads) and more importantly, explain somewhere in the change log why you’re doing so (we used to get the fallback fonts when we remeasured the entire run, which we did without a TextLayout, but now we no longer do that). > Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:54 > - , m_controller(adoptPtr(new ComplexTextController(&m_font, m_run, true))) > { > + m_controller = adoptPtr(new ComplexTextController(&m_font, m_run, true, &m_fallbackFonts)); Any reason to switch from initializer syntax to this? > Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:57 > + float width(unsigned from, unsigned len, HashSet<const SimpleFontData*>* fallbackFonts = 0) No need for “= 0” here. Comment on attachment 167001 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167001&action=review > Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:69 > + if (fallbackFonts && !m_fallbackFonts.isEmpty()) { > + HashSet<const SimpleFontData*>::const_iterator end = m_fallbackFonts.end(); > + for (HashSet<const SimpleFontData*>::const_iterator it = m_fallbackFonts.begin(); it != end; ++it) > + fallbackFonts->add(*it); > + } This doesn’t look very efficient, but what I really worry about here is correctness. m_fallbackFonts contains fallback fonts for the entire TextRun but fallbackFonts should only contain fallback fonts for the range from from to (from + len). > Source/WebCore/rendering/RenderBlock.h:64 > typedef WTF::HashMap<const RenderBox*, HashSet<RenderBlock*>*> TrackedContainerMap; Please add an empty line here. > Source/WebCore/rendering/RenderBlock.h:65 > +class TextMeasureEntry { Can you forward-declare this class here? I think you can. Then the definition can move into the only .cpp file that uses it. I am not happy with the name TextMeasureEntry. For one, I wish we could get rid of “entry” in the name, as it doesn’t add much (it’s like saying “object”, “data”, or “info”). I don’t think “measure”, as a noun, is the right word to use here. The right noun would be “measurement”, but maybe we can find a different term altogether. I also wonder if instead of just “Text” we should say something that indicates that this is a subrange of a RenderText. Maybe we can use the term “word”, since that’s what these ranges are normally. > Source/WebCore/rendering/RenderBlock.h:70 > + TextMeasureEntry() : renderer(0), > + textWidth(0), > + startOffset(0), > + endOffset(0) This is not the normal style for initializer list. The first one should be on a line of its own that starts with a colon, and subsequent lines should start with a comma. > Source/WebCore/rendering/RenderBlock.h:72 > + } Please add an empty line here. > Source/WebCore/rendering/RenderBlock.h:74 > + float textWidth; Can just call this “width”. > Source/WebCore/rendering/RenderBlock.h:80 > +typedef Vector<TextMeasureEntry> TextMeasures; Once we decide on a name for TextMeasureEntry, we’ll need to consider what to name this type. You should forward-declare it here if possible (and then you won’t even need to forward-declare TextMeasureEntry at all). You should give this vector some inline capacity. Without it, I think we always allocate its storage on the heap! Comment on attachment 167001 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167001&action=review > Source/WebCore/rendering/RenderBlockLineLayout.cpp:711 > + bool isKerningEnabled = renderer->style()->font().typesettingFeatures() & Kerning; This should use style(lineInfo.isFirstLine()). Since we need the same Font in the block above, I’d just move the font local out of that block and use it there and again here. I don’t we’re not consistent about naming of boolean variables, but I find that a name that is a statement, like “kerningIsEnabled”, reads better than a name that is a question like “isKerningEnabled” once you use it in an expression: if (kerningIsEnabled)… > Source/WebCore/rendering/RenderBlockLineLayout.cpp:713 > + if (!lineBox->fitsToGlyphs() && !textMeasures.isEmpty()) { Needs a comment about why you’re checking fitsToGlyphs(). > Source/WebCore/rendering/RenderBlockLineLayout.cpp:715 > + for (size_t i = 0; i < textMeasures.size() && lastEndOffset < run->m_stop; i++) { It can be more efficient to get the size once. The new style for doing so is for (size_t i = 0, size = textMeasures.size(); i < size && lastEndOffset < run->m_stop; ++i) { (we also use pre-increment as a rule). > Source/WebCore/rendering/RenderBlockLineLayout.cpp:734 > + // we don't have enough cached data, we'll measure again Please format this as a sentence (start with a capital letter and end with a period). > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2078 > +static ALWAYS_INLINE float textWidth(RenderText* text, unsigned from, unsigned len, const Font& font, float xPos, bool isFixedPitch, bool collapseWhiteSpace, HashSet<const SimpleFontData*>* fallbackFonts = 0, TextLayout* layout = 0) What’s with the change from inline to ALWAYS_INLINE here? > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2263 > + entry.fallbackFonts.clear(); What’s happening here? I don’t think we want to be copying HashSets all the time. I think instead the TextMeasureEntry that the code below updates should already by in the vector. Then if we don’t need it, we can delete it (or reuse it, I suppose). > > Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:54
> > - , m_controller(adoptPtr(new ComplexTextController(&m_font, m_run, true)))
> > {
> > + m_controller = adoptPtr(new ComplexTextController(&m_font, m_run, true, &m_fallbackFonts));
>
> Any reason to switch from initializer syntax to this?
>
I moved it here because I declared m_fallbackFonts after m_controller and m_fallbackFonts wasn't initialized yet, then I forgot to change the order. I'll fix it.
Thanks for all the comments. I'll address them all in the new patch.
(In reply to comment #8) > (From update of attachment 167001 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=167001&action=review > > > Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:69 > > + if (fallbackFonts && !m_fallbackFonts.isEmpty()) { > > + HashSet<const SimpleFontData*>::const_iterator end = m_fallbackFonts.end(); > > + for (HashSet<const SimpleFontData*>::const_iterator it = m_fallbackFonts.begin(); it != end; ++it) > > + fallbackFonts->add(*it); > > + } > > This doesn’t look very efficient, but what I really worry about here is correctness. m_fallbackFonts contains fallback fonts for the entire TextRun but fallbackFonts should only contain fallback fonts for the range from from to (from + len). Is it a problem to have redundant information? I think the worst case is that we'll have fallbackFonts that are not needed for the part of the original run. Is this going to affect the rendering? > > > Source/WebCore/rendering/RenderBlock.h:64 > > typedef WTF::HashMap<const RenderBox*, HashSet<RenderBlock*>*> TrackedContainerMap; > > Please add an empty line here. > > > Source/WebCore/rendering/RenderBlock.h:65 > > +class TextMeasureEntry { > > Can you forward-declare this class here? I think you can. Then the definition can move into the only .cpp file that uses it. > I am not happy with the name TextMeasureEntry. For one, I wish we could get rid of “entry” in the name, as it doesn’t add much (it’s like saying “object”, “data”, or “info”). I don’t think “measure”, as a noun, is the right word to use here. The right noun would be “measurement”, but maybe we can find a different term altogether. I also wonder if instead of just “Text” we should say something that indicates that this is a subrange of a RenderText. Maybe we can use the term “word”, since that’s what these ranges are normally. What about WordMeasurement? or WordWidth? > > You should give this vector some inline capacity. Without it, I think we always allocate its storage on the heap! Great catch, thanks! > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2078 > > +static ALWAYS_INLINE float textWidth(RenderText* text, unsigned from, unsigned len, const Font& font, float xPos, bool isFixedPitch, bool collapseWhiteSpace, HashSet<const SimpleFontData*>* fallbackFonts = 0, TextLayout* layout = 0) > > What’s with the change from inline to ALWAYS_INLINE here? > Ok to all the other comments, I changed this because I saw with Instruments that this was not inline. I know the compiler can override some inline definition based on the number of parameters and their type. ALWAYS_INLINE forces it to be inline and we gain a little performance. > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2263 > > + entry.fallbackFonts.clear(); > > What’s happening here? I don’t think we want to be copying HashSets all the time. I think instead the TextMeasureEntry that the code below updates should already by in the vector. Then if we don’t need it, we can delete it (or reuse it, I suppose). I'm not sure I understand your comment here. This variable entry is used over and over while measuring different runs in different renderers that fit on the same line. We don't want to have the fallbackfonts for one run associated with a different run. (In reply to comment #11) > (In reply to comment #8) > > (From update of attachment 167001 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=167001&action=review > > > > > Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:69 > > > + if (fallbackFonts && !m_fallbackFonts.isEmpty()) { > > > + HashSet<const SimpleFontData*>::const_iterator end = m_fallbackFonts.end(); > > > + for (HashSet<const SimpleFontData*>::const_iterator it = m_fallbackFonts.begin(); it != end; ++it) > > > + fallbackFonts->add(*it); > > > + } > > > > This doesn’t look very efficient, but what I really worry about here is correctness. m_fallbackFonts contains fallback fonts for the entire TextRun but fallbackFonts should only contain fallback fonts for the range from from to (from + len). > > Is it a problem to have redundant information? I think the worst case is that we'll have fallbackFonts that are not needed for the part of the original run. Is this going to affect the rendering? Yes, we may end up making a line unnecessarily tall because of some font that happens to be used only on the next line. > > > > > > Source/WebCore/rendering/RenderBlock.h:64 > > > typedef WTF::HashMap<const RenderBox*, HashSet<RenderBlock*>*> TrackedContainerMap; > > > > Please add an empty line here. > > > > > Source/WebCore/rendering/RenderBlock.h:65 > > > +class TextMeasureEntry { > > > > Can you forward-declare this class here? I think you can. Then the definition can move into the only .cpp file that uses it. > > I am not happy with the name TextMeasureEntry. For one, I wish we could get rid of “entry” in the name, as it doesn’t add much (it’s like saying “object”, “data”, or “info”). I don’t think “measure”, as a noun, is the right word to use here. The right noun would be “measurement”, but maybe we can find a different term altogether. I also wonder if instead of just “Text” we should say something that indicates that this is a subrange of a RenderText. Maybe we can use the term “word”, since that’s what these ranges are normally. > > What about WordMeasurement? or WordWidth? I like Width because it’s short and descriptive but I don’t like that it hides the fact that there’s more than just the width in this class. Measurement sounds better, though not great. > > > > > You should give this vector some inline capacity. Without it, I think we always allocate its storage on the heap! > > Great catch, thanks! (In reply to comment #12) > > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2078 > > > +static ALWAYS_INLINE float textWidth(RenderText* text, unsigned from, unsigned len, const Font& font, float xPos, bool isFixedPitch, bool collapseWhiteSpace, HashSet<const SimpleFontData*>* fallbackFonts = 0, TextLayout* layout = 0) > > > > What’s with the change from inline to ALWAYS_INLINE here? > > > > Ok to all the other comments, I changed this because I saw with Instruments that this was not inline. > I know the compiler can override some inline definition based on the number of parameters and their type. > ALWAYS_INLINE forces it to be inline and we gain a little performance. Thanks for the explanation. Just wanted to confirm that this is the case. > > > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2263 > > > + entry.fallbackFonts.clear(); > > > > What’s happening here? I don’t think we want to be copying HashSets all the time. I think instead the TextMeasureEntry that the code below updates should already by in the vector. Then if we don’t need it, we can delete it (or reuse it, I suppose). > > I'm not sure I understand your comment here. This variable entry is used over and over while measuring different runs in different renderers that fit on the same line. We don't want to have the fallbackfonts for one run associated with a different run. I’m saying that instead of using a local variable, we should use a “provisional” entry in the Vector, so that we don’t need to later copy from the local to the place in the Vector. Comment on attachment 167001 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167001&action=review >> Source/WebCore/ChangeLog:16 >> + about the start and end offset in the run, the renderer, the meaasured with > > “start and end offset of the run” or “start and end offset in the RenderText” > typos: “meaasured with” "measured width" >> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:54 >> + m_controller = adoptPtr(new ComplexTextController(&m_font, m_run, true, &m_fallbackFonts)); > > Any reason to switch from initializer syntax to this? You would have to declare m_fallbackFonts before m_controller, I suspect, but that itself would be OK. > Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:88 > + HashSet<const SimpleFontData*> m_fallbackFonts; I am concerned about the lifetime of the SimpleFontData stored here. Are you certain that the pointers in the set will be cleared when the SimpleFontData is deleted? FontData and derived classes are ref counted, and their ref count may cause deletion when the cache is cleared because no FontFallbackList is using the FontData (in turn because no Font is using that FontFallbackList). There is no need to ref count the SimpleFontData here provided you are sure that the list will never have a pointer that is not also held in m_font.m_fontFallbackList->m_fontList. >> Source/WebCore/rendering/RenderBlockLineLayout.cpp:686 >> + GlyphOverflowAndFallbackFontsMap& textBoxDataMap, VerticalPositionCache& verticalPositionCache, TextMeasures& textMeasures) > > Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] The style bot has started warning about this, where it is line wrapping causing the problems. Apparently we now need to always start the next line of a wrap with just an additional tab (4 spaces). Created attachment 167493 [details]
Patch2
Attachment 167493 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/rendering/RenderBlockLineLayout.cpp:703: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/WebCore/rendering/RenderBlockLineLayout.cpp:856: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 2 in 7 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 167493 [details] Patch2 Attachment 167493 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14228118 New failing tests: fast/text/international/text-combine-image-test.html fast/writing-mode/vertical-baseline-alignment.html Comment on attachment 167493 [details] Patch2 View in context: https://bugs.webkit.org/attachment.cgi?id=167493&action=review r=me but please try to address my comments before landing > Source/WebCore/ChangeLog:15 > + Each entry in the vector is a Measure object that contains information You ended up calling this Measurement, not Measure. > Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:538 > + if (fallbackFonts && complexTextRun.fontData() != m_font.primaryFont()) > + fallbackFonts->add(complexTextRun.fontData()); Doing this here may be too late! There are return statements in the while() loop above. > Source/WebCore/rendering/RenderBlock.h:56 > class LineInfo; > class RenderRubyRun; > class TextLayout; > +class Measurement; These should be kept sorted. > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2489 > + float wordSpacingForTextMeasure = 0; ForTextMeasurement? > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2570 > + measurements.append(Measurement()); You want to avoid creating a temporary Measurement object and then copying it into the Vector. I don’t know if the above code does this. Instead, you can just use Vector’s grow() function. (In reply to comment #19) > (From update of attachment 167493 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=167493&action=review > > r=me but please try to address my comments before landing > > > Source/WebCore/ChangeLog:15 > > + Each entry in the vector is a Measure object that contains information > > You ended up calling this Measurement, not Measure. > > > Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:538 > > + if (fallbackFonts && complexTextRun.fontData() != m_font.primaryFont()) > > + fallbackFonts->add(complexTextRun.fontData()); > > Doing this here may be too late! There are return statements in the while() loop above. > > > Source/WebCore/rendering/RenderBlock.h:56 > > class LineInfo; > > class RenderRubyRun; > > class TextLayout; > > +class Measurement; > > These should be kept sorted. > > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2489 > > + float wordSpacingForTextMeasure = 0; > > ForTextMeasurement? > > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:2570 > > + measurements.append(Measurement()); > > You want to avoid creating a temporary Measurement object and then copying it into the Vector. I don’t know if the above code does this. Instead, you can just use Vector’s grow() function. I will definitely do that. Per our latest discussion, I'll change the types to WordMeasurement and WordMeasurements. Thanks for the review! Committed revision 130812. I wonder if this patch caused this regression: Expected: http://build.chromium.org/f/chromium/layout_test_results/Webkit_Mac10_6/results/layout-test-results/fast/text/shaping/shaping-selection-rect-expected.png Actual: http://build.chromium.org/f/chromium/layout_test_results/Webkit_Mac10_6/results/layout-test-results/fast/text/shaping/shaping-selection-rect-actual.png Notice the gap in the selection between the C and the F. Re-opened since this is blocked by bug 98826 fast/lists/003-vertical.html fast/overflow/scrollRevealButton.html fast/text/international/vertical-text-glyph-test.html fast/text/shaping/shaping-selection-rect.html platform/chromium/fast/forms/date/date-suggestion-picker-appearance-with-scroll-bar.html The tests listed above might have been affected as well. The bots see these failures on at least chromium-mac-lion and chromium-mac-snowleopard. (In reply to comment #24) > fast/lists/003-vertical.html > fast/overflow/scrollRevealButton.html > fast/text/international/vertical-text-glyph-test.html The 3 tests above all pass on Mac. > fast/text/shaping/shaping-selection-rect.html This is skipped for Mac. > platform/chromium/fast/forms/date/date-suggestion-picker-appearance-with-scroll-bar.html > For this one I have no idea. > The tests listed above might have been affected as well. > fast/lists/003-vertical.html > fast/text/international/vertical-text-glyph-test.html > fast/text/shaping/shaping-selection-rect.html ^^^ These three were fixed by the rollout. The others are likely caused by http://trac.webkit.org/changeset/130811 (In reply to comment #27) > > fast/lists/003-vertical.html > > fast/text/international/vertical-text-glyph-test.html > > fast/text/shaping/shaping-selection-rect.html > > ^^^ These three were fixed by the rollout. The others are likely caused by http://trac.webkit.org/changeset/130811 These tests differ by 1px. I think we only need new baseline results. Probably the difference is caused by sub pixel layout being enabled for chromium. > These tests differ by 1px. I think we only need new baseline results.
> Probably the difference is caused by sub pixel layout being enabled for chromium.
Even fast/text/shaping/shaping-selection-rect.html ? The text in the text seems to warn against precisely the failure mode we're seeing.
(In reply to comment #29) > > These tests differ by 1px. I think we only need new baseline results. > > Probably the difference is caused by sub pixel layout being enabled for chromium. > > Even fast/text/shaping/shaping-selection-rect.html ? The text in the text seems to warn against precisely the failure mode we're seeing. I think my patch might have uncovered a pre-existing bug in the code for complex text on the chromium platform. This problem doesn't reproduce on the same mac platforms for our port. Since I have no way to debug the complex text path for HarfBuzz we have two alternatives to move forward: 1. I can land my patch and disable my optimization for HarfBuzz 2. I can land the original patch and mark the three tests as failing in TestExpectations Please let me know what you'd prefer. If I don't hear back I'll disable the optimization for HarfBuzz. > Please let me know what you'd prefer. If I don't hear back I'll disable the optimization for HarfBuzz.
I don't know enough about line layout to know what the best course of action is here. Please use your best judgement or, if you're unsure, consult with some folks who know about HarfBuzz. Tony Chang might be a good person to ask.
Eric Seidel also knows this code and might have more intelligent things to say about the matter than I do. :) I can look at the test, but Emil, Levi, Tony or Jungshik are likely to have more immediate context than I do. I will speak with Levi/Emil tomorrow and make sure someone comments before 2pm tomorrow. Thank you for letting us know. Regarding the test, I suspect that the issue here is that measuring text run lengths and strings can give different results in the complex text path (at least on Chromium). When the string in the test is broken up by the span, findNextLineBreak measures each run individually resulting in three measurements. In createLineBoxesFromBidiRuns, we will measure the string a single time, which can yield different results. I'd say the correct approach here is to begin by disabling this in the complex text case (at least for HarfBuzz platforms), and attack that issue in a separate bug. In other matters: \o/! Awesome! Landed again, disabling the optimization for platforms using HarfBuzz. Committed revision 130851. Wonderful, thanks! I could be mistaken, but I don't think we use HarfBuzz on chromium-mac. My understanding is that we use it on Linux. In any case, the test failures have returned on chromium-mac. I'll mark them as failing in TestExpectations and file another bug where we can sort this issue out. Thanks for your patience. Followup regarding chromium-mac failures is Bug 98867 (In reply to comment #37) > I could be mistaken, but I don't think we use HarfBuzz on chromium-mac. There’s HarfBuzz-related code in #if PLATFORM(CHROMIUM) blocks in FontComplexTextMac.cpp. > My understanding is that we use it on Linux. In any case, the test failures have returned on chromium-mac. I'll mark them as failing in TestExpectations and file another bug where we can sort this issue out. > Perhaps Chormium’s use of HarfBuzz on Mac is not subject to #if USE(HARFBUZZ_NG)? We use harfbuzz-ng on Chromium Mac only when -webkit-font-feature-settings is specified. We're not using harfbuzz-ng yet on Chromium Linux, but plan to soon. bashi is driving that effort. I'll post a follow up on bug 98865 since this patch has landed. (In reply to comment #35) > Landed again, disabling the optimization for platforms using HarfBuzz. > > Committed revision 130851. It caused a regression on Qt - https://bugs.webkit.org/show_bug.cgi?id=98876 |