Closed Bug 1216644 Opened 4 years ago Closed 4 years ago

get rid of nsAutoArrayPtr usage in gfx/

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(3 files, 2 obsolete files)

No description provided.
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.
Attachment #8676359 - Flags: review?(jmuizelaar)
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.
Attachment #8676360 - Flags: review?(jmuizelaar)
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.
Attachment #8676361 - Flags: review?(jmuizelaar)
Attachment #8676360 - Flags: review?(jmuizelaar) → review+
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.
Attachment #8676359 - Flags: review?(jmuizelaar) → review+
(.get() is consistent with the other smart pointer types for getting the underlying pointer, too)
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.
Attachment #8676361 - Flags: review?(jmuizelaar) → review-
(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 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.
Attachment #8676359 - Flags: review+
Depends on: 1217080
Bug 127080 will let us avoid the SharedPlanarYCbCrImage hack
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].
Attachment #8686719 - Flags: review?(jmuizelaar)
Attachment #8676359 - Attachment is obsolete: true
Changing mRecycledBuffers to store UniquePtrs instead of nsAutoArrayPtrs
opens up the possibility of a reasonable facsimile of ownership in
function signatures.

Rebased etc.
Attachment #8686720 - Flags: review?(jmuizelaar)
Attachment #8676361 - Attachment is obsolete: true
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()
Attachment #8686719 - Flags: review?(jmuizelaar) → review+
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.
Attachment #8686720 - Flags: review?(jmuizelaar) → review+
You need to log in before you can comment on or make changes to this bug.