[webkit-reviews] review denied: [Bug 109021] [css3-text] Parsing -webkit-each-line value for text-indent from css3-text : [Attachment 193071] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Jaehun Lim
<ljaehun.lim at samsung.com>'s request for review:
Bug 109021: [css3-text] Parsing -webkit-each-line value for text-indent from
css3-text
https://bugs.webkit.org/show_bug.cgi?id=109021

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

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


r- for regressing calc() and some polish in StyleBuilder but the parsing code
looks good.

> Source/WebCore/ChangeLog:17
> +	   It's prefixed with '-webkit-' and guarded by CSS3_TEXT flag.

As said, prefixed with '-webkit-' is just called prefixed (we don't use other
type of prefixing).

> Source/WebCore/ChangeLog:28
> +	   (WebCore):

These are meaningless entries that should be removed (unfortunately our tools
generate them for no good reason).

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2201
> +	   }

I think we would rather keep the existing behavior if you only specified only
the <length> | <percentage>. This contradicts what I said about sharing code
across call sites, but this is exposed to web-sites so it would be better to
keep backwards-compatibility, especially since it's easy.

> Source/WebCore/css/CSSParser.cpp:9246
> +    // <length> | <percentage>

No inherit here?

> Source/WebCore/css/CSSParser.cpp:9257
> +    // [ <length> | <percentage> ] && -webkit-each-line | inherit

You should probably mention that the case where -webkit-each-line is not
provided is handled above.

> Source/WebCore/css/CSSParser.cpp:9263
> +    CSSParserValue* value = 0;

I would renamed that to lengthOrPercentageValue to make it explicit that this
stores <length> | <percentage>.

> Source/WebCore/css/StyleBuilder.cpp:1835
> +

This is regressing the calc() case: <length> allows calc() per CSS 3 value. The
old code would allow it but not the new one. Btw, this now has to be tested.

> Source/WebCore/css/StyleBuilder.cpp:1836
> +	   return Length();

FWIW, you control the code coming here so you should have some kind of
ASSERT_NOT_REACHED() here.

> Source/WebCore/css/StyleBuilder.cpp:1837
> +    }

StyleBuilder is crazy to allow such rampant duplication but let's not add more.
See below about a better way than re-inventing the wheel :-/

> Source/WebCore/css/StyleBuilder.cpp:1861
> +	   Length textIndent = RenderStyle::initialTextIndent();

That's a bad idea (TM): RenderStyle should be already filled with
initialTextIdent and you *know* you must have a text indent. On top of that, we
prefer to define variable when they are needed, not in some random places (here
it gives you nothing to define |textIndent| here apart from making the intent
less understandable).

> Source/WebCore/css/StyleBuilder.cpp:1872
> +#if ENABLE(CSS3_TEXT)
> +	       styleResolver->style()->setTextIndentLine(TextIndentFirstLine);
> +#endif

I don't think you need this code, it should default to the "initial" value in
this case.

> Source/WebCore/css/StyleBuilder.cpp:1879
> +	   if (valueList->length() != 2)
> +	       return;

Defense in depth is fine but I wouldn't expect this code to be hit ever so this
doesn't buy us much. If someone refactored the code in a bad way, this ASSERT
would hit it.

> Source/WebCore/css/StyleBuilder.cpp:1888
> +	   if (firstValue->isLength() || firstValue->isPercentage() ||
firstValue->isViewportPercentageLength())
> +	       textIndent = computeLength(styleResolver, firstValue);
> +	   else if (secondValue->isLength() || secondValue->isPercentage() ||
secondValue->isViewportPercentageLength())
> +	       textIndent = computeLength(styleResolver, secondValue);

This is stale. Now you are guaranteed to have firstValue as the Length so the
code should be just be:

textIndent = primitiveValue->convertToLength<FixedIntegerConversion |
PercentConversion | ViewportPercentageConversion>(...).
ASSERT(!textIdent.isUndefined());

> LayoutTests/ChangeLog:34
> +	   * transitions/svg-transitions-expected.txt: Modify line number.

I don't understand this change. How is your changing text-indent syntax
affecting this?

'Why' explanations are always better than 'what' and you should concentrate
your ChangeLog to explain as much why you do is correct.