Bug 83002 – [Chromium] Out-of-process font loading garbles text

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Kenichi Ishibashi]

Original reports:

http://crbug.com/108645
http://crbug.com/121351
http://crbug.com/120929

This is not a webfont-related issue. FontPlatformData::ctFont() doesn't point out the correct NSFont* if we load the font from the browser process.

[Kenichi Ishibashi]

Created attachment 135283 [details]
Patch

[Kenichi Ishibashi]

Jeremy, Kent-san, could you take a look?

Cc'ing mitz@ just in case because the patch modifies mac font-related files.

[Jeremy Moskovich]

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?

[Kenichi Ishibashi]

Created attachment 135484 [details]
Patch

[Kenichi Ishibashi]

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.

[Kenichi Ishibashi]

Created attachment 135485 [details]
Fix comments in CrossProcessFontLoading.mm

[Kent Tamura]

EWSs are purple.

[Kenichi Ishibashi]

Created attachment 135487 [details]
Revised to ToT

[Kent Tamura]

Comment on attachment 135487 [details]
Revised to ToT

rubber-stamped

[Kenichi Ishibashi]

Comment on attachment 135487 [details]
Revised to ToT

Thanks. I'll watch bots after the patch is landed.

[WebKit Review Bot]

Comment on attachment 135487 [details]
Revised to ToT

Clearing flags on attachment: 135487

Committed r113132: <http://trac.webkit.org/changeset/113132>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

I wonder if this could be related to this:
http://code.google.com/p/chromium/issues/detail?id=125860