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.
ping
Created attachment 87435 [details] Patch
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.
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 :)
Created attachment 87440 [details] Patch
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.
Comment on attachment 87440 [details] Patch Clearing flags on attachment: 87440 Committed r82378: <http://trac.webkit.org/changeset/82378>
All reviewed patches have been landed. Closing bug.