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

RESOLVED FIXED in mozilla33

Status

()

Core
Graphics
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

Trunk
mozilla33
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

Convert the Thebes backed gfxContext in gfxPlatform's CopySurface() to Moz2D.
Created attachment 8436171 [details] [diff] [review]
patch
Attachment #8436171 - Flags: review?(matt.woodrow)
(Assignee)

Updated

4 years ago
Blocks: 933019
Created attachment 8436324 [details] [diff] [review]
patch
Attachment #8436171 - Attachment is obsolete: true
Attachment #8436171 - Flags: review?(matt.woodrow)
Attachment #8436324 - Flags: review?(matt.woodrow)
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.
Attachment #8436324 - Flags: review?(matt.woodrow) → review+
Created attachment 8440570 [details] [diff] [review]
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.)
Attachment #8440570 - Flags: review?(matt.woodrow)
Created attachment 8440576 [details] [diff] [review]
patch to avoid leak
Attachment #8440570 - Attachment is obsolete: true
Attachment #8440570 - Flags: review?(matt.woodrow)
Attachment #8440576 - Flags: review?(matt.woodrow)
Created attachment 8440746 [details] [diff] [review]
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.
Attachment #8440576 - Attachment is obsolete: true
Attachment #8440576 - Flags: review?(matt.woodrow)
Attachment #8440746 - Flags: review?(matt.woodrow)
Created attachment 8441057 [details] [diff] [review]
patch

Result of IRC discussion.
Attachment #8441057 - Flags: review?(matt.woodrow)
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.
Attachment #8441057 - Flags: review?(matt.woodrow) → review+
(Assignee)

Updated

4 years ago
Attachment #8436324 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8440746 - Attachment is obsolete: true
Attachment #8440746 - Flags: review?(matt.woodrow)
Comment on attachment 8441057 [details] [diff] [review]
patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/51d6530ce1b3
Attachment #8441057 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/51d6530ce1b3
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.