Created attachment 259811 [details] Patch

Attachment 259811 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 259811 [details] Patch Attachment 259811 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/102060 New failing tests: fast/text/font-variant-ligatures.html fast/css/parsing-font-variant-ligatures.html svg/custom/unicode-in-tspan-multi-svg-crash.html

Created attachment 259813 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5

Comment on attachment 259811 [details] Patch Attachment 259811 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/102063 New failing tests: fast/css/parsing-font-variant-ligatures.html fast/text/font-variant-ligatures.html svg/custom/unicode-in-tspan-multi-svg-crash.html

Created attachment 259814 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5

Created attachment 259885 [details] Patch

Created attachment 259895 [details] Patch

Created attachment 259905 [details] Patch

Created attachment 259908 [details] Patch

Comment on attachment 259908 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259908&action=review > Source/WebCore/css/StyleBuilderCustom.h:1493 > + volatile CSSValueID asdf = downcast<CSSPrimitiveValue>(item.get()).getValueID(); > + UNUSED_PARAM(asdf); Whoops

Created attachment 259918 [details] Patch

Comment on attachment 259918 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259918&action=review > Source/WebCore/ChangeLog:47 > + Test: css3/font-variant-all-preinstalled.html I should probably have a test for web fonts, and probably a dedicated test for parsing. Also I should author a font to make these tests less sensitive to platform changes.

Comment on attachment 259918 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259918&action=review > Source/WebCore/platform/text/TextFlags.h:81 > + Common = 1 << 1, > + NoCommon = 2 << 1, > + CommonMask = 3 << 1, These should probably be made into their own enum classes so that no masking is necessary.

Comment on attachment 259918 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259918&action=review > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:147 > + HashMap<String, int> result; Ben says that using this type is good. > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:151 > + result.set(String("liga", String::ConstructFromLiteral), 1); Can do ASCIILiteral("liga")

Comment on attachment 259918 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259918&action=review > Source/WebCore/ChangeLog:15 > + -webkit-font-variant-ligatures > + -webkit-font-variant-position > + -webkit-font-variant-caps > + -webkit-font-variant-numeric > + -webkit-font-variant-alternates > + -webkit-font-variant-east-asian Why are we prefixing these? Do other browsers prefix them? Are the specs still in flux?

Comment on attachment 259918 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259918&action=review > Source/WebCore/css/CSSPropertyNames.in:111 > +-webkit-font-variant-ligatures [Inherited, FontProperty, NameForMethods=VariantLigatures, Custom=Value] I should use converters instead of custom values.

(In reply to comment #16) > Comment on attachment 259918 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=259918&action=review > > > Source/WebCore/ChangeLog:15 > > + -webkit-font-variant-ligatures > > + -webkit-font-variant-position > > + -webkit-font-variant-caps > > + -webkit-font-variant-numeric > > + -webkit-font-variant-alternates > > + -webkit-font-variant-east-asian > > Why are we prefixing these? Do other browsers prefix them? Are the specs > still in flux? Firefox ships these unprefixed. IE/Edge/Chrome do not support these.

Comment on attachment 259918 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259918&action=review >> Source/WebCore/ChangeLog:15 >> + -webkit-font-variant-east-asian > > Why are we prefixing these? Do other browsers prefix them? Are the specs still in flux? I'm happy to unprefix them.

(In reply to comment #19) > Comment on attachment 259918 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=259918&action=review > > >> Source/WebCore/ChangeLog:15 > >> + -webkit-font-variant-east-asian > > > > Why are we prefixing these? Do other browsers prefix them? Are the specs still in flux? > > I'm happy to unprefix them. Let's un-prefix them.

(In reply to comment #20) > > > Why are we prefixing these? Do other browsers prefix them? Are the specs still in flux? > > > > I'm happy to unprefix them. > > Let's un-prefix them. +1

Comment on attachment 259918 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259918&action=review > Source/WebCore/ChangeLog:24 > + The implementation has some caveats, however. They are listed here: I need to do synthesis, too.

Comment on attachment 259918 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=259918&action=review This would be r=me assuming all the changes already noted. >>>>> Source/WebCore/ChangeLog:15 >>>>> + -webkit-font-variant-east-asian >>>> >>>> Why are we prefixing these? Do other browsers prefix them? Are the specs still in flux? >>> >>> Firefox ships these unprefixed. IE/Edge/Chrome do not support these. >> >> I'm happy to unprefix them. > > Let's un-prefix them. +1 > Source/WebCore/css/CSSPrimitiveValueMappings.h:5325 > +template<> inline CSSPrimitiveValue::CSSPrimitiveValue(FontVariantPosition e) e? Please use a better name. > Source/WebCore/css/CSSPrimitiveValueMappings.h:5362 > +template<> inline CSSPrimitiveValue::CSSPrimitiveValue(FontVariantCaps e) e? Please use a better name. > Source/WebCore/css/CSSPrimitiveValueMappings.h:5419 > +template<> inline CSSPrimitiveValue::CSSPrimitiveValue(FontVariantAlternates e) Ditto. >> Source/WebCore/css/CSSPropertyNames.in:111 >> +-webkit-font-variant-ligatures [Inherited, FontProperty, NameForMethods=VariantLigatures, Custom=Value] > > I should use converters instead of custom values. Agreed. > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:622 > + UNUSED_PARAM(featureSettings); > + UNUSED_PARAM(variantSettings); Put these before the return statement. >> Source/WebCore/platform/text/TextFlags.h:81 >> + CommonMask = 3 << 1, > > These should probably be made into their own enum classes so that no masking is necessary. Agreed. > Source/WebCore/platform/text/TextFlags.h:140 > +enum class FontVariantNumeric { > + Normal = 0, > + > + LiningNumbers = 1 << 0, > + OldStyleNumbers = 2 << 0, > + FigureMask = 3 << 0, > + > + ProportionalNumbers = 1 << 2, > + TabularNumbers = 2 << 2, > + SpacingMask = 3 << 2, > + > + DiagonalFractions = 1 << 4, > + StackedFractions = 2 << 4, > + FractionMask = 3 << 4, > + > + Ordinal = 1 << 6, > + SlashedZero = 1 << 7 > +}; Maybe these too? > LayoutTests/css3/font-variant-all-preinstalled.html:22 > +var stylePairs = {"-webkit-font-variant-ligatures" : ["normal", "common-ligatures", "no-common-ligatures", "discretionary-ligatures", "no-discretionary-ligatures", "historical-ligatures", "no-historical-ligatures", "contextual", "no-contextual"], > + "-webkit-font-variant-position" : ["normal", "sub", "super"], > + "-webkit-font-variant-caps" : ["normal", "small-caps", "all-small-caps", "petite-caps", "all-petite-caps", "unicase", "titling-caps"], > + "-webkit-font-variant-numeric" : ["normal", "lining-nums", "oldstyle-nums", "proportional-nums", "tabular-nums", "diagonal-fractions", "stacked-fractions", "ordinal", "slashed-zero"], > + "-webkit-font-variant-alternates" : ["normal", "historical-forms"], > + "-webkit-font-variant-east-asian" : ["normal", "jis78", "jis83", "jis90", "jis04", "simplified", "traditional", "full-width", "proportional-width", "ruby"]}; If you don't use a prefix you won't need quotes for the keys, which is nice.

I added a font in https://trac.webkit.org/r189890 which can be used for testing.

Created attachment 261624 [details] Patch

Created attachment 261625 [details] Patch

Created attachment 261626 [details] Patch

Comment on attachment 261626 [details] Patch Attachment 261626 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/192049 New failing tests: fast/css/parsing-font-variant-ligatures.html svg/css/getComputedStyle-basic.xhtml fast/css/getComputedStyle/computed-style.html fast/css/getComputedStyle/computed-style-without-renderer.html fast/text/font-variant-ligatures.html fast/css/getComputedStyle/computed-style-font-family.html css3/font-variant-all-webfont.html

Created attachment 261627 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5

Comment on attachment 261626 [details] Patch Attachment 261626 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/192052 New failing tests: fast/css/parsing-font-variant-ligatures.html svg/css/getComputedStyle-basic.xhtml fast/css/getComputedStyle/computed-style.html fast/css/getComputedStyle/computed-style-without-renderer.html fast/text/font-variant-ligatures.html fast/css/getComputedStyle/computed-style-font-family.html css3/font-variant-all-webfont.html

Created attachment 261629 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5

Created attachment 261634 [details] Patch

Created attachment 261635 [details] Patch

<rdar://problem/22782154>

Comment on attachment 261635 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=261635&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1372 > +static Ref<CSSValue> getFontVariantAlternatesPropertyValue(FontVariantAlternates alternates) > +{ > + auto& cssValuePool = CSSValuePool::singleton(); > + switch (alternates) { > + case FontVariantAlternates::Normal: > + return cssValuePool.createIdentifierValue(CSSValueNormal); > + case FontVariantAlternates::HistoricalForms: > + return cssValuePool.createIdentifierValue(CSSValueHistoricalForms); > + } > +} Error compiling with the version of GCC used by the efl-wk2 bot: CSSComputedStyleDeclaration.cpp: In function 'WTF::Ref<WebCore::CSSValue> WebCore::getFontVariantAlternatesPropertyValue(WebCore::FontVariantAlternates)': CSSComputedStyleDeclaration.cpp:1372:1: error: control reaches end of non-void function [-Werror=return-type]

Created attachment 261706 [details] Patch

Comment on attachment 261706 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=261706&action=review There is a massive amount of boilerplate code here. Anything we can do to refactor into helper functions to make this tighter would be worthwhile. Specific suggestions below. Testing coverage seems a bit light given the amount of code. Do we have tests that will fail if we make any change to all that code? > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1205 > +static Ref<CSSValue> getFontVariantLigaturesPropertyValue(FontVariantLigatures common, FontVariantLigatures discretionary, FontVariantLigatures historical, FontVariantLigatures contextualAlternates) In WebKit coding style we do not use “get” to name functions that return an object. We use no verb at all, or a verb like “compute”. The verb “get” is reserved for functions that return their results in some different way other than a return value. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1225 > + default: > + ASSERT_NOT_REACHED(); A downside of having a default case is that this turns off the “all cases covered” warning from the compiler. This leaves us with three choices: 1) Do as we have done here, forgo the compile time warning when adding a new enumeration value, but have a runtime check in debug builds in case we have a corrupted enum value. 2) Leave out the default case, making sure we get a compile time warning when adding a new enumeration value, but forgo the runtime check for corrupted enum values. At runtime a corrupted value will be treated the way that “normal” is treated here. 3) Put the logic inside a helper function so we can use return instead of break. Then we can have the ASSERT_NOT_REACHED outside the switch entirely. Like this: static void appendCommonLigaturesValue(CSSValueList& list, FontVariantLigatures value) { switch (value) { case FontVariantLigatures::Normal: return; case FontVariantLigatures::No: list.append(CSSValuePool::singleton().createIdentifierValue(CSSValueNoCommonLigatures)); return; case FontVariantLigatures::Yes: list.append(CSSValuePool::singleton().createIdentifierValue(CSSValueCommonLigatures)); return; } ASSERT_NOT_REACHED(); } Note that if you do (3), you can also do this refactoring: static void appendLigaturesValue(CSSValueList& list, FontVariantLigatures value, CSSValueID noValue, CSSValueID yesValue) { switch (value) { case FontVariantLigatures::Normal: return; case FontVariantLigatures::No: list.append(CSSValuePool::singleton().createIdentifierValue(noValue)); return; case FontVariantLigatures::Yes: list.append(CSSValuePool::singleton().createIdentifierValue(yesValue)); return; } ASSERT_NOT_REACHED(); } static void appendCommonLigaturesValue(CSSValueList& list, FontVariantLigatures value) { appendLigaturesValue(list, value, CSSValueNoCommonLigatures, CSSValueCommonLigatures); } Or we could just use the helper function directly inside the getFontVariantLigaturesPropertyValue function. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1269 > +static Ref<CSSValue> getFontVariantPositionPropertyValue(FontVariantPosition position) Same problem with get in the name. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1274 > + return cssValuePool.createIdentifierValue(CSSValueNormal); I suggest having the switch statement just determine the CSSValueID rather than repeating the createIdentifierValue in each case. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1285 > +static Ref<CSSValue> getFontVariantCapsPropertyValue(FontVariantCaps caps) Ditto. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1290 > + return cssValuePool.createIdentifierValue(CSSValueNormal); Ditto. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1309 > +static Ref<CSSValue> getFontVariantNumericPropertyValue(FontVariantNumericFigure figure, FontVariantNumericSpacing spacing, FontVariantNumericFraction fraction, FontVariantNumericOrdinal ordinal, FontVariantNumericSlashedZero slashedZero) Ditto. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2558 > + for (unsigned i = 0; i < featureSettings.size(); ++i) { > + const FontFeature& feature = featureSettings[i]; > list->append(CSSFontFeatureValue::create(feature.tag(), feature.value())); > } Should use a modern for loop. > Source/WebCore/css/CSSParser.cpp:10475 > + RefPtr<CSSValueList> values = CSSValueList::createSpaceSeparated(); Ref, not RefPtr. Better, use auto. > Source/WebCore/css/CSSParser.cpp:10528 > + RefPtr<CSSValueList> values = CSSValueList::createSpaceSeparated(); Ref, not RefPtr. Better to use auto. > Source/WebCore/css/StyleBuilderConverter.h:1011 > + return FontFeatureSettings(); I think that { } is better than FontFeatureSettings() in a case like this. > Source/WebCore/platform/graphics/FontCache.h:106 > + hasher.add(m_locale.isNull() ? 0 : m_locale.impl()->existingHash()); We should add an existingHash function to String to avoid having to write it like this. > Source/WebCore/platform/graphics/FontCascade.cpp:603 > + // FIXME: This shouldn't be necessary because Font::applyTransforms() should perform all necessary shaping. A clearer comment would be: // FIXME: This shouldn't be necessary once we have enhanced Font::applyTransforms() to perform all necessary shaping. Unless that’s inaccurate in which case I am confused! > Source/WebCore/platform/graphics/FontDescription.cpp:-37 > -struct SameSizeAsFontCascadeDescription { Why did you delete this? We added it for a reason. Did that reason go away? > Source/WebCore/platform/graphics/FontFeatureSettings.h:40 > FontFeature(const AtomicString& tag, int value); > + FontFeature(AtomicString&& tag, int value); Why do we need both of these? Seems we could be fine with only the new one. > Source/WebCore/platform/graphics/FontFeatureSettings.h:51 > - const int m_value { 0 }; > + int m_value { 0 }; Why this change? > Source/WebCore/platform/graphics/FontFeatureSettings.h:66 > + Vector<FontFeature>::const_iterator begin() const { return m_list.begin(); } > + Vector<FontFeature>::const_iterator end() const { return m_list.end(); } This seems a bit peculiar. At this point, why is it helpful to have this be a class instead of just using a typedef for a Vector? > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:151 > + result.set(ASCIILiteral("liga"), 1); add is more efficient than set; the only difference is behavior when adding multiple items with the same value Seems like a HashMap<String, int> isn’t so great for these four character codes. I’d prefer HashMap<unsigned, bool> or something. All the integers are zeros and ones and all the strings are four character codes that can fit into a 32-bit integer. Maybe HashMap<std::array<char, 4>, bool>? > Source/WebCore/platform/text/TextFlags.h:184 > + FontVariantSettings( > + FontVariantLigatures commonLigatures, > + FontVariantLigatures discretionaryLigatures, > + FontVariantLigatures historicalLigatures, > + FontVariantLigatures contextualAlternates, > + FontVariantPosition position, > + FontVariantCaps caps, > + FontVariantNumericFigure numericFigure, > + FontVariantNumericSpacing numericSpacing, > + FontVariantNumericFraction numericFraction, > + FontVariantNumericOrdinal numericOrdinal, > + FontVariantNumericSlashedZero numericSlashedZero, > + FontVariantAlternates alternates, > + FontVariantEastAsianVariant eastAsianVariant, > + FontVariantEastAsianWidth eastAsianWidth, > + FontVariantEastAsianRuby eastAsianRuby) > + : commonLigatures(commonLigatures) > + , discretionaryLigatures(discretionaryLigatures) > + , historicalLigatures(historicalLigatures) > + , contextualAlternates(contextualAlternates) > + , position(position) > + , caps(caps) > + , numericFigure(numericFigure) > + , numericSpacing(numericSpacing) > + , numericFraction(numericFraction) > + , numericOrdinal(numericOrdinal) > + , numericSlashedZero(numericSlashedZero) > + , alternates(alternates) > + , eastAsianVariant(eastAsianVariant) > + , eastAsianWidth(eastAsianWidth) > + , eastAsianRuby(eastAsianRuby) > + { > + } Seems like we should not write this constructor. A struct can and should instead be initialized with aggregate initialization syntax. > Source/WebCore/platform/text/TextFlags.h:203 > + bool normal() const > + { > + return commonLigatures == FontVariantLigatures::Normal > + && discretionaryLigatures == FontVariantLigatures::Normal > + && historicalLigatures == FontVariantLigatures::Normal > + && contextualAlternates == FontVariantLigatures::Normal > + && position == FontVariantPosition::Normal > + && caps == FontVariantCaps::Normal > + && numericFigure == FontVariantNumericFigure::Normal > + && numericSpacing == FontVariantNumericSpacing::Normal > + && numericFraction == FontVariantNumericFraction::Normal > + && numericOrdinal == FontVariantNumericOrdinal::Normal > + && numericSlashedZero == FontVariantNumericSlashedZero::Normal > + && alternates == FontVariantAlternates::Normal > + && eastAsianVariant == FontVariantEastAsianVariant::Normal > + && eastAsianWidth == FontVariantEastAsianWidth::Normal > + && eastAsianRuby == FontVariantEastAsianRuby::Normal; > + } I think there must be a better name. It’s not at all clear to me that "normal" means "contains no non-normal setting". Maybe isAllNormal or perhaps you can come up with an even better term.

Comment on attachment 261706 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=261706&action=review >> Source/WebCore/platform/graphics/FontCascade.cpp:603 >> + // FIXME: This shouldn't be necessary because Font::applyTransforms() should perform all necessary shaping. > > A clearer comment would be: > > // FIXME: This shouldn't be necessary once we have enhanced Font::applyTransforms() to perform all necessary shaping. > > Unless that’s inaccurate in which case I am confused! It should already perform all necessary shaping, but for some reason, it doesn't. I have to figure out why. According to Ned, CTFontTransformGlyphs() runs a full shaping pass, not just K&L. >> Source/WebCore/platform/graphics/FontDescription.cpp:-37 >> -struct SameSizeAsFontCascadeDescription { > > Why did you delete this? We added it for a reason. Did that reason go away? It doesn't seem valuable to me; whenever we modify the contents of FontDescription, we update this thing to be the same size. If our goal is "be cognizant that FontDescription shouldn't get too big" I think we can more directly achieve that goal in other ways without keeping this thing up to date. >> Source/WebCore/platform/graphics/FontFeatureSettings.h:51 >> + int m_value { 0 }; > > Why this change? IIRC you once instructed me to remove "const" from a class member variable. Am I misremembering? >> Source/WebCore/platform/graphics/FontFeatureSettings.h:66 >> + Vector<FontFeature>::const_iterator end() const { return m_list.end(); } > > This seems a bit peculiar. At this point, why is it helpful to have this be a class instead of just using a typedef for a Vector? The Vector typedef idea definitely has potential. I'll investigate doing this in a follow up patch. I didn't do that originally because this class existed when I started my work.

(In reply to comment #37) > Comment on attachment 261706 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=261706&action=review > > There is a massive amount of boilerplate code here. Anything we can do to > refactor into helper functions to make this tighter would be worthwhile. > Specific suggestions below. > > Testing coverage seems a bit light given the amount of code. Do we have > tests that will fail if we make any change to all that code? > I believe my test is actually comprehensive (even though it's short).

Comment on attachment 261706 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=261706&action=review >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1309 >> +static Ref<CSSValue> getFontVariantNumericPropertyValue(FontVariantNumericFigure figure, FontVariantNumericSpacing spacing, FontVariantNumericFraction fraction, FontVariantNumericOrdinal ordinal, FontVariantNumericSlashedZero slashedZero) > > Ditto. This doesn't actually work here because if one of the fields is normal, we shouldn't append normal. >> Source/WebCore/platform/graphics/FontCache.h:106 >> + hasher.add(m_locale.isNull() ? 0 : m_locale.impl()->existingHash()); > > We should add an existingHash function to String to avoid having to write it like this. I'm a little unsure about this (0 is used as a sentinel, so I'd want to pick another value), so I'll do it in a follow-up patch. >>> Source/WebCore/platform/graphics/FontFeatureSettings.h:51 >>> + int m_value { 0 }; >> >> Why this change? > > IIRC you once instructed me to remove "const" from a class member variable. Am I misremembering? FontDescriptions need to be assignable.

Created attachment 261796 [details] Patch for committing

(In reply to comment #39) > (In reply to comment #37) > > Comment on attachment 261706 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=261706&action=review > > > > There is a massive amount of boilerplate code here. Anything we can do to > > refactor into helper functions to make this tighter would be worthwhile. > > Specific suggestions below. > > > > Testing coverage seems a bit light given the amount of code. Do we have > > tests that will fail if we make any change to all that code? > > > > I believe my test is actually comprehensive (even though it's short). I added another test specifically for parsing.

Comment on attachment 261796 [details] Patch for committing View in context: https://bugs.webkit.org/attachment.cgi?id=261796&action=review > Source/WebCore/platform/graphics/FontDescription.cpp:-46 > -struct SameSizeAsFontCascadeDescription { > - void* pointers[3]; > - float sizes[2]; > - // FIXME: Make them fit into one word. > - uint32_t bitfields; > - uint32_t bitfields2 : 8; > -}; > - > -COMPILE_ASSERT(sizeof(FontCascadeDescription) == sizeof(SameSizeAsFontCascadeDescription), FontCascadeDescription_should_stay_small); > - It is not ok to remove this. It is exists so people don't make FontDescription big accidentally.

(In reply to comment #43) > Comment on attachment 261796 [details] > Patch for committing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=261796&action=review > > > Source/WebCore/platform/graphics/FontDescription.cpp:-46 > > -struct SameSizeAsFontCascadeDescription { > > - void* pointers[3]; > > - float sizes[2]; > > - // FIXME: Make them fit into one word. > > - uint32_t bitfields; > > - uint32_t bitfields2 : 8; > > -}; > > - > > -COMPILE_ASSERT(sizeof(FontCascadeDescription) == sizeof(SameSizeAsFontCascadeDescription), FontCascadeDescription_should_stay_small); > > - > > It is not ok to remove this. It is exists so people don't make > FontDescription big accidentally. Did you see my previous comment about this?

(In reply to comment #44) > (In reply to comment #43) > > Comment on attachment 261796 [details] > > Patch for committing > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=261796&action=review > > > > > Source/WebCore/platform/graphics/FontDescription.cpp:-46 > > > -struct SameSizeAsFontCascadeDescription { > > > - void* pointers[3]; > > > - float sizes[2]; > > > - // FIXME: Make them fit into one word. > > > - uint32_t bitfields; > > > - uint32_t bitfields2 : 8; > > > -}; > > > - > > > -COMPILE_ASSERT(sizeof(FontCascadeDescription) == sizeof(SameSizeAsFontCascadeDescription), FontCascadeDescription_should_stay_small); > > > - > > > > It is not ok to remove this. It is exists so people don't make > > FontDescription big accidentally. > > Did you see my previous comment about this? Due to popular demand, I'm adding it back in.

Created attachment 261834 [details] Patch for committing

Created attachment 261847 [details] Patch for committing

Committed r190192: <http://trac.webkit.org/changeset/190192>

The new test fails on Windows: https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/r190194%20(54214)/css3/font-variant-all-webfont-diffs.html

(In reply to comment #49) > The new test fails on Windows: > https://build.webkit.org/results/Apple%20Win%207%20Release%20(Tests)/ > r190194%20(54214)/css3/font-variant-all-webfont-diffs.html It should be skipped on Windows.