[Webkit-unassigned] [Bug 133796] ASSERT_NOT_REACHED() in WebCore::fontWeightIsBold

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=133796


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #234923|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #20 from Darin Adler <darin at apple.com>  2014-07-16 10:59:43 PST ---
(From update of attachment 234923)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.