Bug 83536 – Don't allow fallthrough for CSSPropertyBaselineShift in CSSComputedStyleDeclaration::getSVGPropertyCSSValue().

• Component: New Bugs
• Status: RESOLVED FIXED

[Luke Macpherson]

Don't allow fallthrough for CSSPropertyBaselineShift in CSSComputedStyleDeclaration::getSVGPropertyCSSValue().

[Luke Macpherson]

Created attachment 136370 [details]
Patch

[Daniel Bates]

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?

[Daniel Bates]

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.

[Luke Macpherson]

Created attachment 136390 [details]
Patch for landing

[Luke Macpherson]

Comments addressed. Thanks for the review.

[WebKit Review Bot]

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

[WebKit Review Bot]

Comment on attachment 136390 [details]
Patch for landing

Clearing flags on attachment: 136390

Committed r113748: <http://trac.webkit.org/changeset/113748>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Caio Marcelo de Oliveira Filho]

*** Bug 27902 has been marked as a duplicate of this bug. ***