698630 - Remove --disable-smil build option and "#ifdef MOZ_SMIL" wrappers

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

SMIL support seems to have stuck, so I think we can safely remove the "--disable-smil" build option and just always build with SMIL support.

(We don't produce any builds with --disable-smil, and keeping them buildable has a small but nonzero maintenance cost -- e.g. bug 698534 and earlier bugs)

Note that we do have an about:config pref for SMIL support as well (which I'm not proposing we remove), so anyone who wants to disable SMIL can just use that.

So to fix this bug, we'd want to remove the --disable-smil chunk from configure.in, as well as basically also remove all of these #ifdef lines:
http://mxr.mozilla.org/mozilla-central/search?string=MOZ_SMIL

Comment 1

[Matheus Kerschbaum]

Attachment: patch

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 571509 [details] [diff] [review]
patch

Generally looks great -- thanks for taking the time to do this!

Just a few nit-ish comments:

>--- a/content/smil/Makefile.in
>+EXPORTS	= \
>+	nsSMILKeySpline.h \
>+	nsISMILAnimationElement.h \
>+	nsISMILAttr.h \
>+	nsISMILType.h \
>+	nsSMILAnimationController.h \
>+	nsSMILCompositorTable.h \
>+	nsSMILCSSProperty.h \
>+	nsSMILKeySpline.h \
>+	nsSMILMappedAttribute.h \
>+	nsSMILMilestone.h \
>+	nsSMILTimeContainer.h \
>+	nsSMILTypes.h \
>+	$(NULL)
[...(lots of other lines in between here)...]
>-EXPORTS		+= \
>-	  nsISMILAnimationElement.h \
>-	  nsISMILAttr.h \
>-	  nsISMILType.h \
>-	  nsSMILAnimationController.h \
>-	  nsSMILCompositorTable.h \
>-	  nsSMILCSSProperty.h \
>-	  nsSMILKeySpline.h \
>-	  nsSMILMappedAttribute.h \
>-	  nsSMILMilestone.h \
>-	  nsSMILTimeContainer.h \
>-	  nsSMILTypes.h \
>-	  $(NULL)
>-

So this moves EXPORTS to the top of the file and reindents it.  I'd actually prefer we leave it where it is -- unless there's a clear benefit, best not to reshuffle large chunks of the file and mix up hg blame.

Also: you've added nsSMILKeySpline.h at the top there, but it's actually already in the list! :)  (oops, I guess we were exporting it twice before in SMIL-enabled builds...)

So to sum up this EXPORTS chunk -- from the original code, you should be able to just change the "+=" to an "=" and otherwise leave this list completely untouched.

>+CPPSRCS	= \
>+	nsSMILKeySpline.cpp \
> 	nsDOMTimeEvent.cpp \
> 	nsSMILAnimationController.cpp \
> 	nsSMILAnimationFunction.cpp \
> 	nsSMILCompositor.cpp \
> 	nsSMILCSSProperty.cpp \
> 	nsSMILCSSValueType.cpp \
> 	nsSMILFloatType.cpp \

Looks like this list was in alphabetical order -- could you insert nsSMILKeySpline.cpp further down, to preserve that?

>+++ b/content/svg/content/src/SVGAnimatedPreserveAspectRatio.cpp
>   if (!mIsAnimated) {
>     mAnimVal = mBaseVal;
>   }
>-#ifdef MOZ_SMIL
>   else {
>     aSVGElement->AnimationNeedsResample();
>   }
>-#endif

Nit: I'd slightly prefer that we bump the "else {" up to be on the same line as the "}" in chunks like this, but I don't feel strongly enough to insist on it.  While both styles exist in mozilla code, I think "same-line-else" is much more prevalent & generally preferred.

I think there are 22 instances of this in the patch (from grepping for "  else {").  It'd be great if you could fix those, but it'd also be fine to leave them as-is, if you'd rather.

>+++ b/dom/interfaces/svg/Makefile.in
> 		nsIDOMSVGTransformable.idl \
> 		nsIDOMSVGTSpanElement.idl \
> 		nsIDOMSVGURIReference.idl \
> 		nsIDOMSVGUnitTypes.idl \
> 		nsIDOMSVGUseElement.idl \
> 		nsIDOMSVGViewSpec.idl \
> 		nsIDOMSVGZoomAndPan.idl \
> 		nsIDOMSVGZoomEvent.idl \
>-		$(NULL)
>-
>-ifdef MOZ_SMIL
>-XPIDLSRCS	+= \
> 		nsIDOMSVGAnimateElement.idl \
> 		nsIDOMSVGAnimateTransformElement.idl \
> 		nsIDOMSVGAnimateMotionElement.idl \
> 		nsIDOMSVGAnimationElement.idl \
> 		nsIDOMSVGMpathElement.idl \
> 		nsIDOMSVGSetElement.idl \
> 		$(NULL)

I think this XPIDLSRCS list was in sorted order -- could you interleave the SMIL-related lines to preserve that, rather than appending them at the end?  (Your existing patch is nice for hg-blame-preservation, but preserving the existing sorted list order trumps that, I think.)

>+++ b/layout/style/nsDOMCSSAttrDeclaration.cpp
>-nsDOMCSSAttributeDeclaration::nsDOMCSSAttributeDeclaration(dom::Element* aElement
>-#ifdef MOZ_SMIL
>-                                                           , bool aIsSMILOverride
>-#endif // MOZ_SMIL
>-                                                           )
>+nsDOMCSSAttributeDeclaration::nsDOMCSSAttributeDeclaration(dom::Element* aElement, bool aIsSMILOverride)

Wrap "bool aIsSMILOverride)" to its own line here (as it was before but without the naked-comma ugliness), so this line isn't quite so crazy-long. :)

Comment 3

[Matheus Kerschbaum]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> So this moves EXPORTS to the top of the file and reindents it.  I'd actually
> prefer we leave it where it is -- unless there's a clear benefit, best not
> to reshuffle large chunks of the file and mix up hg blame.
> 
> Also: you've added nsSMILKeySpline.h at the top there, but it's actually
> already in the list! :)  (oops, I guess we were exporting it twice before in
> SMIL-enabled builds...)
> 
> So to sum up this EXPORTS chunk -- from the original code, you should be
> able to just change the "+=" to an "=" and otherwise leave this list
> completely untouched.
>

I'm told that "Messes up hg blame" is a poor excuse for not doing cleanups, and I think the makefile looks clearer with EXPORTS near the top so I left that change in. I also removed the duplicate nsSMILKeySpline.h entry.

> >+CPPSRCS	= \
> >+	nsSMILKeySpline.cpp \
> > 	nsDOMTimeEvent.cpp \
> > 	nsSMILAnimationController.cpp \
> > 	nsSMILAnimationFunction.cpp \
> > 	nsSMILCompositor.cpp \
> > 	nsSMILCSSProperty.cpp \
> > 	nsSMILCSSValueType.cpp \
> > 	nsSMILFloatType.cpp \
> 
> Looks like this list was in alphabetical order -- could you insert
> nsSMILKeySpline.cpp further down, to preserve that?

Done.

> >+++ b/content/svg/content/src/SVGAnimatedPreserveAspectRatio.cpp
> >   if (!mIsAnimated) {
> >     mAnimVal = mBaseVal;
> >   }
> >-#ifdef MOZ_SMIL
> >   else {
> >     aSVGElement->AnimationNeedsResample();
> >   }
> >-#endif
> 
> Nit: I'd slightly prefer that we bump the "else {" up to be on the same line
> as the "}" in chunks like this, but I don't feel strongly enough to insist
> on it.  While both styles exist in mozilla code, I think "same-line-else" is
> much more prevalent & generally preferred.
> 
> I think there are 22 instances of this in the patch (from grepping for " 
> else {").  It'd be great if you could fix those, but it'd also be fine to
> leave them as-is, if you'd rather.

I'm lazy and the patch is big enough as it is so I didn't do this.

> >+++ b/dom/interfaces/svg/Makefile.in
> > 		nsIDOMSVGTransformable.idl \
> > 		nsIDOMSVGTSpanElement.idl \
> > 		nsIDOMSVGURIReference.idl \
> > 		nsIDOMSVGUnitTypes.idl \
> > 		nsIDOMSVGUseElement.idl \
> > 		nsIDOMSVGViewSpec.idl \
> > 		nsIDOMSVGZoomAndPan.idl \
> > 		nsIDOMSVGZoomEvent.idl \
> >-		$(NULL)
> >-
> >-ifdef MOZ_SMIL
> >-XPIDLSRCS	+= \
> > 		nsIDOMSVGAnimateElement.idl \
> > 		nsIDOMSVGAnimateTransformElement.idl \
> > 		nsIDOMSVGAnimateMotionElement.idl \
> > 		nsIDOMSVGAnimationElement.idl \
> > 		nsIDOMSVGMpathElement.idl \
> > 		nsIDOMSVGSetElement.idl \
> > 		$(NULL)
> 
> I think this XPIDLSRCS list was in sorted order -- could you interleave the
> SMIL-related lines to preserve that, rather than appending them at the end? 
> (Your existing patch is nice for hg-blame-preservation, but preserving the
> existing sorted list order trumps that, I think.)

Done.

> >+++ b/layout/style/nsDOMCSSAttrDeclaration.cpp
> >-nsDOMCSSAttributeDeclaration::nsDOMCSSAttributeDeclaration(dom::Element* aElement
> >-#ifdef MOZ_SMIL
> >-                                                           , bool aIsSMILOverride
> >-#endif // MOZ_SMIL
> >-                                                           )
> >+nsDOMCSSAttributeDeclaration::nsDOMCSSAttributeDeclaration(dom::Element* aElement, bool aIsSMILOverride)
> 
> Wrap "bool aIsSMILOverride)" to its own line here (as it was before but
> without the naked-comma ugliness), so this line isn't quite so crazy-long. :)

Done.

Comment 4

[Matheus Kerschbaum]

Attachment: patch

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 571549 [details] [diff] [review]
patch

(In reply to Matheus Kerschbaum from comment #3)
> I'm told that "Messes up hg blame" is a poor excuse for not doing cleanups,
> and I think the makefile looks clearer with EXPORTS near the top so I left
> that change in.

Agreed that "messes up hg blame" shouldn't prevent us from doing _useful_ cleanup -- my concern here was whether or not moving a large list from one end of the file to the other was actually a useful change.

I skimmed some other Makefiles earlier today and found examples with both orderings (EXPORTS before vs. after CPPSRCS), so it didn't seem like either way is particularly established as "correct" -- hence, I wasn't sure there was any compelling case for the move.

However, after a little more Makefile-skimming[1], it appears that EXPORTS-at-the-top is the more common ordering, so I suppose it's reasonable to conform to that pattern.  (I agree that it makes more intuitive sense, too.)

So, r=me.  Thanks again for taking care of this!

[1] an uncscientific survey of the first few Makefiles in an MXR search for "EXPORTS ="

Comment 6

[Matheus Kerschbaum]

Attachment: patch for checkin

Updated to tip.

Comment 7

[Ed Morley [:emorley]]

https://tbpl.mozilla.org/?tree=Try&rev=37490de18b94

:-)

Comment 8

[Ed Morley [:emorley]]

Either this or the other changeset in the comment 38 try push turned linux M1 permaorange, so pushing each separately, to work out which:
https://tbpl.mozilla.org/?tree=Try&rev=8d6d7098cb04

Comment 9

[Daniel Holbert [:dholbert]]

(s/comment 38/comment 7/)  :)

Comment 10

[Ed Morley [:emorley]]

So strangely enough the 4-in-a-row linux opt M1 orange of comment 7 has appeared in neither of the try runs for the two changesets involved. Who knows what was going on, but seems safe to push anyway.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8248dbffd645

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8248dbffd645

Comment 12

[Eric Shepherd [:sheppy]]

This has been listed on Firefox 10 for developers.

Comment 13

[Justin Wood (:Callek)]

We're missing one piece of relatively important pieces (afaict) here http://mxr.mozilla.org/comm-central/source/mozilla/b2g/installer/package-manifest.in#526

Comment 14

[Daniel Holbert [:dholbert]]

Thanks! Fixed:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/9ffa2b1718fb

(for the record, looks like that one straggling line was from a "hg cp" that happened (locally) before this was fixed, but landed about a month after this was fixed.)

Comment 15

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/9ffa2b1718fb