| 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: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Mac | ||||||||
| OS: | OS X 10.6 | ||||||||
| Bug Depends on: | 79833 | ||||||||
| Bug Blocks: | |||||||||
| Attachments: |
|
||||||||
|
Description
Ned Holbrook
2012-02-28 11:09:55 PST
Created attachment 129296 [details]
Proposed changes.
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 *. 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 on attachment 129299 [details] Changes per review. Clearing flags on attachment: 129299 Committed r109137: <http://trac.webkit.org/changeset/109137> All reviewed patches have been landed. Closing bug. 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. (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. Rolled out in http://trac.webkit.org/changeset/109146 Leopard? Hmm. Given the earlier comments, what say I just rip it out altogether? I’m still ok with that. Or just not touching it. Okay, I'll let sleeping dogs lie. |