Remove build time option to disable svg

RESOLVED FIXED in mozilla2.0b8

Status

defect
RESOLVED FIXED
9 years ago
Last year

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
mozilla2.0b8
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

9 years ago
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

Comment 2

9 years ago
Posted patch Patch (obsolete) — Splinter Review
Assignee: nobody → wesongathedeveloper
Status: NEW → ASSIGNED
Attachment #463983 - Flags: review?(dbaron)

Comment 3

9 years ago
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!)

Comment 10

9 years ago
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: --- → ?

Updated

9 years ago
Blocks: 597882

Comment 13

9 years ago
Posted 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?

Updated

9 years ago
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+

Updated

9 years ago
Attachment #476667 - Flags: approval2.0? → approval2.0-
Assignee

Comment 15

9 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

9 years ago
Posted 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+
Assignee

Comment 18

9 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

9 years ago
Attachment #490567 - Flags: approval2.0?
Assignee

Comment 19

9 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

9 years ago
Attachment #490567 - Flags: approval2.0? → approval2.0+
Assignee

Updated

9 years ago
Blocks: 614515
Assignee

Comment 20

9 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: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Assignee

Updated

9 years ago
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]

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.