Bug 92393

Summary: Build broken when svg is disabled.
Product: WebKit Reporter: Chang Shu <cshu>
Component: SVGAssignee: Chang Shu <cshu>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, koivisto, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
surpress unused parameters warning.
none
fix patch
webkit.review.bot: commit-queue-
fix patch none

Description Chang Shu 2012-07-26 10:11:49 PDT
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
Comment 1 Chang Shu 2012-07-26 10:14:26 PDT
Created attachment 154680 [details]
surpress unused parameters warning.
Comment 2 Laszlo Gombos 2012-07-26 10:39:42 PDT
Comment on attachment 154680 [details]
surpress unused parameters warning.

r=me.
Comment 3 WebKit Review Bot 2012-07-26 12:10:34 PDT
Comment on attachment 154680 [details]
surpress unused parameters warning.

Clearing flags on attachment: 154680

Committed r123781: <http://trac.webkit.org/changeset/123781>
Comment 4 WebKit Review Bot 2012-07-26 12:10:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Chang Shu 2013-01-25 08:34:41 PST
build is broken again when svg is disabled.
Comment 6 Chang Shu 2013-01-25 08:36:05 PST
Created attachment 184759 [details]
fix patch
Comment 7 WebKit Review Bot 2013-01-25 08:43:50 PST
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
Comment 8 Chang Shu 2013-01-25 09:21:29 PST
Created attachment 184763 [details]
fix patch
Comment 9 Eric Seidel (no email) 2013-01-25 09:35:57 PST
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. :(
Comment 10 Eric Seidel (no email) 2013-01-25 09:36:40 PST
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.
Comment 11 Antti Koivisto 2013-01-25 10:00:35 PST
I would be nice to get rid of the no-SVG build entirely. I guess everyone ships it.
Comment 12 Eric Seidel (no email) 2013-01-25 10:02:59 PST
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. :(
Comment 13 WebKit Review Bot 2013-01-25 10:12:37 PST
Comment on attachment 184763 [details]
fix patch

Clearing flags on attachment: 184763

Committed r140845: <http://trac.webkit.org/changeset/140845>
Comment 14 WebKit Review Bot 2013-01-25 10:12:41 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Chang Shu 2013-01-25 11:13:42 PST
(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.