Created attachment 286187[details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 286191[details]
Archive of layout-test-results from ews116 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 286242[details]
Archive of layout-test-results from ews100 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 286243[details]
Archive of layout-test-results from ews112 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 286245[details]
Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.5
Created attachment 286416[details]
Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.5
Created attachment 286448[details]
Archive of layout-test-results from ews100 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 286449[details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 286450[details]
Archive of layout-test-results from ews113 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 286451[details]
Archive of layout-test-results from ews125 for ios-simulator-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.5
Comment on attachment 286464[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=286464&action=review> Source/WebCore/platform/graphics/FontMetrics.h:35
> FontMetrics()
> - : m_unitsPerEm(gDefaultUnitsPerEm)
> - , m_ascent(0)
> - , m_descent(0)
> - , m_lineGap(0)
> - , m_lineSpacing(0)
> - , m_xHeight(0)
> - , m_zeroWidth(0)
> - , m_hasXHeight(false)
> - , m_hasZeroWidth(false)
> {
> }
We should just omit this constructor entirely. The compiler will generate it without us defining it.
> Source/WebCore/platform/graphics/FontMetrics.h:100
> - int capHeight() const { return lroundf(m_capHeight); }
> + int capHeight() const { return lroundf(m_capHeight.value()); }
This is a change in behavior. We will now assert if this is called when we don’t have a cap height; before we would presumably return 0. Should this return valueOr(0)?
> Source/WebCore/platform/graphics/FontMetrics.h:125
> + unsigned m_unitsPerEm { gDefaultUnitsPerEm };
Naming here with "g" is so peculiar.
> Source/WebCore/platform/graphics/FontMetrics.h:132
> + Optional<float> m_xHeight;
> + Optional<float> m_capHeight;
This does not seem right. The code in this class seems to handle Nullopt and 0 identically for both of these. I can’t find any code that handled a missing height differently from 0. Although some of the new code will now abort inside Optional in that case.
> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:92
> + RetainPtr<NSString> familyName = adoptNS((NSString *)CTFontCopyFamilyName(font));
The reason we have separate adoptNS and adoptCF functions was originally that under garbage collection the two operations are different. It’s generally not OK to just cast a CF pointer to NS and then do the adopt in the NS context. At the moment it might be OK since WebKit no longer supports garbage collection, but later when adopt ARC it will again likely be important to not use adoptNS on a CF Copy/Create function result. So this needs to use adoptCF instead of adoptNS. We can still cast the result to NSString if we want to use NS methods on it, but not in the RetainPtr. We could also just use CFStringCompare or put in a WTF::String and use equalIgnoringASCIICase functions.
> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:94
> + if (![familyName length])
> + return false;
I don’t think it’s important to optimize the empty string case. I am guessing this is because some other code path was going to use CFStringCompare and in that case we need a null check.
> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:101
> + if ([familyName.get() compare:@"Times" options:NSCaseInsensitiveSearch] == NSOrderedSame
> + || [familyName.get() compare:@"Helvetica" options:NSCaseInsensitiveSearch] == NSOrderedSame
> + || [familyName.get() compare:@".Helvetica NeueUI" options:NSCaseInsensitiveSearch] == NSOrderedSame)
> + return true;
> +
> + return false;
Typically instead of if (x) return true; return false; we just return x;
> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:144
> + RetainPtr<CFStringRef> familyName = adoptCF(CTFontCopyFamilyName(m_platformData.font()));
I suggest using auto here for the RetainPtr type.
> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:145
> +#if !PLATFORM(IOS)
I think this is clearer as #if PLATFORM(MAC).
> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:154
> ascent += floorf(((ascent + descent) * 0.15f) + 0.5f);
Should use std::floor instead of floorf. Also seems like this is rounding, so I would think std::round would be better than std::floor(x + 0.5). Also seems that the "is this one of the three font family names that need adjustment" code should go into a helper function.
> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:176
> + float xHeight = CGFontGetDescriptor(m_platformData.cgFont(), &descriptor) ? (descriptor.xHeight / 1000) * CTFontGetSize(m_platformData.font()) : 0;
Should consider using auto here instead of float.
> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:180
> + lineGap = ceilf(lineGap);
> + lineSpacing = ceil(ascent) + adjustment + ceil(descent) + lineGap;
> + ascent = ceilf(ascent + adjustment);
> + descent = ceilf(descent);
Should use std::ceil instead of ceilf/ceil, which end up converting back and forth between float and double.
In general this code seems confused about float vs. double vs. CGFloat and this will lead to slowness as we convert things back and forth.
> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:212
> + RetainPtr<CFDataRef> os2Table = adoptCF(CTFontCopyTable(m_platformData.font(), kCTFontTableOS2, kCTFontTableOptionNoOptions));
auto
> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:219
> + RetainPtr<CFDataRef> headTable = adoptCF(CTFontCopyTable(m_platformData.font(), kCTFontTableHead, kCTFontTableOptionNoOptions));
auto
> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:566
> + int fixedPitch = 0;
> + if (RetainPtr<CFNumberRef> fixedPitchNumber = adoptCF(static_cast<CFNumberRef>(CTFontCopyAttribute(m_platformData.font(), kCTFontFixedAdvanceAttribute))))
> + CFNumberGetValue(fixedPitchNumber.get(), kCFNumberIntType, &fixedPitch);
I think auto is better here than RetainPtr<CFNumberRef>.
But also, I think a helper function to get an int out of a possibly-null CFNumberRef would make this cleaner.
> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:567
> + m_treatAsFixedPitch = (CTFontGetSymbolicTraits(ctFont) & kCTFontMonoSpaceTrait) || fixedPitch || (caseInsensitiveCompare(fullName.get(), CFSTR("Osaka-Mono")) || caseInsensitiveCompare(fullName.get(), CFSTR("MS-PGothic")) || caseInsensitiveCompare(fullName.get(), CFSTR("MonotypeCorsiva")));
The caseInsensitiveCompare function used here should be used elsewhere in this file. I see lots of other places both above and below that do not use it.
> Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:169
> + RetainPtr<CFStringRef> fontDescription = adoptCF(CFCopyDescription(font()));
Please use auto here.
> Source/WebCore/platform/graphics/opentype/OpenTypeCG.cpp:60
> + if (RetainPtr<CFDataRef> os2Table = adoptCF(CTFontCopyTable(font, kCTFontTableOS2, kCTFontTableOptionNoOptions))) {
auto
> Source/WebCore/platform/graphics/win/FontCGWin.cpp:176
> + CGAffineTransform savedMatrix = CGContextGetTextMatrix(cgContext);
> CGContextSetTextMatrix(cgContext, matrix);
Annoying to not have the ScopedTextMatrix to use here. Is there a header for CG within WebCore that we can use?
Created attachment 286690[details]
Archive of layout-test-results from ews125 for ios-simulator-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125 Port: ios-simulator-elcapitan-wk2 Platform: Mac OS X 10.11.5
Comment on attachment 286464[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=286464&action=review>> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:176
>> + float xHeight = CGFontGetDescriptor(m_platformData.cgFont(), &descriptor) ? (descriptor.xHeight / 1000) * CTFontGetSize(m_platformData.font()) : 0;
>
> Should consider using auto here instead of float.
Unfortunately, tests start failing if you change the precision. I intend to fix this, but I will do it in a separate patch because this one is supposed to have no behavior change.
>> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:180
>> + descent = ceilf(descent);
>
> Should use std::ceil instead of ceilf/ceil, which end up converting back and forth between float and double.
>
> In general this code seems confused about float vs. double vs. CGFloat and this will lead to slowness as we convert things back and forth.
Ditto.
2016-08-16 02:22 PDT, Myles C. Maxfield
2016-08-16 02:37 PDT, Myles C. Maxfield
2016-08-16 11:45 PDT, Build Bot
2016-08-16 12:08 PDT, Build Bot
2016-08-16 12:43 PDT, Myles C. Maxfield
2016-08-16 16:10 PDT, Myles C. Maxfield
2016-08-16 17:15 PDT, Myles C. Maxfield
2016-08-16 18:08 PDT, Build Bot
2016-08-16 18:17 PDT, Build Bot
2016-08-16 18:21 PDT, Build Bot
2016-08-17 03:34 PDT, Myles C. Maxfield
2016-08-18 14:25 PDT, Myles C. Maxfield
2016-08-18 14:45 PDT, Myles C. Maxfield
2016-08-18 15:55 PDT, Myles C. Maxfield
2016-08-18 16:59 PDT, Build Bot
2016-08-18 17:08 PDT, Myles C. Maxfield
2016-08-18 23:34 PDT, Myles C. Maxfield
2016-08-19 00:52 PDT, Myles C. Maxfield
2016-08-19 02:47 PDT, Myles C. Maxfield
2016-08-19 02:55 PDT, Myles C. Maxfield
2016-08-19 03:48 PDT, Build Bot
2016-08-19 03:49 PDT, Build Bot
2016-08-19 04:02 PDT, Build Bot
2016-08-19 04:47 PDT, Build Bot
2016-08-19 11:05 PDT, Myles C. Maxfield
2016-08-19 11:17 PDT, Myles C. Maxfield
2016-08-22 22:59 PDT, Myles C. Maxfield
2016-08-23 00:25 PDT, Myles C. Maxfield
2016-08-23 01:13 PDT, Build Bot
2016-08-23 13:21 PDT, Myles C. Maxfield