Bug 71824 - CSSImageGeneratorValue: Devirtualize image(), isFixedSize() and fixedSize().
Summary: CSSImageGeneratorValue: Devirtualize image(), isFixedSize() and fixedSize().
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
: 64862 (view as bug list)
Depends on:
Blocks: 71666
  Show dependency treegraph
 
Reported: 2011-11-08 09:03 PST by Andreas Kling
Modified: 2012-01-08 15:28 PST (History)
7 users (show)

See Also:


Attachments
Patch (6.67 KB, patch)
2011-11-08 09:07 PST, Andreas Kling
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2011-11-08 09:03:10 PST
SSIA.
Comment 1 Andreas Kling 2011-11-08 09:07:26 PST
Created attachment 114088 [details]
Patch
Comment 2 Darin Adler 2011-11-08 09:18:17 PST
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.
Comment 3 Andreas Kling 2011-11-08 09:22:58 PST
(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().
Comment 4 Darin Adler 2011-11-08 09:30:58 PST
(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.
Comment 5 Andreas Kling 2011-11-08 09:36:08 PST
Committed r99577: <http://trac.webkit.org/changeset/99577>
Comment 6 Simon Fraser (smfr) 2011-11-14 14:28:35 PST
(In reply to comment #0)
> SSIA.

No it doesn't. Why devirtualize? Was this a measurable performance gain? On what content?
Comment 7 Andreas Kling 2011-11-14 15:01:24 PST
(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>.)
Comment 8 David Barr 2012-01-08 15:28:52 PST
*** Bug 64862 has been marked as a duplicate of this bug. ***