Bug 79811 - Reimplement pathFromFont() in SimpleFontDataMac.mm
Summary: Reimplement pathFromFont() in SimpleFontDataMac.mm
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.6
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on: 79833
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-28 11:09 PST by Ned Holbrook
Modified: 2012-02-28 14:22 PST (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Ned Holbrook 2012-02-28 11:09:55 PST
It certainly can't hurt to fix this debugging function.
Comment 1 Ned Holbrook 2012-02-28 11:14:48 PST
Created attachment 129296 [details]
Proposed changes.
Comment 2 mitz 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 *.
Comment 3 Ned Holbrook 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.
Comment 4 WebKit Review Bot 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>
Comment 5 WebKit Review Bot 2012-02-28 12:29:12 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Adam Klein 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.
Comment 7 mitz 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.
Comment 8 Adam Klein 2012-02-28 13:51:04 PST
Rolled out in http://trac.webkit.org/changeset/109146
Comment 9 Ned Holbrook 2012-02-28 14:02:38 PST
Leopard? Hmm. Given the earlier comments, what say I just rip it out altogether?
Comment 10 mitz 2012-02-28 14:10:39 PST
Iā€™m still ok with that. Or just not touching it.
Comment 11 Ned Holbrook 2012-02-28 14:22:09 PST
Okay, I'll let sleeping dogs lie.