1288797 - Replace nsAutoPtr with UniquePtr in nsStyleStruct.h (for variable "mCropRect")

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

We're slowly migrating from using nsAutoPtr to UniquePtr as our RAII strongly-owning type.

Filing this bug on fixing one particular instance, in nsStyleStruct.h:
> struct nsStyleImage
> {
> [...]
>   nsAutoPtr<nsStyleSides> mCropRect;

https://dxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.h#376

In particular, we should:
 - Replace the nsAutoPtr.h include with "mozilla/UniquePtr.h", at the top of nsStyleStruct.h

 - Change the member-variable declaration (quoted above) to mozilla::UniquePtr<nsStyleSides> instead of nsAutoPtr<nsStyleSides>.

 - Change this variable's getter to return a "const UniquePtr<nsStyleSides>&" instead of a raw pointer (per the recommendation in the UniquePtr documentation, "If you want to expose the related resource [...] it's better to expose a |const UniquePtr&|" https://dxr.mozilla.org/mozilla-central/source/mfbt/UniquePtr.h#170 )

 - Change the callers to accommodate this. I think there are only two callers:
 https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22nsStyleImage%3A%3AGetCropRect%28%29+const%22  And probably only the second one needs to change -- to use "const UniquePtr<nsStyleSides>&" as its local-variable-decl type.

 - Change this variable's setter ("SetCropRect") to simply take a UniquePtr instead of a raw pointer (per UniquePtr documentation, "To unconditionally transfer ownership of a UniquePtr"), and perform the assignment using Move(), and add Move() around the setter's argument at the callsites.

 - Probably one or two other things that need to change, too, but hopefully the above list is mostly exhaustive.

(I'm planning on assigning this bug to a new engineer starting next week, so please nobody take it. :))

Comment 1

[Neerja Pancholi[:neerja]]

Attachment: Bug 1288797 - Replace nsAutoPtr with UniquePtr in nsStyleStruct.h (for variable "mCropRect")

Review commit: https://reviewboard.mozilla.org/r/67846/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67846/

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8775705 [details]
Bug 1288797 - Replace nsAutoPtr with UniquePtr in nsStyleStruct.h (for variable "mCropRect")

https://reviewboard.mozilla.org/r/67846/#review64868

This looks pretty good! Marking it r- for now, but I think it'll be r+ with the following addressed:

::: layout/style/nsComputedDOMStyle.cpp:2091
(Diff revision 1)
>      {
>        imgIRequest *req = aStyleImage.GetImageData();
>        nsCOMPtr<nsIURI> uri;
>        req->GetURI(getter_AddRefs(uri));
>  
> -      const nsStyleSides* cropRect = aStyleImage.GetCropRect();
> +      const mozilla::UniquePtr<nsStyleSides>& cropRect = aStyleImage.GetCropRect();

Please shorten this to just "UniquePtr" here. (Not "mozilla::UniquePtr")

(For brevity, in .cpp files, we usually don't bother with the "mozilla::" prefix on mozilla-namespaced stuff -- we add "using" declarations at the top of the file ("using namespace mozilla;" in this case) which tell the compiler about the namespace and let it figure out that UniquePtr really means mozilla::UniquePtr.)

(We only drop the namespace like this for .cpp files.  In .h files, on the other hand, we *do* use the "mozilla::" prefix, because we don't want to put "using" declarations in header files generally [because that'd arbitrarily mess with behavior in .cpp files that include that header file].)

::: layout/style/nsStyleStruct.cpp:1971
(Diff revision 1)
>    } else if (aOther.mType == eStyleImageType_Element) {
>      SetElementId(aOther.mElementId);
>    }
>  
> -  SetCropRect(aOther.mCropRect);
> +  nsStyleSides* copyOfCropRect = new nsStyleSides(*(aOther.mCropRect.get()));
> +  SetCropRect(MakeUnique<nsStyleSides>(*copyOfCropRect));

It looks "copyOfCropRect" is an extra intermediate copy that we're making here (and then leaking it, I think), because MakeUnique allocates its own copy (separate from the one allocated via |new| here).

We should skip the |new| and just do something like:
`SetCropRect(MakeUnique<nsStyleSides>(*aOther.mCropRect.get())`

I think that should work -- it should invoke the nsStyleSides copy-constructor under the hood, I expect...

::: layout/style/nsStyleStruct.cpp:2082
(Diff revision 1)
>      mType = eStyleImageType_Element;
>    }
>  }
>  
>  void
> -nsStyleImage::SetCropRect(nsStyleSides* aCropRect)
> +nsStyleImage::SetCropRect(mozilla::UniquePtr<nsStyleSides> aCropRect)

As noted above, since this is in a .cpp file, you shouldn't use the "mozilla::" prefix here. Just UniquePtr.

::: layout/style/nsStyleStruct.cpp:2085
(Diff revision 1)
>  
>  void
> -nsStyleImage::SetCropRect(nsStyleSides* aCropRect)
> +nsStyleImage::SetCropRect(mozilla::UniquePtr<nsStyleSides> aCropRect)
>  {
>    if (aCropRect) {
> -    mCropRect = new nsStyleSides(*aCropRect);
> +    mCropRect = Move(aCropRect);

You can just collapse this function to:
 mCropRect = Move(aCropRect);
 
No need for the null-check anymore.  (It was needed back when we did the allocation [or not] in this function, but now the caller is doing the allocation for us, it looks like.  So, we can just accept whatever the caller passes in, whether it's null or not.)

::: layout/style/nsStyleStruct.cpp:2253
(Diff revision 1)
>  {
>    if (mType != aOther.mType) {
>      return false;
>    }
>  
> -  if (!EqualRects(mCropRect, aOther.mCropRect)) {
> +  if (!EqualRects(mCropRect.get(), aOther.mCropRect.get())) {

Let's change EqualRects to take a `const UniquePtr<nsStyleSides>&` instead of a raw pointer, so that we can leave this line un-changed.

(Part of the idea with UniquePtr is to avoid dealing with raw pointers as much as possible, so that ownership and lifetime is made explicit.)

Comment 3

[Neerja Pancholi[:neerja]]

Okay, I will make these changes and re-send you the review request. Thanks!

Comment 4

[Neerja Pancholi[:neerja]]

Attachment: Bug 1288797 - Replace nsAutoPtr with UniquePtr in nsStyleStruct.h (for variable "mCropRect"). Changes after review.

Review commit: https://reviewboard.mozilla.org/r/67868/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67868/

Comment 5

[Daniel Holbert [:dholbert]]

(Hmm, looks like the updated review request included a new "MozReview-Commit-ID" in the commit message -- that's how mozreview would figure out that it's an updated version of the same patch, and provide an interdiff for new version vs. old version.

Ideally, hg commands to update the old changeset should preserve that information, but I guess it didn't work out in this case. Not to worry for this case -- I can just compare the old vs. new patches locally.)

Comment 6

[Daniel Holbert [:dholbert]]

(Oh, I see -- it looks like the second commit here is just the new changes; disregard comment 5, I mistakenly thought it was an updated version of the original commit.)

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8775779 [details]
Bug 1288797 - Replace nsAutoPtr with UniquePtr in nsStyleStruct.h (for variable "mCropRect"). Changes after review.

https://reviewboard.mozilla.org/r/67868/#review64888

Looks like this addressed all my comments - thanks! Just one nit, below.

Also: we need to get this combined into a single commit (rather than using separate patches/commits dedicated to fixing review comments).  So, we need to figure out how to squash your two commits into one (with the following nit addressed as well).  Once you've done that (and gotten that updated commit onto this bug), I'll mark it r+ and we can test & land it.

::: layout/style/nsStyleStruct.cpp:2248
(Diff revision 1)
>  {
>    if (mType != aOther.mType) {
>      return false;
>    }
>  
> -  if (!EqualRects(mCropRect.get(), aOther.mCropRect.get())) {
> +  if (!EqualRects(mCropRect, aOther.mCropRect) ) {

Looks like this is introducing an extra space between the close-parenthesis here -- drop that extra space, please.

(Ultimately, this line shouldn't be modified at all by patches on this bug -- it should remain as it was before; and it didn't include a space before.)

Comment 8

[Neerja Pancholi[:neerja]]

Comment on attachment 8775705 [details]
Bug 1288797 - Replace nsAutoPtr with UniquePtr in nsStyleStruct.h (for variable "mCropRect")

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67846/diff/1-2/

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8775705 [details]
Bug 1288797 - Replace nsAutoPtr with UniquePtr in nsStyleStruct.h (for variable "mCropRect")

https://reviewboard.mozilla.org/r/67846/#review64890

Looks great! r=me

Comment 10

[Neerja Pancholi[:neerja]]

Comment on attachment 8775705 [details]
Bug 1288797 - Replace nsAutoPtr with UniquePtr in nsStyleStruct.h (for variable "mCropRect")

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67846/diff/2-3/

Comment 11

[Daniel Holbert [:dholbert]]

https://reviewboard.mozilla.org/r/67846/#review64958

::: layout/style/nsStyleStruct.cpp:1971
(Diff revisions 2 - 3)
>    } else if (aOther.mType == eStyleImageType_Element) {
>      SetElementId(aOther.mElementId);
>    }
>  
> -  SetCropRect(MakeUnique<nsStyleSides>(*aOther.mCropRect.get()));
> +  UniquePtr<nsStyleSides> cropRectCopy;
> +  if(aOther.mCropRect){

Nit: please add spaces between "if" and the open-paren, and between the close-paren and the curly-brace.

Should be:
  if (aOther.mCropRect) {

Otherwise, this updated version looks good!

Comment 12

[Neerja Pancholi[:neerja]]

Comment on attachment 8775705 [details]
Bug 1288797 - Replace nsAutoPtr with UniquePtr in nsStyleStruct.h (for variable "mCropRect")

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67846/diff/3-4/

Comment 13

[Daniel Holbert [:dholbert]]

Great, thanks for fixing that - with a successful Try run (kick that off whenever it's convenient), I think this will be ready to check in!

Comment 14

[Daniel Holbert [:dholbert]]

The latest try run[1] is a bit colorful, but I'm pretty sure all the failures are sporadic / unrelated. So, I'll go ahead and autoland this. Congrats on getting your first patch into the tree!

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a081611b832

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4dc4d45c2115
Replace nsAutoPtr with UniquePtr in nsStyleStruct.h (for variable "mCropRect") r=dholbert

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4dc4d45c2115

Comment 17

[Neerja Pancholi[:neerja]]

(In reply to Daniel Holbert [:dholbert] (mostly OOTO until Aug 9th) from comment #14)
> The latest try run[1] is a bit colorful, but I'm pretty sure all the
> failures are sporadic / unrelated. So, I'll go ahead and autoland this.
> Congrats on getting your first patch into the tree!
> 
> [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a081611b832

Thanks Daniel! :)