Bug 83536 - Don't allow fallthrough for CSSPropertyBaselineShift in CSSComputedStyleDeclaration::getSVGPropertyCSSValue().
Summary: Don't allow fallthrough for CSSPropertyBaselineShift in CSSComputedStyleDecla...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Luke Macpherson
URL:
Keywords:
: 27902 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-04-09 18:52 PDT by Luke Macpherson
Modified: 2012-05-28 21:06 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.74 KB, patch)
2012-04-09 18:54 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch for landing (1.72 KB, patch)
2012-04-09 21:08 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Macpherson 2012-04-09 18:52:25 PDT
Don't allow fallthrough for CSSPropertyBaselineShift in CSSComputedStyleDeclaration::getSVGPropertyCSSValue().
Comment 1 Luke Macpherson 2012-04-09 18:54:30 PDT
Created attachment 136370 [details]
Patch
Comment 2 Daniel Bates 2012-04-09 20:53:43 PDT
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 3 Daniel Bates 2012-04-09 21:00:56 PDT
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.
Comment 4 Luke Macpherson 2012-04-09 21:08:09 PDT
Created attachment 136390 [details]
Patch for landing
Comment 5 Luke Macpherson 2012-04-09 21:50:27 PDT
Comments addressed. Thanks for the review.
Comment 6 WebKit Review Bot 2012-04-09 23:25:39 PDT
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 7 WebKit Review Bot 2012-04-10 12:11:09 PDT
Comment on attachment 136390 [details]
Patch for landing

Clearing flags on attachment: 136390

Committed r113748: <http://trac.webkit.org/changeset/113748>
Comment 8 WebKit Review Bot 2012-04-10 12:11:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Caio Marcelo de Oliveira Filho 2012-05-28 21:06:50 PDT
*** Bug 27902 has been marked as a duplicate of this bug. ***