Bug 41148

Summary: [Chromium] Add support for OOP Font loading
Product: WebKit Reporter: Jeremy Moskovich <playmobil>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: avi, commit-queue, dglazkov, eric, mark, mitz, paulirish
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 41398, 44937    
Attachments:
Description Flags
Fork FontPlatformFontDataMac.mm to enable x-process font loading
dglazkov: review-, dglazkov: commit-queue-
Fork FontPlatformFontDataMac.mm to enable x-process font loading none

Description Jeremy Moskovich 2010-06-24 04:27:22 PDT
http://crbug.com/29729

On OS X, with font management software installed. Fonts can reside in an arbitrary path on disk which is blocked by Chromium's Sandbox.

We need hooks in the Mac version of FontPlatformData that allow us to fall back to loading a font x-process if blocked by the Sandbox.
Comment 1 Jeremy Moskovich 2010-06-24 04:36:24 PDT
Created attachment 59637 [details]
Fork FontPlatformFontDataMac.mm to enable x-process font loading

Fork platform/graphics/mac/FontPlatformFontDataMac.mm to platform/graphics/chromium/FontPlatformDataChromiumMac.mm and add functionality for x-process font loading.

* We need to use the header from platform/graphics/mac since the class is used by a bunch of code in there.
* I'd prefer not to fork the code, suggestions would be much appreciated.
Comment 2 Avi Drissman 2010-06-27 17:59:40 PDT
In the ChangeLog, you can edit out spurious function entries like "(WebCore::):". Otherwise, SLGTM.
Comment 3 Dimitri Glazkov (Google) 2010-06-28 09:12:27 PDT
Comment on attachment 59637 [details]
Fork FontPlatformFontDataMac.mm to enable x-process font loading

Please update the change log and then it'll be ready to land. Also, we need a plan to unfork this. File a bug and block it on this bug.
Comment 4 Eric Seidel (no email) 2010-06-28 13:17:53 PDT
I don't like the word "fork"... chromium is too old of a project with too many webkit reviewers to have any excuse to "fork" files in WebKit anymore.
Comment 5 Eric Seidel (no email) 2010-06-28 13:27:18 PDT
(That said, that's just a drive-by comment, and I'd be the first to admit I don't actually know what's going on here.)
Comment 6 Eric Seidel (no email) 2010-06-28 13:29:04 PDT
As an example of "forks" eating away at the WebKit project.  Both V8 and QtXML nonsense have cost me at least a day of work in the last 2 weeks.  0 value to me.  Huge time sinks when trying to refactor other parts of the code.  Chromium certainly shouldn't be adding to forks these days now that we're big enough to just develop things right in webkit.org and refactor as needed.
Comment 7 Jeremy Moskovich 2010-06-28 13:36:17 PDT
Eric: I think we all whole-heartedly agree with you.

The thinking here was to land this, then immediately submit a cl to merge this with the other Mac code. (If you look, this CL is already structured to make this easy).

The reasoning for having a Chromium codepath in this code isn't obvious, and having this code in the tree will mean that on one hand we can merge with a small patch and on the other it will be easier for reviewers of that patch to understand the context.
Comment 8 Eric Seidel (no email) 2010-06-28 13:38:42 PDT
It would be possible to add sandbox support for DRT.  It would probably require a fork (ha ha) of DRT though, since I doubt that the general AppKit APIs it uses are sandbox safe.
Comment 9 Jeremy Moskovich 2010-06-30 05:41:49 PDT
Created attachment 60113 [details]
Fork FontPlatformFontDataMac.mm to enable x-process font loading
Comment 10 Dimitri Glazkov (Google) 2010-06-30 08:38:50 PDT
Comment on attachment 60113 [details]
Fork FontPlatformFontDataMac.mm to enable x-process font loading

ok.
Comment 11 WebKit Commit Bot 2010-06-30 08:53:10 PDT
Comment on attachment 60113 [details]
Fork FontPlatformFontDataMac.mm to enable x-process font loading

Clearing flags on attachment: 60113

Committed r62187: <http://trac.webkit.org/changeset/62187>
Comment 12 WebKit Commit Bot 2010-06-30 08:53:15 PDT
All reviewed patches have been landed.  Closing bug.