Created attachment 303123[details]
Archive of layout-test-results from ews103 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 303128[details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 303110[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=303110&action=review
I think we should try to figure out how to do more of what the fast/text/font-weights test covers using some kind of reference test or special hook to help detect exactly what font was chosen. The expected PNG file for that test seems especially wasteful since it's covering only the small start of the tall document, so it both has poor coverage and is sensitive to font rendering details that we are not trying to test.
> Source/WebCore/css/StyleBuilderConverter.h:1157
> +inline float StyleBuilderConverter::convertFontStretch(StyleResolver&, const CSSValue& value)
Can this take a CSSPrimitiveValue and put the typecast at the call site, or is there some reason not to do that?
> Source/WebCore/css/parser/CSSPropertyParser.cpp:885
> +static RefPtr<CSSPrimitiveValue> consumeFontStretch(CSSParserTokenRange& range, FontStretchValuesAccepted accept = FontStretchValuesAccepted::AllValues)
This should be two separate functions, not one function with a boolean/enum argument. Something like consumeFontStretchKeyword and consumeFontStretchValue (maybe you can come up with even better names); the value one would call the keyword one.
> Source/WebCore/css/parser/CSSPropertyParser.cpp:888
> + const CSSParserToken& token = range.peek();
> + switch (token.id()) {
I don’t think the local variable is helpful here. I would just write range.peek().id() instead.
> Source/WebCore/platform/graphics/FontCache.h:112
> + hasher.add(m_stretch);
It is best to hash just the integer part of the stretch, guaranteeing collisions any time the difference is only in the fractional part? Another possibility would be to hash the actual 32 bits of the float, using an appropriate typecast to pass those bits as an unsigned 32-bit integer to IntegerHasher. Or maybe IntegerHasher already has an overload for float that does that?
> Source/WebCore/platform/graphics/FontCache.h:114
> for (unsigned flagItem : m_flags)
> hasher.add(flagItem);
We typically would want to hash the length of m_flags too in some way, not just the content. By perhaps hashing a fixed trailing value, or by literally hashing the length.
> Source/WebCore/platform/graphics/FontCache.h:115
> hasher.add(m_featureSettings.hash());
Hash of a hash is not as good as actually hashing all the data, although I suppose we are using that technique elsewhere too.
Ideally, we need to test our hashes with test cases that record collision rates in hash tables.
> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1019
> + return threshold - font.stretch.maximum;
This is indented one tab stop too many.
Created attachment 303129[details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 303110[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=303110&action=review
Some comments about parser-specific stuff:
> Source/WebCore/css/parser/CSSPropertyParser.cpp:888
> + const CSSParserToken& token = range.peek();
> + switch (token.id()) {
You can just write:
switch (range.peek().id())
> Source/WebCore/css/parser/CSSPropertyParser.cpp:898
> + case CSSValueUltraCondensed:
> + case CSSValueExtraCondensed:
> + case CSSValueCondensed:
> + case CSSValueSemiCondensed:
> + case CSSValueNormal:
> + case CSSValueSemiExpanded:
> + case CSSValueExpanded:
> + case CSSValueExtraExpanded:
> + case CSSValueUltraExpanded:
> + return consumeIdent(range);
consumeIdent does take a <> syntax where you can list the idents you want to accept. Will leave it up to you if you want to make that change, e.g.,
auto ident = consumeIdent<CSSValueUltraCondensed, CSSValueExtraCondensed, etc.>(range);
if (ident)
return ident;
Created attachment 303132[details]
Archive of layout-test-results from ews113 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 303110[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=303110&action=review>> Source/WebCore/css/StyleBuilderConverter.h:1157
>> +inline float StyleBuilderConverter::convertFontStretch(StyleResolver&, const CSSValue& value)
>
> Can this take a CSSPrimitiveValue and put the typecast at the call site, or is there some reason not to do that?
Unfortunately, this is called from generated code :(
>> Source/WebCore/platform/graphics/FontCache.h:112
>> + hasher.add(m_stretch);
>
> It is best to hash just the integer part of the stretch, guaranteeing collisions any time the difference is only in the fractional part? Another possibility would be to hash the actual 32 bits of the float, using an appropriate typecast to pass those bits as an unsigned 32-bit integer to IntegerHasher. Or maybe IntegerHasher already has an overload for float that does that?
This is a good idea; I think it's worth migrating to fixed-point values. However, that would be a big enough change to warrant its own patch.
>> Source/WebCore/platform/graphics/FontCache.h:114
>> hasher.add(flagItem);
>
> We typically would want to hash the length of m_flags too in some way, not just the content. By perhaps hashing a fixed trailing value, or by literally hashing the length.
I don't understand. m_flags has a fixed size, so what value is there in hashing a constant "2" for all items in the cache?
Created attachment 303140[details]
Archive of layout-test-results from ews114 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 303141[details]
Archive of layout-test-results from ews103 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 303142[details]
Archive of layout-test-results from ews123 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 303144[details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 303156[details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 303157[details]
Archive of layout-test-results from ews112 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 303158[details]
Archive of layout-test-results from ews126 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 303160[details]
Archive of layout-test-results from ews101 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
2017-02-28 22:44 PST, Myles C. Maxfield
2017-03-01 11:55 PST, Myles C. Maxfield
2017-03-01 12:51 PST, Myles C. Maxfield
2017-03-01 13:07 PST, Myles C. Maxfield
2017-03-01 13:44 PST, Myles C. Maxfield
buildbot: commit-queue-
2017-03-01 14:44 PST, Build Bot
2017-03-01 14:57 PST, Build Bot
2017-03-01 14:59 PST, Build Bot
2017-03-01 15:09 PST, Build Bot
2017-03-01 15:39 PST, Myles C. Maxfield
2017-03-01 15:40 PST, Myles C. Maxfield
2017-03-01 16:57 PST, Build Bot
2017-03-01 17:01 PST, Build Bot
2017-03-01 17:03 PST, Build Bot
2017-03-01 17:05 PST, Build Bot
2017-03-01 17:37 PST, Myles C. Maxfield
2017-03-01 18:50 PST, Build Bot
2017-03-01 18:59 PST, Build Bot
2017-03-01 19:02 PST, Build Bot
2017-03-01 19:13 PST, Build Bot
2017-03-01 19:46 PST, Myles C. Maxfield
2017-03-01 21:14 PST, Myles C. Maxfield