Closed Bug 988818 Opened 7 years ago Closed 7 years ago

Stop creating a Thebes (gfxASurface) backed gfxContext in nsSVGImageFrame

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: jwatt, Assigned: jwatt)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

We should stop creating a Thebes (gfxASurface) backed gfxContext in nsSVGImageFrame.
Attached patch patchSplinter Review
Attachment #8397779 - Flags: review?(longsonr)
Comment on attachment 8397779 [details] [diff] [review]
patch

>+ * Given a float, constrains its value to be between nscoord_MIN and nscoord_MAX.
>+ *
>+ * @param aVal The value to constrain (in/out)
>+ */
>+static void ConstrainToCoordValues(float& aVal)
>+{
>+  if (aVal <= nscoord_MIN)
>+    aVal = nscoord_MIN;
>+  else if (aVal >= nscoord_MAX)
>+    aVal = nscoord_MAX;

use mozilla::clamped to implement? Maybe this shouldn't be static given that nsSVGUtils::TransformOuterSVGPointToChildFrame could use it.

>+}
>+
>+static void ConstrainToCoordValues(float& aStart, float& aSize)
>+{
>+  float max = aStart + aSize;
>+
>+  // Clamp the end points to within nscoord range
>+  ConstrainToCoordValues(aStart);
>+  ConstrainToCoordValues(max);

So we just truncate here but bring both the endpoints in together below. That seems strange. Why?

>+
>+  aSize = max - aStart;
>+  // If the width if still greater than the max nscoord, then bring both

If the width **is** still...
Attachment #8397779 - Flags: review?(longsonr) → review-
(In reply to Robert Longson from comment #2)
> So we just truncate here but bring both the endpoints in together below.
> That seems strange. Why?

I'll talk about the x-axis properties here, but obviously the same applies for the y-axis properties.

There are three properties of the rect that we care about getting in range: x, xmost (aka the right edge) and width. Obviously the bounds of the rect have to be within {nscoord_MIN,nscoord_MAX}, so we clamp x and xmost to that range. That done, we still need width to be less than or equal to nscoord_MAX. Rather than simply clamp, we pull in the edges to keep the rect centered. From my perspective I'm doing that mostly because that's what we've historically done in these situations, but also because it seems the most sane thing to me (particularly for SVG where we're not flowing things and aligning them to an edge).
Maybe it helps to think of this in terms of an object of size {x,0,width,1}. (y and height being given simple values so we can simplify our discussion to just consider the x-axis.)

There are several scenarios where we need to change the bounds of the object:

 1) If the object is positioned fully outside {nscoord_MIN,nscoord_MAX} then
    presumably we can agree that it should probably become zero sized.

 2) If the object is fully within {nscoord_MIN,nscoord_MAX} but its width is
    greater than nscoord_MAX then hopefully we can agree that reducing its
    width to nscoord_MAX while keeping it centered is likely to produce the
    least bad result most of the time?

 3) If the object is overlapping x=nscoord_MIN or x=nscoord_MAX with a width
    of less than nscoord_MAX then what should we do? We could pull in the
    edges until width<nscoord_MAX but that would make an object that should
    have partially been in range zero sized if more than half of it was
    outside {nscoord_MIN,nscoord_MAX}. I think it's better to clamp its
    bounds to keep the nsRect covering as much of the area as the original
    gfxRect covered. That way there's a better chance of a visual indication
    of range loss.

 4) If the object is overlapping x=nscoord_MIN or x=nscoord_MAX with a width
    greater than nscoord_MAX I think we should do the same as (3), _then_
    pull in the edges. Again that keeps the resulting nsRect covering as
    much of the original gfxRect as possible, while still centering when
    the width is too big.
Attached patch patchSplinter Review
Attachment #8399169 - Flags: review?(longsonr)
Attachment #8399169 - Flags: review?(longsonr) → review+
https://hg.mozilla.org/mozilla-central/rev/fe53a01dab3d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Depends on: 993443
You need to log in before you can comment on or make changes to this bug.