1350706 - Get rid of nsSVGDisplayableFrame::GetCoveredRegion and its overrides

Status: RESOLVED FIXED

(Core :: SVG, enhancement)

Description

[Jonathan Watt [:jwatt]]

nsSVGDisplayableFrame::GetCoveredRegion is another piece of code that we never succeeded in getting rid of in the past that has caused some confusion with the new Taipei devs recently. Its overrides are also one of the few remaining bits of code that use GetCanvasTM.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Jonathan Watt [:jwatt]]

Passes our tests in dom/svg/test/test_bounds.html

Comment 3

[Robert Longson [:longsonr]]

Comment on attachment 8851372 [details] [diff] [review]
patch

>@@ -504,16 +482,24 @@ SVGGeometryFrame::GetBBoxContribution(co
> {
>   SVGBBox bbox;
> 
>   if (aToBBoxUserspace.IsSingular()) {
>     // XXX ReportToConsole
>     return bbox;
>   }
> 
>+  if (aFlags & nsSVGUtils::eForGetClientRects) {
>+    if (aToBBoxUserspace.PreservesAxisAlignedRectangles()) {
>+      Rect rect = NSRectToRect(mRect, PresContext()->AppUnitsPerCSSPixel());
>+      bbox = aToBBoxUserspace.TransformBounds(rect);
>+      return bbox;

no test for IsEmpty here.

> SVGBBox
> SVGTextFrame::GetBBoxContribution(const gfx::Matrix &aToBBoxUserspace,
>                                   uint32_t aFlags)
> {
>   NS_ASSERTION(PrincipalChildList().FirstChild(), "must have a child frame");
>   SVGBBox bbox;
>+
>+  if (aFlags & nsSVGUtils::eForGetClientRects) {
>+    Rect rect = NSRectToRect(mRect, PresContext()->AppUnitsPerCSSPixel());
>+    if (!rect.IsEmpty()) {
>+      bbox = aToBBoxUserspace.TransformBounds(rect);

but there is one here? Why the difference

>+SVGBBox
>+nsSVGInnerSVGFrame::GetBBoxContribution(const Matrix &aToBBoxUserspace,
>+                                        uint32_t aFlags)
>+{
>+  // XXXjwatt It seems like authors would want the result to be clipped by the
>+  // viewport we establish if IsScrollableOverflow() is true.  We should
>+  // consider doing that.
>+
>+  SVGBBox bbox;
>+
>+  if (aFlags & nsSVGUtils::eForGetClientRects) {
>+    // XXXjwatt For consistency with the old code this code includes the
>+    // viewport in the result, but that seems undesirable since inner-<svg>
>+    // doesn't paint anything itself.  We should consider changing that to
>+    // make getClientRects/getBoundingClientRect consistent with getBBox.

I think what we do is consistent with Chrome/Safari we had complaints about the difference
before we changed to match.

Comment 4

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #3)
> >@@ -504,16 +482,24 @@ SVGGeometryFrame::GetBBoxContribution(co
>
> no test for IsEmpty here.
>
> > SVGTextFrame::GetBBoxContribution(const gfx::Matrix &aToBBoxUserspace,
>
> but there is one here? Why the difference

It's because SVGTextFrame is different to geometry frames because we create the frame even if it doesn't have valid bounds. So for the geometry frames we never even create the frame if it will have empty bounds. I'm not sure exactly why that is but it's on my list of things to look into.

Comment 5

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #3)
> I think what we do is consistent with Chrome/Safari we had complaints about
> the difference
> before we changed to match.

It looks like that was bug 530985. Well remembered. I'll adjust the comment.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/476f9977f5c2
Get rid of nsSVGDisplayableFrame::GetCoveredRegion and its overrides. r=longsonr

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/476f9977f5c2