[Webkit-unassigned] [Bug 109021] [css3-text] Parsing -webkit-each-line value for text-indent from css3-text
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Mar 19 21:58:41 PDT 2013
https://bugs.webkit.org/show_bug.cgi?id=109021
--- Comment #34 from Jaehun Lim <ljaehun.lim at samsung.com> 2013-03-19 22:01:07 PST ---
(From update of attachment 193754)
View in context: https://bugs.webkit.org/attachment.cgi?id=193754&action=review
Thanks for r+. I fixed as your comments.
>> Source/WebCore/ChangeLog:45
>> + (StyleRareInheritedData):
>
> You should file these entries now that the change is good.
Added comments.
>> 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.
Removed 'value'.
>> Source/WebCore/css/StyleBuilder.cpp:1978
>> + 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);
Fixed. It looks more readable. Thanks.
>> LayoutTests/fast/css3-text/css3-text-indent/getComputedStyle/script-tests/getComputedStyle-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).
Changed to ancestorValue and childValue.
>> LayoutTests/fast/css3-text/css3-text-indent/getComputedStyle/script-tests/getComputedStyle-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.
Added calc(10px + 20px) cases.
>> 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.
* Add [ Failure ].
* I filed https://bugs.webkit.org/show_bug.cgi?id=112755 as meta bug for text-indent.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list