Support repeating gradients in the CoreGraphics backend

RESOLVED FIXED in mozilla25

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Tracking

unspecified
mozilla25
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 775854 [details] [diff] [review]
The linear ones
(Assignee)

Comment 2

5 years ago
Created attachment 775855 [details] [diff] [review]
Actually this time.
Attachment #775854 - Attachment is obsolete: true
(Assignee)

Comment 3

5 years ago
Created attachment 777944 [details] [diff] [review]
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.
Attachment #775855 - Attachment is obsolete: true
Attachment #777944 - Flags: review?(matt.woodrow)
(Assignee)

Comment 4

5 years ago
Created attachment 777970 [details] [diff] [review]
The whole repeating code this time.
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+

Comment 7

5 years ago
https://hg.mozilla.org/mozilla-central/rev/a9a0ef513220
Assignee: nobody → jmuizelaar
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Depends on: 902591
You need to log in before you can comment on or make changes to this bug.