1160335 - Make DrawTargetCG's transform handling better

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: Make DrawTargetCG's transform handling better

Comment 1

[Markus Stange [: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.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Attachment: patch

Comment 3

[Markus Stange [: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.

Comment 4

[Markus Stange [:mstange]]

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.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Attachment: Make DrawTargetCG's transform handling better v3

Comment 6

[Markus Stange [: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?

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/434bb2c9dad6

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/434bb2c9dad6