Summary: | [OS X] Migrate our Font classes entirely off of NSFont | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||||
Component: | New Bugs | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | commit-queue | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Myles C. Maxfield
2016-05-08 11:51:02 PDT
Created attachment 278367 [details]
Patch
Comment on attachment 278367 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=278367&action=review How safe is this switch? Is there any risk of behavior change? > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1019 > + NSDictionary *dict = [NSDictionary dictionaryWithObjectsAndKeys: > + static_cast<NSString *>(adoptCF(CTFontCopyPostScriptName(font)).get()) , NSAccessibilityFontNameKey, > + static_cast<NSString *>(adoptCF(CTFontCopyFamilyName(font)).get()) , NSAccessibilityFontFamilyKey, > + static_cast<NSString *>(adoptCF(CTFontCopyDisplayName(font)).get()) , NSAccessibilityVisibleNameKey, > + [NSNumber numberWithFloat:CTFontGetSize(font)] , NSAccessibilityFontSizeKey, > + nil]; WebKit coding style specifically asks us to avoid this style, where we line things up in columns. > Source/WebCore/bindings/objc/DOM.mm:728 > - RenderObject* renderer = core(self)->renderer(); > - if (!renderer) > - return nil; > - return renderer->style().fontCascade().primaryFont().getCTFont(); > + if (RenderObject* renderer = core(self)->renderer()) > + return renderer->style().fontCascade().primaryFont().getCTFont(); > + return nil; I prefer early return to nested code like this, except where there are multiple cases. Also, I suggest we use auto instead of RenderObject* for the type. We may actually get more efficient code because I am pretty sure Element::renderer returns something more specific than RenderObject. > Source/WebCore/editing/cocoa/HTMLConverter.mm:1041 > + if (auto renderer = element.renderer()) > + return (PlatformFont *)renderer->style().fontCascade().primaryFont().getCTFont(); > + return nil; Same comment about early return. Why is the explicit cast to PlatformFont * needed? > Source/WebKit/mac/WebCoreSupport/PopupMenuMac.mm:175 > + RetainPtr<CTFontRef> defaultFont = adoptCF(CTFontCreateUIFontForLanguage(kCTFontSystemFontType, CTFontGetSize(font), nil)); I suggest using auto here instead of explicit RetainPtr<CTFontRef>. The type would be implicit based on the right side expression. > Source/WebKit2/WebProcess/WebCoreSupport/mac/WebPopupMenuMac.mm:46 > + RetainPtr<CTFontDescriptorRef> fontDescriptor = adoptCF(CTFontCopyFontDescriptor(font)); Ditto. > Source/WebKit2/WebProcess/WebCoreSupport/mac/WebPopupMenuMac.mm:50 > + RetainPtr<CFDictionaryRef> attributes = adoptCF(CTFontDescriptorCopyAttributes(fontDescriptor.get())); Ditto. > Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:370 > + if (CTFontRef ctFont = font ? font->getCTFont() : nullptr) { > + fontName = adoptCF(CTFontCopyPostScriptName(ctFont)).get(); > + fontSize = CTFontGetSize(ctFont); > } How about nesting a second if rather than using ?: here? Created attachment 278371 [details]
Patch
(In reply to comment #2) > Comment on attachment 278367 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=278367&action=review > > How safe is this switch? Is there any risk of behavior change? Almost no risk. There are two slight changes, but they shouldn't have any effect. Created attachment 278373 [details]
Patch for committing
Comment on attachment 278373 [details] Patch for committing Rejecting attachment 278373 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 278373, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/1289552 Created attachment 278374 [details]
Patch for committing
Comment on attachment 278374 [details] Patch for committing View in context: https://bugs.webkit.org/attachment.cgi?id=278374&action=review More ideas for refinement (after landing, presumably). > Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:548 > + m_treatAsFixedPitch = (CTFontGetSymbolicTraits(ctFont) & kCTFontMonoSpaceTrait) || CGFontIsFixedPitch(m_platformData.cgFont()) || (fullName && (CFStringCompare(fullName.get(), CFSTR("Osaka-Mono"), kCFCompareCaseInsensitive) == kCFCompareEqualTo || CFStringCompare(fullName.get(), CFSTR("MS-PGothic"), kCFCompareCaseInsensitive) == kCFCompareEqualTo || CFStringCompare(fullName.get(), CFSTR("MonotypeCorsiva"), kCFCompareCaseInsensitive) == kCFCompareEqualTo)); These calls to CFStringCompare really call for a helper function to make this expression readable. > Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:598 > + CGFontRenderingStyle renderingStyle = kCGFontRenderingStyleAntialiasing | kCGFontRenderingStyleSubpixelPositioning | kCGFontRenderingStyleSubpixelQuantization | kCGFontAntialiasingStyleUnfiltered; Probably could have named this just style instead of renderingStyle. > Source/WebKit/mac/WebCoreSupport/PopupMenuMac.mm:87 > + font = style.font().weight() < FontWeightBold ? adoptCF(CTFontCreateUIFontForLanguage(kCTFontUIFontSystem, size, nullptr)) : adoptCF(CTFontCreateUIFontForLanguage(kCTFontUIFontEmphasizedSystem, size, nullptr)); Seems like we could use the ? : just for the kCTFontUIFont constants instead of repeating the whole expression twice. > Source/WebKit/mac/WebView/WebHTMLView.mm:5650 > + auto aName = [a fontName]; > + if (!fontName || !aName || !CFEqual(fontName.get(), static_cast<CFStringRef>([a fontName]))) Forgot to use aName here inside the static_cast. > Source/WebKit2/WebProcess/WebCoreSupport/mac/WebPopupMenuMac.mm:54 > + data.fontInfo.fontAttributeDictionary = attributes; Surprised you don’t need to call get() here. Comment on attachment 278374 [details] Patch for committing Rejecting attachment 278374 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: ntBase.dia -c /Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLFrameElementBase.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/HTMLFrameElementBase.o ** BUILD FAILED ** The following build commands failed: CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/HTMLConverter.o editing/cocoa/HTMLConverter.mm normal x86_64 objective-c++ com.apple.compilers.llvm.clang.1_0.compiler (1 failure) Full output: http://webkit-queues.webkit.org/results/1289610 Created attachment 278377 [details]
Patch for committing
Comment on attachment 278377 [details] Patch for committing Clearing flags on attachment: 278377 Committed r200563: <http://trac.webkit.org/changeset/200563> All reviewed patches have been landed. Closing bug. |