[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 #14 from Yuki Sekiguchi <yuki.sekiguchi at access-company.com>  2013-05-28 01:40:31 PST ---
(In reply to comment #11)
> I agree, just that you added them to the wrong file. You should have used getComputedStyle-text-underline-position.html for checking the parser of this patch. The other unit tests are for the rendering part. Keep in mind that getComputedStyle-text-underline-position.html is text only test and the others are pixel tests. You do not need pixel tests to test the parser.

OK. I removed rendering part of invalid value test.

> > > Attachment 202967 [details] [details] [details] did not pass style-queue:
> > > 
> > > Source/WebCore/rendering/InlineTextBox.cpp:984:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
> > > Total errors found: 1 in 22 files
> > > 
> > > 
> > > If any of these errors are false positives, please file a bug against check-webkit-style.
> > 
> > The outer "if" dose not conclude with a return.
> > This may be bug of style checker.
> 
> What the error message want to say is that you should do:
> 
> if (...)
>     return <something>;
> if (...)
>     return <something>;
> 
> instead of
> 
> if (...)
>     return <something>;
> else if (...)
>     return <something>;
> 
> You just need to remove the "else"'s in your source code. They are not necessary if there is a return inside the if statement.

I understand it. Thank you.
Fixed.

(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().
I think the following assert is too duplicate.
> if (condition)
>     return foo;
> ASSERT(!condition);
> return bar;

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

Should we renderTextUnderlinePositionFlagsToCSSValue() like the following?
> for (int flag = 1; flag <= TextUnderlinePositionRight; flag <<= 1) {
>     if (style->textUnderlinePosition() & flag)
>         list->append(cssValuePool().createValue(flag))
> }

or
At first, we change CSSPrimitiveValue(TextUnderlinePosition e) to read flag.
After that we change renderTextUnderlinePositionFlagsToCSSValue() like the following:
> TextUnderlinePosition position = style->textUnderlinePosition();
> while (position) {
>     RefPtr<CSSPrimitiveValue> newValue = cssValuePool().createValue(position);
>     TextUnderlinePosition usedPosition = newValue;
>     position &= ~usedPosition;
>     list->append(newValue.release());
> }

I think neither of them are clear code.
Reducing diff is good, but writing clear code is also good.

> > Source/WebCore/css/CSSPrimitiveValueMappings.h:2468
> >      // FIXME: Implement support for 'under left' and 'under right' values.
> 
> Remove obsolete comment.

Removed.

> > 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.

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