294519 - should behave the same when SVG not built, SVG pref disabled, or no SVG renderer present

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

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

This is afollowup to bug 293459.  We should behave the same way in these three
cases:
 * SVG is not built
 * the SVG pref is disabled
 * there is no SVG renderer available (e.g., no gdiplus.dll on the system)

This means that:
 * #ifdef MOZ_SVG, there should be a function somewhere that returns true iff
the SVG pref is enabled and there is an SVG renderer is available
 * all entry points to SVG content node and frame creation should be inside a
test of this function that should be effectively a runtime equivalent of the
#ifdef :
  + nsSVGElementFactory.cpp already has a pref check; this needs to check the
presence of a renderer as well
  + nsCSSFrameConstructor::ConstructSVGFrame needs a call to this function with
an early return in the false case.


This is important because:

 * it will prevent us from releasing a browser that crashes on certain SVG
content for a significant portion of users (since there could easily be other
bugs like bug 293459, which was fixed in the wrong way)

 * it will prevent us from releasing a set of browsers that can have "no SVG
support" in many different ways.  It's much better if there are fewer different
variations in the wild that Web authors have to deal with, so "no SVG" should
continue to behave the way it has in builds from before our SVG support existed
since there's no good reason (none that I've heard, anyway) for it to behave any
other way.

Comment 1

[tor]

Attachment: get all svg failure cases acting the same

Had to remove the missing gdi+ dialog because I couldn't figure out a way of
triggering it at the appriopriate time without the content being generated.

Comment 2

[tor]

Attachment: remove all traces of svg.properties, if we go that route

Comment 3

[Benjamin Smedberg]

Need to get this resolved.

Comment 4

[tor]

Attachment: update to trunk

Comment 5

[Asa Dotzler [:asa]]

Tor, this is your bug and this is your patch. Why won't you own this bug? 

Comment 6

[tor]

Because I see it as low priority compared to the other SVG work I'm doing and
don't want to give the false impression that I'm actively working on this bug.

Comment 7

[Chris Beard]

so we should take this off the list for 1.8/1.5?

Comment 8

[Chris Beard]

or is there someone else who we should get to take it on to ensure it happens?
(within the next week in order to make this release)

Comment 9

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

For the reasons stated in comment 0, I don't think we should ship a release with
SVG support that does not have this bug fixed.

Comment 10

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

Thoughts so far:
 * it might be worth not removing the localization stuff on the branch, in case
you want to readd the dialog after the l10n freeze.
 * doing the element factory changes differently would avoid a startup hit for
loading GDIPlus or cairo.  I still need to look in to whether that's possible.

Comment 11

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

Comment on attachment 191710 [details] [diff] [review]
update to trunk

I think the SVGEnabled check probably really belongs in NS_NewElement rather
than NS_NewSVGElement, because of the XTF check later (or potentially other
additions like it later).  NS_NewSVGElement should probably just have an
NS_PRECONDITION.

In nsContentDLF.cpp, please change |static char* gSVGTypes| to |static const
char* const gSVGTypes| to match all the others.

The conditions under which SVGPrefChanged calls RegisterSVG and UnregisterSVG
seem incorrect given this change.  I think it calls UnregisterSVG in cases
where it should do nothing.

Speaking of which, why doesn't the layout module destructor call UnregisterSVG?
 In fact, why aren't RegisterSVG and UnregisterSVG called from
RegisterDocumentFactories and UnregisterDocumentFactories instead of calling
only the former from nsLayoutModule's Initialize?

Comment 12

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

(In reply to comment #10)
>  * doing the element factory changes differently would avoid a startup hit for
> loading GDIPlus or cairo.  I still need to look in to whether that's possible.

Sorry, I meant nsContentDLF changes (which concern the Gecko-Content-Viewers
category); I still haven't looked to see if it's possible.  But when bug 240493
is fixed, we'd lose any such startup time gain, so I'm not sure if it's worth
worrying about.

Comment 13

[tor]

Attachment: updated patch

Apply some of dbaron's comments.  I tried moving the registration to
RegisterDocumentFactories, but ran into a problem where if svg.enabled was set
to false in a user profile, mozilla would make a half-hearted attempt to handle
it internally (create a blank document, but no svg elements are constructed). 
Couldn't figure out what was causing it (persistent registration?) so left it
in the current state.

Comment 14

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

Comment on attachment 194198 [details] [diff] [review]
updated patch

Stick an NS_ASSERTION(sInitialized, ...) at the beginning of SVGPrefChanged and
r=dbaron

Comment 15

[Asa Dotzler [:asa]]

If this is ready, please land it on the trunk, get it resolved and verified
there and then land on the branch. Time is very short for beta.

Comment 16

[tor]

Checked in on branch and trunk.

Comment 17

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

So it turns out this patch causes us to assert on startup because it recurs into
layout module initialization.  I forgot that the SVG renderers are actually
within the layout library.  I'm not quite sure what needs to be done about this,
but something should be.