950371 - Convert the Cocoa widget consumers of imgIContainer::GetFrame to act on a Moz2D SourceSurface instead of a Thebes gfxASurface

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jonathan Watt [:jwatt]]

As part of converting imgIContainer::GetFrame to return a Moz2D SourceSurface instead of a Thebes gfxASurface, we should convert the Cocoa widget consumers of imgIContainer::GetFrame to act on a Moz2D SourceSurface instead of a Thebes gfxASurface.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Builds, but not tested yet.

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 3

[Matt Woodrow (:mattwoodrow)]

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

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

::: widget/cocoa/nsClipboard.mm
@@ +443,5 @@
> +        continue;
> +      }
> +      RefPtr<SourceSurface> surface =
> +        gfxPlatform::GetPlatform()->GetSourceSurfaceForSurface(
> +          gfxPlatform::GetPlatform()->ScreenReferenceDrawTarget(), thebesSurface);

Passing nullptr for the first param does the same thing, if you want.

::: widget/cocoa/nsCocoaUtils.mm
@@ +276,4 @@
>      return NS_ERROR_FAILURE;
>    }
>  
> +  nsAutoArrayPtr<uint8_t> bits(SurfaceToPackedBGRA(aSurface));

I don't think we need to bother with this. All it does is copy to a packed array (which the code below doesn't need) and potentially convert BGR -> BGRA (which the code below doesn't check, so can't be required).

Just call GetAsDataSurface() instead and save the copy. I know this is unlikely to be perf critical, but it's still wasted work.

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch

Sorry, I had meant to kill off the SurfaceToPackedBGRA call.

Comment 5

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05a5f08f1278

Comment 6

[Carsten Book [:Tomcat]]

sorry had to backout this change for mochitest -bc test failures on OS X like https://tbpl.mozilla.org/php/getParsedLog.php?id=35031787&tree=Mozilla-Inbound

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: patch to make sure dataSurface stays alive as long as the CGImageRef might use it

Comment 8

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a00bd379adb2

Comment 9

[Ed Morley [:emorley]]

Backed out for:
https://tbpl.mozilla.org/php/getParsedLog.php?id=35053395&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/da881aef16f3

Comment 10

[Jonathan Watt [:jwatt]]

It sure would be nice if Try would run the non-unified builds...

https://hg.mozilla.org/integration/mozilla-inbound/rev/97a62f515945

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/97a62f515945

Comment 12

[Jonathan Watt [:jwatt]]

Somehow this was accidentally backed out in the merge to m-c in ce1a691650ef. Relanded:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a2b62ced704d

Comment 13

[Ed Morley [:emorley]]

(In reply to Jonathan Watt [:jwatt] from comment #12)
> Somehow this was accidentally backed out in the merge to m-c in
> ce1a691650ef. Relanded:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a2b62ced704d

Push backed out for crashes:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=a2b62ced704d

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0ee66295c251
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/360307f9ee38

Comment 14

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f472b5d1d7a

Repushed with the Unmap() that we assert for nowadays.

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0f472b5d1d7a