Bug 74415 - [skia] cache typeface in FontPlatformData
Summary: [skia] cache typeface in FontPlatformData
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike Reed
Depends on:
Reported: 2011-12-13 09:00 PST by Mike Reed
Modified: 2011-12-19 13:40 PST (History)
7 users (show)

See Also:

Patch (11.81 KB, patch)
2011-12-13 09:09 PST, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (11.77 KB, patch)
2011-12-14 10:24 PST, Mike Reed
no flags Details | Formatted Diff | Diff
Patch (13.57 KB, patch)
2011-12-16 13:15 PST, Mike Reed
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Reed 2011-12-13 09:00:37 PST
[skia] cache typeface in FontPlatformData
Comment 1 Mike Reed 2011-12-13 09:09:13 PST
Created attachment 119027 [details]
Comment 2 Mike Reed 2011-12-13 12:24:08 PST
local DRT run on windows with no failures
Comment 3 James Robinson 2011-12-13 12:26:07 PST
Afraid this is outside my areas of expertise. I'm happy to officially review if a domain expert can review the logic first.
Comment 4 Cary Clark 2011-12-14 07:51:09 PST
Comment on attachment 119027 [details]

(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();
    unsigned value = m_font ? m_font->hash() : 0;
otherwise, LGTM
Comment 5 Mike Reed 2011-12-14 07:59:01 PST
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.
Comment 6 Stephen White 2011-12-14 08:15:32 PST
Comment on attachment 119027 [details]

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?
Comment 7 Mike Reed 2011-12-14 10:24:24 PST
Created attachment 119242 [details]
Comment 8 Mike Reed 2011-12-14 10:26:59 PST
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.
Comment 9 Stephen White 2011-12-14 12:46:14 PST
Comment on attachment 119242 [details]

Looks good.  r=me
Comment 10 WebKit Review Bot 2011-12-14 13:21:54 PST
Comment on attachment 119242 [details]

Clearing flags on attachment: 119242

Committed r102816: <http://trac.webkit.org/changeset/102816>
Comment 11 WebKit Review Bot 2011-12-14 13:22:00 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Kenneth Russell 2011-12-14 17:19:01 PST
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.



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



Look for "ASSERTION FAILED: fontCache()->generation() == m_generation" plugins/fullscreen-plugins-dont-reload.html
Comment 13 Kenneth Russell 2011-12-14 17:39:05 PST
Reverted r102816 for reason:

Implicated in font-related crashes on Chromium canaries.

Committed r102858: <http://trac.webkit.org/changeset/102858>
Comment 14 Mike Reed 2011-12-16 13:15:57 PST
Created attachment 119660 [details]
Comment 15 Mike Reed 2011-12-16 13:18:33 PST
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.
Comment 16 Mike Reed 2011-12-16 14:31:26 PST
Comment on attachment 119660 [details]

local failures, investigating...
Comment 17 Mike Reed 2011-12-19 10:27:43 PST
Local failures was a flake. Ready for review.
Comment 18 Stephen White 2011-12-19 12:02:07 PST
Comment on attachment 119660 [details]

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.
Comment 19 Mike Reed 2011-12-19 12:26:16 PST
which tests:

Text drawn with transparency or shadows takes the skia code-path.
Comment 20 WebKit Review Bot 2011-12-19 13:40:17 PST
Comment on attachment 119660 [details]

Clearing flags on attachment: 119660

Committed r103262: <http://trac.webkit.org/changeset/103262>
Comment 21 WebKit Review Bot 2011-12-19 13:40:22 PST
All reviewed patches have been landed.  Closing bug.