Bug 71824 – CSSImageGeneratorValue: Devirtualize image(), isFixedSize() and fixedSize().

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Andreas Kling]

SSIA.

[Andreas Kling]

Created attachment 114088 [details]
Patch

[Darin Adler]

Comment on attachment 114088 [details]
Patch

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

> Source/WebCore/css/CSSImageGeneratorValue.cpp:134
> +    default:
> +        ASSERT_NOT_REACHED();

If you leave out the default and put the ASSERT_NOT_REACHED outside the switch statement, then some compilers, including clang, will check that all values of the enum are handled and give a warning if they are not. I think you should do that.

> Source/WebCore/css/CSSImageGeneratorValue.cpp:151
> +    default:
> +        ASSERT_NOT_REACHED();

Here too.

> Source/WebCore/css/CSSImageGeneratorValue.cpp:168
> +    default:
> +        ASSERT_NOT_REACHED();

Here too.

[Andreas Kling]

(In reply to comment #2)
> (From update of attachment 114088 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114088&action=review
> 
> > Source/WebCore/css/CSSImageGeneratorValue.cpp:134
> > +    default:
> > +        ASSERT_NOT_REACHED();
> 
> If you leave out the default and put the ASSERT_NOT_REACHED outside the switch statement, then some compilers, including clang, will check that all values of the enum are handled and give a warning if they are not. I think you should do that.

Only a subset of the CSSValue::ClassType values are CSSImageGeneratorValue subclass types, so we don't want cases for all of them here, hence the ASSERT_NOT_REACHED().

[Darin Adler]

(In reply to comment #3)
> Only a subset of the CSSValue::ClassType values are CSSImageGeneratorValue subclass types, so we don't want cases for all of them here, hence the ASSERT_NOT_REACHED().

OK.

In other cases I have actually added all the cases like this:

{
    switch {
    case HandledCase1:
        [...]
        return x;
    [...]
    case UnhandledCase1:
    case UnhandledCase2:
    case UnhandledCase3:
        break;
    }
    ASSERT_NOT_REACHED();
    return x;
}

Clearly, whether that’s worthwhile or not depends on the likelihood of adding a new case that is not an image generator subclass or adding a new case that is an image generator subclass. You probably want to leave it as-is, but worth thinking about in future.

[Andreas Kling]

Committed r99577: <http://trac.webkit.org/changeset/99577>

[Simon Fraser (smfr)]

(In reply to comment #0)
> SSIA.

No it doesn't. Why devirtualize? Was this a measurable performance gain? On what content?

[Andreas Kling]

(In reply to comment #6)
> (In reply to comment #0)
> > SSIA.
> 
> No it doesn't. Why devirtualize? Was this a measurable performance gain? On what content?

This was part of devirtualizing CSSValue to reduce memory usage (tracked on bug 71666, culminating in this commit: <http://trac.webkit.org/changeset/99592>.)

[David Barr]

*** Bug 64862 has been marked as a duplicate of this bug. ***