Summary: | Disable all the SVG features for WebKit/Qt if ENABLE_SVG=0 | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Laszlo Gombos <laszlo.gombos> | ||||||||
Component: | WebKit Qt | Assignee: | Laszlo Gombos <laszlo.gombos> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | eric, hausmann, zecke | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Laszlo Gombos
2009-03-18 19:07:25 PDT
Created attachment 28744 [details]
Patch to disable all the SVG features for WebKit/Qt if ENABLE_SVG=0
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.
(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. (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. Created attachment 28973 [details]
Revised patch based on feedback !
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. (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 on attachment 28973 [details]
Revised patch based on feedback !
This patch has rotted and no longer applies. marking r-
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.
Created attachment 29904 [details]
Update patch to apply to trunk
Comment on attachment 29904 [details]
Update patch to apply to trunk
Looks fine.
Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/WebCore.pro Committed r43281 |