[webkit-reviews] review granted: [Bug 109021] [css3-text] Parsing -webkit-each-line value for text-indent from css3-text : [Attachment 193754] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Mar 19 16:01:43 PDT 2013
Julien Chaffraix <jchaffraix at webkit.org> has granted 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 193754: Patch
https://bugs.webkit.org/attachment.cgi?id=193754&action=review
------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=193754&action=review
r=me but the requested changes should be done prior to landing.
> Source/WebCore/ChangeLog:45
> + (StyleRareInheritedData):
You should file these entries now that the change is good.
> Source/WebCore/css/StyleBuilder.cpp:1960
> + // The value order is guaranteed. See CSSParser::parseTextIndent.
Nit: 'value' doesn't add much in the previous sentence and could be removed.
> Source/WebCore/css/StyleBuilder.cpp:1978
> + if (valueList->length() == 1) {
> + styleResolver->style()->setTextIndentLine(TextIndentFirstLine);
> + return;
> + }
> +
> + ASSERT(valueList->length() == 2);
> + CSSPrimitiveValue* eachLineValue =
static_cast<CSSPrimitiveValue*>(valueList->itemWithoutBoundsCheck(1));
> + if (eachLineValue->getIdent() == CSSValueWebkitEachLine)
> + styleResolver->style()->setTextIndentLine(TextIndentEachLine);
This could be written (more readable IMO):
ASSERT(valueList->length() <= 2);
CSSPrimitiveValue* eachLineValue =
static_cast<CSSPrimitiveValue*>(valueList->item(1);
if (eachLineValue) {
ASSERT(eachLineValue->getIndent() == CSSValueWebkitEachLine);
styleResolver->style()->setTextIndentLine(TextIndentEachLine);
} else
styleResolver->style()->setTextIndentLine(TextIndentFirstLine);
>
LayoutTests/fast/css3-text/css3-text-indent/getComputedStyle/script-tests/getCo
mputedStyle-text-indent-inherited.js:8
> +function ownValueTest(a_value, c_value)
a_value / c_value are not very readable (I had a hard time understand why a / c
and not a / b).
How about: ancestorValue / childValue (also this should be camelCased to match
WebKit's coding style).
>
LayoutTests/fast/css3-text/css3-text-indent/getComputedStyle/script-tests/getCo
mputedStyle-text-indent.js:49
> +debug('');
This is missing some calc() values that you broke yesterday and asked about
adding some coverage. An example would be calc(20px + 30px) but feel free to be
creative.
> LayoutTests/platform/chromium/TestExpectations:236
> +webkit.org/b/109021 fast/css3-text/css3-text-indent
This entry is not totally correct:
* The default action is SKIPPED which won't catch crashers, it should be FAIL
* This bug will be closed the moment this patch land. The reference bug here
should be something that will be closed when the feature is considered
complete.
You should have a meta bug that covers adding all the pieces needed for
text-indent or what you are targeting and make all the patches block the meta
bug. See for example, bug 60731 (CSS Grid Layout meta bug - shameless plug btw)
for how it works.
More information about the webkit-reviews
mailing list