[webkit-reviews] review denied: [Bug 94475]=?UTF-8?Q?=20CSS=203=20=E2=80=98overflow=2Dwrap=E2=80=99=20property=20implementation=20?=: [Attachment 159417] patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied  review:
Bug 94475: CSS 3 ‘overflow-wrap’ property implementation
https://bugs.webkit.org/show_bug.cgi?id=94475

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

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


> LayoutTests/fast/text/overflow-wrap-expected.txt:20
> +	     text run at (2,110) width 127: "word CSS property."

First you are missing the image result (png) which is why the cq is rejecting
your patch. Now my question is: why do we need a pixel test here? Couldn't we
get away with a ref-test?

See http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree
for writing good pixel tests (hint: adding this test is platform-dependent
which is bad) and http://trac.webkit.org/wiki/Writing%20Reftests about
ref-tests. Also it would be nice to state better what you expect from the test:
basically you are only testing "longlonglonglonglongword" wrapping, the rest of
the explanations are just here to confuse.

> Source/WebCore/ChangeLog:9
> +	   http://www.w3.org/TR/2012/WD-css3-text-20120814/#overflow-wrap.

Where do you implement the following sentence?

For legacy reasons, UAs must treat ‘word-wrap’ as an alternate name for the
‘overflow-wrap’ property, as if it were a shorthand of ‘overflow-wrap’.

> Source/WebCore/ChangeLog:26
> +	   (WebCore::StyleResolver::collectMatchingRulesForList):

Those entries should be filled with details about the implementation.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:162
> +    CSSPropertyOverflowWrap,

What's the rationale behind landing this unprefixed and without a compile time
flag?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1925
> +	   case CSSPropertyOverflowWrap:
> +	       return cssValuePool().createValue(style->wordWrap());

This is not covered by any test you have. You should add more testing to cover
setting the style through JS / CSS and getting it via getComputedStyle. This
*does* include testing that inheritance is properly implemented.