[skia] cache typeface in FontPlatformData
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>