[webkit-reviews] review granted: [Bug 102491] [css3-text] Add parsing support for text-underline-position : [Attachment 191775] Parse text-underline-position

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Lamarque V. Souza
<Lamarque.Souza at basyskom.com>'s request for review:
Bug 102491: [css3-text] Add parsing support for text-underline-position
https://bugs.webkit.org/show_bug.cgi?id=102491

Attachment 191775: Parse text-underline-position
https://bugs.webkit.org/attachment.cgi?id=191775&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=191775&action=review


> Source/WebCore/ChangeLog:3
> +	   [css3-text] Add parsing support text-underline-position property
from CSS3 Text

This should be renamed as you don't totally match the specification in this
change.

> Source/WebCore/ChangeLog:12
> +	   OBS: values 'under left' and 'under right' are not implemented in
this

OBS? If you meant observation, either state it or use 'note'.

Also, you should just mention what you did here instead of just observing it:

This patch extends the existing parsing to support 'alphabetic' and 'under'. We
don't fully match the specification as we don't support [ left | right ] and
this is left for another implementation as the rendering will need to be added.


> Source/WebCore/ChangeLog:42
> +	   * rendering/style/RenderStyleConstants.h:
> +	   (WebCore::operator| ):
> +	   (WebCore::operator|=):
> +	   Added bitwise TextUnderlinePosition enum with operator functions |
and |= operator (used in StyleBuilder).

The ChangeLog is stale. It should be updated before landing.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1362
> +static PassRefPtr<CSSValue>
renderTextUnderlinePositionFlagsToCSSValue(TextUnderlinePosition
textUnderlinePosition)
> +{
> +    if (textUnderlinePosition == TextUnderlinePositionAuto)
> +	   return cssValuePool().createIdentifierValue(CSSValueAuto);
> +
> +    if (textUnderlinePosition == TextUnderlinePositionAlphabetic)
> +	   return cssValuePool().createIdentifierValue(CSSValueAlphabetic);
> +
> +    if (textUnderlinePosition == TextUnderlinePositionUnder)
> +	   return cssValuePool().createIdentifierValue(CSSValueUnder);
> +
> +    // TODO: Implement support for 'under left' and 'under right' values.
> +
> +    ASSERT_NOT_REACHED();
> +    return cssValuePool().createExplicitInitialValue();
> +}

Nit: This could be replaced with defining
CSSPrimitiveValue::CSSPrimitiveValue(TextUnderlinePosition) in
CSSPrimitiveValueMapping.h AFAICT.

> Source/WebCore/css/CSSParser.cpp:9130
> +	   break;

No need for the break.

> Source/WebCore/css/CSSPrimitiveValueMappings.h:2372
> +    // TODO: Implement support for 'under left' and 'under right' values.

We don't have TODO's in WebKit, only FIXME's.

> Source/WebCore/css/CSSValueKeywords.in:949
> +#if !(defined(ENABLE_SVG) && ENABLE_SVG) && defined(ENABLE_CSS3_TEXT) &&
ENABLE_CSS3_TEXT

This will basically work but you still have 2 definitions of the same property
which means that they can get out of sync.

I would rather go with a single definition than 2: either by defining the
property here only using (my preference)

#if (defined(ENABLE_SVG) && ENABLE_SVG) || (defined(ENABLE_CSS3_TEXT) &&
ENABLE_CSS3_TEXT)

and removing the SVG bit or just enabling it always (not great but better than
having to maintain 2 definitions).

> Source/WebCore/rendering/style/RenderStyleConstants.h:363
> +    // TODO: Implement support for 'under left' and 'under right' values.

s/TODO/FIXME.