Closed Bug 585020 Opened 10 years ago Closed 10 years ago
Remove build time option to disable svg
According to David Baron, tests should not pass if SVG isn't enabled in gecko . 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.  bug 579351 comment 7
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
Assignee: nobody → wesongathedeveloper
Status: NEW → ASSIGNED
Attachment #463983 - Flags: review?(dbaron)
wasn't sure if the #ifdefs all over the place are still needed for other purposes?
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?)
I think mathml would be a different bug.
(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 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.
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!)
Updated patch coming in the next day or two...
Duplicate of this bug: 597796
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: --- → ?
> > Leave the comment instead, and file a followup bug on removing the function? > Filed bug 597882.
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+
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.
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.
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+
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
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.
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: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Assignee: wesongathedeveloper → mounir.lamouri
Attachment #490567 - Attachment description: Remove the build option only → Remove the build option only [Checked in: See comment 20]
You need to log in before you can comment on or make changes to this bug.