| Summary: | Don't allow fallthrough for CSSPropertyBaselineShift in CSSComputedStyleDeclaration::getSVGPropertyCSSValue(). | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Luke Macpherson <macpherson> | ||||||
| Component: | New Bugs | Assignee: | Luke Macpherson <macpherson> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | dbates, jhawkins, macpherson, menard, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Luke Macpherson
2012-04-09 18:52:25 PDT
Created attachment 136370 [details]
Patch
Comment on attachment 136370 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136370&action=review > Source/WebCore/ChangeLog:11 > + I don't think this return is reachable, but if it were, falling through to the next case would be > + the wrong thing to do, so this may catch a future bug one day. Although somewhat contrived, the presence of the "return 0;" statement would the catch the case where a person removed the enum values BS_BASELINE, BS_SUB, BS_SUPER, BS_LENGTH and instead declared integer variables of the same name. > Source/WebCore/css/SVGCSSComputedStyleDeclaration.cpp:177 > + return 0; Can we add ASSERT_NOT_REACHED() above this line? Comment on attachment 136370 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136370&action=review > Source/WebCore/ChangeLog:8 > + No new tests / code cleanup from coverity static analysis. Nit: Usually such a sentence comes after the description. > Source/WebCore/ChangeLog:10 > + I don't think this return is reachable, but if it were, falling through to the next case would be In order to understand this description you need to have read the patch. In particular, it is unclear what the phrase "this return" is referring to unless you read the patch. Maybe a sentence like: Add a return statement to the case CSSPropertyBaselineShift so that we don't fall through to the next case statement. Created attachment 136390 [details]
Patch for landing
Comments addressed. Thanks for the review. Comment on attachment 136390 [details] Patch for landing Rejecting attachment 136390 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: ebKit/chromium/gpu --revision 131183 --non-interactive --force --accept theirs-conflict --ignore-externals returned non-zero exit status 1 in /mnt/git/webkit-commit-queue/Source/WebKit/chromium Error: 'depot_tools/gclient sync --force --reset --delete_unversioned_trees' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107. Re-trying 'depot_tools/gclient sync --force --reset --delete_unversioned_trees' No such file or directory at Tools/Scripts/update-webkit line 118. Full output: http://queues.webkit.org/results/12369588 Comment on attachment 136390 [details] Patch for landing Clearing flags on attachment: 136390 Committed r113748: <http://trac.webkit.org/changeset/113748> All reviewed patches have been landed. Closing bug. |