Bug 60516 - Enable casting between CSSPrimitiveValue and FontWeight enum
Summary: Enable casting between CSSPrimitiveValue and FontWeight enum
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 60526
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-09 16:30 PDT by Luke Macpherson
Modified: 2011-05-09 18:53 PDT (History)
3 users (show)

See Also:


Attachments
Patch (5.11 KB, patch)
2011-05-09 16:35 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (5.96 KB, patch)
2011-05-09 17:09 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Macpherson 2011-05-09 16:30:42 PDT
Enable casting between CSSPrimitiveValue and FontWeight enum
Comment 1 Luke Macpherson 2011-05-09 16:35:26 PDT
Created attachment 92883 [details]
Patch
Comment 2 Darin Adler 2011-05-09 16:46:30 PDT
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?
Comment 3 Luke Macpherson 2011-05-09 17:09:28 PDT
Created attachment 92888 [details]
Patch
Comment 4 Luke Macpherson 2011-05-09 17:10:59 PDT
Cast removed, and also cleaned up the logic a little.
Comment 5 Darin Adler 2011-05-09 17:23:36 PDT
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!
Comment 6 Darin Adler 2011-05-09 17:24:33 PDT
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.
Comment 7 Luke Macpherson 2011-05-09 17:27:07 PDT
(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.
Comment 8 Luke Macpherson 2011-05-09 17:49:10 PDT
(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.
Comment 9 Darin Adler 2011-05-09 17:54:26 PDT
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.
Comment 10 WebKit Commit Bot 2011-05-09 18:53:06 PDT
Comment on attachment 92888 [details]
Patch

Clearing flags on attachment: 92888

Committed r86120: <http://trac.webkit.org/changeset/86120>
Comment 11 WebKit Commit Bot 2011-05-09 18:53:11 PDT
All reviewed patches have been landed.  Closing bug.