Closed Bug 459758 Opened 13 years ago Closed 13 years ago

effects on outer svg frames only work accidentally

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: longsonr, Assigned: longsonr)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
outer svg frames are treated as non-svg frames and use the non-svg effects rendering path. This is likely to produce rendering errors when zoomed and won't work at all when the patch for bug 455630 lands.
Attachment #342955 - Flags: superreview?(roc)
Attachment #342955 - Flags: review?(roc)
Egad! Is there no way to avoid duplicating PaintFrameWithEffects?
It's different to the nsSVGUtils and nsSVGEffects versions:

nsSVGUtils

a) does not implement support for panning (mOffset)
b) expects the frame to implement nsISVGChildFrame

nsSVGIntegrationUtils

a) expects the frame not to be an SVG frame
b) converts the dirty rect to different units in the callback
c) expects the callback to be based on a nsDisplayList rather than an nsDisplayItem and we have the latter.
d) needs to paint SVG children via PaintChildWithEffects rather than aInnerList->Paint.

The outer svg frame version is a kind of half-way house between the two existing implementations.

I actually need a version which takes an nsSVGOuterSVGFrame so that I can call GetCanvasTM properly since there is no interface to go at.

Maybe I could try to convert the callback classes to renderer classes, expanding them to hold every aspect of the differences in implementation between the 3 different versions we need.

Do you want me to try that or just hold your nose and swallow quickly and maybe the aftertaste won't linger?
I'd like to try to merge this into the nsSVGUtils version. I guess we could add panning just by adding a parameter. I think we can break the dependency on nsISVGChildFrame somewhat brutally by just allowing that QI to be null, and casting the frame to nsSVGOuterSVGFrame and doing the outer-frame stuff when we would be using svgChildFrame but it's null.
OK I'll try that approach.
Attached patch address review comments (obsolete) — Splinter Review
You'll like this version much better. Nice fallout from bug 459512 don't you think.
Attachment #342955 - Attachment is obsolete: true
Attachment #343789 - Flags: superreview?(roc)
Attachment #343789 - Flags: review?(roc)
Attachment #342955 - Flags: superreview?(roc)
Attachment #342955 - Flags: review?(roc)
Attachment #343789 - Flags: superreview?(roc)
Attachment #343789 - Flags: superreview+
Attachment #343789 - Flags: review?(roc)
Attachment #343789 - Flags: review+
Comment on attachment 343789 [details] [diff] [review]
address review comments

Very nice!

The !IsFrameOfType(eSVG) check should be moved to nsSVGIntegrationUtils::UsingEffectsForFrame.

I guess this could use a reftest.
Attachment #343789 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
checked in 734b871312a4.
You need to log in before you can comment on or make changes to this bug.