Bug 156794 – Make glyphToWidth cache correctly

• Status: NEW
• Component: Layout and Rendering

[Hyungwook Lee]

Currently only English can use glyphToWidth cache so another language's text layout and drawing performance is not good.

[Hyungwook Lee]

Created attachment 276820 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 276820 [details]
Patch

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

> Source/WebCore/platform/graphics/GlyphMetricsMap.h:109
> +                return page;

Why do you skip the page filling for the non-primary pages, but not skip page filling for the primary page? These two should be the same (the zero page is just used as a cache)

[Myles C. Maxfield]

(In reply to comment #2)
> Comment on attachment 276820 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276820&action=review
> 
> > Source/WebCore/platform/graphics/GlyphMetricsMap.h:109
> > +                return page;
> 
> Why do you skip the page filling for the non-primary pages, but not skip
> page filling for the primary page? These two should be the same (the zero
> page is just used as a cache)

Also, please add a test.

[Hyungwook Lee]

(In reply to comment #2)
> Comment on attachment 276820 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=276820&action=review
> 
> > Source/WebCore/platform/graphics/GlyphMetricsMap.h:109
> > +                return page;
> 
> Why do you skip the page filling for the non-primary pages, but not skip
> page filling for the primary page? These two should be the same (the zero
> page is just used as a cache)

Current patch doesn't skip the page filling for the non-primary pages.
I just prevent the page re-filling that already filled when we create it.

I think the root cause of current issue is that we always re-filling non-primary pages even if we set glyph's width in Font::widthForGlyph()

[Hyungwook Lee]

(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 276820 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=276820&action=review
> > 
> > > Source/WebCore/platform/graphics/GlyphMetricsMap.h:109
> > > +                return page;
> > 
> > Why do you skip the page filling for the non-primary pages, but not skip
> > page filling for the primary page? These two should be the same (the zero
> > page is just used as a cache)
> 
> Also, please add a test.

GlyphMetricsMap is not public API and it is related to Cache operation.
Do you have idea to make a test?

[Myles C. Maxfield]

(In reply to comment #5)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Comment on attachment 276820 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=276820&action=review
> > > 
> > > > Source/WebCore/platform/graphics/GlyphMetricsMap.h:109
> > > > +                return page;
> > > 
> > > Why do you skip the page filling for the non-primary pages, but not skip
> > > page filling for the primary page? These two should be the same (the zero
> > > page is just used as a cache)
> > 
> > Also, please add a test.
> 
> GlyphMetricsMap is not public API and it is related to Cache operation.
> Do you have idea to make a test?

I think we should make a layout test (not a unit test). Therefore, we should find a way that a webpage can cause this code to be invoked, and we should make a test page that does so, and make sure the result is as expected.

If you have more questions about how to do this, I am happy to answer them.