846132 - Remove imgIContainer::CopyFrame

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

We don't actually need imgIContainer::CopyFrame. There are too many ways to get access to frames on imgIContainer as it is, and it's desirable to reduce the surface area of the API.

GetFrame can be used for the same things as CopyFrame. The difference, clearly enough, is that GetFrame doesn't necessarily copy the underlying surface (though it can). In situations where GetFrame doesn't copy it can end up returning an optimized surface that can't be read from or written to. We can handle this by adding some convenience methods to gfxASurface - one for the case where we only want to read the resulting surface (which won't necessarily copy) and one for the case where we want to write to it (which always will). Then callers can just call one of these methods on the result of GetFrame, and we've recovered all of the functionality that CopyFrame offered.

This bug is about implementing this change.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) Add gfxASurface methods to produce gfxImageSurfaces.

Here we add the gfxASurface methods that will eventually replace the functionality of CopyFrame.

(Josh, I figured it'd be good to spread the reviews out a little more, so I'm sending you this one. Let me know if you can't review it.)

One thing I don't like about this patch is that GetAsReadableImageSurface can't return an nsRefPtr<const gfxImageSurface>. I'd love to fix our smart pointer implementation to allow this, but that'll have to happen in a separate bug.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) Update CopyFrame callers to use GetFrame.

Switching CopyFrame callers over to the new methods.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Remove imgIContainer::CopyFrame.

Remove CopyFrame and update imgIContainer.

Comment 4

[Seth Fowler [:seth] [:s2h]]

Try job here: https://tbpl.mozilla.org/?tree=Try&rev=840ce5dd09f6

Comment 5

[Josh Matthews [:jdm]]

Comment on attachment 719289 [details] [diff] [review]
(Part 1) Add gfxASurface methods to produce gfxImageSurfaces.

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

I've got little experience with gfxASurface, so I'm not a good choice here.

Comment 6

[Josh Matthews [:jdm]]

Comment on attachment 719290 [details] [diff] [review]
(Part 2) Update CopyFrame callers to use GetFrame.

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

::: widget/cocoa/nsClipboard.mm
@@ +431,5 @@
>          continue;
>        }
>  
> +      nsRefPtr<gfxASurface> frame;
> +      rv = image->GetFrame(   imgIContainer::FRAME_CURRENT,

Let's scrap this weird spacing.

::: widget/cocoa/nsCocoaUtils.mm
@@ +315,5 @@
>  
>  nsresult nsCocoaUtils::CreateNSImageFromImageContainer(imgIContainer *aImage, uint32_t aWhichFrame, NSImage **aResult)
>  {
> +  nsRefPtr<gfxASurface> frame;
> +  nsresult rv = aImage->GetFrame( aWhichFrame,

Here as well.

::: widget/cocoa/nsMenuItemIconX.mm
@@ +388,5 @@
>      mImageRegionRect.SetRect(0, 0, origWidth, origHeight);
>    }
>    
> +  nsRefPtr<gfxASurface> frame;
> +  nsresult rv = imageContainer->GetFrame(   imgIContainer::FRAME_CURRENT,

And here.

Comment 7

[Joe Drew (not getting mail)]

Comment on attachment 719291 [details] [diff] [review]
(Part 3) - Remove imgIContainer::CopyFrame.

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

::: image/src/RasterImage.cpp
@@ +988,5 @@
>  
>  
>  //******************************************************************************
>  /* [noscript] gfxImageSurface copyFrame(in uint32_t aWhichFrame,
>   *                                      in uint32_t aFlags); */

Remove this annotation too

Comment 8

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 719289 [details] [diff] [review]
(Part 1) Add gfxASurface methods to produce gfxImageSurfaces.

Requesting review for part 1 from Jeff since it sounds like he's the right person for this.

Comment 9

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) Update CopyFrame callers to use GetFrame.

Updated patch based on review comments. Also, made the same changes to the Windows, Linux, and OS/2 platform-specific code. I forgot to update the patch with those changes before asking for review. It's probably not worth reviewing again since it's a pretty mechanical change that is done in the same way everywhere, so I'll leave the option open but won't set r?.

Comment 10

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Remove imgIContainer::CopyFrame.

Updated patch based on review comments.

Comment 11

[Seth Fowler [:seth] [:s2h]]

Thanks for the reviews, Joe and Josh. I've submitted a new try job here:

https://tbpl.mozilla.org/?tree=Try&rev=6158febdde50

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 719289 [details] [diff] [review]
(Part 1) Add gfxASurface methods to produce gfxImageSurfaces.

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

You have r=jdm instead of jrmuizel

::: gfx/thebes/gfxASurface.cpp
@@ +330,5 @@
> +    nsRefPtr<gfxPattern> pattern = new gfxPattern(this);
> +
> +    const gfxIntSize size = GetSize();
> +    nsRefPtr<gfxImageSurface> imgSurface =
> +        new gfxImageSurface(size, gfxASurface::ImageFormatARGB32);

Should this always be ImageFormatARGB32 or should it depend on the format of the image?

@@ +335,5 @@
> +
> +    gfxContext ctx(imgSurface);
> +    ctx.SetOperator(gfxContext::OPERATOR_SOURCE);
> +    ctx.Rectangle(gfxRect(0, 0, size.width, size.height));
> +    ctx.SetPattern(pattern);

you can just use SetSource() instead of creating a pattern.

Comment 13

[Seth Fowler [:seth] [:s2h]]

Thanks for the review, Jeff. I'll post an updated patch shortly.

(In reply to Jeff Muizelaar [:jrmuizel] from comment #12)
> Should this always be ImageFormatARGB32 or should it depend on the format of
> the image?

We discussed this on IRC. The original CopyFrame code always returns a surface with ImageFormatARGB32. To avoid triggering any issues with callers, we'll address this by keeping this behavior and changing the method name to make the kind of surface you'll receive more explicit.

Comment 14

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) Add gfxASurface methods to produce gfxImageSurfaces.

Here's an updated patch based upon your review comments. The other patches need to be rebased against this one, but I'll hold off until this gets an r+.

Comment 15

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) Update CopyFrame callers to use GetFrame.

Fix a typo that broke the build on Windows.

Comment 16

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Remove imgIContainer::CopyFrame.

Rebased, but no other changes.

Comment 17

[Seth Fowler [:seth] [:s2h]]

OK, here's a new try job rebased on top of bug 846937 (without that we got xpcshell test failures on OS X) and with the Windows-breaking typo fixed. All known problems that I can test locally should be fixed.

https://tbpl.mozilla.org/?tree=Try&rev=b4d801515719

Comment 18

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) Update CopyFrame callers to use GetFrame.

Thanks for the review, Jeff.

That try job revealed another typo affecting Windows, which this updated patch fixes. It also got polluted by the xpcshell test bustage from Friday. I'll push an updated try job in just a moment.

Comment 19

[Seth Fowler [:seth] [:s2h]]

Here's that updated try job:

https://tbpl.mozilla.org/?tree=Try&rev=44107f80a53f

Comment 20

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) Update CopyFrame callers to use GetFrame.

Argh. OK, that try job looks fine on every platform except Windows, where yet another typo got uncovered. I really wish more than one error got reported at a time. At any rate, here's a new Windows-only try job:

https://tbpl.mozilla.org/?tree=Try&rev=b57ffa0fa76f

Comment 21

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) Add gfxASurface methods to produce gfxImageSurfaces.

Argh, now there are linking issues. I discussed this problem with roc who suggesting trying to make GetAsReadableARGB32ImageSurface virtual to break the link-time dependency between nsWindowsShellService.cpp and gfxASurface.cpp.

Try job here (Windows only as before; all the other platforms look good): https://tbpl.mozilla.org/?tree=Try&rev=02a389af3faf

Comment 22

[Seth Fowler [:seth] [:s2h]]

Try looks good. Pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbaec227c3e0
https://hg.mozilla.org/integration/mozilla-inbound/rev/a959294f4aa5
https://hg.mozilla.org/integration/mozilla-inbound/rev/c72da9fe6160

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cbaec227c3e0
https://hg.mozilla.org/mozilla-central/rev/a959294f4aa5
https://hg.mozilla.org/mozilla-central/rev/c72da9fe6160