Closed Bug 1022031 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

Attachments

(1 file, 5 obsolete files)

Convert the Thebes backed gfxContext in gfxPlatform's CopySurface() to Moz2D.
Attached patch patch (obsolete) — Splinter Review
Attachment #8436171 - Flags: review?(matt.woodrow)
Blocks: 933019
Attached patch patch (obsolete) — Splinter Review
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+
Attached patch patch to avoid leak (obsolete) — Splinter Review
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)
Attached patch patch to avoid leak (obsolete) — Splinter Review
Attachment #8440570 - Attachment is obsolete: true
Attachment #8440570 - Flags: review?(matt.woodrow)
Attachment #8440576 - Flags: review?(matt.woodrow)
Attached patch interdiff patch to avoid leak (obsolete) — Splinter Review
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)
Attached patch patchSplinter Review
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+
Attachment #8436324 - Attachment is obsolete: true
Attachment #8440746 - Attachment is obsolete: true
Attachment #8440746 - Flags: review?(matt.woodrow)
https://hg.mozilla.org/mozilla-central/rev/51d6530ce1b3
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.