Bug 174149 - CSSFontStyleValue::isItalic seems a bit bogus.
Summary: CSSFontStyleValue::isItalic seems a bit bogus.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-07-04 21:19 PDT by Emilio Cobos Álvarez
Modified: 2017-08-10 12:25 PDT (History)
9 users (show)

See Also:


Attachments
Patch (3.96 KB, patch)
2017-07-05 16:10 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Emilio Cobos Álvarez 2017-07-04 21:19:54 PDT
bool isItalic() const
    {
        if (!obliqueValue) {
            auto valueID = fontStyleValue->valueID();
            return valueID == CSSValueItalic || CSSValueOblique;
        }
        return obliqueValue->value<float>(CSSPrimitiveValue::CSS_DEG) >= static_cast<float>(italicValue());
    }

Note how "|| CSSValueOblique" makes the first return always return true.

I'm not familiar with this code, so I'm not sure how to best test it.

Happy to fix it with indications on how to test it. I've CCd the original patch author and reviewer so you can take a look (and fix) if you want to.
Comment 1 Myles C. Maxfield 2017-07-05 13:46:37 PDT
Good catch!
Comment 2 Myles C. Maxfield 2017-07-05 16:10:41 PDT
Created attachment 314662 [details]
Patch
Comment 3 Tim Horton 2017-07-05 16:21:31 PDT
Comment on attachment 314662 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        * css/CSSFontStyleValue.h:

Write some words!

> Source/WebCore/css/CSSFontStyleValue.h:50
>      {

Should this be called isItalicOrOblique?

> LayoutTests/editing/execCommand/italicizeByCharacter-normal.html:11
> +<script src=../editing.js language="JavaScript" type="text/JavaScript" ></script>

This is the ugliest <script> I've seen in a long time :)
Comment 4 WebKit Commit Bot 2017-07-05 17:26:34 PDT
Comment on attachment 314662 [details]
Patch

Clearing flags on attachment: 314662

Committed r219173: <http://trac.webkit.org/changeset/219173>
Comment 5 WebKit Commit Bot 2017-07-05 17:26:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Myles C. Maxfield 2017-07-05 18:14:26 PDT
(In reply to Tim Horton from comment #3)
> Comment on attachment 314662 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=314662&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        * css/CSSFontStyleValue.h:
> 
> Write some words!
> 
> > Source/WebCore/css/CSSFontStyleValue.h:50
> >      {
> 
> Should this be called isItalicOrOblique?
> 
> > LayoutTests/editing/execCommand/italicizeByCharacter-normal.html:11
> > +<script src=../editing.js language="JavaScript" type="text/JavaScript" ></script>
> 
> This is the ugliest <script> I've seen in a long time :)

Oh no, I didn't actually do anything you said!!! :(
I'll do it in a follow-up patch.
Comment 7 Myles C. Maxfield 2017-08-10 09:59:40 PDT
Committed r220531: <http://trac.webkit.org/changeset/220531>
Comment 8 Radar WebKit Bug Importer 2017-08-10 10:00:55 PDT
<rdar://problem/33829710>
Comment 9 Simon Fraser (smfr) 2017-08-10 11:24:52 PDT
Comment on attachment 314662 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        CSSFontStyleValue::isItalic seems a bit bogus.

Vaguest bug title ever.
Comment 10 Antti Koivisto 2017-08-10 12:25:33 PDT
> > +        CSSFontStyleValue::isItalic seems a bit bogus.
> 
> Vaguest bug title ever.

"CSSFontStyleValue::isItalic has high bogosity."