541267 - Explicitly unsupport building without SVG in comm-central

Status: RESOLVED FIXED

(MailNews Core :: Build Config, defect)

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) configure part

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) code part

Sync' with Firefox:
*pageInfo.js: not same code, but same idea anyway.
*nsBrowserContentHandler.js: same code.
 (I assume the '#' lines are "implicitly" preprocessed.)
*contentAreaUtils.js: same code.
 But FF has no #ifdef, so I'm not adding them to SM either...

Comment 3

[Justin Wood (:Callek)]

maybe I'm just blind here, but how does MOZ_SVG adding or not adding support MOZ_SMIL or not?

The only def of MOZ_SMIL is in m-c, so we don't require MOZ_SVG here for that.

The only benefit I see so far is the Bv1 patch uses, which I am not sure can't be just solved with some code, or go with the "assumption" that SVG is enabled for now.

Comment 4

[[e-mail]]

(In reply to comment #2)
> Sync' with Firefox:
> *pageInfo.js: not same code, but same idea anyway.
> *nsBrowserContentHandler.js: same code.
>  (I assume the '#' lines are "implicitly" preprocessed.)
They're not.

> *contentAreaUtils.js: same code.
>  But FF has no #ifdef, so I'm not adding them to SM either...
[I don't see any contentAreaUtils changes]

(In reply to comment #3)
> The only benefit I see so far is the Bv1 patch uses, which I am not sure can't
> be just solved with some code, or go with the "assumption" that SVG is enabled
> for now.
Yeah, I'd just assume SVG is enabled.

Comment 5

[Serge Gautherie (:sgautherie)]

(In reply to comment #3)

> how does MOZ_SVG adding or not adding support MOZ_SMIL or not?
> The only def of MOZ_SMIL is in m-c, so we don't require MOZ_SVG here for that.

See bug 541203 and its |if test -z "$MOZ_SVG"; then|.

> The only benefit I see so far is the Bv1 patch uses, which I am not sure can't
> be just solved with some code, or go with the "assumption" that SVG is enabled
> for now.

If SeaMonkey wants to force SVG on/off, I would probably not mind, but I think we should do it explicitly in confvars.sh or the like (if there is a mean to ensure/detect that m-* config is forced to the same value)...
PS: see below.

(In reply to comment #4)

> > *nsBrowserContentHandler.js: same code.
> >  (I assume the '#' lines are "implicitly" preprocessed.)
> They're not.

Would you know how that's done (in Firefox)?

> [I don't see any contentAreaUtils changes]

Yes, no changes because the code is the same and I'm not ifdef'ing it:
http://mxr.mozilla.org/comm-central/search?string=svg&find=%2FcontentAreaUtils%5C.js%24

> Yeah, I'd just assume SVG is enabled.

Then, I think I could simply modify patch A to error out if MOZ_SVG ends up disabled, for (all c-c apps or) SeaMonkey only...
What do you think?

Comment 6

[[e-mail]]

Comment on attachment 422891 [details] [diff] [review]
(Bv1) code part

>     registerType("text/html");
>     registerType("application/vnd.mozilla.xul+xml");
>+#ifdef MOZ_SVG
>     registerType("image/svg+xml");
>+#endif
We don't actually need to #ifdef this, since we test later on whether nsIWebNavigationInfo supports it. In fact, we probably don't need this at all; it only exists to handle loads through nsDocLoader, which are rarely used.

>diff --git a/suite/browser/pageinfo/pageInfo.js b/suite/browser/pageinfo/pageInfo.js
It's not worth patching page info for an unsupported configuration.

Comment 7

[Robert Kaiser]

Comment on attachment 422886 [details] [diff] [review]
(Av1) configure part

Nah, let's just leave out those changes and for SeaMonkey's sake assume that the defaults of the relevant Mozilla branch apply to all our builds.
As long as there is no known good use case of SeaMonkey with turning those SVG/SMIL defaults to non-defaults, I think we should ignore them.

Comment 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 422891 [details] [diff] [review]
(Bv1) code part


(In reply to comment #6)
> >     registerType("image/svg+xml");
> we test later on whether nsIWebNavigationInfo supports it.
> In fact, we probably don't need this at all;
> it only exists to handle loads through nsDocLoader, which are rarely used.

(Please, file a separate bug, if need be.)

Comment 9

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) configure part

Av1, as discussed here and on irc:
be explicit/safe about only supporting enabled svg.

Ftr,
*Calendar has the packaging part only:
 http://mxr.mozilla.org/comm-central/search?string=SVG&find=%2Fcalendar%2F
*Thunderbird seems to have 1-2 additional references:
 http://mxr.mozilla.org/comm-central/search?string=SVG&find=%2Fmail%2F
*SeaMonkey does use svg:
 http://mxr.mozilla.org/comm-central/search?string=SVG&find=%2Fsuite%2F

Comment 10

[Robert Kaiser]

Comment on attachment 423143 [details] [diff] [review]
(Av2) configure part

>+MOZ_SVG=1

Why set a var that we don't really need in comm-central anyhow?

>+  if test "$MOZILLA_1_9_2_BRANCH" = "1"; then
>+  MOZ_SVG=
>+  fi

Erm, does that mean "if we are on 1.9.2, always fail"? I don't think that's what we want...


> dnl ========================================================
>+dnl SVG
>+dnl ========================================================
>+MOZ_ARG_DISABLE_BOOL(svg,
>+[  --disable-svg            Disable SVG support],
>+    MOZ_SVG=,
>+    MOZ_SVG=1 )
>+if test -z "$MOZ_SVG"; then
>+  AC_MSG_ERROR([comm-central does not support disabling SVG.])
>+fi

1) I don't think we need to advertise that option at all in |configure --help|
2) Can we reduce the patch to this block and just check for "--disable-svg" and fail when it's set? I know, it would be easier if we could fail on analysis of the variables the Mozilla build system has set, but that is more difficult. :(

Comment 11

[Serge Gautherie (:sgautherie)]

(In reply to comment #10)

> Why set a var that we don't really need in comm-central anyhow?

Only to handle the following code.

> >+  if test "$MOZILLA_1_9_2_BRANCH" = "1"; then
> 
> Erm, does that mean "if we are on 1.9.2, always fail"? I don't think that's
> what we want...

No, because these are for non-default embedding profiles.
And one could use the --disable-svg option to (try to) override this, fwiw.
(I don't know much about these, so I'm just being cautious.)

> 1) I don't think we need to advertise that option at all in |configure --help|
> 2) Can we reduce the patch to this block and just check for "--disable-svg" and
> fail when it's set?

What is the code to check for an option without advertising it?

NB: While I do agree with you (and thought about it when doing this patch), I would really suggest to take this (not perfect) patch for now, then revisit it after c-1.9.2 branching. (or I'll just add some more ifdef's.)

Comment 12

[Robert Kaiser]

(In reply to comment #11)
> (In reply to comment #10)
> > 1) I don't think we need to advertise that option at all in |configure --help|
> > 2) Can we reduce the patch to this block and just check for "--disable-svg" and
> > fail when it's set?
> 
> What is the code to check for an option without advertising it?

Ted, do you have a pointer for us on that?

Comment 13

[(not currently active) Ted Mielczarek]

I don't know of any way to do that. You can check for variables that are set, but if you use a MOZ_ARG_{ENABLE,DISABLE}_BOOL, it winds up in the help. There might be a way to check, if you grovel in the autoconf source (or generated configure).

Comment 14

[Serge Gautherie (:sgautherie)]

Attachment: (Av3) configure part

Av2, with comment 10 suggestion(s).

I really can't do any better than that.

Comment 15

[Robert Kaiser]

Comment on attachment 423237 [details] [diff] [review]
(Av3) configure part

>+if test "$MOZILLA_1_9_2_BRANCH" = "1"; then
>+
>+MOZ_ARG_DISABLE_BOOL(svg,
>+[  --disable-svg            Disable SVG support],
>+    MOZ_SVG=,
>+    MOZ_SVG=1 )
>+if test -z "$MOZ_SVG"; then
>+  AC_MSG_ERROR([comm-central does not support disabling SVG.])
>+fi
>+
>+else
>+
>+MOZ_ARG_DISABLE_BOOL(svg,
>+[  --disable-svg            Disable SVG support (NB: unsupported option in comm-central)],
>+    AC_MSG_ERROR([comm-central does not support disabling SVG.]))
>+
>+fi

Nit: please add that comment about being unsupported on the 1.9.2 side of the equation as well.

Comment 16

[Serge Gautherie (:sgautherie)]

Attachment: (Av3a) configure part [Checkin: Comment 22]

Av3, with comment 15 suggestion(s).

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 423240 [details] [diff] [review]
(Av3a) configure part
[Checkin: Comment 22]

Well AFAICT this patch has nothing to do with the bug title. I've just spent 15 minutes trying to figure out what this patch is doing and got very close to r- straight off because its so unclear, and I'm still not 100% sure.

>+if test "$MOZILLA_1_9_2_BRANCH" = "1"; then
>+MOZ_SVG=1
>+fi

I don't see any clear explanation as to why setting MOZ_SVG in various places is only needed on the 1.9.2 branch. Just a comment on the bug would do.

> dnl ========================================================
>+dnl SVG
>+dnl ========================================================
>+if test "$MOZILLA_1_9_2_BRANCH" = "1"; then
>+
>+MOZ_ARG_DISABLE_BOOL(svg,
>+[  --disable-svg            Disable SVG support (NB: unsupported option in comm-central)],
>+    MOZ_SVG=,
>+    MOZ_SVG=1 )
>+if test -z "$MOZ_SVG"; then
>+  AC_MSG_ERROR([comm-central does not support disabling SVG.])
>+fi
>+
>+else
>+
>+MOZ_ARG_DISABLE_BOOL(svg,
>+[  --disable-svg            Disable SVG support (NB: unsupported option in comm-central)],
>+    AC_MSG_ERROR([comm-central does not support disabling SVG.]))
>+
>+fi

Why the difference here? What's missing that we can't have the same check on both sides of the ifdef?

Comment 18

[Serge Gautherie (:sgautherie)]

(In reply to comment #17)
> (From update of attachment 423240 [details] [diff] [review])
> Well AFAICT this patch has nothing to do with the bug title. I've just spent 15

Nothing? It's purpose was to restore MOZ_SVG support then use it in bug   	    541203...

> minutes trying to figure out what this patch is doing and got very close to r-
> straight off because its so unclear, and I'm still not 100% sure.

Currently, this patch has morphed to explicitly un-support --disable-svg in c-c.
KaiRo suggested this (for SM) and I'm looking for your confirmation (for TB, ...).

> I don't see any clear explanation as to why setting MOZ_SVG in various places
> is only needed on the 1.9.2 branch. Just a comment on the bug would do.

See comment 11: just to be in sync' with what m-1.9.2 still has, and m-c hasn't anymore.

> Why the difference here? What's missing that we can't have the same check on
> both sides of the ifdef?

MOZ_SVG was not removed from m-1.9.2.

Comment 19

[Mark Banner (:standard8)]

(In reply to comment #18)
> > minutes trying to figure out what this patch is doing and got very close to r-
> > straight off because its so unclear, and I'm still not 100% sure.
> 
> Currently, this patch has morphed to explicitly un-support --disable-svg in
> c-c.
> KaiRo suggested this (for SM) and I'm looking for your confirmation (for TB,
> ...).

Ok, that is a useful clarification. If we're morphing the bug, then it's useful to morph the subject as well ;-)

> > I don't see any clear explanation as to why setting MOZ_SVG in various places
> > is only needed on the 1.9.2 branch. Just a comment on the bug would do.
> 
> See comment 11: just to be in sync' with what m-1.9.2 still has, and m-c hasn't
> anymore.

Well m-c still has MOZ_SVG=1 in its headlines:

http://hg.mozilla.org/mozilla-central/annotate/6d13028da120//configure.in#l4635

It may have removed the embedding profiles, but the initial MOZ_SVG=1 is still there.

> > Why the difference here? What's missing that we can't have the same check on
> > both sides of the ifdef?
> 
> MOZ_SVG was not removed from m-1.9.2.

Can you explain what I'm looking for then? I ask because this:

http://hg.mozilla.org/mozilla-central/annotate/6d13028da120//configure.in#l5967

and this:

http://mxr.mozilla.org/mozilla-central/search?string=MOZ_SVG&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

both say to me that MOZ_SVG is still supported in mozilla-central.

Comment 20

[Serge Gautherie (:sgautherie)]

(In reply to comment #19)

> Ok, that is a useful clarification. If we're morphing the bug, then it's useful
> to morph the subject as well ;-)

It's true that I usually don't bother :-|
In this case, I'll do, if you confirm/review the underlying decision...

> It may have removed the embedding profiles, but the initial MOZ_SVG=1 is still
> there.

Yeah, that's what I meant :-> (sorry)
That is to enable svg by default in m-c, but c-c only cares about this var if it can be unset (which it can't anymore in m-c).

> http://hg.mozilla.org/mozilla-central/annotate/6d13028da120//configure.in#l5967

Yes, that's exactly what I'm porting: except my patch now explicitly unsupport building c-c without svg.

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 423240 [details] [diff] [review]
(Av3a) configure part
[Checkin: Comment 22]

ok, I think I see what's happening now. Thanks.

Comment 22

[Serge Gautherie (:sgautherie)]

Comment on attachment 423240 [details] [diff] [review]
(Av3a) configure part
[Checkin: Comment 22]


http://hg.mozilla.org/comm-central/rev/3a22a577e53d

Comment 23

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1-191) configure part [Checkin: Comment 24]

"approval-thunderbird3.0.2=?":
Zero risk, build config only.

Comment 24

[Serge Gautherie (:sgautherie)]

Comment on attachment 424997 [details] [diff] [review]
(Cv1-191) configure part
[Checkin: Comment 24]


http://hg.mozilla.org/releases/comm-1.9.1/rev/e1546e3394ee

Comment 25

[Mark Banner (:standard8)]

Verified fixed by code inspection & the fact the trees are still green.