Bug 79811

Summary:

Reimplement pathFromFont() in SimpleFontDataMac.mm

Product:

WebKit

Reporter:

Ned Holbrook <ned>

Component:

Text

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED WONTFIX

Severity:

Enhancement

CC:

adamk, mitz, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

OS X 10.6

Bug Depends on:

79833

Bug Blocks:

Attachments:

Description	Flags
Proposed changes.	mitz: review+
Changes per review.	none

Ned Holbrook

Reported 2012-02-28 11:09:55 PST

It certainly can't hurt to fix this debugging function.

Attachments
Proposed changes. (2.04 KB, patch) 2012-02-28 11:14 PST, Ned Holbrook	mitz: review+	Details Formatted Diff Diff
Changes per review. (2.04 KB, patch) 2012-02-28 11:35 PST, Ned Holbrook	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Ned Holbrook

Comment 1 2012-02-28 11:14:48 PST

Created attachment 129296 [details] Proposed changes.

mitz

Comment 2 2012-02-28 11:30:53 PST

Comment on attachment 129296 [details] Proposed changes. View in context: https://bugs.webkit.org/attachment.cgi?id=129296&action=review OK. I think you could have just as well removed this function and changed its callers. > Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:85 > +static NSString* pathFromFont(NSFont* font) Spaces should go before the star in Objective-C objects (I know this was already wrong in some of this code). > Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:87 > + return [[[font fontDescriptor] objectForKey:(NSString*)kCTFontURLAttribute] path]; Needs a space before the *.

Ned Holbrook

Comment 3 2012-02-28 11:35:54 PST

Created attachment 129299 [details] Changes per review. I fixed the spaces, but I could always remove it if you think it's not valuable.

WebKit Review Bot

Comment 4 2012-02-28 12:29:08 PST

Comment on attachment 129299 [details] Changes per review. Clearing flags on attachment: 129299 Committed r109137: <http://trac.webkit.org/changeset/109137>

WebKit Review Bot

Comment 5 2012-02-28 12:29:12 PST

All reviewed patches have been landed. Closing bug.

Adam Klein

Comment 6 2012-02-28 13:40:09 PST

This breaks the Chromium Mac builders, I think because kCTFontURLAttribute is not available on Leopard. Sample failure: http://build.chromium.org/p/chromium.webkit/builders/Mac%20Builder%20%28dbg%29/builds/4938/steps/compile/logs/stdio Given that this is just for debugging I'd prefer to simply roll this out for now, please speak up if you disagree.

mitz

Comment 7 2012-02-28 13:43:03 PST

(In reply to comment #6) > This breaks the Chromium Mac builders, I think because kCTFontURLAttribute is not available on Leopard. Sorry about that. > Sample failure: > > http://build.chromium.org/p/chromium.webkit/builders/Mac%20Builder%20%28dbg%29/builds/4938/steps/compile/logs/stdio > > Given that this is just for debugging I'd prefer to simply roll this out for now, please speak up if you disagree. Yes, feel free to revert this change.

Adam Klein

Comment 8 2012-02-28 13:51:04 PST

Rolled out in http://trac.webkit.org/changeset/109146

Ned Holbrook

Comment 9 2012-02-28 14:02:38 PST

Leopard? Hmm. Given the earlier comments, what say I just rip it out altogether?

mitz

Comment 10 2012-02-28 14:10:39 PST

I’m still ok with that. Or just not touching it.

Ned Holbrook

Comment 11 2012-02-28 14:22:09 PST

Okay, I'll let sleeping dogs lie.

Note You need to log in before you can comment on or make changes to this bug.