893977 - Support repeating gradients in the CoreGraphics backend

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: The linear ones

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Attachment: Actually this time.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: Radial and Linear repeating gradients

This doesn't handle transformed repeating gradients and doesn't do the interpolation for the center stop if needed. However this is sufficient to pass the reftests we have.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Attachment: The whole repeating code this time.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 777970 [details] [diff] [review]
The whole repeating code this time.

Review of attachment 777970 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/DrawTargetCG.cpp
@@ +353,5 @@
>    }
>    // Will always report BACKEND_COREGRAPHICS, but it is compatible
>    // with BACKEND_COREGRAPHICS_ACCELERATED
>    BackendType GetBackendType() const { return BACKEND_COREGRAPHICS; }
> +  // XXX this should be a union

How about just having separate subclasses for clamp/repeat?

@@ +385,5 @@
> +
> +      CGContextDrawLinearGradient(cg, stops->mGradient, startPoint, endPoint,
> +                                  kCGGradientDrawsBeforeStartLocation | kCGGradientDrawsAfterEndLocation);
> +    } else if (stops->mExtend == EXTEND_REPEAT) {
> +      // extend the gradient line to cover the entire box (we can find the shortest line but that's more work)

A more detailed comment about what we're doing and why would be good.

@@ +390,5 @@
> +      CGPoint startPoint = { pat.mBegin.x, pat.mBegin.y };
> +      CGPoint endPoint = { pat.mEnd.x, pat.mEnd.y };
> +      double xDiff = pat.mEnd.x - pat.mBegin.x;
> +      double yDiff = pat.mEnd.y - pat.mBegin.y;
> +      // XXX what if startPoint has x > endPoint

This should work shouldn't it? Same with y too.

@@ +392,5 @@
> +      double xDiff = pat.mEnd.x - pat.mBegin.x;
> +      double yDiff = pat.mEnd.y - pat.mBegin.y;
> +      // XXX what if startPoint has x > endPoint
> +      int beginMult = 0;
> +      int endMult = 1;

Just have totalMult here, and increment that in both places below.

@@ +483,5 @@
> +      }
> +      r = hypot(endCenter.x-(aExtents.origin.x+aExtents.size.width), endCenter.y-(aExtents.origin.y+aExtents.size.height));
> +      if (r > goal) {
> +        goal = r;
> +      }

std::max?

And comments about how this is finding the furthest corner of aExtents from endCenter to determine how far we need to extend it.

Should also comment about how it's only extending the end circle to cover the extents. It's not obvious what would need to happen to extend the inner circle. Especially if they're not concentric.

@@ +522,5 @@
> +      CGContextDrawRadialGradient(cg, gradient, startCenter, startRadius, endCenter, endRadius,
> +                                  kCGGradientDrawsBeforeStartLocation | kCGGradientDrawsAfterEndLocation);
> +      CGGradientRelease(gradient);
> +
> +    }

This is an enormous function now. Split some of it out into helpers?

@@ +935,5 @@
> +      y1 = bboxes[i].origin.y + positions[i].y;
> +    if (bboxes[i].origin.x + scale*bboxes[i].size.width + positions[i].x < x2)
> +      x2 = bboxes[i].origin.x + scale*bboxes[i].size.width + positions[i].x;
> +    if (bboxes[i].origin.y + scale*bboxes[i].size.height + positions[i].y < y2)
> +      y2 = bboxes[i].origin.y + scale*bboxes[i].size.height + positions[i].y;

Why not just use std::min/max for these?

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out alongside bug 895646 because one or both of them caused build bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/c99f3b6eecd5

https://tbpl.mozilla.org/php/getParsedLog.php?id=25470293&tree=Mozilla-Inbound

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a9a0ef513220