Closed Bug 585020 Opened 10 years ago Closed 10 years ago

Remove build time option to disable svg

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(blocking2.0 -)

RESOLVED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- -

People

(Reporter: mounir, Assigned: mounir)

References

Details

Attachments

(2 files, 2 obsolete files)

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
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
Attached patch Patch (obsolete) — Splinter Review
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...
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: --- → ?
Blocks: 597882
Attached patch PatchSplinter Review
> 
> 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?
blocking2.0: ? → -
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+
Attachment #476667 - Flags: approval2.0? → approval2.0-
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.
Attached patch Remove the build option only (obsolete) — Splinter Review
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+
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?
Attachment #490567 - Flags: approval2.0?
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.
Attachment #490567 - Flags: approval2.0? → approval2.0+
Blocks: 614515
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
Blocks: 617448
Attachment #490567 - Attachment description: Remove the build option only → Remove the build option only [Checked in: See comment 20]
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.