Closed
Bug 294519
Opened 19 years ago
Closed 19 years ago
should behave the same when SVG not built, SVG pref disabled, or no SVG renderer present
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
People
(Reporter: dbaron, Unassigned)
Details
(Keywords: fixed1.8, Whiteboard: [needs trunk checkin])
Attachments
(1 file, 3 obsolete files)
19.61 KB,
patch
|
dbaron
:
review+
asa
:
approval1.8b4+
|
Details | Diff | Splinter Review |
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.
Updated•19 years ago
|
Flags: blocking1.8b3?
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.
Attachment #183857 -
Attachment is obsolete: true
Comment 3•19 years ago
|
||
Need to get this resolved.
Flags: blocking1.8b4+
Flags: blocking1.8b3?
Flags: blocking1.8b3-
Whiteboard: [bs] has patch by tor, no review requested
Updated•19 years ago
|
Assignee: general → tor
Updated•19 years ago
|
Assignee: general → tor
Attachment #183914 -
Attachment is obsolete: true
Comment 5•19 years ago
|
||
Tor, this is your bug and this is your patch. Why won't you own this bug?
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•19 years ago
|
||
so we should take this off the list for 1.8/1.5?
Comment 8•19 years ago
|
||
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)
Reporter | ||
Comment 9•19 years ago
|
||
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.
Updated•19 years ago
|
Flags: blocking1.8b5+
Updated•19 years ago
|
Flags: blocking1.8b5+
Attachment #191710 -
Flags: review?(dbaron)
Reporter | ||
Comment 10•19 years ago
|
||
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.
Reporter | ||
Comment 11•19 years ago
|
||
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?
Reporter | ||
Comment 12•19 years ago
|
||
(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.
Reporter | ||
Updated•19 years ago
|
Attachment #191710 -
Flags: review?(dbaron) → review-
Comment 13•19 years ago
|
||
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.
Attachment #191710 -
Attachment is obsolete: true
Attachment #194198 -
Flags: review?(dbaron)
Reporter | ||
Comment 14•19 years ago
|
||
Comment on attachment 194198 [details] [diff] [review] updated patch Stick an NS_ASSERTION(sInitialized, ...) at the beginning of SVGPrefChanged and r=dbaron
Attachment #194198 -
Flags: review?(dbaron) → review+
Updated•19 years ago
|
Attachment #194198 -
Flags: approval1.8b4+
Comment 15•19 years ago
|
||
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.
Whiteboard: [bs] has patch by tor, no review requested
Updated•19 years ago
|
Whiteboard: [needs trunk checkin]
Comment 16•19 years ago
|
||
Checked in on branch and trunk.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 17•19 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•