Closed Bug 523576 Opened 10 years ago Closed 10 years ago

trunk doesn't compile with --disable-smil/--disable-svg

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bartml, Assigned: longsonr)

References

Details

Attachments

(4 files, 4 obsolete files)

User-Agent:       Konqueror
Build Identifier: 

Compiling trunk with --disable-smil and --disable-svg results in errors in two files from layout/style: nsStyleAnimation.cpp and nsTransitionManager.cpp.

nsStyleAnimation.cpp:
/home/users/firefox/mozhg/src/layout/style/nsStyleAnimation.cpp: In static member function 'static PRBool nsStyleAnimation::ExtractComputedValue(nsCSSProperty, nsStyleContext*, nsStyleAnimation::Value&)':
/home/users/firefox/mozhg/src/layout/style/nsStyleAnimation.cpp:802: error: expected initializer before '&' token
/home/users/firefox/mozhg/src/layout/style/nsStyleAnimation.cpp:805: error: 'paint' was not declared in this scope
/home/users/firefox/mozhg/src/layout/style/nsStyleAnimation.cpp:805: error: 'eStyleSVGPaintType_Color' was not declared in this scope
/home/users/firefox/mozhg/src/layout/style/nsStyleAnimation.cpp:809: error: 'paint' was not declared in this scope
/home/users/firefox/mozhg/src/layout/style/nsStyleAnimation.cpp:809: error: 'eStyleSVGPaintType_None' was not declared in this scope

nsTransitionManager.cpp
/home/users/firefox/mozhg/src/layout/style/nsTransitionManager.cpp:50:29: error: nsSMILKeySpline.h: Nie ma takiego pliku ani katalogu ((<-it means: "No such file or directory"))
/home/users/firefox/mozhg/src/layout/style/nsTransitionManager.cpp:76: error: 'nsSMILKeySpline' does not name a type
/home/users/firefox/mozhg/src/layout/style/nsTransitionManager.cpp: In member function 'virtual nsresult ElementTransitionsStyleRule::MapRuleInfoInto(nsRuleData*)':
/home/users/firefox/mozhg/src/layout/style/nsTransitionManager.cpp:233: error: 'struct ElementPropertyTransition' has no member named 'mTimingFunction'
/home/users/firefox/mozhg/src/layout/style/nsTransitionManager.cpp: In member function 'void nsTransitionManager::ConsiderStartingTransition(nsCSSProperty, const nsTransition&, nsIContent*, ElementTransitions*&, nsStyleContext*, nsStyleContext*, PRBool*, nsCSSPropertySet*)':
/home/users/firefox/mozhg/src/layout/style/nsTransitionManager.cpp:612: error: 'struct ElementPropertyTransition' has no member named 'mTimingFunction'

I created quick fixes that make compilation going. I will attach them soon (as I apparently can't attach them during filling a new bug...).

Reproducible: Always
Quick fix that made nsStyleAnimation.cpp compiling. I'm not sure this is a proper solution, someone who know this code should review this.
This is truly lame and dirty hack to make compilation going. Someone who knows this code should prepare a proper fix.
Adding dbaron@ to CC list as he is the last person that was changing these files.
Comment on attachment 407528 [details] [diff] [review]
adding #ifdef MOZ_SMIL in few places to compile nsTransitionManager.cpp

This is unacceptable; nsSMILKeySpline needs to be accessible even in --disable-smil builds.
Comment on attachment 407526 [details] [diff] [review]
adding #ifdef MOZ_SVG in nsStyleAnimation::ExtractComputedValue()

This is probably the right approach, except you should ifdef the eStyleAnimType_PaintServer type itself.
Attachment #407526 - Flags: review-
You mean something like this? I can't ifdef it in nsCSSProps.h since it is used in few places in nsStyleAnimation.cpp...

This patch generates warning:
/home/users/firefox/mozhg/src/layout/style/nsStyleAnimation.cpp: In static member function 'static PRBool nsStyleAnimation::ExtractComputedValue(nsCSSProperty, nsStyleContext*, nsStyleAnimation::Value&)':
/home/users/firefox/mozhg/src/layout/style/nsStyleAnimation.cpp:707: warning: enumeration value 'eStyleAnimType_PaintServer' not handled in switch
Attachment #407526 - Attachment is obsolete: true
Attachment #407545 - Flags: review?(dbaron)
Comment on attachment 407545 [details] [diff] [review]
fixing nsStyleAnimation.cpp errors - second try

I meant ifdef it in nsCSSProps.h
Attachment #407545 - Flags: review?(dbaron) → review-
(In reply to comment #7)
> I meant ifdef it in nsCSSProps.h

(Though of course we do also need the ifdef wherever that enum value is used in nsStyleAnimation.cpp, as in attachment 407545 [details] [diff] [review])

> nsTransitionManager.cpp

To get nsSMILKeySpline built into non-SMIL-enabled builds, we'll need some minor Makefile tweaking.  Currently, /mozilla/content/Makefile.in completely skips the 'smil' subdirectory if SMIL is disabled, but I guess we want to change this to use more fine-grained file-skipping in /mozilla/content/smil/Makefile.in

> Someone who knows
> this code should prepare a proper fix.

BartZilla, I'd be happy to take a crack at this -- but I don't want to stomp on you if you're working on a patch.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to comment #8)
> 
> BartZilla, I'd be happy to take a crack at this -- but I don't want to stomp on
> you if you're working on a patch.

Feel free to take it, so far I was only working on nsStyleAnimation.cpp, I'm attaching the patch right now... It includes requested change in nsCSSProps.h. It also fixes the problem that appeared today ("nsStyleAnimation.cpp:891: error: 'eCSSProperty_stroke_dasharray' was not declared in this scope" etc.).
Attachment #407545 - Attachment is obsolete: true
Attachment #407610 - Flags: review?(dbaron)
Ok -- from a quick glance, your latest nsStyleAnimation patch looks ok to me, so I'll write another patch for getting nsSMILKeySplines built with --disable-smil.
Attachment #407610 - Flags: superreview?(bzbarsky)
Attachment #407610 - Flags: review?(dholbert)
Attachment #407610 - Flags: review?(dbaron)
Attachment #407610 - Flags: review?(dholbert) → review+
Comment on attachment 407610 [details] [diff] [review]
fixing nsStyleAnimation.cpp errors - third try; also patching nsCSSProps.h

>+#ifdef MOZ_SVG
>       if (nsCSSProps::kAnimTypeTable[aProperty] == eStyleAnimType_PaintServer) {
>         NS_ABORT_IF_FALSE(nsCSSProps::kTypeTable[aProperty] ==
>                             eCSSType_ValuePair, "type mismatch");
>         static_cast<nsCSSValuePair*>(aSpecifiedValue)->
>           SetBothValuesTo(nsCSSValue(eCSSUnit_None));
>       } else {
>         NS_ABORT_IF_FALSE(nsCSSProps::kTypeTable[aProperty] == eCSSType_Value,
>                           "type mismatch");
>         static_cast<nsCSSValue*>(aSpecifiedValue)->SetNoneValue();
>       }
>+#endif

Wait, sorry -- I didn't look close enough when I was reviewing this before.

In these "nsCSSProps::kAnimTypeTable[aProperty] == eStyleAnimType_PaintServer" checks, we still want contents of the "else" clause to run, when SVG is disabled.

Also, this patch has bitrotted a bit, now that bug 520486 landed.

I'll post updated fixes for both of the issues here, with the above problem addressed.
Attachment #407610 - Flags: review+ → review-
Who uses these options such that they're worth the energy to maintain?
Version: unspecified → Trunk
Attachment #407610 - Attachment is obsolete: true
Attachment #407692 - Flags: review?(dbaron)
Attachment #407610 - Flags: superreview?(bzbarsky)
Attachment #407528 - Attachment is obsolete: true
Attachment #407692 - Attachment description: MOZ_SVG fix for nsStyleAnimation.cpp → ifdef MOZ_SVG fixes for building nsStyleAnimation.cpp
(In reply to comment #12)
> Who uses these options such that they're worth the energy to maintain?

I'm not sure... but IMHO, they're not too bad to maintain, at least in the case of animation-related code.

We shouldn't have any more "--disable-smil" issues after attachment 407693 [details] [diff] [review] -- that patch makes sure we build nsSMILKeySpline, which is the only SMIL code used outside of SVG/SMIL.

And as for --disable-svg, we'll need to keep it in mind when adding more svg-specific datatypes in nsStyleAnimation, which is a minor hassle but doable.
I think we should just remove MOZ_SVG from layout/style (and layout/base) altogether as was suggested in bug 374216 comment 6. I was planning to do that in that bug in order to have the SVG properties come before the NS ones so I'd end up reverting the patch in this bug.
To be clear I'd be happy to submit a removal patch here as I've done most of that while working on the patch for bug 374216 and therefore I'd rather the comment 13 patch not land.
Attachment #407716 - Flags: review?(dbaron)
(In reply to comment #18)
> Created an attachment (id=407716) [details]
> alternative MOZ_SVG patch

FYI: it seems bitrotten:
* patch from file: fix-svg-bug523576-att407716-alt.diff
patching file layout/base/nsStyleConsts.h
patching file layout/style/nsCSSDataBlock.h
patching file layout/style/nsCSSDeclaration.cpp
Hunk #1 succeeded at 1063 (offset -2 lines).
patching file layout/style/nsCSSKeywordList.h
patching file layout/style/nsCSSPropList.h
Hunk #2 FAILED at 3027.
1 out of 2 hunks FAILED

OTOH attachment 407692 [details] [diff] [review] works fine.
(In reply to comment #19)
> (In reply to comment #18)
> > Created an attachment (id=407716) [details] [details]
> > alternative MOZ_SVG patch
>
> FYI: it seems bitrotten:

Only slightly -- it applies fine with fuzz 7 (pass "-F7" to patch when applying).

I'm fine with either strategy (add #ifdef MOZ_SVG to nsStyleAnimation vs remove MOZ_SVG from layout/style & layout/base).

However, note that "alternative MOZ_SVG patch" still leaves a lot of #ifdef MOZ_SVG's behind -- e.g. in nsCSSParser.cpp, nsCSSFrameConstructor.cpp, and nsLayoutUtils.cpp, and a few other places.  Do we need to catch those too, or are they different for some reason?
Comment on attachment 407693 [details] [diff] [review]
fix for builds with "--disable-smil" [landed: c21]

Landed fix for building with --disable-smil:
http://hg.mozilla.org/mozilla-central/rev/62c9beaa202c
Attachment #407693 - Attachment description: ifdef MOZ_SMIL fixes, for building CSS transitions even with --disable-smil → fix for builds with "--disable-smil" [landed: c21]
(In reply to comment #20)
> However, note that "alternative MOZ_SVG patch" still leaves a lot of #ifdef
> MOZ_SVG's behind -- e.g. in nsCSSParser.cpp, nsCSSFrameConstructor.cpp, and
> nsLayoutUtils.cpp, and a few other places.  Do we need to catch those too, or
> are they different for some reason?

I just fixed the css hierarchy i.e. recognition of SVG styles. I left the SVG parser tweaks alone e.g. stroke-width="10e3" (scientific notation and no units). If I did nsSVGFrameConstructor I'd basically have to remove all MOZ_SVG from the build as that would require layout/svg/base/src to exist always - that seems a step too far. I could do something with nsLayoutUtils - depends what dbaron thinks there I guess.
blocking2.0: --- → ?
Comment on attachment 407716 [details] [diff] [review]
alternative MOZ_SVG patch

r=dbaron.  Sorry for the delay.
Attachment #407716 - Flags: review?(dbaron) → review+
Comment on attachment 407692 [details] [diff] [review]
ifdef MOZ_SVG fixes for building nsStyleAnimation.cpp

This patch is OK, but I think I prefer longsonr's approach, so canceling this review request.
Attachment #407692 - Flags: review?(dbaron)
Attached patch update to tipSplinter Review
Blocks: 374216
pushed http://hg.mozilla.org/mozilla-central/rev/b7755e1380c3

Marking in-testsuite- as it's #ifdef changes so the compiled code is basically the same and covered by the existing mochitests.
Status: NEW → RESOLVED
blocking2.0: ? → ---
Closed: 10 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Assignee: nobody → longsonr
You need to log in before you can comment on or make changes to this bug.