Bug 65543 – [Chromium] Fix OOP font loading to work on 10.6.6 and above.

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Jeremy Moskovich]

In 10.6.6 the function used to get the unique ID from the font in the  renderer was changed so it fails in the sandbox (it now tries to access the on-disk font file).

[Jeremy Moskovich]

Created attachment 102656 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 102656 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102656&action=review

> Source/WebCore/ChangeLog:6
> +        In 10.6.6 the function used to get the unique ID for an NSFont in the 

I don't feel qualified to review the patch's functional parts, so I'll nitpick style and spirit.

> Source/WebCore/platform/graphics/chromium/CrossProcessFontLoading.mm:148
> +        // TODO: PlatformBridge::loadFont() should consult the id cache

TODO -> FIXME, file a bug and reference here.

[Ryosuke Niwa]

Comment on attachment 102656 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102656&action=review

I don't know enough about Cocoa to review this patch either :(

> Source/WebCore/ChangeLog:8
> +        the on-disk font file).  In order to work around this, we get the font

We don't double-space sentences.

> Source/WebCore/ChangeLog:11
> +        To speed things up, we introduce 2 levels of caching in WebKit.  A font

Ditto about double-spacing.

[Kenneth Russell]

Comment on attachment 102656 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102656&action=review

The code looks good to me overall. (If this started failing in 10.6.6 why wasn't it found until now?) Please fix the nits upon landing. r=me

> Source/WebCore/platform/graphics/chromium/CrossProcessFontLoading.mm:132
> +    if (font.get())

You should be able to just write "if (font) ...".

> Source/WebCore/platform/graphics/chromium/CrossProcessFontLoading.mm:138
> +    if (!PlatformBridge::loadFont(nsFont, &container, &fontID)) {

No braces on single-line if statements.

> Source/WebKit/chromium/public/mac/WebSandboxSupport.h:59
> +    virtual bool loadFont(NSFont* srcFont, ATSFontContainerRef*, uint32_t* fontID) = 0;

I see you've already landed the downstream override of the new signature -- great.

[Jeremy Moskovich]

Created attachment 102754 [details]
Patch

[WebKit Review Bot]

Comment on attachment 102754 [details]
Patch

Rejecting attachment 102754 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 1

Last 500 characters of output:
hangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
Updating OpenSource
Current branch master is up to date.
Updating chromium port dependencies using gclient...

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/9284990

[Jeremy Moskovich]

Created attachment 102762 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 102762 [details]
Patch for landing

Clearing flags on attachment: 102762

Committed r92269: <http://trac.webkit.org/changeset/92269>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.