Bug 123358

Created attachment 215210 [details] Patch

<rdar://problem/15323293>

Created attachment 215220 [details] Patch

Comment on attachment 215220 [details] Patch Attachment 215220 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/12698041

Created attachment 215228 [details] Patch

Comment on attachment 215228 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=215228&action=review > Source/JavaScriptCore/ChangeLog:4 > + -webkit-text-decoration-skip: ink; isn't parsed in CSS > + https://bugs.webkit.org/show_bug.cgi?id=123358 I think this title should be what you're doing, not what is broken before this patch. > Source/WebCore/ChangeLog:9 > + -webkit-text-decoration-skip: ink; isn't parsed in CSS > + https://bugs.webkit.org/show_bug.cgi?id=123358 > + > + Reviewed by NOBODY (OOPS!). > + > + Adds initial parsing support for parsing -webkit-text-decoration-skip with > + values of "none" and "skip". This work is behind the new I'm confused. The title says "ink" and here you're saying "none" and "skip". > Source/WebCore/css/DeprecatedStyleBuilder.cpp:1272 > + static TextDecorationSkip valueToDecorationSkip(CSSPrimitiveValue& primitiveValue) Please move this below public: into a new private: section, or just make it a standalone file static function. > Source/WebCore/css/DeprecatedStyleBuilder.cpp:1298 > + for (CSSValueListIterator i(value); i.hasMore(); i.advance()) { > + CSSValue* item = i.value(); I believe both of these cases could use "auto". Not worth changing though. > Source/WebCore/rendering/style/RenderStyleConstants.h:386 > + TextDecorationSkipNone = 0x0, TextDecorationSkipInk = 1 << 0 Separate lines for these. And I think we tend to just use 0 rather than 0x0. > LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-skip/text-decoration-skip-roundtrip.html:60 > + stylesheet.insertRule(".p { -webkit-text-decoration-skip: none; }", 0); > + testNoneIsValid(stylesheet); One more test: stylesheet.insertRule(".p { -webkit-text-decoration-skip: ; }", 0);

Comment on attachment 215228 [details] Patch 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?

Comment on attachment 215228 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=215228&action=review >> Source/JavaScriptCore/ChangeLog:4 >> + https://bugs.webkit.org/show_bug.cgi?id=123358 > > I think this title should be what you're doing, not what is broken before this patch. Done. >> Source/WebCore/ChangeLog:9 >> + values of "none" and "skip". This work is behind the new > > I'm confused. The title says "ink" and here you're saying "none" and "skip". Typo. Done. >> Source/WebCore/ChangeLog:36 >> + * rendering/style/StyleRareInheritedData.h: > > Could always use more words here. Done. >> Source/WebCore/css/DeprecatedStyleBuilder.cpp:1272 >> + static TextDecorationSkip valueToDecorationSkip(CSSPrimitiveValue& primitiveValue) > > Please move this below public: into a new private: section, or just make it a standalone file static function. Done. >>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:1298 >>> + CSSValue* item = i.value(); >> >> I believe both of these cases could use "auto". Not worth changing though. > > no real need for this local Is there a reason to use auto instead of the explicit type? I'd prefer to use the actual type because the type is short and straightforward (and easily known) Done. >>> Source/WebCore/rendering/style/RenderStyleConstants.h:386 >>> + TextDecorationSkipNone = 0x0, TextDecorationSkipInk = 1 << 0 >> >> Separate lines for these. And I think we tend to just use 0 rather than 0x0. > > 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? Done. >> LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-skip/text-decoration-skip-roundtrip.html:60 >> + testNoneIsValid(stylesheet); > > One more test: stylesheet.insertRule(".p { -webkit-text-decoration-skip: ; }", 0); Done.

Created attachment 215327 [details] Patch

Comment on attachment 215327 [details] Patch Clearing flags on attachment: 215327 Committed r158127: <http://trac.webkit.org/changeset/158127>

All reviewed patches have been landed. Closing bug.

*** Bug 92801 has been marked as a duplicate of this bug. ***