1022031 - Convert the Thebes backed gfxContext in gfxPlatform's CopySurface() to Moz2D

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jonathan Watt [:jwatt]]

Convert the Thebes backed gfxContext in gfxPlatform's CopySurface() to Moz2D.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 3

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8436324 [details] [diff] [review]
patch

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

::: gfx/thebes/gfxPlatform.cpp
@@ +623,1 @@
>  CopySurface(gfxASurface* aSurface)

This name doesn't make much sense any more.

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch to avoid leak

As it is, the r+'ed patch causes SourceSurfaceCairo and gfxASurface leaks to be reported on a small number of SVG reftests on debug WinXP builds. The reason for that is explained in the comment I've added to this extra patch that fixes those leaks. (For these tests we end up with a gfxD2DSurface for which the |ifdef XP_WIN| block in GetSourceSurfaceForSurface fails because aTarget is a Cairo DT.)

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: patch to avoid leak

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: interdiff patch to avoid leak

Turns out removing the old check causes failures. I'd added a more detailed comment there to explain why. This now passes Try.

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: patch

Result of IRC discussion.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8441057 [details] [diff] [review]
patch

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

::: gfx/thebes/gfxPlatform.cpp
@@ +711,5 @@
> +      // creating a reference loop and a memory leak. Not caching is fine since
> +      // wrapping is cheap enough (no copying) so we can just wrap again next
> +      // time we're called.
> +      return srcBuffer.forget();
> +    }

I think we should always return here. If this failed, then I can't see how the code below could ever do better.

@@ +756,5 @@
> +  }
> +
> +  if (!srcBuffer &&
> +      // We tried CreateSourceSurfaceFromNativeSurface above if this is false:
> +      aTarget->GetType() != BackendType::CAIRO) {

We should be able to drop this and assert that CAIRO never makes it this far.

Comment 9

[Jonathan Watt [:jwatt]]

Comment on attachment 8441057 [details] [diff] [review]
patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/51d6530ce1b3

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/51d6530ce1b3