From 1ced62033ffe82134c2f5707b6ef197fa3e85375 Mon Sep 17 00:00:00 2001 From: alexis Date: Thu, 10 Jan 2013 13:46:39 +0000 Subject: ASSERT_NOT_REACHED in StylePropertySet::fontValue when accessing font style property through JS after setting style font size. https://bugs.webkit.org/show_bug.cgi?id=88866 Reviewed by Alexander Pavlov. Source/WebCore: StylePropertySet::fontValue always assumed that it was called using style.font after a subsequent call which set the shorthand font. The ASSERT_NOT_REACHED assumed that all longhands of the font shorthand not set by the shorthand itself were set to initial. While it's true when we set the font shorthand (i.e all longhands are set to implicit initial) it is not true when you set the longhands individually. For example setting font-size will not set other font properties to initial. It is the behavior of all other shorthands in WebKit. When reconstructing the shorthand other properties tests whether the value of each longhands is initial or not (if not then we omit the value, as we should always construct the shortest shorthand possible) or if the value is set or not (if set then we include it in the shorthand if not then we omit it). The comment removed was also talking about invalid font property potentially built by fontValue(). So far appendFontLonghandValueIfExplicit will always construct a valid value as it takes care of adding ' ' or '/' when needed, so the return value is parsable and correct. Test: fast/css/font-shorthand-from-longhands.html * css/StylePropertySet.cpp: (WebCore::StylePropertySet::appendFontLonghandValueIfExplicit): (WebCore::StylePropertySet::fontValue): * css/StylePropertySet.h: git-svn-id: http://svn.webkit.org/repository/webkit/trunk@139313 268f45cc-cd09-0410-ab3c-d52691b4dbfc Change-Id: I46a4b1d023f3cb8f4e7b4c0b7f176b5b9f850870 Reviewed-by: Simon Hausmann --- Source/WebCore/ChangeLog | 31 +++++++++++++++++++++++++++ Source/WebCore/css/StylePropertySet.cpp | 37 ++++++++++++++------------------- Source/WebCore/css/StylePropertySet.h | 2 +- 3 files changed, 48 insertions(+), 22 deletions(-) (limited to 'Source') diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog index 7b3d70b5a..3f75558f6 100644 --- a/Source/WebCore/ChangeLog +++ b/Source/WebCore/ChangeLog @@ -1,3 +1,34 @@ +2013-01-10 Alexis Menard + + ASSERT_NOT_REACHED in StylePropertySet::fontValue when accessing font style property through JS after setting style font size. + https://bugs.webkit.org/show_bug.cgi?id=88866 + + Reviewed by Alexander Pavlov. + + StylePropertySet::fontValue always assumed that it was called using + style.font after a subsequent call which set the shorthand font. The + ASSERT_NOT_REACHED assumed that all longhands of the font shorthand not + set by the shorthand itself were set to initial. While it's true when + we set the font shorthand (i.e all longhands are set to implicit initial) + it is not true when you set the longhands individually. For example setting + font-size will not set other font properties to initial. It is the behavior of all + other shorthands in WebKit. When reconstructing the shorthand other + properties tests whether the value of each longhands is initial or not + (if not then we omit the value, as we should always construct the + shortest shorthand possible) or if the value is set or not (if set then + we include it in the shorthand if not then we omit it). The comment + removed was also talking about invalid font property potentially built + by fontValue(). So far appendFontLonghandValueIfExplicit will always + construct a valid value as it takes care of adding ' ' or '/' when + needed, so the return value is parsable and correct. + + Test: fast/css/font-shorthand-from-longhands.html + + * css/StylePropertySet.cpp: + (WebCore::StylePropertySet::appendFontLonghandValueIfExplicit): + (WebCore::StylePropertySet::fontValue): + * css/StylePropertySet.h: + 2012-12-19 Simon Hausmann , Jedrzej Nowacki [Qt] JS bridge does not transmit QVariants anymore in Qt5 diff --git a/Source/WebCore/css/StylePropertySet.cpp b/Source/WebCore/css/StylePropertySet.cpp index f662ccacf..ccd7d8a62 100644 --- a/Source/WebCore/css/StylePropertySet.cpp +++ b/Source/WebCore/css/StylePropertySet.cpp @@ -225,15 +225,15 @@ String StylePropertySet::borderSpacingValue(const StylePropertyShorthand& shorth return horizontalValueCSSText + ' ' + verticalValueCSSText; } -bool StylePropertySet::appendFontLonghandValueIfExplicit(CSSPropertyID propertyID, StringBuilder& result, String& commonValue) const +void StylePropertySet::appendFontLonghandValueIfExplicit(CSSPropertyID propertyID, StringBuilder& result, String& commonValue) const { int foundPropertyIndex = findPropertyIndex(propertyID); if (foundPropertyIndex == -1) - return false; // All longhands must have at least implicit values if "font" is specified. + return; // All longhands must have at least implicit values if "font" is specified. if (propertyAt(foundPropertyIndex).isImplicit()) { commonValue = String(); - return true; + return; } char prefix = '\0'; @@ -258,37 +258,32 @@ bool StylePropertySet::appendFontLonghandValueIfExplicit(CSSPropertyID propertyI result.append(value); if (!commonValue.isNull() && commonValue != value) commonValue = String(); - - return true; } String StylePropertySet::fontValue() const { - int foundPropertyIndex = findPropertyIndex(CSSPropertyFontSize); - if (foundPropertyIndex == -1) + int fontSizePropertyIndex = findPropertyIndex(CSSPropertyFontSize); + int fontFamilyPropertyIndex = findPropertyIndex(CSSPropertyFontFamily); + if (fontSizePropertyIndex == -1 || fontFamilyPropertyIndex == -1) return emptyString(); - PropertyReference fontSizeProperty = propertyAt(foundPropertyIndex); - if (fontSizeProperty.isImplicit()) + PropertyReference fontSizeProperty = propertyAt(fontSizePropertyIndex); + PropertyReference fontFamilyProperty = propertyAt(fontFamilyPropertyIndex); + if (fontSizeProperty.isImplicit() || fontFamilyProperty.isImplicit()) return emptyString(); String commonValue = fontSizeProperty.value()->cssText(); StringBuilder result; - bool success = true; - success &= appendFontLonghandValueIfExplicit(CSSPropertyFontStyle, result, commonValue); - success &= appendFontLonghandValueIfExplicit(CSSPropertyFontVariant, result, commonValue); - success &= appendFontLonghandValueIfExplicit(CSSPropertyFontWeight, result, commonValue); + appendFontLonghandValueIfExplicit(CSSPropertyFontStyle, result, commonValue); + appendFontLonghandValueIfExplicit(CSSPropertyFontVariant, result, commonValue); + appendFontLonghandValueIfExplicit(CSSPropertyFontWeight, result, commonValue); if (!result.isEmpty()) result.append(' '); result.append(fontSizeProperty.value()->cssText()); - success &= appendFontLonghandValueIfExplicit(CSSPropertyLineHeight, result, commonValue); - success &= appendFontLonghandValueIfExplicit(CSSPropertyFontFamily, result, commonValue); - if (!success) { - // An invalid "font" value has been built (should never happen, as at least implicit values - // for mandatory longhands are always found in the style), report empty value instead. - ASSERT_NOT_REACHED(); - return emptyString(); - } + appendFontLonghandValueIfExplicit(CSSPropertyLineHeight, result, commonValue); + if (!result.isEmpty()) + result.append(' '); + result.append(fontFamilyProperty.value()->cssText()); if (isInitialOrInherit(commonValue)) return commonValue; return result.toString(); diff --git a/Source/WebCore/css/StylePropertySet.h b/Source/WebCore/css/StylePropertySet.h index 456f7ef2e..40eab1d13 100644 --- a/Source/WebCore/css/StylePropertySet.h +++ b/Source/WebCore/css/StylePropertySet.h @@ -197,7 +197,7 @@ private: String get4Values(const StylePropertyShorthand&) const; String borderSpacingValue(const StylePropertyShorthand&) const; String fontValue() const; - bool appendFontLonghandValueIfExplicit(CSSPropertyID, StringBuilder& result, String& value) const; + void appendFontLonghandValueIfExplicit(CSSPropertyID, StringBuilder& result, String& value) const; bool removeShorthandProperty(CSSPropertyID); bool propertyMatches(const PropertyReference&) const; -- cgit v1.2.1