Bug 56288

Summary: Improve the massive switch statement in CSSStyleSelector::applyProperty.
Product: WebKit Reporter: David Levin <levin>
Component: CSSAssignee: Luke Macpherson <macpherson>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric, macpherson, ojan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: OS X 10.5   
Bug Depends on: 54707    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch none

David Levin
Reported 2011-03-13 22:18:36 PDT
1. Handle the FIXME which I added at the end (to fix the build for folks who didn't have svg enabled). 2. Ideally this switch statement would not have a default. (Due to this default: clause, the build broke for people who did not have SVG enabled and had certain compile warnings on about handling enums). 3. Since every case does a "return" The end of the function could have a not reached assert to verify that only valid enum values were passed. Similar comments (to 2 and 3) apply to CSSStyleSelector::applyProperty.
Attachments
Patch (1.58 KB, patch)
2011-03-29 16:57 PDT, Luke Macpherson 		no flags
DetailsFormatted DiffDiff
Patch (1.65 KB, patch)
2011-03-29 17:24 PDT, Luke Macpherson 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
David Levin
Comment 1 2011-03-21 20:04:38 PDT
ping
Luke Macpherson
Comment 2 2011-03-29 16:57:20 PDT
Created attachment 87435 [details] Patch
WebKit Review Bot
Comment 3 2011-03-29 17:02:33 PDT
Comment on attachment 87435 [details] Patch Rejecting attachment 87435 [details] from commit-queue. macpherson@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.
David Levin
Comment 4 2011-03-29 17:14:22 PDT
Comment on attachment 87435 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=87435&action=review cq- due to the change log issue This is better. It would be nice if you had a short explanation in the bug of why those items aren't being addressed (why not get rid of the default and replace it with explicit enum values and then have and ASSERT_NOT_REACHED() after the switch statement?). > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) This line needs to be removed. It should be replaced by either: 1. A line which indicates the tests. 2. A reason why there cannot be tests. 3. An explanation of why no tests are needed. (No new functionality exposed so no new tests.) Choose #3 :)
Luke Macpherson
Comment 5 2011-03-29 17:24:56 PDT
Created attachment 87440 [details] Patch
Luke Macpherson
Comment 6 2011-03-29 17:31:39 PDT
The default: case for svg properties was pre-existing. I agree that having that default case was a bad idea and should be removed, but given that I'm intending to remove the entire switch statement I will not endeavor to clean up the existing code right now.
WebKit Commit Bot
Comment 7 2011-03-29 18:26:51 PDT
Comment on attachment 87440 [details] Patch Clearing flags on attachment: 87440 Committed r82378: <http://trac.webkit.org/changeset/82378>
WebKit Commit Bot
Comment 8 2011-03-29 18:26:56 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.