|
Description
Myles C. Maxfield
2017-02-26 21:09:06 PST
*** Bug 169017 has been marked as a duplicate of this bug. *** Created attachment 303042 [details]
Needs @font-face and font shorthand
Created attachment 303094 [details]
Patch
Created attachment 303100 [details]
Patch
Created attachment 303103 [details]
Patch
Created attachment 303110 [details]
Patch
Comment on attachment 303110 [details] Patch Attachment 303110 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3217940 New failing tests: fast/text/font-stretch-parse.html fast/css/font-shorthand-mix-inherit.html 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
Comment on attachment 303110 [details] Patch Attachment 303110 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3217950 New failing tests: fast/text/font-stretch-parse.html fast/css/font-shorthand-mix-inherit.html fast/css/css2-system-fonts.html 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; Comment on attachment 303110 [details] Patch Attachment 303110 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3217994 New failing tests: fast/text/font-stretch-parse.html fast/css/font-shorthand-mix-inherit.html 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 303136 [details]
Patch for committing
Created attachment 303137 [details]
Patch for committing
Comment on attachment 303137 [details] Patch for committing Attachment 303137 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3218680 New failing tests: imported/blink/fast/css/font-face-inherit-initial.html fast/css/font-shorthand-mix-inherit.html fast/text/font-stretch-shorthand.html fast/text/font-stretch-parse.html 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
Comment on attachment 303137 [details] Patch for committing Attachment 303137 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3218726 New failing tests: fast/text/font-stretch-shorthand.html fast/css/font-shorthand-mix-inherit.html fast/text/font-stretch-parse.html 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
Comment on attachment 303137 [details] Patch for committing Attachment 303137 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3218692 New failing tests: fast/text/font-stretch-shorthand.html fast/css/font-shorthand-mix-inherit.html fast/text/font-stretch-parse.html 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
Comment on attachment 303137 [details] Patch for committing Attachment 303137 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3218733 New failing tests: fast/text/font-stretch-shorthand.html fast/css/font-shorthand-mix-inherit.html fast/text/font-stretch-parse.html 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 303149 [details]
Patch for committing
Comment on attachment 303149 [details] Patch for committing Attachment 303149 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3219224 New failing tests: fast/inspector-support/style.html fast/css/font-shorthand.html fast/text/font-stretch-parse.html 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
Comment on attachment 303149 [details] Patch for committing Attachment 303149 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3219279 New failing tests: fast/inspector-support/style.html fast/css/font-shorthand.html fast/text/font-stretch-parse.html 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
Comment on attachment 303149 [details] Patch for committing Attachment 303149 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3219258 New failing tests: fast/inspector-support/style.html fast/css/font-shorthand.html fast/text/font-stretch-parse.html 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
Comment on attachment 303149 [details] Patch for committing Attachment 303149 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3219350 New failing tests: fast/inspector-support/style.html fast/css/font-shorthand.html fast/text/font-stretch-parse.html 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
Created attachment 303162 [details]
Patch for committing
Created attachment 303173 [details]
Patch for committing
Comment on attachment 303173 [details] Patch for committing Clearing flags on attachment: 303173 Committed r213267: <http://trac.webkit.org/changeset/213267> |