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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Matt Woodrow (:mattwoodrow)]

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.

Comment 3

[Jonathan Watt [:jwatt]]

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.

Comment 4

[Jonathan Watt [:jwatt]]

(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.)

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9024979c8286