Summary: | [skia] cache typeface in FontPlatformData | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mike Reed <reed> | ||||||||
Component: | New Bugs | Assignee: | Mike Reed <reed> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | brettw, caryclark, cc-bugs, jamesr, kbr, senorblanco, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Mike Reed
2011-12-13 09:00:37 PST
Created attachment 119027 [details]
Patch
local DRT run on windows with no failures Afraid this is outside my areas of expertise. I'm happy to officially review if a domain expert can review the logic first. 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
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 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? Created attachment 119242 [details]
Patch
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 on attachment 119242 [details]
Patch
Looks good. r=me
Comment on attachment 119242 [details] Patch Clearing flags on attachment: 119242 Committed r102816: <http://trac.webkit.org/changeset/102816> All reviewed patches have been landed. Closing bug. 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 Reverted r102816 for reason: Implicated in font-related crashes on Chromium canaries. Committed r102858: <http://trac.webkit.org/changeset/102858> Created attachment 119660 [details]
Patch
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 on attachment 119660 [details]
Patch
local failures, investigating...
Local failures was a flake. Ready for review. 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. which tests: Text drawn with transparency or shadows takes the skia code-path. Comment on attachment 119660 [details] Patch Clearing flags on attachment: 119660 Committed r103262: <http://trac.webkit.org/changeset/103262> All reviewed patches have been landed. Closing bug. |