Bug 92393 – Build broken when svg is disabled.

• Component: SVG
• Status: RESOLVED FIXED

[Chang Shu]

Source/WebCore/rendering/FilterEffectRenderer.cpp does not compile when the following command is issued:
Tools/Scripts/build-webkit --no-svg --no-svg-fonts --no-svg-dom-objc-bindings

[Chang Shu]

Created attachment 154680 [details]
surpress unused parameters warning.

[Laszlo Gombos]

Comment on attachment 154680 [details]
surpress unused parameters warning.

r=me.

[WebKit Review Bot]

Comment on attachment 154680 [details]
surpress unused parameters warning.

Clearing flags on attachment: 154680

Committed r123781: <http://trac.webkit.org/changeset/123781>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chang Shu]

build is broken again when svg is disabled.

[Chang Shu]

Created attachment 184759 [details]
fix patch

[WebKit Review Bot]

Comment on attachment 184759 [details]
fix patch

Rejecting attachment 184759 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-04', 'validate-changelog', '--non-interactive', 184759, '--port=chromium-xvfb']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

/mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/16121415

[Chang Shu]

Created attachment 184763 [details]
fix patch

[Eric Seidel (no email)]

Comment on attachment 184763 [details]
fix patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184763&action=review

> Source/WebCore/css/StyleResolver.cpp:4110
> -#if ENABLE(SVG)
>      default:
> +#if ENABLE(SVG)

This part should not be needed.  We're missing some non-svg properties from this list which are being masked by the presence of applySVGProperty. :(

[Eric Seidel (no email)]

I'm not saying it's your fault.  But the build break for non-svg is just being masked in teh SVG case, and is a real bug.

[Antti Koivisto]

I would be nice to get rid of the no-SVG build entirely. I guess everyone ships it.

[Eric Seidel (no email)]

before SVG came along, all CSS properties were handled in applyProperty().  For various reasons, SVG property handling was made separate in svgApplyProperty, but they share in enum, thus we had to add that default: clause.  Now, since SVG has been enabled for so long, someone must have added a new proprty and failed to support it in applyProperty like they should.  We didn't catch it, because both applyProperty and applySVGPrperty use default: clauses since neither of them can handle the whole list of properties in their switch. :(

[WebKit Review Bot]

Comment on attachment 184763 [details]
fix patch

Clearing flags on attachment: 184763

Committed r140845: <http://trac.webkit.org/changeset/140845>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chang Shu]

(In reply to comment #12)
> before SVG came along, all CSS properties were handled in applyProperty().  For various reasons, SVG property handling was made separate in svgApplyProperty, but they share in enum, thus we had to add that default: clause.  Now, since SVG has been enabled for so long, someone must have added a new proprty and failed to support it in applyProperty like they should.  We didn't catch it, because both applyProperty and applySVGPrperty use default: clauses since neither of them can handle the whole list of properties in their switch. :(

I agree with you guys. The correct way for the switch should be:

case <a non-svg case>:
 blah-blah;
#if ENABLE(SVG)
case <a svg case>:
 blah-blah;
#endif
default:
 assert(0);

But my fix at least didn't introduce new bugs. :) I just wanted to commit it so I don't have to patch it every time. Thanks.