Closed Bug 950371 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Graphics, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: jwatt, Assigned: jwatt)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 2 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
Builds, but not tested yet.
Blocks: 950372
Attached patch patch (obsolete) — Splinter Review
Attachment #8347639 - Attachment is obsolete: true
Attachment #8378316 - Flags: review?(matt.woodrow)
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.
Attached patch patchSplinter Review
Sorry, I had meant to kill off the SurfaceToPackedBGRA call.
Attachment #8378316 - Attachment is obsolete: true
Attachment #8378316 - Flags: review?(matt.woodrow)
Attachment #8379340 - Flags: review?(matt.woodrow)
Attachment #8379340 - Flags: review?(matt.woodrow) → review+
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
Attachment #8379656 - Flags: review?(mstange) → review+
It sure would be nice if Try would run the non-unified builds...

https://hg.mozilla.org/integration/mozilla-inbound/rev/97a62f515945
https://hg.mozilla.org/mozilla-central/rev/97a62f515945
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Somehow this was accidentally backed out in the merge to m-c in ce1a691650ef. Relanded:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a2b62ced704d
(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
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f472b5d1d7a

Repushed with the Unmap() that we assert for nowadays.
See Also: → 991434
See Also: 991434
You need to log in before you can comment on or make changes to this bug.