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

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.

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

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

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

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

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

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

Av2, with comment 10 suggestion(s). I really can't do any better than that.

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.

Av3, with comment 15 suggestion(s).

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?

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

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

(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 on attachment 423240 [details] [diff] [review] (Av3a) configure part [Checkin: Comment 22] ok, I think I see what's happening now. Thanks.

Comment on attachment 423240 [details] [diff] [review] (Av3a) configure part [Checkin: Comment 22] http://hg.mozilla.org/comm-central/rev/3a22a577e53d

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

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

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