459758 - effects on outer svg frames only work accidentally

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

Attachment: patch

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.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Egad! Is there no way to avoid duplicating PaintFrameWithEffects?

Comment 2

[Robert Longson [:longsonr]]

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?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 4

[Robert Longson [:longsonr]]

OK I'll try that approach.

Comment 5

[Robert Longson [:longsonr]]

Attachment: address review comments

You'll like this version much better. Nice fallout from bug 459512 don't you think.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 7

[Robert Longson [:longsonr]]

Attachment: address sr comment

Comment 8

[Robert Longson [:longsonr]]

checked in 734b871312a4.