Bug 24693 - Disable all the SVG features for WebKit/Qt if ENABLE_SVG=0
Summary: Disable all the SVG features for WebKit/Qt if ENABLE_SVG=0
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Laszlo Gombos
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-03-18 19:07 PDT by Laszlo Gombos
Modified: 2009-05-05 22:47 PDT (History)
3 users (show)

See Also:


Attachments
Patch to disable all the SVG features for WebKit/Qt if ENABLE_SVG=0 (1.30 KB, patch)
2009-03-18 19:11 PDT, Laszlo Gombos
no flags Details | Formatted Diff | Diff
Revised patch based on feedback ! (2.21 KB, patch)
2009-03-26 11:51 PDT, Laszlo Gombos
eric: review-
Details | Formatted Diff | Diff
Update patch to apply to trunk (2.33 KB, patch)
2009-04-30 00:17 PDT, Laszlo Gombos
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Laszlo Gombos 2009-03-18 19:07:25 PDT
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.
Comment 1 Laszlo Gombos 2009-03-18 19:11:56 PDT
Created attachment 28744 [details]
Patch to disable all the SVG features for WebKit/Qt if ENABLE_SVG=0
Comment 2 Holger Freyther 2009-03-25 00:15:31 PDT
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.
Comment 3 Laszlo Gombos 2009-03-25 12:05:03 PDT
(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.

Comment 4 Holger Freyther 2009-03-25 19:50:43 PDT
(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.

Comment 5 Laszlo Gombos 2009-03-26 11:51:52 PDT
Created attachment 28973 [details]
Revised patch based on feedback !
Comment 6 Holger Freyther 2009-04-02 23:16:07 PDT
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.
Comment 7 Laszlo Gombos 2009-04-03 05:37:03 PDT
(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.
Comment 8 Eric Seidel (no email) 2009-04-29 14:45:15 PDT
Comment on attachment 28973 [details]
Revised patch based on feedback !

This patch has rotted and no longer applies.  marking r-
Comment 9 Eric Seidel (no email) 2009-04-29 14:57:02 PDT
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.
Comment 10 Laszlo Gombos 2009-04-30 00:17:30 PDT
Created attachment 29904 [details]
Update patch to apply to trunk
Comment 11 Eric Seidel (no email) 2009-05-03 21:50:19 PDT
Comment on attachment 29904 [details]
Update patch to apply to trunk

Looks fine.
Comment 12 Eric Seidel (no email) 2009-05-05 22:47:31 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/WebCore.pro
Committed r43281