Closed
Bug 585020
Opened 14 years ago
Closed 14 years ago
Remove build time option to disable svg
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(blocking2.0 -)
RESOLVED
FIXED
mozilla2.0b8
Tracking | Status | |
---|---|---|
blocking2.0 | --- | - |
People
(Reporter: mounir, Assigned: mounir)
References
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
dbaron
:
review+
benjamin
:
approval2.0-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
benjamin
:
approval2.0+
|
Details | Diff | Splinter Review |
According to David Baron, tests should not pass if SVG isn't enabled in gecko [1]. So, I think we should consider removing this compilation option considering disabling svg is considered as 'invalid' and having svg enabled should not hurt the user.
[1] bug 579351 comment 7
Comment 1•14 years ago
|
||
I think this should be raised on mozilla.dev.platform to warn folks this is coming.
Summary: Remove svg compilation option → Remove build time option to disable svg
Comment 2•14 years ago
|
||
Assignee: nobody → wesongathedeveloper
Status: NEW → ASSIGNED
Attachment #463983 -
Flags: review?(dbaron)
Comment 3•14 years ago
|
||
wasn't sure if the #ifdefs all over the place are still needed for other purposes?
Comment 4•14 years ago
|
||
In dom/Makefile.in,
@@ -61,19 +61,17 @@ DIRS = \
interfaces/xul \
interfaces/storage \
interfaces/json \
interfaces/offline \
interfaces/geolocation \
interfaces/threads \
$(NULL)
-ifdef MOZ_SVG
DIRS += interfaces/svg
-endif
could be
interfaces/xul \
interfaces/storage \
interfaces/json \
interfaces/offline \
interfaces/geolocation \
interfaces/threads \
+ interfaces/svg \
$(NULL)
-ifdef MOZ_SVG
-DIRS += interfaces/svg
-endif
(Also, should we kill --disable-mathml as well?)
Comment 5•14 years ago
|
||
I think mathml would be a different bug.
Comment 6•14 years ago
|
||
(In reply to comment #5)
> I think mathml would be a different bug.
Yes, certainly.
Also, looks like some new ifdefs were added:
http://hg.mozilla.org/mozilla-central/rev/c799b49b3f26
http://hg.mozilla.org/mozilla-central/rev/4a50f3c34d5a
Comment 7•14 years ago
|
||
Comment on attachment 463983 [details] [diff] [review]
Patch
>-#if defined(MOZ_SVG) || defined(MOZ_MATHML)
>+#if defined(MOZ_MATHML)
> GK_ATOM(xor_, "xor")
> #endif
You should just remove this #ifdef and #endif; otherwise you'll break
mathml-disabled builds since SVG code needs this.
In layout/build/Makefile.in, could you sort the added line properly within
LOCAL_INCLUDES?
In layout/build/nsContentDLF.h, could you just put the SVG item into
CONTENTDLF_CATEGORIES and remove CONTENTDLF_SVG_CATEGORIES.
In nsFrame.cpp:
>- * parent. This function is used so that if MOZ_SVG is not defined, we still
>- * have unified control paths in the InvalidateInternal chain.
>+ * parent. This function is used so that we have unified control paths
>+ * in the InvalidateInternal chain for both SVG and regular content.
Leave the comment instead, and file a followup bug on removing the function?
Same in nsIFrame.h
In toolkit-makefiles.sh, could you just add the items to the appropriate
layout/content/dom lists rather than making MAKEFILES_svg?
r=dbaron with that
(It might make sense to land everything except configure.in and
autoconf.mk.in first, and then remove those after double-checking that
no new MOZ_SVG checks slipped in. But don't leave too long between
landing the two either. Then again, landing it all at once is probably
fine too.)
Attachment #463983 -
Flags: review?(dbaron) → review+
Can we get an updated patch here? Would be nice to get this into 2.0. Shipping broken configure options isn't ideal.
Comment 9•14 years ago
|
||
Saint Wesonga, any updates here? (Do you think you'll get a chance to finish this off, or should someone else pick this up? As khuey said, it'd be great to see this make it into Gecko 2.0 / Firefox 4.0!)
Comment 10•14 years ago
|
||
Updated patch coming in the next day or two...
Comment 12•14 years ago
|
||
khuey: <humor type="wry">That wasn't very nice of you, duping my bug while I was finding out who broke compiling with --disable-svg (bz, bug 584293).</humor>
blocking2.0: --- → ?
Comment 13•14 years ago
|
||
>
> Leave the comment instead, and file a followup bug on removing the function?
>
Filed bug 597882.
Attachment #463983 -
Attachment is obsolete: true
Attachment #476667 -
Flags: review?(dbaron)
Attachment #476667 -
Flags: approval2.0?
Updated•14 years ago
|
blocking2.0: ? → -
Comment 14•14 years ago
|
||
Comment on attachment 476667 [details] [diff] [review]
Patch
r=dbaron, although the sorting in layout/build/Makefile.in was better before. I must have meant to comment on a different file.
Attachment #476667 -
Flags: review?(dbaron) → review+
Updated•14 years ago
|
Attachment #476667 -
Flags: approval2.0? → approval2.0-
Assignee | ||
Comment 15•14 years ago
|
||
At least we should remove the option in the configure.in, shouldn't we? There are no risks and it will prevent users to try building Gecko without SVG support.
Assignee | ||
Comment 16•14 years ago
|
||
This patch is only removing the build option (and keep MOZ_SVG defined and defaulted to 1) so we prevent users to build Gecko with --disable-svg because it will not work but we don't take the risk of taking a huge patch so late in the beta process.
The dead code could be removed after branching.
Attachment #490560 -
Flags: review?(khuey)
Attachment #490560 -
Flags: approval2.0?
Comment on attachment 490560 [details] [diff] [review]
Remove the build option only
Hardcode MOZ_SVG=1 in autoconf.mk.in too.
Attachment #490560 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 18•14 years ago
|
||
Oups, I forgot to refresh the patch. I didn't meant to remove MOZ_SVG from configure.in.
In addition, I've added AC_DEFINE(MOZ_SVG) too in case of something depend on it as requested by khuey on IRC.
r=khuey
Attachment #490560 -
Attachment is obsolete: true
Attachment #490560 -
Flags: approval2.0?
Assignee | ||
Updated•14 years ago
|
Attachment #490567 -
Flags: approval2.0?
Assignee | ||
Comment 19•14 years ago
|
||
Benjamin, I think this patch might be much safer than the previous one: it's only removing the build option and keep MOZ_SVG set to 1. We will be able to remove the dead code after branching.
Updated•14 years ago
|
Attachment #490567 -
Flags: approval2.0? → approval2.0+
Assignee | ||
Comment 20•14 years ago
|
||
Pushed:
http://hg.mozilla.org/mozilla-central/rev/f9939057f8a1
Please use bug 614515 for removing the dead code and the useless conditions.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Assignee | ||
Updated•14 years ago
|
Assignee: wesongathedeveloper → mounir.lamouri
Updated•14 years ago
|
Attachment #490567 -
Attachment description: Remove the build option only → Remove the build option only
[Checked in: See comment 20]
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•