[webkit-reviews] review requested: [Bug 123358] -webkit-text-decoration-skip: ink; isn't parsed in CSS : [Attachment 215228] Patch

[bugzilla-daemon at webkit.org]

Tim Horton <thorton at apple.com> has asked  for review:
Bug 123358: -webkit-text-decoration-skip: ink; isn't parsed in CSS
https://bugs.webkit.org/show_bug.cgi?id=123358

Attachment 215228: Patch
https://bugs.webkit.org/attachment.cgi?id=215228&action=review

------- Additional Comments from Tim Horton <thorton at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=215228&action=review


> Source/WebCore/ChangeLog:36
> +	   * Configurations/FeatureDefines.xcconfig:
> +	   * css/CSSComputedStyleDeclaration.cpp:
> +	   (WebCore::renderTextDecorationSkipFlagsToCSSValue):
> +	   (WebCore::ComputedStyleExtractor::propertyValue):
> +	   * css/CSSParser.cpp:
> +	   (WebCore::CSSParser::parseValue):
> +	   (WebCore::CSSParser::parseTextDecorationSkip):
> +	   * css/CSSParser.h:
> +	   * css/CSSPropertyNames.in:
> +	   * css/CSSValueKeywords.in:
> +	   * css/DeprecatedStyleBuilder.cpp:
> +	   (WebCore::ApplyPropertyTextDecorationSkip::valueToDecorationSkip):
> +	   (WebCore::ApplyPropertyTextDecorationSkip::applyValue):
> +	   (WebCore::ApplyPropertyTextDecorationSkip::createHandler):
> +	   (WebCore::DeprecatedStyleBuilder::DeprecatedStyleBuilder):
> +	   * css/StyleResolver.cpp:
> +	   (WebCore::StyleResolver::applyProperty):
> +	   * rendering/style/RenderStyle.h:
> +	   * rendering/style/RenderStyleConstants.h:
> +	   * rendering/style/StyleRareInheritedData.cpp:
> +	   (WebCore::StyleRareInheritedData::StyleRareInheritedData):
> +	   (WebCore::StyleRareInheritedData::operator==):
> +	   * rendering/style/StyleRareInheritedData.h:

Could always use more words here.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:1298
> +	       CSSValue* item = i.value();

no real need for this local

> Source/WebCore/rendering/style/RenderStyleConstants.h:386
> +    TextDecorationSkipNone = 0x0, TextDecorationSkipInk = 1 << 0

Oddly, this file uses hex for masks, but I like the shift thing better. That
said, it feels odd to use both in one line. Maybe TextDecorationSkipNone = 0?