Mac: OOP font loading should run on FILE thread.

Mac: OOP font loading should run on FILE thread.

FontLoader calls file_util::GetFileSize() and file_util::ReadFile(),
which should be called from the file thread, but FontLoader runs on the
io thread now. Make it run on the file thread.

BUG=108645
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=131324

[bashi, 2012-04-04 04:28:56]

(brettw@: OWNERS review, feel free to add/remove OWNERS reviewers)

Hi Jeremy,

Could you take a look?

[jeremy, 2012-04-04 08:35:37]

http://chromiumcodereview.appspot.com/9950141/diff/1006/content/browser/rende...
File content/browser/renderer_host/render_message_filter.h (right):

http://chromiumcodereview.appspot.com/9950141/diff/1006/content/browser/rende...
content/browser/renderer_host/render_message_filter.h:137: #if
defined(OS_MACOSX)
Can you add a comment here:
// Messages for OOP font loading.

http://chromiumcodereview.appspot.com/9950141/diff/1006/content/common/mac/fo...
File content/common/mac/font_loader.h (right):

http://chromiumcodereview.appspot.com/9950141/diff/1006/content/common/mac/fo...
content/common/mac/font_loader.h:30: struct Result {
can you add a comment here describing what the structure is used for and why
it's needed.

http://chromiumcodereview.appspot.com/9950141/diff/1006/content/common/mac/fo...
content/common/mac/font_loader.h:40: // the font file.
Can you add a comment as to what to expect in this field on failure?

http://chromiumcodereview.appspot.com/9950141/diff/1006/content/common/mac/fo...
File content/common/mac/font_loader.mm (right):

http://chromiumcodereview.appspot.com/9950141/diff/1006/content/common/mac/fo...
content/common/mac/font_loader.mm:58: DCHECK(result);
Can you DCHECK that this is being called on the file thread?

http://chromiumcodereview.appspot.com/9950141/diff/1006/content/common/mac/fo...
content/common/mac/font_loader.mm:119: if
(!result->font_data.CreateAndMapAnonymous(font_file_size_32)) {
If this or the case below fails do you leak this?

[bashi, 2012-04-04 09:36:22]

Thank you for review!

http://chromiumcodereview.appspot.com/9950141/diff/1006/content/browser/rende...
File content/browser/renderer_host/render_message_filter.h (right):

http://chromiumcodereview.appspot.com/9950141/diff/1006/content/browser/rende...
content/browser/renderer_host/render_message_filter.h:137: #if
defined(OS_MACOSX)
On 2012/04/04 08:35:38, jeremy wrote:
> Can you add a comment here:
> // Messages for OOP font loading.

Done.

http://chromiumcodereview.appspot.com/9950141/diff/1006/content/common/mac/fo...
File content/common/mac/font_loader.h (right):

http://chromiumcodereview.appspot.com/9950141/diff/1006/content/common/mac/fo...
content/common/mac/font_loader.h:30: struct Result {
On 2012/04/04 08:35:38, jeremy wrote:
> can you add a comment here describing what the structure is used for and why
> it's needed.

Done.

http://chromiumcodereview.appspot.com/9950141/diff/1006/content/common/mac/fo...
content/common/mac/font_loader.h:40: // the font file.
On 2012/04/04 08:35:38, jeremy wrote:
> Can you add a comment as to what to expect in this field on failure?

The buffer won't be valid on failure. Added description.

http://chromiumcodereview.appspot.com/9950141/diff/1006/content/common/mac/fo...
File content/common/mac/font_loader.mm (right):

http://chromiumcodereview.appspot.com/9950141/diff/1006/content/common/mac/fo...
content/common/mac/font_loader.mm:58: DCHECK(result);
On 2012/04/04 08:35:38, jeremy wrote:
> Can you DCHECK that this is being called on the file thread?

Done.

http://chromiumcodereview.appspot.com/9950141/diff/1006/content/common/mac/fo...
content/common/mac/font_loader.mm:119: if
(!result->font_data.CreateAndMapAnonymous(font_file_size_32)) {
On 2012/04/04 08:35:38, jeremy wrote:
> If this or the case below fails do you leak this?

Added result->font_data.Close().

[brettw, 2012-04-04 21:42:17]

content LGTM rubberstamp

[jeremy, 2012-04-05 10:36:50]

http://chromiumcodereview.appspot.com/9950141/diff/7001/content/common/mac/fo...
File content/common/mac/font_loader.h (right):

http://chromiumcodereview.appspot.com/9950141/diff/7001/content/common/mac/fo...
content/common/mac/font_loader.h:32: // task which sends the result to
renderers.
nit: *renderers -> the originating renderer.

http://chromiumcodereview.appspot.com/9950141/diff/7001/content/common/mac/fo...
File content/common/mac/font_loader.mm (right):

http://chromiumcodereview.appspot.com/9950141/diff/7001/content/common/mac/fo...
content/common/mac/font_loader.mm:123: result->font_data.Close();
Can you use a scoper for this rather than manually closing the handle here?

[bashi, 2012-04-05 10:49:46]

http://chromiumcodereview.appspot.com/9950141/diff/7001/content/common/mac/fo...
File content/common/mac/font_loader.h (right):

http://chromiumcodereview.appspot.com/9950141/diff/7001/content/common/mac/fo...
content/common/mac/font_loader.h:32: // task which sends the result to
renderers.
On 2012/04/05 10:36:50, jeremy wrote:
> nit: *renderers -> the originating renderer.

Done.

http://chromiumcodereview.appspot.com/9950141/diff/7001/content/common/mac/fo...
File content/common/mac/font_loader.mm (right):

http://chromiumcodereview.appspot.com/9950141/diff/7001/content/common/mac/fo...
content/common/mac/font_loader.mm:123: result->font_data.Close();
On 2012/04/05 10:36:50, jeremy wrote:
> Can you use a scoper for this rather than manually closing the handle here?

I've just noticed that SharedBuffer always calls Close() in the destructor so we
don't need to call Close() here. Removed.

[jeremy, 2012-04-06 05:07:30]

LGTM

[commit-bot: I haz the power, 2012-04-06 05:09:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bashi@chromium.org/9950141/12001

[commit-bot: I haz the power, 2012-04-06 06:26:56]

Try job failure for 9950141-12001 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[bashi, 2012-04-06 07:30:31]

I should have revised the unit test too. jeremy@, could you take another look?

[jeremy, 2012-04-07 10:12:47]

Still LGTM

[commit-bot: I haz the power, 2012-04-08 23:58:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bashi@chromium.org/9950141/12002

[commit-bot: I haz the power, 2012-04-09 02:41:16]

Change committed as 131324