56288 2011-03-13 22:18:36 -0700 Improve the massive switch statement in CSSStyleSelector::applyProperty. 2011-03-29 18:26:56 -0700 1 1 1 Unclassified WebKit CSS 528+ (Nightly build) All OS X 10.5 RESOLVED FIXED P2 Normal --- 54707 1 levin macpherson commit-queue eric macpherson ojan oldest_to_newest 366838 0 levin 2011-03-13 22:18:36 -0700 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. 371159 1 levin 2011-03-21 20:04:38 -0700 ping 375891 2 87435 macpherson 2011-03-29 16:57:20 -0700 Created attachment 87435 Patch 375898 3 87435 webkit.review.bot 2011-03-29 17:02:33 -0700 Comment on attachment 87435 Patch Rejecting attachment 87435 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. 375915 4 87435 levin 2011-03-29 17:14:22 -0700 Comment on attachment 87435 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 :) 375922 5 87440 macpherson 2011-03-29 17:24:56 -0700 Created attachment 87440 Patch 375926 6 macpherson 2011-03-29 17:31:39 -0700 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. 375970 7 87440 commit-queue 2011-03-29 18:26:51 -0700 Comment on attachment 87440 Patch Clearing flags on attachment: 87440 Committed r82378: <http://trac.webkit.org/changeset/82378> 375971 8 commit-queue 2011-03-29 18:26:56 -0700 All reviewed patches have been landed. Closing bug. 87435 2011-03-29 16:57:20 -0700 2011-03-29 17:24:49 -0700 Patch bug-56288-20110330105718.patch text/plain 1621 macpherson SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDgyMzYyKQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTEtMDMtMjkgIEx1a2UgTWFj cGhlcnNvbiAgIDxtYWNwaGVyc29uQGNocm9taXVtLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBJbXByb3ZlIHRoZSBtYXNzaXZlIHN3aXRjaCBz dGF0ZW1lbnQgaW4gQ1NTU3R5bGVTZWxlY3Rvcjo6YXBwbHlQcm9wZXJ0eS4KKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTU2Mjg4CisKKyAgICAgICAgTm8g bmV3IHRlc3RzLiAoT09QUyEpCisKKyAgICAgICAgKiBjc3MvQ1NTU3R5bGVTZWxlY3Rvci5jcHA6 CisgICAgICAgIChXZWJDb3JlOjpDU1NTdHlsZVNlbGVjdG9yOjphcHBseVByb3BlcnR5KToKKyAg ICAgICAgQXNzZXJ0ZWQgdGhhdCBjYXNlcyBpbXBsZW1lbnRlZCBpbiBsb29rdXAgdGFibGUgYXJl IHVucmVhY2hhYmxlLgorICAgICAgICBVcGRhdGVkIGNvbW1lbnQuCisKIDIwMTEtMDMtMjkgIFN0 ZXZlIEJsb2NrICA8c3RldmVibG9ja0Bnb29nbGUuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5 IEplcmVteSBPcmxvdy4KSW5kZXg6IFNvdXJjZS9XZWJDb3JlL2Nzcy9DU1NTdHlsZVNlbGVjdG9y LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9jc3MvQ1NTU3R5bGVTZWxlY3Rvci5j cHAJKHJldmlzaW9uIDgyMzQ4KQorKysgU291cmNlL1dlYkNvcmUvY3NzL0NTU1N0eWxlU2VsZWN0 b3IuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC02MTkyLDcgKzYxOTIsNyBAQCB2b2lkIENTU1N0eWxl U2VsZWN0b3I6OmFwcGx5UHJvcGVydHkoaW50CiAgICAgICAgIHJldHVybjsKICAgICB9CiAKLSAg ICAvLyBGSVhNRTogTW92ZSB0aGVzZSBwcm9wZXJ0aWVzIHRvIHRoZSBhcHByb3ByaWF0ZSBwbGFj ZXMgaW4gdGhpcyBzd2l0Y2ggc3RhdGVtZW50LgorICAgIC8vIFRoZXNlIHByb3BlcnRpZXMgYXJl IGltcGxlbWVudGVkIGluIHRoZSBDU1NTdHlsZUFwcGx5UHJvcGVydHkgbG9va3VwIHRhYmxlLgog ICAgIGNhc2UgQ1NTUHJvcGVydHlDb2xvcjoKICAgICBjYXNlIENTU1Byb3BlcnR5QmFja2dyb3Vu ZENvbG9yOgogICAgIGNhc2UgQ1NTUHJvcGVydHlCb3JkZXJCb3R0b21Db2xvcjoKQEAgLTYyMDQs NiArNjIwNCw3IEBAIHZvaWQgQ1NTU3R5bGVTZWxlY3Rvcjo6YXBwbHlQcm9wZXJ0eShpbnQKICAg ICBjYXNlIENTU1Byb3BlcnR5V2Via2l0VGV4dEVtcGhhc2lzQ29sb3I6CiAgICAgY2FzZSBDU1NQ cm9wZXJ0eVdlYmtpdFRleHRGaWxsQ29sb3I6CiAgICAgY2FzZSBDU1NQcm9wZXJ0eVdlYmtpdFRl eHRTdHJva2VDb2xvcjoKKyAgICAgICAgQVNTRVJUX05PVF9SRUFDSEVEKCk7CiAgICAgICAgIHJl dHVybjsKIAogI2lmIEVOQUJMRShTVkcpCg== 87440 2011-03-29 17:24:56 -0700 2011-03-29 18:26:51 -0700 Patch bug-56288-20110330112454.patch text/plain 1687 macpherson SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDgyMzYyKQorKysgU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTEtMDMtMjkgIEx1a2UgTWFj cGhlcnNvbiAgIDxtYWNwaGVyc29uQGNocm9taXVtLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBJbXByb3ZlIHRoZSBtYXNzaXZlIHN3aXRjaCBz dGF0ZW1lbnQgaW4gQ1NTU3R5bGVTZWxlY3Rvcjo6YXBwbHlQcm9wZXJ0eS4KKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTU2Mjg4CisKKyAgICAgICAgTm8g bmV3IHRlc3RzIGFyZSBuZWVkZWQgYmVjYXVzZSBubyBuZXcgZnVuY3Rpb25hbGl0eSBleHBvc2Vk LgorCisgICAgICAgICogY3NzL0NTU1N0eWxlU2VsZWN0b3IuY3BwOgorICAgICAgICAoV2ViQ29y ZTo6Q1NTU3R5bGVTZWxlY3Rvcjo6YXBwbHlQcm9wZXJ0eSk6CisgICAgICAgIEFzc2VydGVkIHRo YXQgY2FzZXMgaW1wbGVtZW50ZWQgaW4gdGhlIENTU1N0eWxlQXBwbHlQcm9wZXJ0eSBsb29rdXAg dGFibGUgYXJlIHVucmVhY2hhYmxlLgorICAgICAgICBVcGRhdGVkIGNvbW1lbnQuCisKIDIwMTEt MDMtMjkgIFN0ZXZlIEJsb2NrICA8c3RldmVibG9ja0Bnb29nbGUuY29tPgogCiAgICAgICAgIFJl dmlld2VkIGJ5IEplcmVteSBPcmxvdy4KSW5kZXg6IFNvdXJjZS9XZWJDb3JlL2Nzcy9DU1NTdHls ZVNlbGVjdG9yLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9jc3MvQ1NTU3R5bGVT ZWxlY3Rvci5jcHAJKHJldmlzaW9uIDgyMzQ4KQorKysgU291cmNlL1dlYkNvcmUvY3NzL0NTU1N0 eWxlU2VsZWN0b3IuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC02MTkyLDcgKzYxOTIsNyBAQCB2b2lk IENTU1N0eWxlU2VsZWN0b3I6OmFwcGx5UHJvcGVydHkoaW50CiAgICAgICAgIHJldHVybjsKICAg ICB9CiAKLSAgICAvLyBGSVhNRTogTW92ZSB0aGVzZSBwcm9wZXJ0aWVzIHRvIHRoZSBhcHByb3By aWF0ZSBwbGFjZXMgaW4gdGhpcyBzd2l0Y2ggc3RhdGVtZW50LgorICAgIC8vIFRoZXNlIHByb3Bl cnRpZXMgYXJlIGltcGxlbWVudGVkIGluIHRoZSBDU1NTdHlsZUFwcGx5UHJvcGVydHkgbG9va3Vw IHRhYmxlLgogICAgIGNhc2UgQ1NTUHJvcGVydHlDb2xvcjoKICAgICBjYXNlIENTU1Byb3BlcnR5 QmFja2dyb3VuZENvbG9yOgogICAgIGNhc2UgQ1NTUHJvcGVydHlCb3JkZXJCb3R0b21Db2xvcjoK QEAgLTYyMDQsNiArNjIwNCw3IEBAIHZvaWQgQ1NTU3R5bGVTZWxlY3Rvcjo6YXBwbHlQcm9wZXJ0 eShpbnQKICAgICBjYXNlIENTU1Byb3BlcnR5V2Via2l0VGV4dEVtcGhhc2lzQ29sb3I6CiAgICAg Y2FzZSBDU1NQcm9wZXJ0eVdlYmtpdFRleHRGaWxsQ29sb3I6CiAgICAgY2FzZSBDU1NQcm9wZXJ0 eVdlYmtpdFRleHRTdHJva2VDb2xvcjoKKyAgICAgICAgQVNTRVJUX05PVF9SRUFDSEVEKCk7CiAg ICAgICAgIHJldHVybjsKIAogI2lmIEVOQUJMRShTVkcpCg==