SSIA.
Created attachment 114088 [details] Patch
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.
(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().
(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.
Committed r99577: <http://trac.webkit.org/changeset/99577>
(In reply to comment #0) > SSIA. No it doesn't. Why devirtualize? Was this a measurable performance gain? On what content?
(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>.)
*** Bug 64862 has been marked as a duplicate of this bug. ***