Closed Bug 893977 Opened 6 years ago Closed 6 years ago

Support repeating gradients in the CoreGraphics backend

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: jrmuizel, Assigned: jrmuizel)

References

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
Attached patch The linear ones (obsolete) — Splinter Review
Attached patch Actually this time. (obsolete) — Splinter Review
Attachment #775854 - Attachment is obsolete: true
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.
Attachment #775855 - Attachment is obsolete: true
Attachment #777944 - Flags: review?(matt.woodrow)
Attachment #777944 - Attachment is obsolete: true
Attachment #777944 - Flags: review?(matt.woodrow)
Attachment #777970 - Flags: review?(matt.woodrow)
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?
Attachment #777970 - Flags: review?(matt.woodrow) → review+
https://hg.mozilla.org/mozilla-central/rev/a9a0ef513220
Assignee: nobody → jmuizelaar
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.