[webkit-reviews] review granted: [Bug 92000] [css3-text] Implement CSS3 text-decoration shorthand : [Attachment 209114] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Bruno de Oliveira Abinader
<bruno.d at partner.samsung.com>'s request for review:
Bug 92000: [css3-text] Implement CSS3 text-decoration shorthand
https://bugs.webkit.org/show_bug.cgi?id=92000

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=209114&action=review


> Source/WebCore/css/StylePropertyShorthand.h:116
>  const StylePropertyShorthand& webkitMaskRepeatShorthand();
> +#if ENABLE(CSS3_TEXT)
> +const StylePropertyShorthand& webkitTextDecorationShorthand();
> +#endif // CSS3_TEXT
>  const StylePropertyShorthand& webkitTextEmphasisShorthand();

Awkward to have a conditional in the middle of a sorted list. Makes it hard to
re-sort, for example.

Instead would be better to put this in a separate sorted list/paragraph. The
same way we do with includes.

Also, the #endif comment really rubs me the wrong way when the #if is less
than, say, 10 lines long. I understand the value of the comment when it might
be scrolled off screen, but it seems like noise to me in these short ifs. Would
be nice to remove some of these and maybe even make this official in our style
guide. Unless others don’t agree with me on this.