Summary: | [Chromium] Out-of-process font loading garbles text | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kenichi Ishibashi <bashi> | ||||||||||
Component: | Platform | Assignee: | Kenichi Ishibashi <bashi> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | cc-bugs, eric, jamesr, mitz, playmobil, tkent, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
URL: | http://crbug.com/108645 | ||||||||||||
Attachments: |
|
Description
Kenichi Ishibashi
2012-04-02 23:36:01 PDT
Created attachment 135283 [details]
Patch
Jeremy, Kent-san, could you take a look? Cc'ing mitz@ just in case because the patch modifies mac font-related files. Comment on attachment 135283 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135283&action=review (disclaimer: As you know, I'm not a WebKit reviewer) LGTM Thanks for fixing! Did you test on 10.6 and 10.7 ? > Source/WebCore/platform/graphics/chromium/CrossProcessFontLoading.mm:194 > +// failed. How about: The font that was actually loaded or null if loading failed. > Source/WebCore/platform/graphics/chromium/CrossProcessFontLoading.mm:217 > + // If we still can't load the font, set the null value to|outNSFont| so that the FontPlatformData won't be used. nit : space after "to". How about: If we still can't load the font, set |outNSFont| to null so that FontPlatformData won't be used. > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:234 > + if (!platformData->font()) Can you add a comment that this this can be false for the case of Chrome OOP font loading? Created attachment 135484 [details]
Patch
Comment on attachment 135283 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=135283&action=review Thank you for review! I confirmed the fix on SnowLeopard and Lion. >> Source/WebCore/platform/graphics/chromium/CrossProcessFontLoading.mm:194 >> +// failed. > > How about: > The font that was actually loaded or null if loading failed. Done. >> Source/WebCore/platform/graphics/chromium/CrossProcessFontLoading.mm:217 >> + // If we still can't load the font, set the null value to|outNSFont| so that the FontPlatformData won't be used. > > nit : space after "to". > > How about: > If we still can't load the font, set |outNSFont| to null so that FontPlatformData won't be used. Done. >> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:234 >> + if (!platformData->font()) > > Can you add a comment that this this can be false for the case of Chrome OOP font loading? Done. Created attachment 135485 [details]
Fix comments in CrossProcessFontLoading.mm
EWSs are purple. Created attachment 135487 [details]
Revised to ToT
Comment on attachment 135487 [details]
Revised to ToT
rubber-stamped
Comment on attachment 135487 [details]
Revised to ToT
Thanks. I'll watch bots after the patch is landed.
Comment on attachment 135487 [details] Revised to ToT Clearing flags on attachment: 135487 Committed r113132: <http://trac.webkit.org/changeset/113132> All reviewed patches have been landed. Closing bug. I wonder if this could be related to this: http://code.google.com/p/chromium/issues/detail?id=125860 |