REGRESSION: Lines jump up and down while typing Chinese or Japanese

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Fallback fonts are not used when computing line heights in some cases.

[Ryosuke Niwa]

Created attachment 201416 [details]
Fix (without change log)

[Ryosuke Niwa]

<rdar://problem/12803147>

[Ryosuke Niwa]

Created attachment 201441 [details]
Fixes one bug

[Ryosuke Niwa]

Created attachment 201442 [details]
Reverted erroneous test change

[Darin Adler]

Comment on attachment 201442 [details]
Reverted erroneous test change

View in context: https://bugs.webkit.org/attachment.cgi?id=201442&action=review

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:3094
> +                    if (wordMeasurement.fallbackFonts.isEmpty() && !fallbackFonts.isEmpty())
> +                        wordMeasurement.fallbackFonts.swap(fallbackFonts);

Sure could use a why comment.

[Ryosuke Niwa]

Created attachment 203893 [details]
More comprehensive WIP fix

There was more fundamental flaw in our code.

[Ryosuke Niwa]

Created attachment 203991 [details]
Fixes the bug

I thought passing a NULL fallbackFonts pointer into floatWidthFor{Simple,Complex}Text was a significant optimization. Have you checked whether this patch has any impact performance?

[Ryosuke Niwa]

(In reply to comment #8)
> I thought passing a NULL fallbackFonts pointer into floatWidthFor{Simple,Complex}Text was a significant optimization. Have you checked whether this patch has any impact performance?

Yeah, I'm getting the numbers now.

[Ryosuke Niwa]

Created attachment 203994 [details]
A/B test results

Thankfully I don't see any regression from running some perf. tests but please let me know if there is some other benchmark or page where this patch may cause a regression.

[Darin Adler]

Comment on attachment 203991 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=203991&action=review

I hope creating and destroying these empty hash sets is inexpensive enough.

> Source/WebCore/platform/graphics/Font.cpp:325
> +    ASSERT(fallbackFonts);

This assertion is really unnecessary. The code that guarantees this is non-zero is just a couple lines up from here.

[Ryosuke Niwa]

(In reply to comment #11)
> (From update of attachment 203991 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203991&action=review
> 
> I hope creating and destroying these empty hash sets is inexpensive enough.

I think andersca optimized empty hash maps recently to reduce the memory usage, etc... so HashTable only initializes various member variables to 0 now.

> > Source/WebCore/platform/graphics/Font.cpp:325
> > +    ASSERT(fallbackFonts);
> 
> This assertion is really unnecessary. The code that guarantees this is non-zero is just a couple lines up from here.

Removed.

[Ryosuke Niwa]

Committed r151327: <http://trac.webkit.org/changeset/151327>