Closed Bug 1160335 Opened 9 years ago Closed 9 years ago

Make DrawTargetCG's transform handling better

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: jrmuizel, Assigned: jrmuizel)

References

(Blocks 1 open bug)

Details

(Whiteboard: gfx-noted)

Attachments

(1 file, 2 obsolete files)

      No description provided.
Attachment #8600098 - Flags: review?(mstange)
Comment on attachment 8600098 [details] [diff] [review]
Make DrawTargetCG's transform handling better

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

This breaks the UnboundnessFixer because the return value of CGContextGetClipBoundingBox is in user space, and with this patch we enter UnboundnessFixer::Check with a different transform, and the clip bounds will be misinterpreted.
Attachment #8600098 - Flags: review?(mstange) → review-
Attached patch patch (obsolete) — Splinter Review
Attachment #8600118 - Flags: review?(mstange)
Comment on attachment 8600118 [details] [diff] [review]
patch

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

This also breaks the mSavedClipBounds checks, so let's just remove those completely.

::: gfx/2d/DrawTargetCG.cpp
@@ +284,5 @@
>          // If we're entirely clipped out or if the drawing operation covers the entire clip then
>          // we don't need to create a temporary surface.
>          if (CGRectIsEmpty(mClipBounds) ||
>              (maskBounds && maskBounds->Contains(CGRectToRect(mClipBounds)))) {
>            return baseCg;

You also need to reset the transform in this error case.
Attachment #8600118 - Flags: review?(mstange) → review+
Comment on attachment 8600118 [details] [diff] [review]
patch

Matt Woodrow pointed out that DrawSurfaceWithShadow and CopySurface take parameters in dest space, so those would be treated as user space with this patch.

It seems reviewing this patch involves looking at all the code that's *not* touched by it.
Attachment #8600118 - Flags: review+ → review-
Attachment #8600098 - Attachment is obsolete: true
Attachment #8600118 - Attachment is obsolete: true
Attachment #8600388 - Flags: review?(mstange)
Comment on attachment 8600388 [details] [diff] [review]
Make DrawTargetCG's transform handling better v3

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

::: gfx/2d/DrawTargetCG.cpp
@@ +936,5 @@
> +DrawTargetCG::SetTransform(const Matrix &aTransform)
> +{
> +  mTransform = aTransform;
> +  CGContextSetCTM(mCg, CGAffineTransformMake(1, 0, 0, -1, 0, mSize.height));
> +  CGContextConcatCTM(mCg, GfxMatrixToCGAffineTransform(aTransform));

Would it be faster to multiply the transforms first and then setting the result in one go?
Attachment #8600388 - Flags: review?(mstange) → review+
Assignee: nobody → jmuizelaar
https://hg.mozilla.org/mozilla-central/rev/434bb2c9dad6
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: