1216644 - get rid of nsAutoArrayPtr usage in gfx/

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - simple s/nsAutoArrayPtr/UniquePtr/ changes in gfx/

This patch handles all the cases where we just want to swap out
nsAutoArrayPtr for UniquePtr and don't require code changes other than
updating uses that implicitly converted to T* to follow UniquePtr's
conventions.  I chose &x[0] instead of x.get() for such uses as &x[0] is
slightly shorter and feels more idiomatic.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - make gfxFontEntry::mUVSData a UniquePtr

This patch could have been in the last patch, but it felt like modifying
ReadCMAPTableFormat14 to accept a UniquePtr<> made the code clearer.
This change was therefore separated into its own patch for easier
review.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 3 - make BufferRecycleBin store UniquePtrs

Changing mRecycledBuffers store UniquePtrs instead of nsAutoArrayPtrs
opens up the possibility of a reasonable facsimile of ownership in
function signatures.  To do that, we have to make
PlanarYCbCrImage::AllocateBuffer return UniquePtr, which necessitates
changing a few overloads.  The SharedPlanarYCbCrImage change for this is
a little dodgy, but does make implicit assumptions made prior to this
patch a tad more explicit.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8676359 [details] [diff] [review]
part 1 - simple s/nsAutoArrayPtr/UniquePtr/ changes in gfx/

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

I personally like x.get() better. &x[0] reads to me as get the address of the first element and not get the address of the array, but I'm willing to resign to consistency if needed. It seems like this code would be all around better if we had something like dyn_array and array_view from CppCoreGuidelines.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(.get() is consistent with the other smart pointer types for getting the underlying pointer, too)

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8676361 [details] [diff] [review]
part 3 - make BufferRecycleBin store UniquePtrs

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

I'm not comfortable with the change in SharedPlanarYCbCrImage. It makes the ownership explicit when that's not necessarily true. I'm going to try and dig into an alternative that will keep us from having to lie.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #5)
> (.get() is consistent with the other smart pointer types for getting the
> underlying pointer, too)

Yeah, the more I think about it, the more I prefer .get(). The brevity argument for &x[0] doesn't appeal to me at all. Anytime, you write this you should be thinking "Am I sure I'm not introducing a use-after-free here" and I think .get() communicates that a little more explicitly than &x[0]

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8676359 [details] [diff] [review]
part 1 - simple s/nsAutoArrayPtr/UniquePtr/ changes in gfx/

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

I personally like x.get() better. &x[0] reads to me as get the address of the first element and not get the address of the array, but I'm willing to resign to consistency if needed. It seems like this code would be all around better if we had something like dyn_array and array_view from CppCoreGuidelines.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Bug 127080 will let us avoid the SharedPlanarYCbCrImage hack

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: part 1 - simple s/nsAutoArrayPtr/UniquePtr/ changes in gfx/

This patch handles all the cases where we just want to swap out
nsAutoArrayPtr for UniquePtr and don't require code changes other than
updating uses that implicitly converted to T* to follow UniquePtr's
conventions.

Changed to use MakeUniqueFallible and x.get() instead of &x[0].

Comment 11

[Nathan Froyd [:froydnj]]

Attachment: part 3 - make BufferRecycleBin store UniquePtrs

Changing mRecycledBuffers to store UniquePtrs instead of nsAutoArrayPtrs
opens up the possibility of a reasonable facsimile of ownership in
function signatures.

Rebased etc.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8686719 [details] [diff] [review]
part 1 - simple s/nsAutoArrayPtr/UniquePtr/ changes in gfx/

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

::: gfx/gl/GLBlitHelper.cpp
@@ +370,5 @@
>                      break;
>                  }
>  
> +                UniquePtr<char[]> buffer(new char[length]);
> +                mGL->fGetProgramInfoLog(program, length, nullptr, &buffer[0]);

buffer.get()

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8686720 [details] [diff] [review]
part 3 - make BufferRecycleBin store UniquePtrs

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

This is a nice patch now.

::: gfx/layers/ImageContainer.h
@@ +734,5 @@
>  
>    /**
>     * Return a buffer to store image data in.
>     * The default implementation returns memory that can
> +   * be freed with delete[]

I think we can just drop "The default implementation returns memory that can be freed wit delete[]" comment as the UniquePtr is now responsible for freeing the memory instead of caller.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/db42f398f454
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ca297b17a82
https://hg.mozilla.org/integration/mozilla-inbound/rev/198d31b65082

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/db42f398f454
https://hg.mozilla.org/mozilla-central/rev/4ca297b17a82
https://hg.mozilla.org/mozilla-central/rev/198d31b65082