[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