[webkit-reviews] review denied: [Bug 85755] [CSS3 Values] Add support for the "ch" unit : [Attachment 179815] CSS3 ch unit patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Lamarque V. Souza
<Lamarque.Souza at basyskom.com>'s request for review:
Bug 85755: [CSS3 Values] Add support for the "ch" unit
https://bugs.webkit.org/show_bug.cgi?id=85755

Attachment 179815: CSS3 ch unit patch
https://bugs.webkit.org/attachment.cgi?id=179815&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=179815&action=review


>> Source/WebCore/css/CSSParser.cpp:1632
>> +	       || (value->unit >= CSSPrimitiveValue::CSS_TURN && value->unit <=
CSSPrimitiveValue::CSS_CHS)
> 
> Weird number of spaces at line-start.  Are you using a 4-space indent? 
[whitespace/indent] [3]

I would just fix these and the lines in the same expression.  You should indent
4 spaces in from the ASSERT (8 spaces from the left edge of the file).

>> Source/WebCore/css/CSSParser.cpp:1637
>> +	       || (value->unit >= CSSPrimitiveValue::CSS_TURN && value->unit <=
CSSPrimitiveValue::CSS_CHS)
> 
> Weird number of spaces at line-start.  Are you using a 4-space indent? 
[whitespace/indent] [3]

I would fix this too.

>> Source/WebCore/css/CSSPrimitiveValue.h:118
>> +	    CSS_CHS = 109,
> 
> enum members should use InterCaps with an initial capital letter. 
[readability/enum_casing] [4]

It's OK to keep following the existing style here.  It would be nice for
someone to fix this in a separate patch.

>> Source/WebCore/css/CSSPrimitiveValue.h:166
>> +		   || m_primitiveUnitType == CSS_EXS
> 
> Weird number of spaces at line-start.  Are you using a 4-space indent? 
[whitespace/indent] [3]

These are lines you added, so they should follow the style guide and be
indented 4 spaces from the return.

> LayoutTests/ChangeLog:10
> +	   * fast/css/css3-ch-unit.html: Added.
> +	   * platform/chromium-linux/fast/css/css3-ch-unit-expected.txt: Added.

> +	   * platform/qt/fast/css/css3-ch-unit-expected.txt: Added.

This test is failing on the chromium EWS bots because it doesn't have the pixel
result (png file checked in).  Maybe we can do better and make this a dump as
text test.  For example, you could have a div with the width of 1ch and you
could have a div that shrink wraps (e.g., inline-block) the number 0.  The divs
should be the same width which you could check in javascript.  For testing
font-size:1ch, you might be able to use the Ahem font and get the same number
on all platforms.  You might be able to do the pseudo-element tests using a ref
test.

If you find that something can only be tested using a pixel test, we should at
least split that off into as small and focused of a test as possible to make it
easier for people maintaining the tests to tell if they are correct or not.