[webkit-reviews] review denied: [Bug 88866] ASSERT_NOT_REACHED in StylePropertySet::fontValue when accessing font style property through JS after setting style font size. : [Attachment 181540] Attempt to fix the test in cross-platform way

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jan 9 04:17:31 PST 2013


Alexander Pavlov (apavlov) <apavlov at chromium.org> has denied Alexis Menard
(darktears) <alexis at webkit.org>'s request for review:
Bug 88866: ASSERT_NOT_REACHED in StylePropertySet::fontValue when accessing
font style property through JS after setting style font size.
https://bugs.webkit.org/show_bug.cgi?id=88866

Attachment 181540: Attempt to fix the test in cross-platform way
https://bugs.webkit.org/attachment.cgi?id=181540&action=review

------- Additional Comments from Alexander Pavlov (apavlov)
<apavlov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=181540&action=review


> Source/WebCore/ChangeLog:15
> +	   other shorthands in of WebKit. When reconstructing the shorthand
other

stray "of"

> Source/WebCore/ChangeLog:18
> +	   shortest shorthand possible) or if the value is set or not (if set
then

..but not shorter than it is allowed to be :) (see below)

> Source/WebCore/ChangeLog:22
> +	   construct a valid value at it takes care of adding ' ' or '/' when

at -> as

> Source/WebCore/css/StylePropertySet.cpp:262
> +    return;

You definitely can drop it :)

> LayoutTests/fast/css/font-shorthand-from-longhands-expected.txt:6
> +PASS style.font is '20px'

According to http://www.w3.org/TR/CSS2/fonts.html#font-shorthand, "20px" is not
a valid value for "font" (if a non-keyword value is used, the <'font-size'> and
<'font-family'> components are mandatory.) Thus,
style.font should be "20px foobar", not bare "20px".

> LayoutTests/fast/css/font-shorthand-from-longhands-expected.txt:37
> +PASS style.font is 'bold 40px'

This doesn't seem a valid value, either

> LayoutTests/fast/css/font-shorthand-from-longhands.html:10
> +    font-family: "foobar";

It would be nice to test the case when there's no explicit "font-family" value
for the checked element.

> LayoutTests/fast/css/font-shorthand-from-longhands.html:17
> +description("Test the parsing and the computed style values of the
transitions properties.")

doesn't look like "transitions properties" :)

> LayoutTests/fast/css/font-shorthand-from-longhands.html:24
> +e = document.getElementById('test');

Use "var" declarations for all variables - this is the common "right" way

> LayoutTests/fast/css/font-shorthand-from-longhands.html:28
> +// This function check the return value of style.font and verify WebKit can
parse it.

checks
verifies

> LayoutTests/fast/css/font-shorthand-from-longhands.html:33
> +    return (e.style.getPropertyValue('font') == before);

My impression is that all JS code uses '===' whenever an identity check is
implied (at least the Web Inspector)


More information about the webkit-reviews mailing list