Bug 74415 – [skia] cache typeface in FontPlatformData

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Mike Reed]

[skia] cache typeface in FontPlatformData

[Mike Reed]

Created attachment 119027 [details]
Patch

[Mike Reed]

local DRT run on windows with no failures

[James Robinson]

Afraid this is outside my areas of expertise. I'm happy to officially review if a domain expert can review the logic first.

[Cary Clark]

Comment on attachment 119027 [details]
Patch

(I can't comment inline, but)
Here's my only nit:
    unsigned value = 0;
    if (m_font)
        value ^= m_font->hash();
would be clearer as either:
    unsigned value = 0;
    if (m_font)
        value = m_font->hash();
or:
    unsigned value = m_font ? m_font->hash() : 0;
otherwise, LGTM

[Mike Reed]

nit: possibly. I wrote it the way I did so that it matched the line below. i.e. you can reorder the font->hash and the typeface->uniqueid and its all the same.

[Stephen White]

Comment on attachment 119027 [details]
Patch

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

Looks good.  r=me

> Source/WebCore/platform/graphics/chromium/FontChromiumWin.cpp:439
> +        paintSkiaText(graphicsContext, font->platformData(), curLen, &glyphs[0], &advances[0], 0, &origin);

Nit:  Looks like you can get rid of the hfont local var now.

> Source/WebCore/platform/graphics/chromium/FontPlatformDataChromiumWin.h:85
> +            value ^= m_typeface->uniqueID();

Is there a possibility for hash collisions if both font and typeface are specified?

[Mike Reed]

Created attachment 119242 [details]
Patch

[Mike Reed]

new patch tries to revert the changes to operator== and hash(), since the new fields (typeface, quality) are just caches derived from the hfont, and therefore need not be part of either of those operators.

[Stephen White]

Comment on attachment 119242 [details]
Patch

Looks good.  r=me

[WebKit Review Bot]

Comment on attachment 119242 [details]
Patch

Clearing flags on attachment: 119242

Committed r102816: <http://trac.webkit.org/changeset/102816>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Russell]

Sorry, I am rolling out this patch. There are font-related crashes seen on the Chromium canaries and this patch is the one implicated in the revision list.

See:

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Win%20%28dbg%29%282%29/builds/7126

(Look for the assertion failures of fontOK around fast/text/international/vertical-text-metrics-test.html and transforms/2d/hindi-rotated.html)

and:

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.6%20%28dbg%29/builds/6739

Look for "ASSERTION FAILED: fontCache()->generation() == m_generation" plugins/fullscreen-plugins-dont-reload.html

[Kenneth Russell]

Reverted r102816 for reason:

Implicated in font-related crashes on Chromium canaries.

Committed r102858: <http://trac.webkit.org/changeset/102858>

[Mike Reed]

Created attachment 119660 [details]
Patch

[Mike Reed]

new patch fixes the crash that was reported: Placing a FontPlatformData on the stack caused its hfont parameter to be Deleted when the stack object went out of scope.

I no longer create a temp object (to avoid that), so I had to pass the parameters explicitly, but other than that the code flow is the same as before. I have also added a comment to the header documenting this ownership rule.

[Mike Reed]

Comment on attachment 119660 [details]
Patch

local failures, investigating...

[Mike Reed]

Local failures was a flake. Ready for review.

[Stephen White]

Comment on attachment 119660 [details]
Patch

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

Looks good, round two.  r=me

> Source/WebCore/ChangeLog:8
> +        No new tests. optimization only, existing tests in play

Nit:  It's nice to know which ones.

[Mike Reed]

which tests:

Text drawn with transparency or shadows takes the skia code-path.

[WebKit Review Bot]

Comment on attachment 119660 [details]
Patch

Clearing flags on attachment: 119660

Committed r103262: <http://trac.webkit.org/changeset/103262>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.