Bug 123358 – Parsing support for -webkit-text-decoration-skip: ink

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

-webkit-text-decoration-skip: ink; isn't parsed in CSS

[Myles C. Maxfield]

Created attachment 215210 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/15323293>

[Myles C. Maxfield]

Created attachment 215220 [details]
Patch

[kov's GTK+ EWS bot]

Comment on attachment 215220 [details]
Patch

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

[Myles C. Maxfield]

Created attachment 215228 [details]
Patch

[Dean Jackson]

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);

[Tim Horton]

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?

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

Created attachment 215327 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 215327 [details]
Patch

Clearing flags on attachment: 215327

Committed r158127: <http://trac.webkit.org/changeset/158127>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Bruno Abinader]

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