[Webkit-unassigned] [Bug 112615] [css3-text] Implement support for vertical writing mode in -webkit-text-underline-position

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=112615





--- Comment #15 from Lamarque V. Souza <Lamarque.Souza at basyskom.com>  2013-05-28 18:05:09 PST ---
(In reply to comment #14)
> (In reply to comment #12)
> > View in context: https://bugs.webkit.org/attachment.cgi?id=202967&action=review
> > 
> > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1389
> > > +    return list;
> > 
> > You should add an assert here, list must never be empty.
> 
> I don't think so.
> I think your draft patch needs assert because it is not clear whether list is empty or not.
> However, my implementation is copied from renderTextDecorationFlagsToCSSValue().
> Since "if" at 2nd line above the return line assures that list must not be empty, and there is no assert in renderTextDecorationFlagsToCSSValue().

"none" is a valid value for TextDecoration (http://dev.w3.org/csswg/css-text-decor-3/#text-decoration-property), that is why defaulting to CSSValueNone when the list is empty is Ok there and no assert is needed there. However, "none" is not a valid value for text-underline-position, so the list must not be empty, an assert is needed and we should not default to CSSValueNone.

> I think the following assert is too duplicate.
> > if (condition)
> >     return foo;
> > ASSERT(!condition);
> > return bar;

asserts are used to detect bugs not to make code more readable. Besides, they are not compiled if debugging is disabled, so they do not cause any runtime overhead in release mode. In our case the code above should be something like:

if (condition)
    list->append(...);
ASSERT(!list->isEmpty())
return list;

See? No return inside the if statement.

> > Besides, I do not see a need to replace CSSPrimitiveValue::CSSPrimitiveValue(TextUnderlinePosition e) with this function. This just makes your patch bigger. I see that you used one of my draft patch for text-underline-position to implement this function but since WebKit already uses CSSPrimitiveValue::CSSPrimitiveValue(TextUnderlinePosition e) you should implement your patch using it unless there is a really good reason for not to.
> 
> Hmm. How to use CSSPrimitiveValue::CSSPrimitiveValue(TextUnderlinePosition e)?

Ok, I tested it here and trying to use CSSPrimitiveValue::CSSPrimitiveValue() for a list is a big pain. I am ok with using renderTextUnderlinePositionFlagsToCSSValue() instead of CSSPrimitiveValue::CSSPrimitiveValue().

> > > Source/WebCore/rendering/InlineTextBox.cpp:1251
> > > +                FloatPoint start(localOrigin.x(), localOrigin.y() + underlineOffset + (isTopSide ? -doubleOffset : doubleOffset));
> > 
> > We should add tests similar to LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-style.html to check for this change. Maybe a text-decoration-style-underline-left.html and text-decoration-style-underline-right.html
> 
> text-underline-position-double.html will cover this change.
> I changed this to reftest.

Ok, that's better than using pixel test :-)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.