Enable casting between CSSPrimitiveValue and FontWeight enum

• Component: New Bugs
• Status: RESOLVED FIXED

[Luke Macpherson]

Enable casting between CSSPrimitiveValue and FontWeight enum

[Luke Macpherson]

Created attachment 92883 [details]
Patch

[Darin Adler]

Comment on attachment 92883 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92883&action=review

> Source/WebCore/css/CSSStyleSelector.cpp:3688
> +                        fontDescription.setWeight((FontWeight)*primitiveValue);

An explicit cast should not be needed here. Could you try without it?

[Luke Macpherson]

Created attachment 92888 [details]
Patch

[Luke Macpherson]

Cast removed, and also cleaned up the logic a little.

[Darin Adler]

Comment on attachment 92888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92888&action=review

> Source/WebCore/css/CSSStyleSelector.cpp:-3717
> -                    default:
> -                        return;

Wait, no, this changes behavior!

[Darin Adler]

Comment on attachment 92888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92888&action=review

>> Source/WebCore/css/CSSStyleSelector.cpp:-3717
>> -                        return;
> 
> Wait, no, this changes behavior!

The old code returned from the function without doing anything if the identifier was not one of the supported ones. The new code asserts and sets the weight to normal. I don’t think that behavior change is OK.

[Luke Macpherson]

(In reply to comment #6)
> (From update of attachment 92888 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92888&action=review
> 
> >> Source/WebCore/css/CSSStyleSelector.cpp:-3717
> >> -                        return;
> > 
> > Wait, no, this changes behavior!
> 
> The old code returned from the function without doing anything if the identifier was not one of the supported ones. The new code asserts and sets the weight to normal. I don’t think that behavior change is OK.

I think the default case previously should have been ASSERT_UNREACHABLE.

[Luke Macpherson]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 92888 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=92888&action=review
> > 
> > >> Source/WebCore/css/CSSStyleSelector.cpp:-3717
> > >> -                        return;
> > > 
> > > Wait, no, this changes behavior!
> > 
> > The old code returned from the function without doing anything if the identifier was not one of the supported ones. The new code asserts and sets the weight to normal. I don’t think that behavior change is OK.
> 
> I think the default case previously should have been ASSERT_UNREACHABLE.

Moreover, you can see the list of valid values in the parser:
// normal | bold | bolder | lighter | 100 | 200 | 300 | 400 | 500 | 600 | 700 | 800 | 900 | inherit
Any other value would indicate a parser bug.

[Darin Adler]

Comment on attachment 92888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92888&action=review

>>>>> Source/WebCore/css/CSSStyleSelector.cpp:-3717
>>>>> -                        return;
>>>> 
>>>> Wait, no, this changes behavior!
>>> 
>>> The old code returned from the function without doing anything if the identifier was not one of the supported ones. The new code asserts and sets the weight to normal. I don’t think that behavior change is OK.
>> 
>> I think the default case previously should have been ASSERT_UNREACHABLE.
> 
> Moreover, you can see the list of valid values in the parser:
> // normal | bold | bolder | lighter | 100 | 200 | 300 | 400 | 500 | 600 | 700 | 800 | 900 | inherit
> Any other value would indicate a parser bug.

OK. That makes sense and matches what I remember from other times working with the parser. I’d feel more comfortable if there was an actual test case covering this.

[WebKit Commit Bot]

Comment on attachment 92888 [details]
Patch

Clearing flags on attachment: 92888

Committed r86120: <http://trac.webkit.org/changeset/86120>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.