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)
171.37 KB,
patch
|
dbaron
:
review+
benjamin
:
approval2.0-
|
Details | Diff | Splinter Review |
1.47 KB,
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 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 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•13 years ago
|
Attachment #490567 -
Attachment description: Remove the build option only → Remove the build option only
[Checked in: See comment 20]
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•