Bug 98317 – Only measure text once instead of twice when performing line layout

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Enrica Casucci]

Since we are measuring each word to find where the line break should occur,
we should cache that information to avoid measuring the run again when
creating the line box.

<rdar://problem/12080821>

[Enrica Casucci]

Created attachment 167001 [details]
Patch

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

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. :)

[Build Bot]

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

[Enrica Casucci]

(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%.

[Enrica Casucci]

(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).

[Enrica Casucci]

> > 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.

[Enrica Casucci]

(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!

[Enrica Casucci]

 
> > 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.

[Stephen Chenney]

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).

[Enrica Casucci]

Created attachment 167493 [details]
Patch2

[WebKit Review Bot]

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.

[Build Bot]

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.

[Enrica Casucci]

(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!

[Enrica Casucci]

Committed revision 130812.

[Adam Barth]

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.

[WebKit Review Bot]

Re-opened since this is blocked by bug 98826

[Adam Barth]

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.

[Adam Barth]

The bots see these failures on at least chromium-mac-lion and chromium-mac-snowleopard.

[Enrica Casucci]

(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.

[Adam Barth]

> 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

[Enrica Casucci]

(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.

[Adam Barth]

> 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.

[Enrica Casucci]

(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.

[Adam Barth]

> 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.

[Adam Barth]

Eric Seidel also knows this code and might have more intelligent things to say about the matter than I do.  :)

[Eric Seidel (no email)]

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.

[Levi Weintraub]

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!

[Enrica Casucci]

Landed again, disabling the optimization for platforms using HarfBuzz.

Committed revision 130851.

[Levi Weintraub]

Wonderful, thanks!

[Adam Barth]

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.

[Adam Barth]

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)?

[Tony Chang]

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.

[Csaba Osztrogonác]

(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