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

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Bartłomiej Brzozowiec (BartZilla)]

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

Comment 1

[Bartłomiej Brzozowiec (BartZilla)]

Attachment: adding #ifdef MOZ_SVG in nsStyleAnimation::ExtractComputedValue()

Quick fix that made nsStyleAnimation.cpp compiling. I'm not sure this is a proper solution, someone who know this code should review this.

Comment 2

[Bartłomiej Brzozowiec (BartZilla)]

Attachment: adding #ifdef MOZ_SMIL in few places to compile nsTransitionManager.cpp

This is truly lame and dirty hack to make compilation going. Someone who knows this code should prepare a proper fix.

Comment 3

[Bartłomiej Brzozowiec (BartZilla)]

Adding dbaron@ to CC list as he is the last person that was changing these files.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 6

[Bartłomiej Brzozowiec (BartZilla)]

Attachment: fixing nsStyleAnimation.cpp errors - second try

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

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 407545 [details] [diff] [review]
fixing nsStyleAnimation.cpp errors - second try

I meant ifdef it in nsCSSProps.h

Comment 8

[Daniel Holbert [:dholbert]]

(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.

Comment 9

[Bartłomiej Brzozowiec (BartZilla)]

Attachment: fixing nsStyleAnimation.cpp errors - third try; also patching nsCSSProps.h

(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.).

Comment 10

[Daniel Holbert [:dholbert]]

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.

Comment 11

[Daniel Holbert [:dholbert]]

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.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Who uses these options such that they're worth the energy to maintain?

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: ifdef MOZ_SVG fixes for building nsStyleAnimation.cpp

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: fix for builds with "--disable-smil" [landed: c21]

Comment 15

[Daniel Holbert [:dholbert]]

(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.

Comment 16

[Robert Longson [:longsonr]]

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.

Comment 17

[Robert Longson [:longsonr]]

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.

Comment 18

[Robert Longson [:longsonr]]

Attachment: alternative MOZ_SVG patch

Comment 19

[Bartłomiej Brzozowiec (BartZilla)]

(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.

Comment 20

[Daniel Holbert [:dholbert]]

(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 21

[Daniel Holbert [:dholbert]]

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

Comment 22

[Robert Longson [:longsonr]]

(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.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 407716 [details] [diff] [review]
alternative MOZ_SVG patch

r=dbaron.  Sorry for the delay.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 25

[Robert Longson [:longsonr]]

Attachment: update to tip

Comment 26

[Robert Longson [:longsonr]]

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.