[webkit-reviews] review denied: [Bug 133796] ASSERT_NOT_REACHED() in WebCore::fontWeightIsBold : [Attachment 234923] Patch v3

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Tibor Mészáros
<tmeszaros.u-szeged at partner.samsung.com>'s request for review:
Bug 133796: ASSERT_NOT_REACHED() in WebCore::fontWeightIsBold
https://bugs.webkit.org/show_bug.cgi?id=133796

Attachment 234923: Patch v3
https://bugs.webkit.org/attachment.cgi?id=234923&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=234923&action=review


Patch is well meaning but wrong.

The real problem is far away from this code, and quieting the assertion here is
not a real fix.

> LayoutTests/ChangeLog:12
> +	   * css1/font_properties/font_weight_bolder.html: Added.
> +	   * css1/font_properties/font_weight_lighter.html: Added.

Test cases should be reference tests.

I think it’s really strange to have these tests in the css1 directory. These
are editing tests and belong in the editing directory.

In a CSS test directory (not this css1 one) we could have tests to make sure
that bolder and lighter are rejected as keyword values during the CSS parsing
process. Only a few special paths in editing code should parse in a mode that
allows these values.

> LayoutTests/css1/font_properties/font_weight_bolder.html:4
> +	   font-weight:bolder;

We should not allow this syntax in a style sheet. The “bolder” and “lighter”
values are a special CSS hack for editing, and it’s an error to allow a style
rule to actually contain that value. The real fix would be to find a way to
address that issue.

> LayoutTests/css1/font_properties/font_weight_bolder.html:12
> +		  <animatemotion
onload='document.execCommand("selectall")'></animatemotion>

Why on earth does this test case include SVG? Is that needed to reproduce the
bug or just a strange choice we are making?

>> Source/WebCore/editing/EditingStyle.cpp:1533
>> +	    case CSSValueLighter:
> 
> This doesn't seem to conceptually make sense... "Lighter" means bold?

Myles is absolutely right. It’s wrong to say that bolder and lighter are bold
weights. The current behavior of this function where it asserts in that case is
correct.

To correctly deal with the values “bolder” and “lighter”, they need to never be
considered the same as existing style; a correct fix would always skip the
removeProperty call when the mutable style’s property was bolder or lighter,
and continue to assert if the other style contains bolder or lighter. See
below.

>> Source/WebCore/editing/EditingStyle.cpp:1557
>> +	ASSERT(baseStyle);
> 
> If you're going to ASSERT this, why not make it a reference?

Good idea for future refactoring. Both arguments to this function template
should be references.

> Source/WebCore/editing/EditingStyle.cpp:1572
> +    if ((baseFontWeight = extractPropertyValue(baseStyle,
CSSPropertyFontWeight))
> +	   && (fontWeight =
mutableStyle->getPropertyCSSValue(CSSPropertyFontWeight))
> +	   && (fontWeightIsBold(*fontWeight) ==
fontWeightIsBold(*baseFontWeight))) {
> +	       mutableStyle->removeProperty(CSSPropertyFontWeight);
> +    }

If fontWeight is bolder or lighter, then we should skip removeProperty
regardless of the value of baseFontWeight. That needs to be done here; I don’t
see a way to correctly encapsulate it in the fontWeightIsBold helper function.

If the font weight is the minimum the we could remove “lighter”, and if the
font weight is the maximum we could remove “bolder”. That would be nice to get
right, but it’s an edge case that I don’t think is critical.

I’d like to see test cases covering this behavior, including the edge cases.

If baseFontWeight is bolder or lighter, then the bug has already happened. Such
styles should never be allowed in CSS styles on actual elements. Those values
are a hack to be used inside the editing machinery and in API not exposed to
JavaScript. If JavaScript can ever see those values, then we have a bug.

The refactoring to use references instead of pointers we are doing here messes
up the logic here and makes this function harder to read. We should think about
how to handle that, and why boldness is different from the other properties
below in this respect. I suspect that refactoring is not really part of this
bug fix and we should consider doing it separately.