Bug 71824

Summary: CSSImageGeneratorValue: Devirtualize image(), isFixedSize() and fixedSize().
Product: WebKit Reporter: Andreas Kling <kling>
Component: CSSAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, koivisto, macpherson, simon.fraser, tczene, thorton, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 71666    
Attachments:
Description Flags
Patch darin: review+

Andreas Kling
Reported 2011-11-08 09:03:10 PST
SSIA.
Attachments
Patch (6.67 KB, patch)
2011-11-08 09:07 PST, Andreas Kling 		darin: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Andreas Kling
Comment 1 2011-11-08 09:07:26 PST
Created attachment 114088 [details] Patch
Darin Adler
Comment 2 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.
Andreas Kling
Comment 3 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().
Darin Adler
Comment 4 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.
Andreas Kling
Comment 5 2011-11-08 09:36:08 PST
Committed r99577: <http://trac.webkit.org/changeset/99577>
Simon Fraser (smfr)
Comment 6 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?
Andreas Kling
Comment 7 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>.)
David Barr
Comment 8 2012-01-08 15:28:52 PST
*** Bug 64862 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.