Bug 109021

Created attachment 187771 [details] Patch

Attachment 187771 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/css/text-indent-each-line-expected.html', u'LayoutTests/fast/css/text-indent-each-line.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSParser.h', u'Source/WebCore/css/CSSValueKeywords.in', u'Source/WebCore/css/StyleBuilder.cpp', u'Source/WebCore/rendering/RenderBlockLineLayout.cpp', u'Source/WebCore/rendering/style/RenderStyle.cpp', u'Source/WebCore/rendering/style/RenderStyle.h', u'Source/WebCore/rendering/style/RenderStyleConstants.h', u'Source/WebCore/rendering/style/StyleRareInheritedData.cpp', u'Source/WebCore/rendering/style/StyleRareInheritedData.h']" exit_code: 1 Source/WebCore/css/CSSParser.cpp:2102: One space before end of line comments [whitespace/comments] [5] Total errors found: 1 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 187774 [details] Patch

Comment on attachment 187774 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187774&action=review The change should be guarded by ENABLE(CSS3_TEXT). As mentioned on IRC, the testing is inadequate and I would rather see the style parsing, application and getComputedStyle part done first (with good testing) *before* attacking the rendering. Good testing mean that you do validate that we can set the style and get it back from getComputedStyle, including the wrong cases (that obviously shouldn't update our style) and 'inherit'. Obviously if this is already covered by existing tests, you should mention it in the ChangeLog and only add the missing pieces. > Source/WebCore/css/CSSParser.cpp:2103 > + // [ <length> | <percentage> ] && [ hanging || each-line ]? This line is wrong and it confused me. You are adding parsing for a subset of that and this should reflect what we *do* support. Also inherit is supported and should still be. > Source/WebCore/css/CSSParser.cpp:9019 > + bool haveTextIndent = false; > + bool haveEachLine = false; have (first it should be 'has') is a bit too generic here. Also I don't see why you seperate text-indent from each-line as each-line *is part of text-indent*. How about: hasSeenFixedOrPercentage, hasParsedFixedPercentage, hasEncounteredFixedOrPercentage (slight preference for hasParsedTextIndent)

Comment on attachment 187774 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187774&action=review >> Source/WebCore/css/CSSParser.cpp:2103 >> + // [ <length> | <percentage> ] && [ hanging || each-line ]? > > This line is wrong and it confused me. You are adding parsing for a subset of that and this should reflect what we *do* support. Also inherit is supported and should still be. I have a plan to support "hanging" value. I'll do it after "each-line". "hanging" is removed, "inherit" is added. [ <length> | <percentage> ] && each-line? | inherit is it right ? >> Source/WebCore/css/CSSParser.cpp:9019 >> + bool haveEachLine = false; > > have (first it should be 'has') is a bit too generic here. Also I don't see why you seperate text-indent from each-line as each-line *is part of text-indent*. How about: > > hasSeenFixedOrPercentage, hasParsedFixedPercentage, hasEncounteredFixedOrPercentage > > (slight preference for hasParsedTextIndent) I like hasParsedTextIndent.

Created attachment 189632 [details] Patch

Thanks, jchaffraix. I split previous patch into parsing and rendering parts as you said. This bug is for parsing. I use ENABLE_CSS3_TEXT guard. And add some testcases. I referred "text-align-last".

Comment on attachment 189632 [details] Patch Attachment 189632 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16689618 New failing tests: svg/custom/empty-className-baseVal-crash.html

Created attachment 189991 [details] Patch

Created attachment 191150 [details] Patch

Comment on attachment 191150 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191150&action=review This will need another round, preferably let's try to keep one code path, adding #if'defs around the new syntax. > Source/WebCore/css/CSSParser.cpp:2177 > +#if ENABLE(CSS3_TEXT) > + case CSSPropertyTextIndent: // [ <length> | <percentage> ] && each-line? | inherit > + parsedValue = parseTextIndent(); > + if (!parsedValue) > + return false; > + break; > +#else > case CSSPropertyTextIndent: // <length> | <percentage> | inherit > validPrimitive = (!id && validUnit(value, FLength | FPercent)); > break; > +#endif This is not a good way of handling the new value. Ideally we want the code path to be as shared as possible instead of splitting the code. Here it's a lot easier to do: case CSSPropertyTextIndent: parsedValue = parseTextIndent(); break; PassRefPtr<CSSValueList> CSSParser::parseTextIndent() { CSSParserValue* value = m_valueList->current(); if (!value->id || validUnit(value, FLength | FPercent)) return createPrimitiveNumericValue(value); #if ENABLE(CSS3_TEXT) // Handle the extended grammar, namely [ each-line && <length> | <percentage> ] #else return 0; #endif } > Source/WebCore/css/CSSParser.cpp:9162 > + for (CSSParserValue* value = m_valueList->current(); value; value = m_valueList->next()) { I don't think having a for loop for 2 values is really good. I would rather see 2 checks as: * they really depend on the condition in which you parse (did I see CSSValueEachLine? A <length> | <percentage>?) * you are allowing more than 2 values (see below) > Source/WebCore/css/CSSParser.cpp:9165 > + else if (!hasParsedTextIndent && !value->id && validUnit(value, FLength | FPercent)) { Do you know if the !value->id is required? It seems like an overkill to me. > Source/WebCore/css/CSSParser.cpp:9170 > + } Your parsing allows the following: * text-indent: each-line each-line; * text-indent: each-line 10px each-line; > Source/WebCore/css/CSSValueKeywords.in:1004 > +each-line You are not prefixing text-indent (which is fine as it is an existing property). However this is new thus it should be prefixed. > Source/WebCore/css/StyleBuilder.cpp:1819 > + for (size_t i = 0; i < valueList->length(); i++) { Again think about what you are parsing: you don't expect length() to be 15 so you should probably ASSERT it and have your code following the fact that we are probably get at most 2 values. > Source/WebCore/css/StyleBuilder.cpp:1834 > + else if (primitiveValue->isCalculatedPercentageWithLength()) > + textIndent = Length(primitiveValue->cssCalcValue()->toCalcValue(styleResolver->style(), styleResolver->rootElementStyle(), styleResolver->style()->effectiveZoom())); calc() is not allowed during parsing so I wouldn't expect it to show up here. > LayoutTests/fast/css3-text/css3-text-indent/getComputedStyle/getComputedStyle-text-indent-expected.txt:27 > + Missing some cases here or at least you should try different <length> values (how about viewport as you do have custom code for it?). Also how about <percentage>? > LayoutTests/fast/css3-text/css3-text-indent/getComputedStyle/getComputedStyle-text-indent-expected.txt:39 > +PASS computedStyle.getPropertyValue('text-indent') is '0px' Good test that you check invalid values but you should be a bit more extensive: how about 2 values? 3 values (including and not including each-line)? ... > LayoutTests/fast/css3-text/css3-text-indent/getComputedStyle/getComputedStyle-text-indent-expected.txt:40 > + How about inheritance which wasn't tested AFAICT? > LayoutTests/platform/wincairo/TestExpectations:2989 > +fast/css3-text/css3-text-indent/getComputedStyle/getComputedStyle-text-indent.html We should just skip the whole *directory* instead of touching every platforms everytime some new feature / test case is added.

Comment on attachment 191150 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191150&action=review >> Source/WebCore/css/CSSParser.cpp:2177 >> +#endif > > This is not a good way of handling the new value. Ideally we want the code path to be as shared as possible instead of splitting the code. Here it's a lot easier to do: > > case CSSPropertyTextIndent: > parsedValue = parseTextIndent(); > break; > > PassRefPtr<CSSValueList> CSSParser::parseTextIndent() > { > CSSParserValue* value = m_valueList->current(); > if (!value->id || validUnit(value, FLength | FPercent)) > return createPrimitiveNumericValue(value); > > #if ENABLE(CSS3_TEXT) > // Handle the extended grammar, namely [ each-line && <length> | <percentage> ] > #else > return 0; > #endif > } parseTextIndent() returns 2 types. "return createPrimitiveNumericValue(value);" return CSSValue and "// Handle the extended grammar, namely [ each-line && <length> | <percentage> ]" returns CSSValueList. There is no problem in parseTextIndent() because CSSValueList is CSSValue, but other part like StyleBuilder needs to check whether CSSValue is list or not. I think it's better that parseTextIndent() returns CSSvalueList when CSS3_TEXT in enabled and returns CSSValue when CSS3_TEXT is disabled. How about ? >> Source/WebCore/css/CSSParser.cpp:9162 >> + for (CSSParserValue* value = m_valueList->current(); value; value = m_valueList->next()) { > > I don't think having a for loop for 2 values is really good. I would rather see 2 checks as: > * they really depend on the condition in which you parse (did I see CSSValueEachLine? A <length> | <percentage>?) > * you are allowing more than 2 values (see below) I remove for loop and make 2 if statements >> Source/WebCore/css/CSSParser.cpp:9165 >> + else if (!hasParsedTextIndent && !value->id && validUnit(value, FLength | FPercent)) { > > Do you know if the !value->id is required? It seems like an overkill to me. Fixed. >> Source/WebCore/css/CSSParser.cpp:9170 >> + } > > Your parsing allows the following: > * text-indent: each-line each-line; > * text-indent: each-line 10px each-line; Fixed. >> Source/WebCore/css/CSSValueKeywords.in:1004 >> +each-line > > You are not prefixing text-indent (which is fine as it is an existing property). However this is new thus it should be prefixed. Use -webkit-each-line and CSSValueWebkitEachLine. >> Source/WebCore/css/StyleBuilder.cpp:1819 >> + for (size_t i = 0; i < valueList->length(); i++) { > > Again think about what you are parsing: you don't expect length() to be 15 so you should probably ASSERT it and have your code following the fact that we are probably get at most 2 values. Fixed. >> Source/WebCore/css/StyleBuilder.cpp:1834 >> + textIndent = Length(primitiveValue->cssCalcValue()->toCalcValue(styleResolver->style(), styleResolver->rootElementStyle(), styleResolver->style()->effectiveZoom())); > > calc() is not allowed during parsing so I wouldn't expect it to show up here. Remove this line. >> LayoutTests/fast/css3-text/css3-text-indent/getComputedStyle/getComputedStyle-text-indent-expected.txt:27 >> + > > Missing some cases here or at least you should try different <length> values (how about viewport as you do have custom code for it?). Also how about <percentage>? Add 20ex and <Percentage>. viewport values are tested another testcase, css3/viewport-percentage-lengths/css3-viewport-percentage-lengths-getStyle.html. >> LayoutTests/fast/css3-text/css3-text-indent/getComputedStyle/getComputedStyle-text-indent-expected.txt:39 >> +PASS computedStyle.getPropertyValue('text-indent') is '0px' > > Good test that you check invalid values but you should be a bit more extensive: how about 2 values? 3 values (including and not including each-line)? ... Add 2 and 3 values cases with/without -webkit-each-line. >> LayoutTests/fast/css3-text/css3-text-indent/getComputedStyle/getComputedStyle-text-indent-expected.txt:40 >> + > > How about inheritance which wasn't tested AFAICT? Inheritance is tested another test case, getComputedStyle-text-indent-inherited.js >> LayoutTests/platform/wincairo/TestExpectations:2989 >> +fast/css3-text/css3-text-indent/getComputedStyle/getComputedStyle-text-indent.html > > We should just skip the whole *directory* instead of touching every platforms everytime some new feature / test case is added. CSS3_TEXT is enabled only for EFL, GTK ports. So I should edit each platform TestExpectations file. And some other CSS3_TEXT test cases are registered like that. is it wrong ?

Created attachment 191884 [details] Patch

Created attachment 191931 [details] Patch

Created attachment 192151 [details] Patch

Comment on attachment 191931 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191931&action=review > Source/WebCore/ChangeLog:17 > + It is implemented under ENABLE_CSS3_TEXT with "-webkit" prefix. '"-webkit" prefix' is what is usually called 'prefixed' (see all the threads on prefixing / unprefixing on the internets). > Source/WebCore/css/CSSParser.cpp:2188 > + if (!parsedValue) > + return false; This is unneeded and will be handled after the switch. > Source/WebCore/css/CSSParser.cpp:9184 > + // [ <length> | <percentage> ] && -webkit-each-line? | inherit > + RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated(); To answer your question, you are trying to keep the existing behavior of returning a numeric value here if !ENABLE(CSS3_TEXT). I think it's artificial as this representation is not exposed and you can do the massaging when we return it in CSSComputedStyleDeclaration. Furthermore, you expect at most 2 values so you could actually use a Pair to get the values. > Source/WebCore/css/CSSParser.cpp:9193 > + return list; You need to call release on your RefPtr to convert it to a PassRefPtr. While this is semantically equivalent, it actually touches m_refCount twice (create a new PassRefPtr (+1) and destroying the RefPtr (-1)) instead of just transferring m_refCount untouched. See http://www.webkit.org/coding/RefPtr.html as a good intro on RefPtr / PassRefPtr. > Source/WebCore/css/CSSParser.cpp:9215 > + list->append(cssValuePool().createIdentifierValue(CSSValueWebkitEachLine)); > + list->append(createPrimitiveNumericValue(secondValue)); I would keep the ordering in which you have the values constant as it would simplify the StyleBuilder code (you can ASSERT it to catch offenders). > Source/WebCore/css/CSSParser.cpp:9230 > +#endif As mentioned, forking the code is not nice as now we have code duplication and different code being stressed differently based on CSS3_TEXT. Here you can easily parse the first value, store it into a variable and for the CSS3_TEXT case handle the extra grammar. A way to achieve that is to do: PassRefPtr<CSSPrimitiveValue> textIndentLength = 0; PassRefPtr<CSSPrimitiveValue> eachLine = 0; if (!parseEachLine(textIndendLength, eachLine)) return 0; #if ENABLE(CSS3_TEXT) if (!parseEachLine(textIndendLength, eachLine)) return 0; // We shouldn't accept more than 2 arguments. if (m_value->current) return false; #endif return createPrimitiveValuePair(textIndendLength, eachLine); with parseEachLine (you can find a better naming) taking the PassRefPtr as reference so that you can fill them and check if one was already set (to handle the duplicated case).

Created attachment 192870 [details] Patch

Comment on attachment 191931 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191931&action=review Thanks for your comments. >> Source/WebCore/ChangeLog:17 >> + It is implemented under ENABLE_CSS3_TEXT with "-webkit" prefix. > > '"-webkit" prefix' is what is usually called 'prefixed' (see all the threads on prefixing / unprefixing on the internets). Fixed. >> Source/WebCore/css/CSSParser.cpp:2188 >> + return false; > > This is unneeded and will be handled after the switch. if, return statements are removed. >> Source/WebCore/css/CSSParser.cpp:9193 >> + return list; > > You need to call release on your RefPtr to convert it to a PassRefPtr. While this is semantically equivalent, it actually touches m_refCount twice (create a new PassRefPtr (+1) and destroying the RefPtr (-1)) instead of just transferring m_refCount untouched. See http://www.webkit.org/coding/RefPtr.html as a good intro on RefPtr / PassRefPtr. Oops. I fixed. >> Source/WebCore/css/CSSParser.cpp:9230 >> +#endif > > As mentioned, forking the code is not nice as now we have code duplication and different code being stressed differently based on CSS3_TEXT. > > Here you can easily parse the first value, store it into a variable and for the CSS3_TEXT case handle the extra grammar. A way to achieve that is to do: > > PassRefPtr<CSSPrimitiveValue> textIndentLength = 0; > PassRefPtr<CSSPrimitiveValue> eachLine = 0; > if (!parseEachLine(textIndendLength, eachLine)) > return 0; > #if ENABLE(CSS3_TEXT) > if (!parseEachLine(textIndendLength, eachLine)) > return 0; > > // We shouldn't accept more than 2 arguments. > if (m_value->current) > return false; > #endif > return createPrimitiveValuePair(textIndendLength, eachLine); > > with parseEachLine (you can find a better naming) taking the PassRefPtr as reference so that you can fill them and check if one was already set (to handle the duplicated case). As I said on irc, Pair is not proper when text-indent has only one value. I used CSSValueList, Please review again.

Comment on attachment 192870 [details] Patch Attachment 192870 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17074424 New failing tests: svg/css/getComputedStyle-basic.xhtml transitions/text-indent-transition.html

Comment on attachment 192870 [details] Patch Attachment 192870 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17128084 New failing tests: svg/css/getComputedStyle-basic.xhtml transitions/text-indent-transition.html

Comment on attachment 192870 [details] Patch Attachment 192870 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17202032 New failing tests: svg/css/getComputedStyle-basic.xhtml transitions/text-indent-transition.html

Comment on attachment 192870 [details] Patch Attachment 192870 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17197064 New failing tests: svg/css/getComputedStyle-basic.xhtml transitions/text-indent-transition.html

Comment on attachment 192870 [details] Patch Attachment 192870 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17131294 New failing tests: svg/css/getComputedStyle-basic.xhtml

Created attachment 193032 [details] Patch

Comment on attachment 193032 [details] Patch Attachment 193032 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17015459 New failing tests: transitions/svg-transitions.html

Comment on attachment 193032 [details] Patch Attachment 193032 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17008907 New failing tests: transitions/svg-transitions.html

Comment on attachment 193032 [details] Patch Attachment 193032 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17133136 New failing tests: transitions/svg-transitions.html

Comment on attachment 193032 [details] Patch Attachment 193032 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17138265 New failing tests: transitions/svg-transitions.html

Created attachment 193071 [details] Patch

Comment on attachment 193071 [details] Patch 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.

Comment on attachment 193071 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193071&action=review Thanks for your comments. >> 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). I misunderstood your previous comments. I fixed. >> Source/WebCore/ChangeLog:28 >> + (WebCore): > > These are meaningless entries that should be removed (unfortunately our tools generate them for no good reason). Removed manually. >> 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. Fixed. New patch returns CSSValue when only <length>(or <percentage>) value, CSSValueList when -webkit-each-line exists. >> 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. I added some comments. >> Source/WebCore/css/CSSParser.cpp:9263 >> + CSSParserValue* value = 0; > > I would renamed that to lengthOrPercentageValue to make it explicit that this stores <length> | <percentage>. Renamed. >> 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 :-/ computeLength() is removed. >> Source/WebCore/css/StyleBuilder.cpp:1872 >> +#endif > > I don't think you need this code, it should default to the "initial" value in this case. This line is required when -webkit-each-line is removed dynamically. For example, text-indent is changed from "4em -webkit-each-line" to "2em". In this case, we need to initialize textIndentLine value of RenderStyle. >> Source/WebCore/css/StyleBuilder.cpp:1888 >> + 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()); I fixed to use convertToLength() instead of computeLength(). >> 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. After changes to return CSSValue instead of CSSValueList, no need to change the expected results. svg/css/getComputedStyle-basic-expected.txt, transitions/resources/transition-test-helpers.js, transitions/svg-transitions-expected.txt are recovered.

Created attachment 193754 [details] Patch

Comment on attachment 193754 [details] Patch 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/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). > 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. > 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.

Comment on attachment 193754 [details] Patch 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.

Created attachment 193986 [details] Patch

Comment on attachment 193986 [details] Patch Rejecting attachment 193986 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 193986, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue Last 500 characters of output: t/git/webkit-commit-queue/Source/WebKit/chromium/v8 --revision 13971 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 55>At revision 13971. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://webkit-commit-queue.appspot.com/results/17155729

Created attachment 194140 [details] Patch Fixed a conflict

Comment on attachment 194140 [details] Patch Clearing flags on attachment: 194140 Committed r146408: <http://trac.webkit.org/changeset/146408>

All reviewed patches have been landed. Closing bug.