Closed Bug 1083221 Opened 5 years ago Closed 5 years ago

Port all code using nsRenderingContext::DrawEllipse/FillEllipse and gfxContext::Ellipse to Moz2D

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jwatt, Assigned: jwatt)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

No description provided.
Attached patch patchSplinter Review
Attachment #8505525 - Flags: review?(matt.woodrow)
Comment on attachment 8505525 [details] [diff] [review]
patch

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

It looks like all the callers of AppendEllipseToPath do so without adding anything else to the path. Might be worth adding a helper that includes the CreatePathBuilder/Finish calls, something like Path* GetEllipsePath(DT*, Point, Size);

::: layout/generic/nsImageFrame.cpp
@@ +1267,5 @@
> +      // stroked rect:
> +      nsRect rect(iconXPos, inner.y, size, size);
> +      Rect devPxRect =
> +        ToRect(nsLayoutUtils::RectToGfxRect(rect, PresContext()->AppUnitsPerDevPixel()));
> +      drawTarget->StrokeRect(rect, color);

The existing code was stroking this rect before setting the red color, so it was getting whatever color was set on the rendering context previously.

I don't see us setting it anywhere, so we might be getting the default? Which is transparent.

It also looks like the old code would have called Restore() twice and messed things up.

I feel like this code doesn't run very often.
Attachment #8505525 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8505525 [details] [diff] [review]
patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/9024979c8286

Huh, I missed your review comments. I'll land a follow-up shortly.
Attachment #8505525 - Flags: checkin+
Blocks: 776197
(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> The existing code was stroking this rect before setting the red color, so it
> was getting whatever color was set on the rendering context previously.
> 
> I don't see us setting it anywhere, so we might be getting the default?
> Which is transparent.
> 
> It also looks like the old code would have called Restore() twice and messed
> things up.
> 
> I feel like this code doesn't run very often.

So this code will only be hit if resource://gre-resources/loading-image.png or resource://gre-resources/broken-image.png haven't loaded yet. Since these global imgRequests are loaded the very first time in the process that an nsImageFrame is Init()'ed, it's very unlikely we will ever actually hit this code.

I think the rect might as well be red as well as the circle (and the failure to set the color in the previous code was a bug), so I don't think I'm going to land a follow-up after all. (The ellipse thing can happen at some point in the future.)
https://hg.mozilla.org/mozilla-central/rev/9024979c8286
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Blocks: 1087958
You need to log in before you can comment on or make changes to this bug.