Bug 24693 – Disable all the SVG features for WebKit/Qt if ENABLE_SVG=0

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Laszlo Gombos]

This might not be a bug per se; this is a request to have a simple way of disabling all the SVG features for WebKit/Qt. A similar build logic exist for Gtk builds (see ChangeSet r33926 in configure.ac), this is a request to bring the Qt build up-to-par with Gtk.

[Laszlo Gombos]

Created attachment 28744 [details]
Patch to disable all the SVG features for WebKit/Qt if ENABLE_SVG=0

[Holger Freyther]

Comment on attachment 28744 [details]
Patch to disable all the SVG features for WebKit/Qt if ENABLE_SVG=0

Looks sensible and will work with the below macros. Another way would be to put the ENABLE_SVG_* macros into a block...

e.g. contains(DEFINES, ENABLE_SVG=1) {
   !contains(FOO): DEFINES += ENABLE_FOO=1
}

but I would have to check the qmake manual... it would be nice if you could do a follow up.

[Laszlo Gombos]

(In reply to comment #2)
> (From update of attachment 28744 [details] [review])
> Looks sensible and will work with the below macros. Another way would be to put
> the ENABLE_SVG_* macros into a block...
> 
> e.g. contains(DEFINES, ENABLE_SVG=1) {
>    !contains(FOO): DEFINES += ENABLE_FOO=1
> }
> 
> but I would have to check the qmake manual... it would be nice if you could do
> a follow up.
> 

Thanks for the review !

It seems to me that one of the goals of the build system is to make sure that all the ENABLE_FOO flags are defined (either to 0 or 1) instead of leaving them undefined (as an example ENABLE_WML is set to 0 if it is not defined otherwise).

The patch originally proposed maintains this goal, but the suggestion above does not maintain this goal and will leave ENABLE_FOO undefined if ENABLE_SVG is set to 0. 

This might be a matter of taste, but I think that the original patch is more consistent with the existing code.

[Holger Freyther]

(In reply to comment #3)

> This might be a matter of taste, but I think that the original patch is more
> consistent with the existing code.

hehe, you can always add an else { } makes this even more symmetric, minor details though.

[Laszlo Gombos]

Created attachment 28973 [details]
Revised patch based on feedback !

[Holger Freyther]

Comment on attachment 28973 [details]
Revised patch based on feedback !


> +!contains(DEFINES, ENABLE_SVG=0) {
> +    !contains(DEFINES, ENABLE_SVG=.): DEFINES += ENABLE_SVG=1

this looks a bit weird but should still do what you wanted.

[Laszlo Gombos]

(In reply to comment #6)
> (From update of attachment 28973 [details] [review])
> > +!contains(DEFINES, ENABLE_SVG=0) {
> > +    !contains(DEFINES, ENABLE_SVG=.): DEFINES += ENABLE_SVG=1
> this looks a bit weird but should still do what you wanted.

The intention was not to define ENABLE_SVG=1 one more time if it was already defined.

[Eric Seidel (no email)]

Comment on attachment 28973 [details]
Revised patch based on feedback !

This patch has rotted and no longer applies.  marking r-

[Eric Seidel (no email)]

Comment on attachment 28744 [details]
Patch to disable all the SVG features for WebKit/Qt if ENABLE_SVG=0

Clearing review flag to remove from commit queue.

[Laszlo Gombos]

Created attachment 29904 [details]
Update patch to apply to trunk

[Eric Seidel (no email)]

Comment on attachment 29904 [details]
Update patch to apply to trunk

Looks fine.

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/WebCore.pro
Committed r43281