Closed Bug 698630 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: dholbert, Assigned: matjk7)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

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
Assignee: nobody → matjk7
Status: NEW → ASSIGNED
Flags: in-testsuite-
Keywords: dev-doc-needed
Attached patch patch (obsolete) — Splinter Review
Attachment #571509 - Flags: review?(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. :)
(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.
Attached patch patch (obsolete) — Splinter Review
Attachment #571509 - Attachment is obsolete: true
Attachment #571509 - Flags: review?(dholbert)
Attachment #571549 - Flags: review?(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 ="
Attachment #571549 - Flags: review?(dholbert) → review+
Keywords: checkin-needed
Updated to tip.
Attachment #571549 - Attachment is obsolete: true
Attachment #571818 - Flags: review+
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
(s/comment 38/comment 7/)  :)
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
Keywords: checkin-needed
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/8248dbffd645
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
This has been listed on Firefox 10 for developers.
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
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.)
You need to log in before you can comment on or make changes to this bug.